Service exposes ports for receivers which are not enabled

[bhiravabhatla]

If I understand open-telemetry config correctly, a receiver is enabled only if its mentioned in service section of config

Configuring a receiver does not enable it. Receivers are enabled via pipelines within the service section.

Reference - https://opentelemetry.io/docs/collector/configuration/

In our current implementation of service reconciliation, while fetching inferred Ports - I do not see this check.

opentelemetry-operator/pkg/collector/adapters/config_to_ports.go

Line 35 in db40230

func ConfigToReceiverPorts(logger logr.Logger, config map[interface{}]interface{}) ([]corev1.ServicePort, error) {

opentelemetry-operator/pkg/collector/reconcile/service.go

Lines 78 to 82 in db40230

	ports, err := adapters.ConfigToReceiverPorts(params.Log, config)
	if err != nil {
	params.Log.Error(err, "couldn't build the service for this instance")
	return nil
	}

This would imply that we would have disabled receiver ports in service spec. Is this expected. Or am I missing something?

[jpkrohling]

You are absolutely correct. Our logic should indeed check if the receiver is being used in any pipelines before deciding to open the port.

[bhiravabhatla]

I can work on this once I am done with unit test cases

[bhiravabhatla]

@jpkrohling - started working on this. Have a question, as of now in operator we are not umarshalling collector config to a struct. We are unmarshalling it to map[interface]interface{}.

To get the list of enabled receivers it would be easier to unmarshall the config into a Config struct - rather than recursively parsing through a map[interface]interface{}. opentelemetry-collector code base already has logic to unmarshall the config. Can we reuse it?

[jpkrohling]

I think @pavolloffay, @rubenvp8510, and now you have proposed that. While I think that not having a dependency on the otel-collector API is advantageous, as the interface the collector has with its users is indeed the config as a string (and we are one of such users), I do see the benefits of reusing their config structs.

If both @pavolloffay and @rubenvp8510 are still of this opinion, I'm OK with merging a PR dedicated to introducing this new pattern.

[yuriolisa]

@bhiravabhatla are you still working on that issue?

[yuriolisa]

/assign