Mark env var carrier as RC#5142
Conversation
|
CC @open-telemetry/semconv-cicd-approvers |
marcalff
left a comment
There was a problem hiding this comment.
I have some concerns about this.
First, process wise, this is too soon.
The spec just was changed last week with PR #5102.
Not every SIG implementation had a chance to implement it yet, and in fact, we found issues with it in C++, leading to the second point.
Secondly, the spec mandates that if a given normalized key (TRACEPARENT) is not found, then the code should:
- iterate on all keys in the carrier (i.e., inspect every environment variable)
- normalize the env var name
- compare the normalized name to the key searched
- return the value for the key found.
This has two consequences:
- (a) the SDK is very inefficient. When there are 200 env vars present, everyone of them must be inspected, and this each time a key is looked up.
- (b) the SDK is unpredictable and non deterministic in case of multiple entries found.
For example, the implementations I have seen would return the first key that matches, so if the following keys are present:
traceparent=11,TraceParent=22,TrAcEpArEnT=33
the value returned by implementations (Java for example) are random, and this happens silently.
My proposal is to remove this specific clause from the spec.
When searching for traceparent, only lookup the normalized key (TRACEPARENT), and not consider anything else.
This will improve performances, and make propagation deterministic.
The rest of the spec, including normalizing keys on Get/Set, looks ok.
|
Related, C++ implementation: |
|
@marcalff, thanks! Can you please create a new issue out of #5142 (review) so that we can mark this PR as blocked by it and address it in a separate thread? |
|
Marking as draft until this PR is unblocked |
@open-telemetry/semconv-cicd-approvers, @open-telemetry/technical-committee, @open-telemetry/governance-committee I propose to have this PR merged and additionally create a blog post just before KubeCon NA 2026 (e.g. beginning of November). |
|
Related issue, to resolve (one way or another) before stability: |
|
Now that #5143 is resolved and merged, this change is no longer blocked. |
|
SIG meeting: It would be good to have approvals from maintainers of languages that support this. PTAL @open-telemetry/swift-core-maintainers |
Towards #5040
Please double-check: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/context/env-carriers.md
Issue tracking the implementations: #4771
Note that this is already implemented in 7 languages (Java, .NET, C++, Go, Python, Swift, JavaScript) and 2 is in progress with approvals (Rust).
Also see #5040 (comment)