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

Allow builder to accept multiple config providers #4759

Closed
jpkrohling opened this issue Jan 27, 2022 · 8 comments · Fixed by #9513
Closed

Allow builder to accept multiple config providers #4759

jpkrohling opened this issue Jan 27, 2022 · 8 comments · Fixed by #9513
Assignees
Labels
area:builder release:required-for-ga Must be resolved before GA release

Comments

@jpkrohling
Copy link
Member

This is the builder part of #4567.

@jpkrohling
Copy link
Member Author

Question to @pavankrish123, @pmm-sumo, and @bogdandrutu: how would the main.go look like to support this? Once I know that, we can find a way to support it in the builder.

Without looking into many details about the config provider feature, I think it could work in a similar way as the components: it could accept a list of factories which would be loaded in order. The builder could then have a new config option, "configproviders", accepting go modules for those providers like we do for the other components.

@Aneurysm9
Copy link
Member

I think for this to work we'd need either a conventional way to instantiate a provider given just a package name, or we'd need to include an option to specify a function to invoke to instantiate the provider.

We should probably do the same for map converters, as well.

@jpkrohling
Copy link
Member Author

or we'd need to include an option to specify a function to invoke to instantiate the provider.

We assume "NewFactory" for the components:

@pmm-sumo
Copy link
Contributor

I like the idea with "configproviders" (or "configmapproviders" actually?).

As @Aneurysm9 noted, doing it at the level of configmapproviders also brings a question how to handle map converters and perhaps unmarshaller (separate set of config entries I presume?). I think refactoring it to make it consistent with other components ("NewFactory()") would be the way to go

@Aneurysm9
Copy link
Member

One limitation of the conventional approach is that it can only support one (provider|converter|unmarshaller) per package. The collector currently has several of each in common packages and those would need to be restructured.

bogdandrutu added a commit to bogdandrutu/opentelemetry-collector that referenced this issue Mar 17, 2022
This PR:
1. moves the `configmapprovider.Provider` to `config.MapProvider` and related structs.
2. every provider (env, file, yaml) are split in their own packages to help open-telemetry#4759.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
bogdandrutu added a commit to bogdandrutu/opentelemetry-collector that referenced this issue Mar 17, 2022
This PR:
1. moves the `configmapprovider.Provider` to `config.MapProvider` and related structs.
2. every provider (env, file, yaml) are split in their own packages to help open-telemetry#4759.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
bogdandrutu added a commit to bogdandrutu/opentelemetry-collector that referenced this issue Mar 17, 2022
This PR:
1. moves the `configmapprovider.Provider` to `config.MapProvider` and related structs.
2. every provider (env, file, yaml) are split in their own packages to help open-telemetry#4759.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
bogdandrutu added a commit to bogdandrutu/opentelemetry-collector that referenced this issue Mar 17, 2022
This PR:
1. moves the `configmapprovider.Provider` to `config.MapProvider` and related structs.
2. every provider (env, file, yaml) are split in their own packages to help open-telemetry#4759.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
bogdandrutu added a commit to bogdandrutu/opentelemetry-collector that referenced this issue Mar 17, 2022
This PR:
1. moves the `configmapprovider.Provider` to `config.MapProvider` and related structs.
2. every provider (env, file, yaml) are split in their own packages to help open-telemetry#4759.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
bogdandrutu added a commit to bogdandrutu/opentelemetry-collector that referenced this issue Mar 17, 2022
This PR:
1. moves the `configmapprovider.Provider` to `config.MapProvider` and related structs.
2. every provider (env, file, yaml) are split in their own packages to help open-telemetry#4759.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
bogdandrutu added a commit to bogdandrutu/opentelemetry-collector that referenced this issue Mar 17, 2022
This PR:
1. moves the `configmapprovider.Provider` to `config.MapProvider` and related structs.
2. every provider (env, file, yaml) are split in their own packages to help open-telemetry#4759.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
bogdandrutu added a commit to bogdandrutu/opentelemetry-collector that referenced this issue Mar 21, 2022
This PR:
1. moves the `configmapprovider.Provider` to `config.MapProvider` and related structs.
2. every provider (env, file, yaml) are split in their own packages to help open-telemetry#4759.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
bogdandrutu added a commit to bogdandrutu/opentelemetry-collector that referenced this issue Mar 21, 2022
This PR:
1. moves the `configmapprovider.Provider` to `config.MapProvider` and related structs.
2. every provider (env, file, yaml) are split in their own packages to help open-telemetry#4759.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
bogdandrutu added a commit that referenced this issue Mar 23, 2022
This PR:
1. moves the `configmapprovider.Provider` to `config.MapProvider` and related structs.
2. every provider (env, file, yaml) are split in their own packages to help #4759.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
bogdandrutu added a commit to bogdandrutu/opentelemetry-collector that referenced this issue Mar 23, 2022
This is another required step in order to support configuring map providers via the Builder.

Updates open-telemetry#4759

This is a breaking change, since adds a new func to an interface. It is hard to make this backwards compatible, and would like to move forward with this exception.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
bogdandrutu added a commit to bogdandrutu/opentelemetry-collector that referenced this issue Mar 23, 2022
This is another required step in order to support configuring map providers via the Builder.

Updates open-telemetry#4759

This is a breaking change, since adds a new func to an interface. It is hard to make this backwards compatible, and would like to move forward with this exception.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
bogdandrutu added a commit to bogdandrutu/opentelemetry-collector that referenced this issue Mar 23, 2022
This is another required step in order to support configuring map providers via the Builder.

Updates open-telemetry#4759

This is a breaking change, since adds a new func to an interface. It is hard to make this backwards compatible, and would like to move forward with this exception.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
bogdandrutu added a commit that referenced this issue Apr 4, 2022
This is another required step in order to support configuring map providers via the Builder.

Updates #4759

This is a breaking change, since adds a new func to an interface. It is hard to make this backwards compatible, and would like to move forward with this exception.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>

Co-authored-by: Alex Boten <aboten@lightstep.com>
Nicholaswang pushed a commit to Nicholaswang/opentelemetry-collector that referenced this issue Jun 7, 2022
…n-telemetry#5030)

This PR:
1. moves the `configmapprovider.Provider` to `config.MapProvider` and related structs.
2. every provider (env, file, yaml) are split in their own packages to help open-telemetry#4759.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
Nicholaswang pushed a commit to Nicholaswang/opentelemetry-collector that referenced this issue Jun 7, 2022
This is another required step in order to support configuring map providers via the Builder.

Updates open-telemetry#4759

This is a breaking change, since adds a new func to an interface. It is hard to make this backwards compatible, and would like to move forward with this exception.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>

Co-authored-by: Alex Boten <aboten@lightstep.com>
@atoulme
Copy link
Contributor

atoulme commented Jul 11, 2023

This seems like something we should address in a short order. Any ideas?

@Aneurysm9
Copy link
Member

I believe that my prior concern about conventional initialization regarding multiple providers living in the same package has been addressed and now all provider implementations in this and the contrib repos are in independent packages and have a New() initializer. I think we should have a clear path to including config provider implementations via ocb if we're comfortable with New() as a conventional initializer. I might be slightly more comfortable with NewProvider(), but I think it generally best for these implementations to remain in standalone packages so there should be little risk of conflict in the initializer name.

@atoulme atoulme added the release:required-for-ga Must be resolved before GA release label Dec 19, 2023
@evan-bradley
Copy link
Contributor

I think the providers are in a good place for this to move forward. Per the discussion in #6956, we want to configure providers using URI fragments, which are passed when they resolve a config URI, so the New() function in each provider package should be sufficient to instantiate the provider.

In order to allow the builder to specify confmap providers (and converters), we will need to make some modifications to the otelcol package to make this easier. Right now this isn't really feasible since NewCommand takes otelCol.CollectorSettings which only accepts an instantiated otelcol.ConfigProvider. This creates a bit of a circular dependency since the otelcol Command is what reads in config URIs from the command line arguments, but the ConfigProvider must be created prior to the arguments being read. I can think of a few ways to approach this:

  1. Create otelcol.CommandSettings that translates between the Command and otelcol.CollectorSettings. This would allow exposing options to pass in a slice of providers. We could also potentially hide the option to pass in a custom ConfigProvider in the command to simplify the API. A third option could be to allow passing providers in through an optional argument if we deem that would result in the cleanest API.
  2. Have otelcol.Collector create its own ConfigProvider. This would remove the ability to provide a custom ConfigProvider, but I think the existing one should be sufficient for nearly all users.
  3. Allow updating the set of URIs used in a ConfigProvider. So the builder could create a ConfigProvider which the Command then updates to include the URIs passed in through the command line arguments. We would need to determine how to handle an empty set of URIs or an appropriate default value.

mx-psi pushed a commit that referenced this issue Feb 12, 2024
Resolves
#9460

Required for
#4759

I made this a single PR to reduce the review load, but I can break the
changes into multiple PRs if we would like.
mx-psi pushed a commit that referenced this issue Feb 29, 2024
**Description:**

One way to work toward
#4759.
This implements the second approach that I've outlined here:
#4759 (comment).

I think the main advantage of this approach is that it cleans up the API
for the package. `otelcol.ConfigProvider` is a fairly thin wrapper
around `confmap.Resolver`, so I think we could ultimately remove the
interface, and any custom functionality for config merging or
unmarshaling could be exposed to users through settings rather through a
custom implementation.

**Link to tracking Issue:**

Works toward
#4759

**Testing:**

Unit tests

**Documentation:**

Added Godoc comments.
@evan-bradley evan-bradley self-assigned this Apr 17, 2024
mx-psi added a commit that referenced this issue Apr 22, 2024
**Description:**

Allow configuring confmap providers in the builder's config. If the
field isn't set, the default set of providers is used.

**Link to tracking Issue:**

Resolves
#4759.

**Testing:**

Extended unit tests.

**Documentation:**

Updated the readme to include the new options in the example manifest
file.

cc @mx-psi

---------

Co-authored-by: Evan Bradley <evan-bradley@users.noreply.github.com>
Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:builder release:required-for-ga Must be resolved before GA release
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants