Implement Environment Variable Context Propagation carriers in api/in…#8074
Implement Environment Variable Context Propagation carriers in api/in…#8074Bhagirath00 wants to merge 6 commits intoopen-telemetry:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8074 +/- ##
============================================
+ Coverage 90.20% 90.33% +0.13%
- Complexity 7592 7629 +37
============================================
Files 841 841
Lines 22911 22920 +9
Branches 2288 2286 -2
============================================
+ Hits 20666 20704 +38
+ Misses 1529 1506 -23
+ Partials 716 710 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
api/incubator/src/main/java/io/opentelemetry/api/incubator/propagation/EnvironmentGetter.java
Outdated
Show resolved
Hide resolved
| return; | ||
| } | ||
| // Spec recommends using uppercase for environment variable names. | ||
| carrier.put(key.toUpperCase(Locale.ROOT), value); |
There was a problem hiding this comment.
There was a problem hiding this comment.
Marking as unresolved because I don't see anything related to the restriction of characters to those that are valid in HTTP header fields, or explanation for how we should be thinking about the platform specific size limitations. Size limitations could be an optional config parameter, for example. Not sure.
There was a problem hiding this comment.
Okay, I've added value validation for HTTP header field characters per RFC 9110 — the setter skips and the getter ignores values with invalid characters. For size limits, I've documented the platform-specific limits in the Javadoc for now since the caller controls how many entries go into the map. Let me know if you'd prefer something more like a configurable limit instead.
api/incubator/src/main/java/io/opentelemetry/api/incubator/propagation/EnvironmentGetter.java
Outdated
Show resolved
Hide resolved
| * TRACESTATE}, {@code BAGGAGE}). This setter translates keys to uppercase before inserting them | ||
| * into the carrier. | ||
| */ | ||
| public enum EnvironmentSetter implements TextMapSetter<Map<String, String>> { |
There was a problem hiding this comment.
So where would this type of thing be used in practice? Since its just setting into a map, the only thing about it specific to env vars is the formatting of the keys.
The caller would have to be managing sub processes and need to manipulate their environment. Something like:
Map<String, String> env = new HashMap<>();
contextPropagators.getTextMapPropagator().inject(context, env, EnvironmentSetter.getInstance());
ProcessBuilder processBuilder = new ProcessBuilder();
processBuilder.environment().putAll(env);
Is this what you had in mind? Have any users asked for this or this just to be spec complete?
There was a problem hiding this comment.
Exactly. I just wanted a simple way to make sure the keys get formatted correctly for environment variables (uppercase/underscores) without having to do it manually every time we use something like a ProcessBuilder.
There was a problem hiding this comment.
Makes sense to me. Marking as unresolved just so it be a more visible explanation for why we have this, when we don't have any other built-in getter / setter implementations. (Its occasionally useful to come back to PRs and find the key comments.)
There was a problem hiding this comment.
I've added a ProcessBuilder usage example and a clearer explanation directly in the Javadoc now, so future readers can see the purpose without digging through PR comments.
Apply spotless auto-formatting to align with project code style
|
Well i think Build are failing!!! |
|
@jack-berg Plz review! |
|
Please respond: #8074 (comment) |
|
@jack-berg Sorry for inconvenience! |
|
@jack-berg Updated the PR based on your feedback!! |
api/incubator/src/main/java/io/opentelemetry/api/incubator/propagation/EnvironmentGetter.java
Outdated
Show resolved
Hide resolved
api/incubator/src/main/java/io/opentelemetry/api/incubator/propagation/EnvironmentGetter.java
Outdated
Show resolved
Hide resolved
|
Plz update this Pr |
| // Spec recommends using uppercase and underscores for environment variable | ||
| // names for maximum | ||
| // cross-platform compatibility. | ||
| String sanitizedKey = key.replace('.', '_').replace('-', '_').toUpperCase(Locale.ROOT); |
There was a problem hiding this comment.
Since EnvironmentGetter reads from the environment, keeping a copy, shouldn't this be toLowerCase instead of toUpperCase? Upper case I don't think will end up matching header values since the spec for w3c for example is traceparent not TRACEPARENT. environment variables in the environment should be uppercase, _ separated, but to auto be mapped to the w3c spec the normalized to lower.
Hopefully this makes sense, and hopefully I'm reading this right. I haven't touched Java in a long time.
Thanks all for the work on this. I checked it out. Everything looks pretty good, I had one key question though about lower/upper case within the carrier. |
@adrielp now that you mention it, it's not possible for lossless round tripping as a result of both the case and the fact that both . and - are converted to _. Did this come up when landing the spec work? |
Can you elaborate more on that statement? |
Issue: #7992
This PR implements environmental variable context propagation by adding
EnvironmentGetterandEnvironmentSettercarriers to theapi/incubatormodule. This allows the Java SDK to extract and inject trace data through standard environment variables.Changes
EnvironmentGetterto handle context extraction from environment variables.EnvironmentSetterto handle context injection for child processes.Terminal Screenshots
All new unit tests passed successfully.

Code has been formatted and passed style checks.
