Jaeger Receiver enable all protocols even if configured for just one

[pjanotti]

The Jaeger receiver enables all protocols for collector and agent no matter the configuration protocol. This is a bug that needs to be fixed: each receiver/protocol specification should enable only the corresponding endpoint.

[tigrannajaryan]

This is likely due to #113

[pjanotti]

Yes, but it is more than that: the receiver code assumes one configuration and all protocols. We need to take out the common code and actually make separate receivers for each protocol.

[tigrannajaryan]

Looking at the code I see that protocols specified in the config are checked and only the specified protocol port is passed: https://github.com/open-telemetry/opentelemetry-service/blob/8721e3beaf1c449d88c2f6f78517cd03d6b2c7f3/receiver/jaegerreceiver/factory.go#L100-L114

The problem seems to be because of not checking the Enabled field of each proto, but I may be wrong.

We need to take out the common code and actually make separate receivers for each protocol.

Is this required? Is there a reason the current code will not work once Enabled flag is correctly taken into account?

[pjanotti]

You can make it work just taking into account the enabled flag and it seems acceptable in the short run, but, the we should do a proper refactor. At minimum we need to not enable the agent until we have configurations for it.

[ccaraman]

By refactoring, we can modify the receiver to use the configmodels.ReceiverSettings like the rest of the receivers(even make it a requirement maybe?) resulting in a consistent way to enable a receiver and making it easier for us and our customers to work with.

[tigrannajaryan]

@ccaraman I do not see how it can be done. configmodels.ReceiverSettings the way it is defined currently is intended to specify a single protocol. Some receivers support multiple protocols so they need plurality of such settings.

The config format was defined and approved with multiple stakeholders (https://docs.google.com/document/d/1GWOzV0H0RTN1adiwo7fTmkjfCATDDFGuOB4jp3ldCc8/edit#heading=h.g6rvvz3l271i). We can change the format if there is a better proposal but it will have to pass through review and approval process again.

If you have improvement suggestions it is best to target them post 0.1.0 milestone and before the first stable release (date TBD).

[ccaraman]

From further reading the code - it is more than just respecting the enabled flag the code itself doesn't expect there to be protocols not enabled.

When the Receiver is started
https://github.com/open-telemetry/opentelemetry-service/blob/master/receiver/jaegerreceiver/trace_receiver.go#L185-L204
(lets ignore the agent part for now)
The collector component ends up starting all of the protocols

The following ends up using the default tchannel port if no tchannel port was set during the factory methods.
Ex:
https://github.com/open-telemetry/opentelemetry-service/blob/master/receiver/jaegerreceiver/trace_receiver.go#L381-L395

	tch, terr := tchannel.NewChannel("jaeger-collector", new(tchannel.ChannelOptions))
	if terr != nil {
		return fmt.Errorf("failed to create NewTChannel: %v", terr)
	}

	server := thrift.NewServer(tch)
	server.Register(jaeger.NewTChanCollectorServer(jr))

	taddr := jr.tchannelAddr()
	tln, terr := net.Listen("tcp", taddr)

where https://github.com/open-telemetry/opentelemetry-service/blob/master/receiver/jaegerreceiver/trace_receiver.go#L137-L146

func (jr *jReceiver) tchannelAddr() string {
	var port int
	if jr.config != nil {
		port = jr.config.CollectorThriftPort
	}
	if port <= 0 {
		port = defaultTChannelPort
	}
	return fmt.Sprintf(":%d", port)
}

Resulting in the ThriftPort being started even if a config specified otherwise.

The same thing for the Thrift HTTP port and the gRPC port.

[pjanotti]

Fixed via #490