envcar: ignore non-normalized env names on Get and Keys#9112
Conversation
pellared
changed the title
There was a problem hiding this comment.
Pull request overview
Updates the envcar propagator’s environment variable handling to ignore non-normalized environment variable names when reading from the current process, aligning behavior with the Environment Carrier specification discussion (opentelemetry-specification#5143).
Changes:
- Add a
normalizedhelper to detect already-normalized environment variable names. - Update
Carrierto cache only already-normalized environment variables on first read (affectingGet/Keysbehavior). - Adjust tests, examples, docs, and changelog to reflect the new behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| propagators/envcar/normalize.go | Adds normalized helper used to filter env var names. |
| propagators/envcar/normalize_test.go | Adds unit test + benchmark coverage for normalized. |
| propagators/envcar/doc.go | Minor doc comment punctuation/link formatting tweak. |
| propagators/envcar/carrier.go | Changes env cache population to ignore non-normalized env var names; updates docs. |
| propagators/envcar/carrier_test.go | Updates/extends tests to validate ignoring non-normalized env var names. |
| propagators/envcar/carrier_example_test.go | Removes example output assertions (reduces executable-doc coverage). |
| CHANGELOG.md | Documents the changed behavior under Unreleased. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9112 +/- ##
=====================================
Coverage 83.8% 83.8%
=====================================
Files 195 195
Lines 16077 16082 +5
=====================================
+ Hits 13476 13481 +5
Misses 2129 2129
Partials 472 472
🚀 New features to boost your workflow:
|
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…etry#5144) Fixes open-telemetry#5143 Prototypes: - open-telemetry/opentelemetry-go-contrib#9112 - open-telemetry/opentelemetry-cpp#4141 - open-telemetry/opentelemetry-java#8474 - open-telemetry/opentelemetry-js#6774 - open-telemetry/opentelemetry-python#5289 - open-telemetry/opentelemetry-swift-core#83 ### Rationale The previous wording required environment variable carriers to normalize every key present in the carrier before matching a requested propagation key. This PR removes that requirement for two reasons: - **Efficiency**: extraction should not require reading every environment variable - **Determinism**: if multiple environment variable names normalize to the same key, selecting one is ambiguous and implementation-dependent. Instead, carriers normalize the key requested by the propagator and read only the corresponding normalized environment variable name. `Keys` similarly returns only environment variable names that are already normalized. This keeps injection behavior unchanged while making extraction predictable and avoiding unnecessary environment enumeration.
| return c.values[normalize(key)] | ||
| // Get returns the value associated with the normalized key. | ||
| // It reads the normalized key directly from the current process environment. | ||
| func (*Carrier) Get(key string) string { |
There was a problem hiding this comment.
Why the change back from store on first call? The Carrier was not supposed to (potentially) return different values for the same key, see the original PR for discussions on this.
There was a problem hiding this comment.
Please check this discussion #9112 (comment)
This is also a related PR open-telemetry/opentelemetry-specification#5166
There was a problem hiding this comment.
Long story short the appropriate usage is to use this carrier once near application startup.
Caching and enumeration causes unnecessary overhead that we want to avoid.
There was a problem hiding this comment.
I unresolved the mentioned thread so that you can navigate to it
There was a problem hiding this comment.
Does that mean that Keys and Get can respond differently (if used contra to that startup expectation). Shouldn't we drop the store behaviour for Keys too? If the expectation is basically only one call to Keys ever then the store is pretty pointless.
There was a problem hiding this comment.
I get wanting to avoid enumerating all environment variables for efficiency, however Get now reads directly from the env vars.
The mutability of env vars is a concern of mine and I worry that reading directly from env vars without a cache could lead to unpredictable runtime bugs (eg. when an instrumented process emits several spans using the env var context as parent context and the vars were changed in between). Even instantiating the carrier once during startup would not protect against this.
The more I think about it, the more I want to add normative language to the spec like: Get MUST NOT return different values when called with the same argument implying the use of a cache.
There was a problem hiding this comment.
@Joibel, @kamphaus, thanks a lot for your questions/concerns. These are exactly the discussions that we need to have before going RC.
Does that mean that Keys and Get can respond differently (if used contra to that startup expectation). Shouldn't we drop the store behaviour for Keys too? If the expectation is basically only one call to Keys ever then the store is pretty pointless.
@Joibel, I agree that Keys and Get should have the same observation model. If the expected usage is extracting once near startup, then Keys will typically be called at most once for that extraction path (e.g. when using https://pkg.go.dev/go.opentelemetry.io/contrib/propagators/ot which is the only known propagator that uses Keys method).
Removing the Keys cache also seems consistent with the current intended usage, and I think it would avoid the confusing case where Get is live but Keys is a snapshot. It also looks fine for your vendored carrier usage here: https://github.com/argoproj/argo-workflows/blob/4cb7eb4e89299595ba8fddd3b7f9d95872c3b39d/workflow/executor/tracing/tracing.go#L26.
Addressed aeaccb0
Just to double-check. Would you see any benefit from caching Get and Keys, or would it just be unnecessary state/overhead?
Even instantiating the carrier once during startup would not protect against this.
@kamphaus, agreed that construction alone would not protect against later environment mutation. What I meant is that applications should extract from the environment once during startup and then use the extracted context, rather than repeatedly using the process environment as a live parent-context source.
Addressed ef3ca09
The more I think about it, the more I want to add normative language to the spec like:
Get MUST NOT return different values when called with the same argumentimplying the use of a cache.
@kamphaus, I do not think we have evidence yet that this is something which needs to be normative.
For OTel Go, C++ and Rust, we usually care very much about any overhead, I think it is fine if we can expect users to not call Get more than once for the same argument.
At the same time, I think that for some languages it may be fine to cache. But maybe even for these languages it should be opt-in as it cause overhead that users may not want.
At this point of time, I lean towards the non-normative proposal from open-telemetry/opentelemetry-specification#5166
I do not take this lightly and I plan to further discuss this with maintainers of other language implementations.
@Joibel, WDYT?
There was a problem hiding this comment.
I do not take this lightly and I plan to also discuss this with other language maintainers.
I agree that this needs to be discussed with other language maintainers and thank you for taking this seriously.
Might I suggest in the meantime to document in the godoc that applications using envcar/Carrier should cache the context based on it?
There was a problem hiding this comment.
Might I suggest in the meantime to document in the godoc that applications using envcar/Carrier should cache the context based on it?
I did this before I read your comment 😉
PTAL ef3ca09
There was a problem hiding this comment.
PTAL ef3ca09
And I reviewed it before seeing this.
…try#5163) Issue discovered by @MrAlias in open-telemetry/opentelemetry-go-contrib#9112 (comment) Also see open-telemetry#5163 (comment) Prototypes: - open-telemetry/opentelemetry-go-contrib@0fedf62 - open-telemetry/opentelemetry-dotnet#7424 - open-telemetry/opentelemetry-swift-core#86 - open-telemetry/opentelemetry-python#5328 - open-telemetry/opentelemetry-js#6827 - open-telemetry/opentelemetry-java#8481 For values to be fully portable across POSIX-compliant systems and shell utilities, the standard dictates that environment variable names must consist solely of uppercase letters, digits, and underscores (_), and must not begin with a digit. While some individual implementations might tolerate unusual characters, an empty string lacks any valid characters entirely and cannot act as a recognizable identifier. I verified that on Ubuntu it is not possible to set a env var with an empty name. --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…ive (open-telemetry#5165) ### Motivation The Environment Variables as Context Propagation Carriers specification used normative wording in the "Operational Guidance" section for application-level usage advice, such as when applications read environment variables or how they prepare child process environments. That made the section read like it was adding specification requirements for application behavior, even though the intent is guidance for how environment variable carriers are typically used. This came up in discussion with @dashpole and also I mentioned this in open-telemetry/opentelemetry-go-contrib#9112 (comment) Here is a simple example how the languages can be document some operational guidance: open-telemetry/opentelemetry-go-contrib@5b1606f --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
MrAlias
left a comment
MrAlias
left a comment
There was a problem hiding this comment.
Thanks, this looks good overall. I left one non-blocking question about the example-only SDK dependency.
…y#5179) ## Changes This PR removes the non-normative implementation guidance that suggested environment variable propagation carriers could cache environment variables or individual `Get` lookup results. The environment carrier specification already frames environment variables as process-startup input: applications should extract context during initialization, then continue using the extracted `Context`. With that usage model, the specification does not need to recommend caching as an implementation pattern. Implementations can still use owned storage or explicit snapshots where their language API requires it, but that should be a language-specific design choice rather than general spec guidance. Non-caching implementations so far: - Go: https://github.com/open-telemetry/opentelemetry-go-contrib/blob/main/propagators/envcar/carrier.go - Swift: https://github.com/open-telemetry/opentelemetry-swift-core/blob/main/Sources/OpenTelemetrySdk/Trace/Propagation/EnvironmentContextPropagator.swift - JavaScript: open-telemetry/opentelemetry-js#6853 ## Motivation This follows the same direction as the discussion that led to open-telemetry#5143 and open-telemetry#5144. The concern raised in the open-telemetry#5142 review was that environment carrier extraction should not require inspecting every environment variable. Full-carrier enumeration is inefficient, especially when OpenTelemetry is present but no environment propagation variables are configured. open-telemetry#5144 addressed that by requiring `Get` to normalize the requested propagator key and read only that normalized environment variable name. `Keys` remains the operation that may enumerate keys, and it returns only names that are already normalized. The caching examples added in open-telemetry#5166 can be read as encouraging implementations to reintroduce hidden first-use state, such as: - loading the whole environment into a cache during initialization or on first use, or - caching individual `Get` results by normalized key. That guidance is unnecessary and can work against the intent of open-telemetry#5143/open-telemetry#5144: - Standard extraction typically reads only a small set of keys such as `TRACEPARENT`, `TRACESTATE`, and `BAGGAGE`. - Repeated extraction from the process environment is not the intended hot path; users should extract once at startup and reuse the resulting `Context`. - Whole-environment snapshots can cost more than direct lookup for the common extraction path. - Hidden caches can make later environment changes invisible in a way that is surprising unless the API explicitly describes itself as a snapshot. This also aligns with [Performance](https://opentelemetry.io/community/mission/#we-value-performance), which is a core OpenTelemetry Engineering Value. The Go PR reviews showed the same issue in an implementation: review feedback called out that `Get` should do a direct lookup of the normalized key and leave enumeration to `Keys`, instead of enumerating and storing the process environment on the first `Get`. - open-telemetry/opentelemetry-go-contrib#9112 (comment) - open-telemetry/opentelemetry-go-contrib#9112 (comment) Also see: open-telemetry#5142 (comment) ## Compatibility This PR does not add or remove normative requirements. It only removes non-normative guidance that could steer language implementations toward default caching. Language implementations remain free to use internal storage when required by their API shape, and users can still choose explicit snapshots or caller-provided maps where that is the clearer model. The specification should avoid presenting caching as the recommended default for environment carrier extraction. ## Related - open-telemetry#5142 - open-telemetry#5143 - open-telemetry#5144 - open-telemetry#5166 - open-telemetry#5040 - open-telemetry#4771
I think adding this note may be helpful for implementation and troubleshooting. Personally, even though I was fully aware of the behavior on Windows, I still forgot about it at least twice while working on the implementations for OTel JS (open-telemetry/opentelemetry-js#6853 (comment)) and OTel Go (open-telemetry/opentelemetry-go-contrib#9112). In OTel Go, I even decided to include this in method documentation. ```go // Get normalizes key and returns the corresponding value from the current // process environment. It returns an empty string if the normalized // environment variable is unset or set to an empty value. // // On platforms with case-insensitive environment lookup, such as Windows, the // lookup may match an environment variable whose name differs from the // normalized key only by case. func (*Carrier) Get(key string) string { return os.Getenv(normalize(key)) } ```
Follows open-telemetry/opentelemetry-specification#5144 and open-telemetry/opentelemetry-specification#5163.
Also related to the follow-up caching/stability discussion in open-telemetry/opentelemetry-specification#5166.
This updates
go.opentelemetry.io/contrib/propagators/envcarso reads from the current process environment follow the Environment Variables as Context Propagation Carriers key-normalization model without pre-populating a normalized copy of the whole environment.Changes
Carrier.Getto normalize the requested key and perform a direct lookup of the matching normalized environment variable.Carrier.Keysto return only environment variable names that are already normalized, so non-normalized names such astraceparentare ignored instead of being normalized intoTRACEPARENT.GetandKeysuse the same live-read observation model and avoid extra state/enumeration overhead.normalizedhelper coverage and update empty-key normalization sonormalize("")returns_, matching the documented valid environment-name conversion.Rationale
The PR discussion raised two related concerns:
Getcalls stable if a process mutates its environment later.This PR keeps the Go implementation lightweight and documents the intended usage instead: extract environment context once near application startup, then pass/use that extracted context. It also keeps
GetandKeysconsistent by making both read directly from the current environment rather than mixing a liveGetwith snapshot-styleKeys.