feat(propagator-env-carrier): make EnvironmentGetter read the current process.env#6853
Merged
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6853 +/- ##
==========================================
- Coverage 95.57% 95.57% -0.01%
==========================================
Files 387 387
Lines 13162 13146 -16
Branches 3016 3001 -15
==========================================
- Hits 12580 12564 -16
Misses 582 582
🚀 New features to boost your workflow:
|
Member
Author
|
PTAL @maryliag, @pichlermarc, @open-telemetry/semconv-cicd-approvers |
kamphaus
reviewed
Jun 25, 2026
kamphaus
approved these changes
Jun 25, 2026
trentm
requested changes
Jun 25, 2026
pellared
marked this pull request as draft
June 26, 2026 04:51
pellared
marked this pull request as ready for review
June 26, 2026 12:25
trentm
approved these changes
Jun 26, 2026
pellared
added a commit
to pellared/opentelemetry-specification
that referenced
this pull request
Jun 29, 2026
…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
pellared
added a commit
to pellared/opentelemetry-specification
that referenced
this pull request
Jun 29, 2026
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)) } ```
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follows open-telemetry/opentelemetry-specification#5179
Update the environment variable propagation carrier to avoid default caching of process environment data. Environment propagation is intended to be extracted once during application startup, so
EnvironmentGettershould make a direct normalized-key lookup instead of preloading or caching the whole environment. This aligns the implementation with the OpenTelemetry's Performance engineering value.