Extends TextMapGetter with GetAll() method, implement usage in W3CBaggagePropagator

[jamesmoessis]

Implements proposed spec changes in open-telemetry/opentelemetry-specification#433 and #6837

• Adds getList() method to TextMapGetter, with default method
• Changes W3CBaggagePropagator to handle multiple headers for baggage, using new getList() method.
• Includes tests.

The vast majority of HTTP server implementation are able to return multiple header values, including the commonly used HttpServletRequest. I have published this branch to my local maven and demonstrated it's usage in the Spring Web MVC instrumentation module, which uses the HttpServletRequest interface. See here: open-telemetry/opentelemetry-java-instrumentation#12581

[codecov]

Codecov Report

Attention: Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.

Project coverage is 90.29%. Comparing base (a6b3302) to head (b3f4af6).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
.../api/baggage/propagation/W3CBaggagePropagator.java	81.81%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #6852      +/-   ##
============================================
+ Coverage     90.28%   90.29%   +0.01%     
- Complexity     6588     6593       +5     
============================================
  Files           729      730       +1     
  Lines         19768    19773       +5     
  Branches       1944     1947       +3     
============================================
+ Hits          17847    17855       +8     
+ Misses         1327     1324       -3     
  Partials        594      594              

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

[laurit]

Perhaps using List<String> isn't the best option. Maybe Iterable<String> as used in keys or Iterator<String> would work better? When using List<String> and carrier is HttpServletRequest where getHeaders returns Enumeration you'd have to copy the enumeration to list with Collections.list(carrier.getHeaders(key)). When using Iterator<String> you could skip copying the enumeration to list with

Enumeration<String> enumeration = carrier.getHeaders(key);
return new Iterator<String>() {
  @Override
  public boolean hasNext() {
    return enumeration.hasMoreElements();
  }

  @Override
  public String next() {
    return enumeration.nextElement();
  }
};

[jamesmoessis]

Yes I agree Iterator<String> is a better choice given the use of Enumeration in HttpServletRequest. I had just copied @jack-berg's implementation from #6837. Will update once we get alignment with Jack.

In terms of the name of the new method, getList also would no longer make sense. Possible names could be getIterator or getAllValues, let me know preference.

[jack-berg]

Hmm... iterator is easier / less allocation for some TextMapGetter implementations, but (slighty) worse ergonomics for callers.

Given this, I think iterator is the better choice since we expect most (all?) propagator instances to be maintained by opentelemetry, and its fine for us to have slightly more burden to improve performance.

[jamesmoessis]

Cool, I've changed it to Iterator. I've also renamed the method to getAll()

[jack-berg]

It might be necessary for us to add this in an experimental fashion, rather than jumping straight to the stable API.

For example, we could have an ExtendedTextMapGetter interface in opentelemetry-api-incubator or in an internal package of opentelemetry-context. Then, W3CBaggagePropagator could check if (getter instanceOf ExtendedTextMapGetter) and adjust its behavior accordingly.

If we do this, I think it would have to go in opentelemetry-context instead of opentelemetry-api-incubator since W3CBaggagePropagator doesn't have access to opentelemetry-api-incubator.

[jamesmoessis]

Makes sense to me

[jamesmoessis]

Ok, this is done - please take a look

[jack-berg]

Suggested change

	package io.opentelemetry.context.internal.propagation;
	package io.opentelemetry.context.propagation.internal;

[jack-berg]

I think this looks great.