Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add/bring back API to programmatically change config #3023

Closed
pavolloffay opened this issue Apr 26, 2021 · 5 comments · Fixed by #3706
Closed

Add/bring back API to programmatically change config #3023

pavolloffay opened this issue Apr 26, 2021 · 5 comments · Fixed by #3706
Assignees

Comments

@pavolloffay
Copy link
Member

pavolloffay commented Apr 26, 2021

Is your feature request related to a problem? Please describe.

PR #2868 broked API and removed config factory https://github.com/open-telemetry/opentelemetry-collector/pull/2868/files#diff-d6c4c49032868b2ceaa66558ff307319762c635975313d30109923f1faeab594L104. The config factory made it possible to change config.Config before supplying it to the service.

This API was used in Jaeger to programmatically change config object. It is also used at Traceable to programmatically change configured objects (we wrap processor and make them remotely configurable).

cc) @bogdandrutu

Describe the solution you'd like

API that allows me to change config.Config before it gets used by the service.

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Additional context

@pavolloffay pavolloffay changed the title Config factory Add/bring back API to programmatically change config Apr 26, 2021
@bogdandrutu
Copy link
Member

@pavolloffay
Copy link
Member Author

@pavolloffay you can implement this interface https://github.com/open-telemetry/opentelemetry-collector/blob/main/service/parserprovider/provider.go#L25

That is not sufficient and provides a different API with much less flexibility than the API that the PR removed. The Provider returns a Parser that encapsulates viper. In other words, this API only allows me to configure viper and it does not allow me to change the resolved config.Config.

I need access to resolved config.Config and change it before it gets used by the service.

@pavolloffay
Copy link
Member Author

@bogdandrutu @tigrannajaryan ^^ Could we think of a solution for this API breaking change?

@bogdandrutu
Copy link
Member

@jcchavezs @pavolloffay Short answer, to be convinced show me some use-case where changing the config is necessary and how that would help.

If we have an example use-case we can add a "ConfigLoader" interface that takes a Parser and produces the Config, that way you also have that option.

@jcchavezs
Copy link
Contributor

Our use case is pretty simple:

We want to support live config reload, for that we group all the processors of a pipeline into a single macroprocessor. Inside the macroprocessor it is easy and safe to control the processors lifecycle. For that matter we need to intercept the resulting config and:

  • inject the factories for our macroprocessor
  • modify the pipelines to only have one processor (the macroprocessor)
  • copy all the config of the processors in the pipeline so the macroprocessor can bootstrap the processors

Given the change I considered the alternative to create my own koanf parser but that would add a lot of burden and make us sensitive to changes in koanf's implementation for the config.

My proposal is that configparser.Parser becomes an interface which default implementation is the current implementation.

Ping @bogdandrutu

hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this issue Apr 27, 2023
)

* add bundle.d/smartagent/postgres k8s rules

* Apply suggestions from code review

Co-authored-by: Fabrizio Ferri-Benedetti <[email protected]>

* Update bundle.d and remove noop change

---------

Co-authored-by: Fabrizio Ferri-Benedetti <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants