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

[confmap] Decide on how/whether to allow to configure confmap providers #6956

Closed
mx-psi opened this issue Jan 16, 2023 · 4 comments
Closed
Labels
area:config enhancement New feature or request priority:p2 Medium wontfix This will not be worked on

Comments

@mx-psi
Copy link
Member

mx-psi commented Jan 16, 2023

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

In both #6683 and open-telemetry/opentelemetry-collector-contrib/pull/14897 there are use cases where confmap providers need to have some sort of configuration passed to them.

Currently, while not explicitly enforced by an interface it is assumed that all providers have a constructor with signature func New() confmap.Provider that does not allow configuring it in any way.

Describe the solution you'd like

Define a way to configure providers or decide that we are not going to support this. My preferred way of doing this for global settings would be to allow passing a Config struct to the New builder that is populated from CLI flags (example on this comment #6683 (review)). We would need to figure out a way to namespace the CLI flags.

Describe alternatives you've considered

An alternative is to pass this in the URI (see #6683 (comment)) and handle specific configuration on each Retrieve call. This has the following implications:

  • We don't support configuration of providers in any special way: it is up to the individual providers to implement URI parsing however they want.
  • We risk a lack of consistency among providers; it may also be the case that any strategy that we generally recommend (e.g. URL fragments in the case of the httpsprovider) is not really doable in some other provider.
  • It seems like a less comfortable option when dealing with the Go API directly (e.g. building a downstream distro).
  • We delay building the struct that fetches the configuration until a Retrieve call happens. This may have performance implications.

cc @Aneurysm9 @rapphil @neelayu

@mx-psi mx-psi added enhancement New feature or request priority:p2 Medium area:config labels Jan 16, 2023
@mx-psi mx-psi changed the title [confmap] Decide on how/whether to allow to configure providers [confmap] Decide on how/whether to allow to configure confmap providers Jan 16, 2023
@rapphil
Copy link
Contributor

rapphil commented Jan 16, 2023

Here is the proposal to use url fragments. #6892

@Aneurysm9
Copy link
Member

A concern I would have with passing a Config struct extracted from CLI args is that there would be no way to have differing configurations for different configuration locations. Changing the cert pool or validation settings for the HTTP provider would apply to all HTTP-based configurations and not allow having some not perform cert validation or use a specific cert pool. A similar issue might arise with S3 or other AWS resources providing configuration where it might be necessary to provide a distinct role to assume to access different resources.

@bogdandrutu
Copy link
Member

Define a way to configure providers or decide that we are not going to support this. My preferred way of doing this for global settings would be to allow passing a Config struct to the New builder that is populated from CLI flags (example on this comment #6683 (review)). We would need to figure out a way to namespace the CLI flags.

The other problem with this proposal is that the "URI" like configs are also embedded into the config (not only provided via CLI flags, where is that config coming from in that case)?

@mx-psi
Copy link
Member Author

mx-psi commented Jan 30, 2023

Per last week's Collector SIG we can close this as wontfix, we decided that the way to support this will be by passing configuration as URI fragments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:config enhancement New feature or request priority:p2 Medium wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

4 participants