Remove component.UnmarshalConfig

open-telemetry / opentelemetry-collector

OpenTelemetry Collector

https://opentelemetry.io

Apache License 2.0

4.47k stars 1.47k forks source link

Remove component.UnmarshalConfig #7102

Closed bogdandrutu closed 5 months ago

bogdandrutu commented 1 year ago

There are mostly historical reasons why this exists, but we should clean this. Here are few options:

Provide a confmap.Unmarshal equivalent with component.UnmarshalConfig. Few caveats here:
- The issue with Conf.Umarshal https://github.com/open-telemetry/opentelemetry-collector/issues/7101 which kind of require to have a separate func outside of Conf to avoid the circular call.
- The default "Options" for Unmarshal are different, maybe we can fix this by making the ErrorUnused enable by default.
Fix https://github.com/open-telemetry/opentelemetry-collector/issues/7101 and default enable ErrorUnused. This will become super clean, the Conf.Unmarshal can be used, offers options for customizations, etc.

bogdandrutu commented 1 year ago

@gbbr when I say "remove" it means doing the correct "deprecation" process that we have in place for this repo.

gbbr commented 1 year ago

I have checked all (component.Config).Unmarshal implementations in this repository and the contrib distro and they all call (confmap.Conf).Unmarshal without exception. This is an obvious necessity, so for that reason I'm proposing to move the (confmap.Conf).Unmarshal call out of there, so that the collector does this automatically. There is no reason to expect all users to do this. This will not only solve the cyclic issue but also makes more sense as de-duplication.

Lastly, why is (component.Config) even expected to implement Unmarshaler? It is not really unmarshalling anything (within the new context). It is simply validating config. In this scenario it should either be called PostUnmarshal or somehow merged with Validate.

What do you think about going down this path?

rnishtala-sumo commented 10 months ago

@atoulme this change in v0.92.0 seems to have broken one of our tests and the UnMarshalOption WithIgnoreUnused doesn't seem to ignore the extra keys in the source config.

https://github.com/SumoLogic/sumologic-otel-collector/blob/ot-upgrade/pkg/receiver/jobreceiver/config_test.go#L94

Get the following error

Received unexpected error:
1 error(s) decoding:

* error decoding 'output': 1 error(s) decoding:

* '' has invalid keys: log_entries

Is there any other change needed to not error on extra keys? This used to work for v0.91.0

atoulme commented 10 months ago

You use a nested struct which needs to change the way it unmarshals to set WithIgnoreUnused. https://github.com/SumoLogic/sumologic-otel-collector/blob/ot-upgrade/pkg/receiver/jobreceiver/output/config.go#L55C44-L55C44

You should not change your test for this.