-
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
[confmap] Add featuregate to use stable expansion rules #10259
[confmap] Add featuregate to use stable expansion rules #10259
Conversation
399d98f
to
26a6d75
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10259 +/- ##
=======================================
Coverage 92.53% 92.53%
=======================================
Files 388 388
Lines 18317 18317
=======================================
Hits 16949 16949
Misses 1022 1022
Partials 346 346 ☔ View full report in Codecov by Sentry. |
{{ if .Distribution.SupportsConfmapFactories }} | ||
if confmap.UseStableExpansionRules.IsEnabled() { | ||
set.ConfigProviderSettings.ResolverSettings.DefaultScheme = "env" | ||
} else { | ||
set.ConfigProviderSettings.ResolverSettings.ConverterFactories = []confmap.ConverterFactory{ | ||
expandconverter.NewFactory(), | ||
} | ||
} | ||
{{- end}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mx-psi @evan-bradley this solution has the downside that it will only apply to people that use ocb. Is that ok?
Does it need to live in otelcol
, like in NewCommand
, and be allowed to mess with the incoming settings, ignoring any configured expandconverter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If possible, this is the process I think would work best:
- Implement the feature gate in
NewCommand
/NewCollector
/etc. with what you have here. - Turn the feature gate on by default.
- Allow configuring the default scheme in an ocb manifest.
- Remove the feature gate and the expand converter from ocb in one go, since there's no way to configure converters in the ocb manifest right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the two behaviors can coexist, I don't think we should remove a configured expand converter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about this again I propose a alternative:
Since this is a change to how CollectorSettings
is configured, we handle the transition for only our own usage (ocb) - anyone using NewCommand
is responsible for updating their own CollectorSettings
once expandconverter
is deprecated.
This approach would mean moving the featuregate into the main.go
generated by ocb, deprecating expandconverter
and then as part of that deprecation following a standard feature gate process.
This approach saves us the trouble of guessing how other users of NewCommand
want to handle the deprecation of the expandconverter
while at the same time enforcing our own Provider expectations established in the rfc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would first have to handle this TODO first, where we start requiring incoming CollectorSettings
to have the providers/converts supplied, making NewCommand
more dependent on a full set of settings when invoked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@evan-bradley @mx-psi @codeboten I am still working towards this PR but trying to unblock/simplify it by working on #10290 first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about this some more, I believe by moving the feature gate to builder and only controlling our own use of expandconverter
allows this PR and #10359 to happen in parallel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest moving the feature gate to a different package to prevent all the imports this is causing
0e8fb5d
to
36431dc
Compare
36431dc
to
e8a88ff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lint fails:
/home/runner/work/opentelemetry-collector/opentelemetry-collector/.tools/golangci-lint run
Error: configprovider.go:145:51: SA1019: expandconverter.NewFactory is deprecated: [v0.102.0] Expanding $ENV is no longer supported. To expand ${} using env vars use DefaultScheme="env" or the otelcol.useStableExpansionRules feature gate. (staticcheck)
ConverterFactories: []confmap.ConverterFactory{expandconverter.NewFactory()},
^
Error: command_test.go:57:52: SA1019: expandconverter.NewFactory is deprecated: [v0.102.0] Expanding $ENV is no longer supported. To expand ${} using env vars use DefaultScheme="env" or the otelcol.useStableExpansionRules feature gate. (staticcheck)
ConverterFactories: []confmap.ConverterFactory{expandconverter.NewFactory()},
^
Error: command_test.go:91:52: SA1019: expandconverter.NewFactory is deprecated: [v0.102.0] Expanding $ENV is no longer supported. To expand ${} using env vars use DefaultScheme="env" or the otelcol.useStableExpansionRules feature gate. (staticcheck)
I believe the actual deprecation depends on #10281 |
#### 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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to add some config provider tests that show what the change in behavior is before and after enabling the feature gate
This PR is currently under the assumption that we'll stop providing default providers and converters in NewCommand. If we decide to keep doing that, I'll open a new PR that keeps the feature gate in otelcol and does the same logic as the builder when setting defaults. |
Created #10361 as an alternative |
So it turns out featuregates don't work in So I need to rework this PR to do the manipulation inside otelcol like I originally had it 😢 |
This solution doesn't work, need #10391 instead |
#### Description This PR adds a feature gate that will handle transitioning users away from expandconverter, specifically expanding `$VAR` syntax. The wholistic strategy is: 1. create a new feature gate, `confmap.unifyEnvVarExpansion`, that will be the single feature gate to manage unifying collector configuraiton resolution. 2. Update expandconverter to return an error if the feature gate is enabled and it is about to expand `$VAR` syntax. 3. Update `otelcol.NewCommand` to set a `DefaultScheme="env"` when the feature gate is enabled and no `DefaultScheme` is set, this handles `${VAR}` syntax. 4. Separately, deprecate `expandconverter`. 5. Follow a normal feature gate cycle. 6. Removed the `confmap.unifyEnvVarExpansion` feature gates and `expandconverter` at the same time Supersedes #10259 #### Link to tracking issue Related to #10161 Related to #8215 Related to #7111 #### Testing Unit tests
Description
Add a feature gate that provides access to stable expansion rules. When enabled:
env
is used as the default scheme$ENV
syntax is no longer expandedLink to tracking issue
Related to #10161
Related to #8215
Related to #7111
Testing
Tested manually locally