Add OTLP header supplier configuration option

[jack-berg]

Resolves #5959.

Currently, OTLP exporters allow you to specify headers, but the headers are static. There's also the experimental concept of an Authenticator, which mirrors the OkHttp concept of the same name. Authenticator is able to respond to 401 response codes and supply headers to retry the request. This Authenticator concept is incomplete though, since it requires every request to first get a 401 response before invoking the supplier and allowing the headers to change. Gross. What we really want to do is something like I described in this comment:

we really want is the ability to set a supplier of headers, instead of fixed static ones, and also give the ability for a particular export attempt to retry after refreshing the authorization header in the event it receives a 401 back. I think we
want to enable something like this:

AtomicReference<String> token = new AtomicReference<>();
token.set(refreshToken());

OtlpHttpSpanExporter exporter = OtlpHttpSpanExporter.builder()
  .setHeaderSupplier(() -> Map.of("Authorization", token.get()))
  .setAuthenticator(() -> {
    String newToken = refreshToken();
    token.set(newToken);
    return Map.of("Authorization", newToken);
  })
  .build();

This PR extends all the OTLP exporter builders with a setHeaders(Supplier<Map<String, String>>) method, to provide more flexibility for authentication mechanisms which are more complex than a static header.

[codecov]

Codecov Report

Attention: 17 lines in your changes are missing coverage. Please review.

Comparison is base (902d68c) 91.17% compared to head (b8b5d8e) 91.06%.

❗ Current head b8b5d8e differs from pull request most recent head e4d92b1. Consider uploading reports for the commit e4d92b1 to get more accurate results

Files	Patch %	Lines
...ry/exporter/internal/grpc/GrpcExporterBuilder.java	79.16%	3 Missing and 2 partials ⚠️
...ry/exporter/internal/http/HttpExporterBuilder.java	83.33%	3 Missing and 1 partial ⚠️
...orter/sender/okhttp/internal/OkHttpGrpcSender.java	66.66%	1 Missing and 2 partials ⚠️
...edchannel/internal/UpstreamGrpcSenderProvider.java	71.42%	0 Missing and 2 partials ⚠️
...pc/managedchannel/internal/UpstreamGrpcSender.java	90.90%	0 Missing and 1 partial ⚠️
...ry/exporter/sender/jdk/internal/JdkHttpSender.java	66.66%	0 Missing and 1 partial ⚠️
...orter/sender/okhttp/internal/OkHttpHttpSender.java	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #6004      +/-   ##
============================================
- Coverage     91.17%   91.06%   -0.11%     
+ Complexity     5709     5685      -24     
============================================
  Files           628      625       -3     
  Lines         16722    16702      -20     
  Branches       1650     1653       +3     
============================================
- Hits          15246    15210      -36     
- Misses         1021     1033      +12     
- Partials        455      459       +4     

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

[shadow0wolf]

any updates on this feature being available in release ? I can see it being pending for review and merge for a long time now

[breedx-splk]

Disclaimer: I'm ok with this the way it is, but I wanted to give the API some additional usability consideration. I also hate to drag this out, but let's consider....

While most users will probably just be fine with the constant headers, the dynamic case is certainly expected, especially for things like auth tokens as discussed elsewhere. The cases where a user has several dynamic header values, or the case where the user has a multi-valued dynamic header should be unusual.

The setHeaderSupplier(Supplier<Map<String,List<String>>>) feels like a pretty big hammer...and while I do agree that it solves all use cases we can think of, it's at the expense of the user having to do a bunch of work in order to use it. This also takes away from the fluent nature and convenience of having a builder in the first place.

So I took at swing at some of the Grpc classes to see what another surface might look like. It's not complete (I didn't touch some of the other builders), but it ends up looking like:

• addHeader(String key, String value)
• addHeader(String key, Supplier<String> headerSupplier)
• addMultiValuedHeader(String key, Supplier<List<String>> headerSupplier)

The 3rd one is only named differently because java can't resolve it when overloaded. I also think that it might be overkill and could probably just go away until someone asks for it.

Another disclaimer: I didn't try and think too much about efficiency/optimization, and the supplier implementations might end up being a little allocation happy, but that's the nature of the dynamic headers. I have no sense of if it would be a real problem yet.

Anyway, going to approve the current impl because I think it's fine...but wanted to share an alternative approach in case it stirs up some ideas.

[jack-berg]

@breedx-splk per the conversation in the spec today, I reverted the setHeaders(Supplier<Map<String, List<String>>>) to be the more simpler setHeaders(Supplier<Map<String, String>>). @trask pointed out that a comma separated list is the same as including the same header key multiple times with different values. See this stackoverflow issue for more info.

So for a similar reason, we wouldn't want to add the addMultiValuedHeader(String key, Supplier<List<String>> headerSupplier) method of your suggestion.

But I think the suggestion of switching the to addHeader(String, Supplier<String>) is interesting. I agree that most users will likely want to set exactly one dynamic header, representing their authorization token. And your suggestion does seem to improve the ergonomics of this. Let's see what example from the description looks like:

AtomicReference<String> token = new AtomicReference<>();
token.set(refreshToken());

OtlpHttpSpanExporter exporter = OtlpHttpSpanExporter.builder()
  .addHeader("Authorization", token::get)
  .setAuthenticator(() -> {
    String newToken = refreshToken();
    token.set(newToken);
    return Map.of("Authorization", newToken);
  })
  .build();

Its a little bit better, but one think sticks out: The API isn't symmetric with Authentication, which expects to return a Map<String, String>. Maybe the cleaner way to implement the example with the current API is something like:

Map<String, String> authHeaders = new ConcurrentHashMap<>();
authHeaders.put("Authorization", refreshToken());

OtlpHttpSpanExporter exporter = OtlpHttpSpanExporter.builder()
  .setHeaderSupplier(() -> authHeaders)
  .setAuthenticator(() -> {
    authHeaders.put("Authorization", refreshToken());
    return authHeaders;
  })
  .build();

I have a slight preference for the Supplier<Map<String,String>> option because:

• The ergonomics are pretty good. Especially with java 11 Map.of(), its trivial to return simple maps from supplier.
• Its symmetric with Authenticator, which is experimental, but probably needs to return a Map<String, String> to accommodate situations where the authentication scheme does require multiple headers.

[breedx-splk]

Thanks for the response and taking the time to consider an alternate approach, I appreciate it. 👍🏻