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

Remove expandconverter #7111

Closed
bogdandrutu opened this issue Feb 3, 2023 · 4 comments · Fixed by #10510
Closed

Remove expandconverter #7111

bogdandrutu opened this issue Feb 3, 2023 · 4 comments · Fixed by #10510
Assignees

Comments

@bogdandrutu
Copy link
Member

Need to investigate if the current expand providers logic in the confmap.Resolver has all the equivalent functionality. Then we also need to propose some kind of deprecation process: e.g. disable by default via a featuregate; or other alternatives.

One caveat that I know and need to document is that because of this we have a workaround in prometheus receiver see the note here. Once this is disable that is no longer necessary which is a good thing, but may break some users so we need to find a way to handle/communicate that.

@mx-psi
Copy link
Member

mx-psi commented Feb 8, 2023

but may break some users so we need to find a way to handle/communicate that.

We may want to have some sort of subcommand/flag/separate tool to migrate a config from the old syntax to the new one. This could be useful for other future changes in the configuration (whether breaking or just recommendations). If that sounds like a good idea, should we create a separate issue to discuss how to go about it?

@bogdandrutu
Copy link
Member Author

Let's start that issue. We definitely need it. I think the best we can do is to have it as a subcommand but this is just me prematurely commenting 👯

@mx-psi
Copy link
Member

mx-psi commented May 4, 2023

See #7631 :)

@mx-psi
Copy link
Member

mx-psi commented Jan 4, 2024

Per #8215 (comment) before doing this we need to add support for ${...} to the general expansion before removing this converter, to align with open-telemetry/opentelemetry-specification#3744

@TylerHelmuth TylerHelmuth self-assigned this May 14, 2024
codeboten pushed a commit that referenced this issue May 29, 2024
#### Description
This PR adds support for expanding `${}` syntax when no schema is
provided by allowing Resolver to use a default provider.

In a followup PR I'll update otelcol with a feature gate that allow
switching between a default envProvider and the expandconverter.

#### Link to tracking issue
Related to
#10161
Related to
#7111

#### Testing
Added unit tests

#### Documentation
Added godoc strings

---------

Co-authored-by: Evan Bradley <[email protected]>
steves-canva pushed a commit to Canva/opentelemetry-collector that referenced this issue Jun 14, 2024
#### Description
This PR adds support for expanding `${}` syntax when no schema is
provided by allowing Resolver to use a default provider.

In a followup PR I'll update otelcol with a feature gate that allow
switching between a default envProvider and the expandconverter.

#### Link to tracking issue
Related to
open-telemetry#10161
Related to
open-telemetry#7111

#### Testing
Added unit tests

#### Documentation
Added godoc strings

---------

Co-authored-by: Evan Bradley <[email protected]>
mx-psi pushed a commit that referenced this issue Jun 14, 2024
#### 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
mx-psi added a commit that referenced this issue Jun 28, 2024
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
Moves confmap.unifyEnvVarExpansion to beta. This means the collector
will, by default, use the env var provider to expand `${FOO}` synatx and
will error if the expandconverter is used to expand `$FOO` syntax.

<!-- Issue number if applicable -->
#### Link to tracking issue
Related to
#10161
Related to
#8215
Related to
#7111

---------

Co-authored-by: Pablo Baeyens <[email protected]>
dmitryax pushed a commit that referenced this issue Aug 7, 2024
…0508)

#### Description

This PR promotes the `confmap.unifyEnvVarExpansion` feature gate to
stable and sets a `ToVersion` of `v0.106.0`, anticipating that the gate
be completely removed in that version.

We should weigh if switching the Stable should be done in `v0.105.0` or
if it needs more time in `Beta` to give users more time to switch.
Delaying promotion to `Stable` delays confmap 1.0.

If we merge this we need to commit to merging
#10510 in
the same release.

#### Link to tracking issue
Related to
#10161
Related to
#7111
Related to
#8215

---------

Co-authored-by: Evan Bradley <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants