open-telemetry / opentelemetry-collector

OpenTelemetry Collector
https://opentelemetry.io
Apache License 2.0
3.94k stars 1.32k forks source link

[otelcol] confmap ResolverSettings should retain all features custom config provider has #10005

Open splunkericl opened 1 month ago

splunkericl commented 1 month ago

Describe the bug As part of https://github.com/open-telemetry/opentelemetry-collector/pull/9228, custom configProvider is being deprecated from the collector.

The main proposal was to move all features from configProvider to derive from confmap.ResolverSettings. However, one feature that was missing was that custom configProvider allows users to provide a a custom Watch method, which allows notifying the collector framework of settings update. This is a crucial use case of our product as we update out settings dynamically.

Steps to reproduce

What did you expect to see?

What did you see instead?

What version did you use? starting v0.95

What config did you use? N/A

Environment mac sonoma 14.2.1

Additional context

splunkericl commented 1 month ago

Hey @evan-bradley and @mx-psi , I filed a github issue here to follow up with the discussion from https://github.com/open-telemetry/opentelemetry-collector/pull/9228

evan-bradley commented 1 month ago

Thanks for opening this @splunkericl. Could you provide more details on why you need a custom Watch method? I would expect that updating the config at runtime would be triggered by confmap.Provider implementations that support live updates. All Providers are given a confmap.WatcherFunc that they can trigger whenever there is a configuration change and the Collector's configuration should be updated. This will then bubble up to the Watch method on otelcol.ConfigProvider and reload the Collector service.

splunkericl commented 1 month ago

Is the latest recommended approach to trigger the passed in confmap.WatcherFunc when there is an update? I think our code base is based on a much older architecture that requires custom configProvider.Watch method. If that is the recommended approach, we can update and close this issue.

evan-bradley commented 1 month ago

Is the latest recommended approach to trigger the passed in confmap.WatcherFunc when there is an update?

That's my understanding, yes. I will let others chime in here if they know more about how this may change in the future. At a minimum, I expect reload functionality to be provided through the confmap.Provider interface and not in anything living in otelcol.

I think the design likely needs another review and the docs should be updated, but you can see a description for confmap.WatcherFunc here. None of the default Providers currently take advantage of this, but I have tested this with a custom Provider and it works as expected.

mx-psi commented 1 day ago

Agreed with @evan-bradley's analysis here. @splunkericl, if there are still remaining features that are not available under the new API, could you write a minimal example that showcases them?