Update Env var getter / setter to reflect spec changes

[jack-berg]

See open-telemetry/opentelemetry-specification#4961

Changes:

• EnvironemtnGetter#keys() no longer convert keys to lower case
• EnvironmentGetter / EnvironmentSetter treat values as opaque, skipping any detection / normalization related to characters or size. See Decouple the responsibilities of the environment variable propagation carrier opentelemetry-specification#4961 (comment)
• Update key normalization to reflect spec guidance:
  • Convert ASCII letters to uppercase
  • Any non-ASCII letter, digit, or underscore is replaced with underscore
  • Add a _ prefix if the key would start with a digit

cc @Bhagirath00

[pellared]

Per open-telemetry/opentelemetry-specification#4944

[codecov]

Codecov Report

❌ Patch coverage is 75.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.30%. Comparing base (7f50d5b) to head (222c4f3).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...y/api/incubator/propagation/EnvironmentSetter.java	69.23%	1 Missing and 3 partials ⚠️

❌ Your patch check has failed because the patch coverage (75.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #8233      +/-   ##
============================================
- Coverage     90.32%   90.30%   -0.03%     
+ Complexity     7653     7649       -4     
============================================
  Files           843      843              
  Lines         23080    23073       -7     
  Branches       2312     2312              
============================================
- Hits          20846    20835      -11     
- Misses         1516     1517       +1     
- Partials        718      721       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

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

[laurit]

could be a bit surprising that this method may return keys that can't be read (because get normalizes keys)

[pellared]

Do you think we should only returns keys that can be actually read (conform to the normalized name)?

[laurit]

The keys method is only used in deprecated JaegerPropagator and OtTracePropagator so it doesn't really have much of a impact. Even if the environment getter would return empty keys it would probably be fine.
In

opentelemetry-java/extensions/trace-propagators/src/main/java/io/opentelemetry/extension/trace/propagation/JaegerPropagator.java

Line 235 in c856c2b

if (key.startsWith(BAGGAGE_PREFIX)) {

there is a check that key starts with uberctx-. If the key is normalized to upper case the check will fail. If it is not then later get will fail to find the value.
In

opentelemetry-java/extensions/trace-propagators/src/main/java/io/opentelemetry/extension/trace/propagation/OtTracePropagator.java

Lines 125 to 130 in c856c2b

	for (String key : getter.keys(carrier)) {
	String lowercaseKey = key.toLowerCase(Locale.ROOT);
	if (!lowercaseKey.startsWith(PREFIX_BAGGAGE_HEADER)) {
	continue;
	}
	String value = getter.get(carrier, key);

I think having the keys normalized would actually work.

[pellared]

Thanks! Then I think that the keys should be normalized.

Moreover, if you add key = key.toLowerCase(Locale.ROOT); before

opentelemetry-java/extensions/trace-propagators/src/main/java/io/opentelemetry/extension/trace/propagation/JaegerPropagator.java

Line 235 in c856c2b

if (key.startsWith(BAGGAGE_PREFIX)) {

then even the deprecated JaegerPropagatorwould work.

I am going address the same issue in Go implementation and also add something the the OTel Specification.

[pellared]

@jack-berg @laurit PTAL open-telemetry/opentelemetry-go-contrib#8761

[laurit]

Actually the OtTracePropagator wouldn't work either since the prefix is ot-baggage- and - gets replaced with _.

[jack-berg]

The keys method is only used in deprecated JaegerPropagator and OtTracePropagator so it doesn't really have much of a impact.

Deprecated and only used for baggage propagation within those.

So what I'm hearing is that keys is incompatible with any propagator which calls it and has keys with characters which would be normalized

Options:

• Throw when keys is called. Probably a non-starter since you don't know which propagators are configured and throwing an unchecked exception would be disruptive. However, if you are doing this propagation, you do know that you're using EnvironmentGetter and we could document that for this use case you need to handle exceptions when you call TextMapPropagator.extract(Context.current(), env, EnvironmentGetter.getInstance()).
• When keys is called, return empty iterable and:
  • Log a warning. Means that propagation will fail with certain propagators, but at least there's a breadcrumb point to what happened.
  • Do nothing extra. Means that propagation will fail silently with certain propagators.
• Return normalized or lowercased keys, and adjust JaegerPropagator / OtTracePropagator to reduce dependency on keys and make it more resilient to - vs. _, and:
  • Log a warning when keys is called. This would allow future propagators to have bread crumb if they use keys, which may not function in env var context.
  • Do nothing extra.

I lean towards logging a warning when keys is called, and not very opinionated about what the actual return value of keys is (as is, normalized, lowercased, or empty).

[pellared]

If Keys returns keys in normalized form then Get should also normalize the cached env vars. Otherwise we will have similar problem as described in #8233 (comment). This is what is done in open-telemetry/opentelemetry-go-contrib#8761