fix(api): make EnvironmentCarrier spec-compliant (#4190) by om7057 · Pull Request #4264 · open-telemetry/opentelemetry-cpp

om7057 · 2026-07-17T12:33:33Z

Three changes to align with the OTel env-carriers specification:

Empty key normalization (normative, spec#5163): NormalizeKey('') now returns '_' per the spec requirement that 'An empty environment variable name is non-normalized and normalizes to _.'
Remove caching semantics (non-normative, spec#5179): Get() previously cached the first getenv() result and served stale values on subsequent calls for the same key. The carrier now calls getenv() on every Get() invocation so the current environment state is always reflected. A value_store_ map is retained only to own the memory backing the returned string_view (required by the TextMapCarrier contract), not to serve cached data.
Operational guidance documentation (non-normative): The class-level doc comment now documents the expected usage patterns: initialization-time extraction, child process environment handling via an injected env map, and security considerations around environment variable visibility.

Tests updated:

Replaced GetCachesValues with GetReadsCurrentEnvironment to reflect the no-caching-semantics behavior.
Replaced GetCacheKeyedByNormalizedName (cache stale-value test) with NormalizeKeyEmpty (empty-key -> '_' normalization).
Added GetEmptyKeyNormalizesToUnderscore to cover Get('') path.

Fixes #4190

Fixes # (issue)

Changes

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

CHANGELOG.md updated for non-trivial changes
Unit tests have been added
Changes in public API reviewed

Three changes to align with the OTel env-carriers specification: 1. Empty key normalization (normative, spec#5163): NormalizeKey('') now returns '_' per the spec requirement that 'An empty environment variable name is non-normalized and normalizes to _.' 2. Remove caching semantics (non-normative, spec#5179): Get() previously cached the first getenv() result and served stale values on subsequent calls for the same key. The carrier now calls getenv() on every Get() invocation so the current environment state is always reflected. A value_store_ map is retained only to own the memory backing the returned string_view (required by the TextMapCarrier contract), not to serve cached data. 3. Operational guidance documentation (non-normative): The class-level doc comment now documents the expected usage patterns: initialization-time extraction, child process environment handling via an injected env map, and security considerations around environment variable visibility. Tests updated: - Replaced GetCachesValues with GetReadsCurrentEnvironment to reflect the no-caching-semantics behavior. - Replaced GetCacheKeyedByNormalizedName (cache stale-value test) with NormalizeKeyEmpty (empty-key -> '_' normalization). - Added GetEmptyKeyNormalizesToUnderscore to cover Get('') path. Fixes open-telemetry#4190

om7057 · 2026-07-17T12:35:56Z

CC @pellared @pranitaurlam: This PR addresses the three items outlined in #4190:

Empty key normalizes to _ (normative, spec#5163)
Removed caching semantics from Get() (non-normative, spec#5179)
Added operational guidance doc comments (non-normative)

codecov · 2026-07-17T13:31:28Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.31%. Comparing base (c8189c0) to head (2dc3b02).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4264      +/-   ##
==========================================
+ Coverage   81.31%   81.31%   +0.01%     
==========================================
  Files         445      445              
  Lines       18857    18859       +2     
==========================================
+ Hits        15331    15333       +2     
  Misses       3526     3526

Files with missing lines	Coverage Δ
...elemetry/context/propagation/environment_carrier.h	`100.00% <100.00%> (ø)`

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

lalitb · 2026-07-20T23:23:28Z

-  test_setenv("TRACEPARENT", "changed-value");
+  // Without caching, a second Get reflects the current environment state.
+  // (Per the spec, the carrier is intended to be used for a single Extract() at
+  // initialization time; repeated Get() calls on a carrier are not a typical usage pattern.)


I think this test should change TRACEPARENT before the second Get(). As written, it would still pass with the old caching behavior, so it does not really cover the “no stale cache” change.

Yes, I verified that the test wasn't actually proving the no-caching behavior since the env var never changed between calls. Updated the test to set TRACEPARENT to a different value before the second Get(), so it now fails with the old caching implementation and only passes with the new behavior.

- GetReadsCurrentEnvironment test: change TRACEPARENT between the two Get() calls so the test actually proves no stale value is served (old caching behavior would now fail this test) - Get() doc comment: narrow string_view lifetime guarantee from 'valid for the lifetime of this carrier' to 'valid until the next Get() call for the same key', which is the accurate bound given value_store_ is overwritten on each call

om7057 · 2026-07-21T02:49:45Z

@lalitb I've resolved the comments, please go through the changes once again.
Thank you!

marcalff · 2026-07-21T15:48:33Z

@om7057

Per spec PR #5179:

Language implementations remain free to use internal storage when required by their API shape.

I propose to keep the cache here, for the sake of providing a safe storage to support the std::string_view returned by
Get().

In practice, caching the result of getenv(), or not caching it and calling getenv() all the time, should not make any difference: propagation using environment variables is used when a parent process forks a child process, I don't see how and why anyone would want to call setenv() after the child process is started.

What matters is that the code can not be abused to cause a crash, so there must be stable memory to back up the std::string_view returned, in all cases, hence: we cache and never call getenv() again.

marcalff

Thanks for the patch.

Ok with the normalization of "" -> "_".

Please keep the caching logic as is.

Per spec PR #5179, language implementations are free to use internal storage when required by their API shape. Revert the cache removal: - Restore cache_ (renamed from value_store_) with first-read-wins semantics, ensuring the returned string_view stays valid for the lifetime of the carrier regardless of subsequent setenv() calls - Restore GetCachesValues and GetCacheKeyedByNormalizedName tests which verify the cache behavior explicitly - Update Get() doc comment to accurately reflect the caching rationale The only spec-normative change retained is empty key -> '_' normalization.

om7057 · 2026-07-21T19:16:16Z

@om7057

Per spec PR #5179:

Language implementations remain free to use internal storage when required by their API shape.

I propose to keep the cache here, for the sake of providing a safe storage to support the std::string_view returned by Get().

In practice, caching the result of getenv(), or not caching it and calling getenv() all the time, should not make any difference: propagation using environment variables is used when a parent process forks a child process, I don't see how and why anyone would want to call setenv() after the child process is started.

What matters is that the code can not be abused to cause a crash, so there must be stable memory to back up the std::string_view returned, in all cases, hence: we cache and never call getenv() again.

Agreed. Reverted to the original cache_ with first-read-wins semantics. The spec PR #5179 explicitly allows internal storage for API shape reasons, and the cache is exactly the right tool here, stable memory for the returned string_view, with no risk of it being invalidated.

marcalff

LGTM, thanks for the fix.

marcalff · 2026-07-21T20:34:31Z

@om7057

Approved.

Please update the change set comments for this PR, to reflect the final code.

om7057 requested a review from a team as a code owner July 17, 2026 12:33

ci: retrigger CI (DocFX choco 503 transient failure)

ec1c356

om7057 and others added 3 commits July 17, 2026 22:45

ci: retrigger DocFX (choco CDN transient 503)

ec7d82a

ci: retrigger (gcc14 maintainer mode transient failure)

9b1f705

Merge branch 'main' into fix/env-carrier-spec-compliance-4190

83130ed

lalitb reviewed Jul 20, 2026

View reviewed changes

Comment thread api/include/opentelemetry/context/propagation/environment_carrier.h

Merge branch 'main' into fix/env-carrier-spec-compliance-4190

4d30fb5

marcalff requested changes Jul 21, 2026

View reviewed changes

Comment thread api/include/opentelemetry/context/propagation/environment_carrier.h Outdated

marcalff approved these changes Jul 21, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(api): make EnvironmentCarrier spec-compliant (#4190)#4264

fix(api): make EnvironmentCarrier spec-compliant (#4190)#4264
om7057 wants to merge 8 commits into
open-telemetry:mainfrom
om7057:fix/env-carrier-spec-compliance-4190

om7057 commented Jul 17, 2026

Uh oh!

om7057 commented Jul 17, 2026

Uh oh!

codecov Bot commented Jul 17, 2026 •

edited

Loading

Uh oh!

lalitb Jul 20, 2026

Uh oh!

om7057 Jul 21, 2026

Uh oh!

Uh oh!

om7057 commented Jul 21, 2026

Uh oh!

marcalff commented Jul 21, 2026

Uh oh!

marcalff left a comment

Uh oh!

Uh oh!

om7057 commented Jul 21, 2026

Uh oh!

marcalff left a comment

Uh oh!

marcalff commented Jul 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

om7057 commented Jul 17, 2026

Changes

Uh oh!

om7057 commented Jul 17, 2026

Uh oh!

codecov Bot commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lalitb Jul 20, 2026

Choose a reason for hiding this comment

Uh oh!

om7057 Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

om7057 commented Jul 21, 2026

Uh oh!

marcalff commented Jul 21, 2026

Uh oh!

marcalff left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

om7057 commented Jul 21, 2026

Uh oh!

marcalff left a comment

Choose a reason for hiding this comment

Uh oh!

marcalff commented Jul 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Jul 17, 2026 •

edited

Loading