-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[otelcol] Determine if NewCommand
should supply any default providers/converters
#10290
Comments
Setting of default converters is part of the conversation of #10259, where we are deprecating If we continue to supply defaults, instead of setting the |
I'd like to remove all defaults and have these supplied by distro authors through the APIs or through the builder. |
@yurishkuro how is Jaeger all-in-one depending on this feature? Is it using the otelcol API ( |
I'm guessing the relevant code that would need to start explicitly defining providers/converters is here |
#### Description Allows configuring `DefaultScheme` via the builder config. #### Link to tracking issue Related to #10259 Related to #10290 #### Testing Local testing and unit tests --------- Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Co-authored-by: Alex Boten <223565+codeboten@users.noreply.github.com>
Currently we still include all the providers as dependencies because of the newDefaultConfigProviderSettings function, even if distro builders explicitly define a subset of providers to use via the builder config file, correct? 🤔 |
This function is only called here when distro authors don't specify any of these in the |
Why are we trying to remove default behavior? There is a general principle in stdlib that structs should be largely usable with zero values. The ability to cherry-pick config resolvers seems quite obscure capability to me (e.g. the viper lib doesn't force you to choose resolvers), having a sensible default is much more useful. |
Another question: what guarantees do we provide for default providers/converters? Are we allowed to change them without a breaking change or do that need to follow the deprecation process? |
I would say that we're moving where the default behavior lives rather than removing it. Specifically, we're moving it from our APIs to the Collector builder. I think this change would fit the users of each:
To me, moving the defaults out of the If we were to keep these defaults in our APIs and want to look to to the principle of structs being usable with zero-valued settings, I would argue we should consider moving these to
In my opinion they are considered part of the stability guarantees for the package that includes them and would need to adhere to the deprecation process. I think this applies whether we have them in our APIs or in the builder. |
## Which problem is this PR solving? Related to open-telemetry/opentelemetry-collector#10290 ## Description of the changes Updates the `otelcol.CollectorSettings` passed to `otelcol.NewCommand` to include desired providers and converters instead of relying on `otelcol.NewCommand` to provide defaults. These providers/converters as the exact same being set by defaults [here](https://github.com/open-telemetry/opentelemetry-collector/blob/6888f8f7a45fd6dff4dfc29a82802c95459f619c/otelcol/configprovider.go#L134), which will probably stop getting set in the future. ## How was this change tested? - unit tests ## Checklist - [X] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [X] I have signed all commits - [X] I have added unit tests for the new functionality - [X] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` Signed-off-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
Having
This is not something followed by some popular libraries (think
We will have a default on the builder, just no the Go API. |
Add a featuregate to cause an error when no providers or converters are set in `CollectorSettings` that are passed to `NewCommand`. Also deprecate the functions in `otelcoltest` with versions that require `ConfigProviderSettings` so that all providers and converters deps can be removed when the feature gate is stable. Works towards #10290. End state goal here: #10357 --------- Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Co-authored-by: Alex Boten <223565+codeboten@users.noreply.github.com>
There is currently a TODO in
otelcol
here that states:We have removed
ConfigProvider
via #10281 so we could start working on this todo, except that it would be a breaking change to stop providing default providers/converters.We have an explicit unit test via #9302 that tests that we supply default providers/converters for jaeger all-in-one.
Should we move forward with the breaking change (which we know would break jaeger)? What problem are we solving by not providing default providers/converters?
The text was updated successfully, but these errors were encountered: