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 90.90909% with 2 lines in your changes missing coverage. Please review.

Project coverage is 90.11%. Comparing base (7829f53) to head (c95cec8).
Report is 14 commits behind head on main.

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

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #6852      +/-   ##
============================================
- Coverage     90.23%   90.11%   -0.13%     
- Complexity     6594     6603       +9     
============================================
  Files           729      730       +1     
  Lines         19800    19850      +50     
  Branches       1947     1957      +10     
============================================
+ Hits          17867    17888      +21     
- Misses         1341     1369      +28     
- Partials        592      593       +1     

☔ 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]

I think this looks great.

[jamesmoessis]

@jack-berg FYI - just made some slight changes. I realised I needed to check for get() returning null in the default getAll otherwise it returns an iterator of Iterator.of(null) instead of an empty iterator.

I've also added tests to verify this behaviour, and a couple more baggage tests (the build was complaining the coverage wasn't quite there, so I covered the remanining lines)

[jack-berg]

Two additional minor comments. Will get this in before Friday's 1.45.0 release.