-
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
[confmap] Move confmap.unifyEnvVarExpansion to beta #10435
[confmap] Move confmap.unifyEnvVarExpansion to beta #10435
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10435 +/- ##
=======================================
Coverage 92.40% 92.41%
=======================================
Files 393 393
Lines 18584 18584
=======================================
+ Hits 17173 17174 +1
Misses 1056 1056
+ Partials 355 354 -1 ☔ View full report in Codecov by Sentry. |
We'll want to make a big deal in this release notes/changelog about this breaking change. |
Agreed about the changelog. I would alsp like to check our docs and examples to remove any references to the |
I searched on several repositories using
|
Updated the error message to resemble the warning log, but with details about the feature gate. |
I confirmed that opentelemetry-helm-charts and opentelemetry-operator have no relevant hits |
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.
LGTM. This is likely to impact some users, so I would like to have more reviews from other approvers before merging
Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
cc @open-telemetry/collector-approvers I would like more approvals here before merging |
This change is breaking my prometheus receiver config where i have relabelling like
How can we now put config variables without otel treating it like an environment variable? |
@azunna1 I am unable to reproduce your issue, can you open an issue with more details. |
#### Description When we promoted `confmap.unifyEnvVarExpansion` to beta, we found that the new expansion logic in `confmap` wasn't handling escaping of `$$` like it is supposed to. This PR fixes that bug, but adding escaping logic for `$$`. @azunna1 this fixes the bug you mentioned in #10435 around the metricstransformprocessor: ```yaml metricstransform: transforms: - include: '^k8s\.(.*)\.(.*)$$' match_type: regexp action: update new_name: 'kubernetes.$${1}.$${2}' - include: '^container_([0-9A-Za-z]+)_([0-9A-Za-z]+)_.*' match_type: regexp action: update new_name: 'container.$${1}.$${2}' ``` #### Testing Added new unit tests explicitly for escaping logic
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.Link to tracking issue
Related to #10161
Related to #8215
Related to #7111