Refactor Jaeger Receiver config (#490)

Fixes #445, #158 

This PR addresses some Jaeger receiver config cleanup as well as makes some breaking changes to the way the config is handled.   See below for details.

**Fixes/Updates**
- Disabled flag is respected per protocol
- Unspecified protocols will no longer be started
- Empty protocol configs can now be specified to start the protocol with defaults. e.g.
  ```
  jaeger:
    protocols:
      grpc:
  ```
- Updated readmes
- Naming and behavior of per protocol Addr/Enabled functions in `trace_reciever.go` has been standardized.
- Added thrift tchannel test to meet code coverage

**Breaking Change**
Changed the way an empty `jaeger:` config is handled.  An empty/default config does not start any jaeger protocols.  Previously it started all three collector protocols.  This is a consequence of not starting unspecified protocols.

Author: joe-elliott

README.md

@@ -162,15 +162,10 @@ receivers:
   jaeger:
     protocols:
       grpc:
-        endpoint: "localhost:9876"
       thrift-http:
-        endpoint: "localhost:14268"
       thrift-tchannel:
-        endpoint: "localhost:14267"
       thrift-compact:
-        endpoint: "localhost:6831"
       thrift-binary:
-        endpoint: "localhost:6832"
 
   prometheus:
     config:

config/config.go

@@ -366,7 +366,7 @@ func loadReceivers(v *viper.Viper, factories map[string]receiver.Factory) (confi
 		customUnmarshaler := factory.CustomUnmarshaler()
 		if customUnmarshaler != nil {
 			// This configuration requires a custom unmarshaler, use it.
-			err = customUnmarshaler(subViper, key, receiverCfg)
+			err = customUnmarshaler(subViper, key, sv, receiverCfg)
 		} else {
 			err = sv.UnmarshalExact(receiverCfg)
 		}

receiver/README.md

@@ -107,32 +107,27 @@ This receiver receives traces in the [Jaeger](https://www.jaegertracing.io)
 format. It translates them into the internal format and sends
 it to processors and exporters.
 
-It supports the Jaeger Collector protocols:
+It supports the Jaeger Collector and Agent protocols:
+- gRPC
 - Thrift HTTP
 - Thrift TChannel
-- gRPC
+- Thrift Compact
+- Thrift Binary
 
-By default, the Jaeger receiver supports all three protocols on the default ports
-specified in [factory.go](jaegerreceiver/factory.go). The following demonstrates
-how to specify the default Jaeger receiver.
+By default, the Jaeger receiver will not serve any protocol. A protocol must be named
+for the jaeger receiver to start.  The following demonstrates how to start the Jaeger
+receiver with only gRPC enabled on the default port.
 ```yaml
 receivers:
   jaeger:
+    protocols:
+      grpc:
 ```
 
-It also supports the Jaeger Agent protocols:
-- Thrift Compact
-- Thrift Binary
-
-By default, these services are not started unless an endpoint is explicitly defined.
-
 It is possible to configure the protocols on different ports, refer to
 [config.yaml](jaegerreceiver/testdata/config.yaml) for detailed config
 examples.
 
-// TODO Issue https://github.com/open-telemetry/opentelemetry-collector/issues/158
-// The Jaeger receiver enables all protocols even when one is specified or a
-// subset is enabled. The documentation should be updated when that fix occurs.
 ### Communicating over TLS
 This receiver supports communication using Transport Layer Security (TLS), but only using the gRPC protocol. It can be configured by specifying a `tls-crendentials` object in the gRPC receiver configuration.   
 ```yaml

receiver/factory.go

@@ -59,7 +59,17 @@ type Factory interface {
 
 // CustomUnmarshaler is a function that un-marshals a viper data into a config struct
 // in a custom way.
-type CustomUnmarshaler func(v *viper.Viper, viperKey string, intoCfg interface{}) error
+// v *viper.Viper
+//   A viper instance at the "receivers" node in the config yaml.  v.Sub(viperKey) is
+//   the raw config this function should load.
+// viperKey string
+//   The name of this config.  i.e. "jaeger/custom".  v.Sub(viperKey) is the raw config
+//   this function should load.
+// sourceViperSection *viper.Viper
+//   The value of v.Sub(viperKey) with all environment substitution complete.
+// intoCfg interface{}
+//   An empty interface wrapping a pointer to the config struct to unmarshal into.
+type CustomUnmarshaler func(v *viper.Viper, viperKey string, sourceViperSection *viper.Viper, intoCfg interface{}) error
 
 // Build takes a list of receiver factories and returns a map of type map[string]Factory
 // with factory type as keys. It returns a non-nil error when more than one factories

receiver/jaegerreceiver/config.go

@@ -18,6 +18,9 @@ import (
 	"github.com/open-telemetry/opentelemetry-collector/receiver"
 )
 
+// The config field name to load the protocol map from
+const protocolsFieldName = "protocols"
+
 // RemoteSamplingConfig defines config key for remote sampling fetch endpoint
 type RemoteSamplingConfig struct {
 	FetchEndpoint string `mapstructure:"fetch_endpoint"`

receiver/jaegerreceiver/config_test.go

@@ -37,12 +37,9 @@ func TestLoadConfig(t *testing.T) {
 	require.NoError(t, err)
 	require.NotNil(t, cfg)
 
-	// The receiver `jaeger/disabled` doesn't count because disabled receivers
+	// The receiver `jaeger/disabled` and `jaeger` don't count because disabled receivers
 	// are excluded from the final list.
-	assert.Equal(t, len(cfg.Receivers), 3)
-
-	r0 := cfg.Receivers["jaeger"]
-	assert.Equal(t, r0, factory.CreateDefaultConfig())
+	assert.Equal(t, len(cfg.Receivers), 4)
 
 	r1 := cfg.Receivers["jaeger/customname"].(*Config)
 	assert.Equal(t, r1,
@@ -81,6 +78,59 @@ func TestLoadConfig(t *testing.T) {
 			},
 		})
 
+	rDefaults := cfg.Receivers["jaeger/defaults"].(*Config)
+	assert.Equal(t, rDefaults,
+		&Config{
+			TypeVal: typeStr,
+			NameVal: "jaeger/defaults",
+			Protocols: map[string]*receiver.SecureReceiverSettings{
+				"grpc": {
+					ReceiverSettings: configmodels.ReceiverSettings{
+						Endpoint: defaultGRPCBindEndpoint,
+					},
+				},
+				"thrift-http": {
+					ReceiverSettings: configmodels.ReceiverSettings{
+						Endpoint: defaultHTTPBindEndpoint,
+					},
+				},
+				"thrift-tchannel": {
+					ReceiverSettings: configmodels.ReceiverSettings{
+						Endpoint: defaultTChannelBindEndpoint,
+					},
+				},
+				"thrift-compact": {
+					ReceiverSettings: configmodels.ReceiverSettings{
+						Endpoint: defaultThriftCompactBindEndpoint,
+					},
+				},
+				"thrift-binary": {
+					ReceiverSettings: configmodels.ReceiverSettings{
+						Endpoint: defaultThriftBinaryBindEndpoint,
+					},
+				},
+			},
+		})
+
+	rMixed := cfg.Receivers["jaeger/mixed"].(*Config)
+	assert.Equal(t, rMixed,
+		&Config{
+			TypeVal: typeStr,
+			NameVal: "jaeger/mixed",
+			Protocols: map[string]*receiver.SecureReceiverSettings{
+				"grpc": {
+					ReceiverSettings: configmodels.ReceiverSettings{
+						Endpoint: "localhost:9876",
+					},
+				},
+				"thrift-compact": {
+					ReceiverSettings: configmodels.ReceiverSettings{
+						Endpoint: defaultThriftCompactBindEndpoint,
+					},
+				},
+			},
+		})
+
 	tlsConfig := cfg.Receivers["jaeger/tls"].(*Config)
 
 	assert.Equal(t, tlsConfig,
@@ -110,3 +160,19 @@ func TestLoadConfig(t *testing.T) {
 			},
 		})
 }
+
+func TestFailedLoadConfig(t *testing.T) {
+	factories, err := config.ExampleComponents()
+	assert.Nil(t, err)
+
+	factory := &Factory{}
+	factories.Receivers[typeStr] = factory
+	_, err = config.LoadConfigFile(t, path.Join(".", "testdata", "bad_proto_config.yaml"), factories)
+	assert.EqualError(t, err, `error reading settings for receiver type "jaeger": unknown Jaeger protocol badproto`)
+
+	_, err = config.LoadConfigFile(t, path.Join(".", "testdata", "bad_no_proto_config.yaml"), factories)
+	assert.EqualError(t, err, `error reading settings for receiver type "jaeger": must specify at least one protocol when using the Jaeger receiver`)
+
+	_, err = config.LoadConfigFile(t, path.Join(".", "testdata", "bad_empty_config.yaml"), factories)
+	assert.EqualError(t, err, `error reading settings for receiver type "jaeger": Jaeger receiver config is empty`)
+}

receiver/jaegerreceiver/factory.go

@@ -22,6 +22,7 @@ import (
 	"net"
 	"strconv"
 
+	"github.com/spf13/viper"
 	"go.uber.org/zap"
 	"google.golang.org/grpc"
 
@@ -48,6 +49,9 @@ const (
 	defaultGRPCBindEndpoint     = "localhost:14250"
 	defaultHTTPBindEndpoint     = "localhost:14268"
 	defaultTChannelBindEndpoint = "localhost:14267"
+
+	defaultThriftCompactBindEndpoint = "localhost:6831"
+	defaultThriftBinaryBindEndpoint  = "localhost:6832"
 )
 
 // Factory is the factory for Jaeger receiver.
@@ -59,33 +63,48 @@ func (f *Factory) Type() string {
 	return typeStr
 }
 
-// CustomUnmarshaler returns nil because we don't need custom unmarshaling for this config.
+// CustomUnmarshaler is used to add defaults for named but empty protocols
 func (f *Factory) CustomUnmarshaler() receiver.CustomUnmarshaler {
-	return nil
+	return func(v *viper.Viper, viperKey string, sourceViperSection *viper.Viper, intoCfg interface{}) error {
+		// first load the config normally
+		err := sourceViperSection.UnmarshalExact(intoCfg)
+		if err != nil {
+			return err
+		}
+
+		receiverCfg, ok := intoCfg.(*Config)
+		if !ok {
+			return fmt.Errorf("config type not *jaegerreceiver.Config")
+		}
+
+		// next manually search for protocols in viper that do not appear in the normally loaded config
+		// these protocols were excluded during normal loading and we need to add defaults for them
+		vSub := v.Sub(viperKey)
+		if vSub == nil {
+			return fmt.Errorf("Jaeger receiver config is empty")
+		}
+		protocols := vSub.GetStringMap(protocolsFieldName)
+		if len(protocols) == 0 {
+			return fmt.Errorf("must specify at least one protocol when using the Jaeger receiver")
+		}
+		for k := range protocols {
+			if _, ok := receiverCfg.Protocols[k]; !ok {
+				if receiverCfg.Protocols[k], err = defaultsForProtocol(k); err != nil {
+					return err
+				}
+			}
+		}
+
+		return nil
+	}
 }
 
 // CreateDefaultConfig creates the default configuration for Jaeger receiver.
 func (f *Factory) CreateDefaultConfig() configmodels.Receiver {
 	return &Config{
-		TypeVal: typeStr,
-		NameVal: typeStr,
-		Protocols: map[string]*receiver.SecureReceiverSettings{
-			protoGRPC: {
-				ReceiverSettings: configmodels.ReceiverSettings{
-					Endpoint: defaultGRPCBindEndpoint,
-				},
-			},
-			protoThriftTChannel: {
-				ReceiverSettings: configmodels.ReceiverSettings{
-					Endpoint: defaultTChannelBindEndpoint,
-				},
-			},
-			protoThriftHTTP: {
-				ReceiverSettings: configmodels.ReceiverSettings{
-					Endpoint: defaultHTTPBindEndpoint,
-				},
-			},
-		},
+		TypeVal:   typeStr,
+		NameVal:   typeStr,
+		Protocols: map[string]*receiver.SecureReceiverSettings{},
 	}
 }
 
@@ -208,3 +227,29 @@ func extractPortFromEndpoint(endpoint string) (int, error) {
 	}
 	return int(port), nil
 }
+
+// returns a default value for a protocol name.  this really just boils down to the endpoint
+func defaultsForProtocol(proto string) (*receiver.SecureReceiverSettings, error) {
+	var defaultEndpoint string
+
+	switch proto {
+	case protoGRPC:
+		defaultEndpoint = defaultGRPCBindEndpoint
+	case protoThriftHTTP:
+		defaultEndpoint = defaultHTTPBindEndpoint
+	case protoThriftTChannel:
+		defaultEndpoint = defaultTChannelBindEndpoint
+	case protoThriftBinary:
+		defaultEndpoint = defaultThriftBinaryBindEndpoint
+	case protoThriftCompact:
+		defaultEndpoint = defaultThriftCompactBindEndpoint
+	default:
+		return nil, fmt.Errorf("unknown Jaeger protocol %s", proto)
+	}
+
+	return &receiver.SecureReceiverSettings{
+		ReceiverSettings: configmodels.ReceiverSettings{
+			Endpoint: defaultEndpoint,
+		},
+	}, nil
+}