WIP: Adds listener method to register callback on context change

[HaloFour]

Prototype API for registering a callback which is triggered when the current Context for the current thread is changing.

Exposes the current ContextStorage from the method Context#storage.

Adds a new method ContextStorage#onAttach(Consumer<Context>) to register a callback interface which is invoked when the current context is going to be changed.

I've not touched Javadocs or tests yet, wanted to first get feedback on the API surface.

Example of usage:

ContextStorage.get().onAttach(toAttach -> {

    Span currentSpan = Span.fromContextOrNull(toAttach);
    if (currentSpan != null) {
        SpanContext spanContext = currentSpan.getSpanContext();
        MDC.put("trace_id", spanContext.getTraceIdAsHexString());
        MDC.put("span_id", spanContext.getSpanIdAsHexString());
    } else {
        MDC.remove("trace_id");
        MDC.remove("span_id");
    }
});

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ HaloFour (9e3c62f)

[codecov]

Codecov Report

Merging #1961 into master will decrease coverage by 0.10%.
The diff coverage is 54.54%.

@@             Coverage Diff              @@
##             master    #1961      +/-   ##
============================================
- Coverage     86.63%   86.52%   -0.11%     
  Complexity     1494     1494              
============================================
  Files           184      184              
  Lines          5655     5664       +9     
  Branches        584      586       +2     
============================================
+ Hits           4899     4901       +2     
- Misses          551      555       +4     
- Partials        205      208       +3     

Impacted Files	Coverage Δ	Complexity Δ
.../java/io/opentelemetry/context/ContextStorage.java	100.00% <ø> (ø)	1.00 <0.00> (ø)
...entelemetry/context/ThreadLocalContextStorage.java	76.66% <54.54%> (-13.81%)	8.00 <3.00> (+2.00)	⬇️
...rc/main/java/io/opentelemetry/context/Context.java	93.33% <0.00%> (-6.67%)	11.00% <0.00%> (-1.00%)
...k/extensions/trace/jaeger/sampler/RateLimiter.java	94.11% <0.00%> (-5.89%)	4.00% <0.00%> (-1.00%)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4295275...e0f5db7. Read the comment docs.

[HaloFour]

I removed this since it only sets the value on whichever thread happens to run the static constructor. Every other thread would remain uninitialized and will return null. Overriding initialValue() or calling the static withInitialValue method can provide a default value but the latter seemingly doesn't exist in Android and it's not really necessary since read access to the ThreadLocal is always through current() anyway.

[anuraaga]

I think I would take a approach that allows registering wrappers - as long as they are registered early i the app before storage is initialized, it allows customizing without thread sync issues.

public interface ContextStorage {
  static void wrap(Function<ContextStorage, ContextStorage> wrapper) {
    ContextStorageWrappers.add(wrapper);
  }
}

LazyStorage {
  ContextStorage contextStorage = createStorage();
  for (Function wrapper : ContextStorageWrappers.get()) {
    contextStorage = wrapper.apply(contextStorage);
  }
}

What do you think?

[HaloFour]

Registering decorators for the ContextStorage would be an interesting approach. How would you get to the ContextStorage to call wrap without triggering LazyStorage to create the storage, though?

Did you intend ContextStorage#wrap to be static instead of default?

[anuraaga]

LazyStorage will only be loaded when get is called so I think it should be ok :-D

Edit: yeah meant static thanks

[HaloFour]

Got it, that makes sense now. My only concern with this approach is that you only have that window of time during which you can register these decorators (which I get is intentional). Perhaps ContextStorage#wrap should warn or throw if the storage has already been created? Should there also be an SPI for ContextStorageWrappers?

[HaloFour]

Maybe it's sufficient to just make it easy to wrap ThreadLocalContextStorage via SPI.

The Javadocs on ContextStorage imply that you can access the instance of ThreadLocalContextStorage via Context#threadLocalStorage but that method doesn't exist and I don't see another way to access that instance. Without the ability to reference that instance rolling your own ContextStorage would involve rewriting thread-local storage which is where I think the opportunities for error mostly lie.

[anuraaga]

Exposing ThreadLocalContextStorage was definitely required (it was javadoc'd, just the method got lost during some refactoring 😅 ).

I think wrap can help where SPI is too tedious, or to allow wrapping the non-threadlocal storage, but if our current code satisfies your use case well, we can consider that later. I think we could add it now though if it helps at all (it was the very first ask from users when we implemented storage SPI in Armeria so I do expect it to happen eventually)

[HaloFour]

The SPI approach scratches my itch for now, and now that ThreadLocalContextStorage is exposed again it makes my implementation that much simpler. As for establishing an API here I think it worthwhile. I'd be curious what the other use cases might be.

[anuraaga]

Here's some reference of what led to the API in Armeria

line/armeria#2634

In particular it seems, from my interpretation, to be about having a similar API for hooking as reactor for users of both.

[anuraaga]

@HaloFour FYI sent out #2044

[HaloFour]

Looks great, thanks!

[HaloFour]

Closing in favor of #2044