add ExtendedBaggageBuilder

[zeitlinger]

Follow-up to #6017, where baggage was scoped out, because it doesn't into ExtendedTracer.

As this is in incubator, it should fit so well that it can be added to Baggage eventually.

Example:

    String value =
        ExtendedBaggageBuilder.current()
            .set("key", "value")
            .call(() -> Baggage.current().getEntryValue("key"));

    assertThat(value).isEqualTo("value");

[zeitlinger]

@jack-berg here's my attempt based on our conversation yesterday.

Here's what you proposed:

String result = Baggage.fromMap(Map.of("key", "value"))
  .storeInContext(Context.current())
  .wrap(() -> "result")
  .call();

This is what I find surprising as a user:

• it's not clear if my current baggage items are added or replaced
• I have to understand that baggage is stored in Context - which I actually don't have to be aware of
• I have to know that wrap comes before call - it's not necessarily long, but harder to discover

[codecov]

Codecov Report

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

Comparison is base (f1fc1c7) 91.17% compared to head (82aeb9d) 91.13%.

Files	Patch %	Lines
...ension/incubator/trace/ExtendedBaggageBuilder.java	75.00%	4 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #6063      +/-   ##
============================================
- Coverage     91.17%   91.13%   -0.04%     
- Complexity     5709     5714       +5     
============================================
  Files           628      629       +1     
  Lines         16722    16738      +16     
  Branches       1650     1650              
============================================
+ Hits          15246    15255       +9     
- Misses         1021     1027       +6     
- Partials        455      456       +1     

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

[jack-berg]

This doesn't seem right. Why have a method that performs the same function with a different name?

[zeitlinger]

it's a different level of abstraction. "put" relates to Map - as a user, I don't case if it's stored in a Map.

[jack-berg]

I think this ends up being the only helpful thing we get. I'm comfortable extending BaggageBuilder with a putAll(Map<String, String>) method an skipping incubation.

[jack-berg]

I don't think this is right. This is a builder, and I think we should stick to builders being comprised of strictly setters and build method. Another problem with this is that it accepts a SpanCallable, but has nothing to do with spans. Maybe this could be fixed by renaming SpanCallable to something like ThrowingCallable, but let's go back to my original suggestion:

String result = Baggage.fromMap(Map.of("key", "value"))
  .storeInContext(Context.current())
  .wrap(() -> "result")
  .call();

You gave the feedback:

it's not clear if my current baggage items are added or replaced
I have to understand that baggage is stored in Context - which I actually don't have to be aware of
I have to know that wrap comes before call - it's not necessarily long, but harder to discover

I think we should extend BaggageBuilder to include a putAll method (as mentioned above). This would remove the ambiguity about when items are added or replaced.

To add, call Baggage.current().toBuilder().putAll(Map.of("key", "value"))
To replace (i.e. start with a clean slate), call Baggage.empty().toBuilder().putAll(Map.of("key", "value"))

I also think that we could extend Context with wrapAndRun and wrapAndCall methods (in addition to the existing wrap and call methods. This simplifies the call to something like:

String result = Baggage.current().toBuilder().putAll(Map.of("key", "value"))
  .storeInContext(Context.current())
  .wrapAndCall(() -> "result")

The user still has to know that baggage goes in context, but I actually think its better the user is forced to understand that up front.

So taken together, my recommendations are:

• Add putAll(Map<String, String>) to BaggageBuilder
• Add wrapAndRun(Runnable), T wrapAndCall(Callable<T>) to Context

If we add these, I think we can improve the ergonomics without needed to incubate anything at all.

[zeitlinger]

@jack-berg it was good that we split this out - this is more difficult to get right than I thought 😄

its better the user is forced to understand that up front.

Why is that better? In this simple use case, it can be transparent to the user.

However, I'm fine with teaching the user - if we can find a way that doesn't hurt autocompletion/explorability.

In particular, this is what I guess users will stumble across:

• what is the "current" baggage vs. "empty"
• what is a "context"
• what is the "current" context
• what is "wrapping"

[jack-berg]

How do you propose we proceed?

[zeitlinger]

I'll add it to the meeting agenda: https://docs.google.com/document/d/1WK9h4p55p8ZjPkxO75-ApI9m0wfea6ENZmMoFRvXSCw/edit#bookmark=id.ptrb4y8kwfc4

[trask]

linking to related: open-telemetry/opentelemetry-java-instrumentation#10403