-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Comments
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. |
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. |
We assume "NewFactory" for the components: opentelemetry-collector/cmd/builder/internal/builder/templates/components.go.tmpl Line 27 in 32301d9
|
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 |
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. |
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]> Co-authored-by: Alex Boten <[email protected]>
…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 <[email protected]>
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 <[email protected]> Co-authored-by: Alex Boten <[email protected]>
This seems like something we should address in a short order. Any ideas? |
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 |
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 In order to allow the builder to specify confmap providers (and converters), we will need to make some modifications to the
|
**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.
**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 <[email protected]> Co-authored-by: Pablo Baeyens <[email protected]>
This is the builder part of #4567.
The text was updated successfully, but these errors were encountered: