Update Env var getter / setter to reflect spec changes

[jack-berg]

See open-telemetry/opentelemetry-specification#4961

Changes:

• EnvironemtnGetter#keys() normalizes results and logs a one time warning explaining that it may not work with certain propagators
• 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 88.46154% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.31%. Comparing base (7f50d5b) to head (f829e51).
⚠️ Report is 12 commits behind head on main.

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

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #8233      +/-   ##
============================================
- Coverage     90.32%   90.31%   -0.01%     
- Complexity     7653     7654       +1     
============================================
  Files           843      842       -1     
  Lines         23080    23081       +1     
  Branches       2312     2314       +2     
============================================
- Hits          20846    20845       -1     
+ Misses         1516     1515       -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.

[pellared]

I think the issue described in #8233 (comment) still exists

I suggest changing this line to e.g.

Suggested change

	carrier.put("OTEL_BAGGAGE_KEY", "val2");
	carrier.put("otel-baggage-key", "val2");

Note that in this scenario keys would return OTEL_TRACE_ID and OTEL_BAGGAGE_KEY as described in the test keys_valuesAreNormalized

If Keys returns keys in normalized form then Get should also normalize the cached env vars.

cached env vars == keys in the carrier

[jack-berg]

I understand the point now. Thanks! f829e51

[pellared]

Feel free to resolve (I do not have such power here 😉)

[jack-berg]

Not great that evertime we read a key, we have to normalize potentially all the keys in the carrier. Go gets around this by computing the normalized keys once and using the carrier as a cache. In java, its not safe to assume that the map is mutable.

I imagine this isn't a real problem though because env var propagation implies your going to start a new process and pass the context to it via env vars, which is much heavier weight than what we're doing here.

[pellared]

I imagine this isn't a real problem though because env var propagation implies your going to start a new process and pass the context to it via env vars, which is much heavier weight than what we're doing here.

Agreed and at least it is a robust implementation 😁

[pellared]

I guess that thus can be done a little more efficient.
We could break when we find a matching key instead of iterating over all env vars.
I guess iterating over carrier.entrySet() and early return the value when normalized key matches.

[jack-berg]

Yup meant to do that

[jack-berg]

Oh yeah can't exit early from for each. So it's either use a traditional loop which allocated but can exit early, or use forEach which doesn't allocate but which can't exit early

[jack-berg]

Given that strings are being allocated for every key, trying to avoid allocating for map iteration is silly optimization. Will switch to tradition for loop and exit early