Add static wrappers for Executor / ExecutorService using current cont…

[anuraaga]

…ext at invocation time.

Some asynchronous instrumentation requires registering an ExecutorService statically which propagates context from invocations to executions. For example, to properly trace OkHttpClient to support enqueue, we need to do something like this

client
  .dispatcher(new Dispatcher(Context.currentContextWrapping(new Dispatcher().executorService())))
  .addInterceptor(OkHttpTracing.create(openTelemetry).newInterceptor())

I thought of names like currentContextWrapping, wrapWithCurrentContext but don't know if I like either name so suggestions appreciated.

[codecov]

Codecov Report

Merging #2988 (cf91f1b) into main (938dd17) will increase coverage by 90.29%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##             main    #2988       +/-   ##
===========================================
+ Coverage        0   90.29%   +90.29%     
- Complexity      0     2791     +2791     
===========================================
  Files           0      323      +323     
  Lines           0     8677     +8677     
  Branches        0      875      +875     
===========================================
+ Hits            0     7835     +7835     
- Misses          0      579      +579     
- Partials        0      263      +263     

Impacted Files	Coverage Δ	Complexity Δ
...rc/main/java/io/opentelemetry/context/Context.java	100.00% <100.00%> (ø)	14.00 <2.00> (?)
.../opentelemetry/context/ContextExecutorService.java	100.00% <100.00%> (ø)	10.00 <9.00> (?)
...lemetry/context/CurrentContextExecutorService.java	100.00% <100.00%> (ø)	9.00 <9.00> (?)
...entelemetry/context/ForwardingExecutorService.java	100.00% <100.00%> (ø)	9.00 <9.00> (?)
...a/io/opentelemetry/api/baggage/ImmutableEntry.java	100.00% <0.00%> (ø)	2.00% <0.00%> (?%)
...lemetry/exporter/otlp/trace/ResourceMarshaler.java	100.00% <0.00%> (ø)	4.00% <0.00%> (?%)
...etry/sdk/metrics/aggregator/LongSumAggregator.java	100.00% <0.00%> (ø)	6.00% <0.00%> (?%)
...elemetry/sdk/trace/export/SimpleSpanProcessor.java	88.46% <0.00%> (ø)	11.00% <0.00%> (?%)
...in/java/io/opentelemetry/api/trace/TraceState.java	100.00% <0.00%> (ø)	2.00% <0.00%> (?%)
...elemetry/exporter/otlp/internal/CommonAdapter.java	97.14% <0.00%> (ø)	26.00% <0.00%> (?%)
... and 317 more

---

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 938dd17...5b615a2. Read the comment docs.

[jkwatson]

This seems like a reasonable addition to the API to me. @bogdandrutu any thoughts?

[kenfinnigan]

Looked over this without any knowledge of prior discussions, apologies if I'm missing context.

My initial thought is there's a lot of nearly identical code and whether there's another way to approach it. For instance, could ForwardingExectorService actually be a ContextWrappedExecutorService where Context is added to every method? Granted this creates lots of protected methods, which may not be ideal either. ContextExecutorService passes in the context from the instance, and CurrentContextExecutorService passes in Context.current() to each call?

An alternative to currentContextWrapping() could be executeWithCurrentContext() or executeInsideCurrentContext()?

[anuraaga]

For instance, could ForwardingExectorService actually be a ContextWrappedExecutorService where Context is added to every method?

I'm not sure I follow the suggestion but I think this may not work mostly because it's not the methods that need context, it's the callbacks that need to be wrapped. But if there are ways to reduce duplication in these internal classes it'd be great to follow up on it :-)

executeWithCurrentContext()

I think the problem I had with method names with current context on the right side is it sounded even more so that the context used is what's current when calling this method itself, not what's current when the b executor is invoked.

Another idea that came to mind which I like now is executingWithInvocationContext - it's clear at the expense of a new concept, invocation context, but that's needed to understand what's happening anyways I guess.

[kenfinnigan]

For instance, could ForwardingExectorService actually be a ContextWrappedExecutorService where Context is added to every method?

I'm not sure I follow the suggestion but I think this may not work mostly because it's not the methods that need context, it's the callbacks that need to be wrapped. But if there are ways to reduce duplication in these internal classes it'd be great to follow up on it :-)

I can certainly put together a PR with the idea once this is in place.

executeWithCurrentContext()

I think the problem I had with method names with current context on the right side is it sounded even more so that the context used is what's current when calling this method itself, not what's current when the b executor is invoked.

I think I'm misunderstanding something, as I thought what you describe is what it does. The current context is passed in such a way that it's active during the callback on the Executor.

Any help in understanding where I went wrong would be appreciated.

Another idea that came to mind which I like now is executingWithInvocationContext - it's clear at the expense of a new concept, invocation context, but that's needed to understand what's happening anyways I guess.

InvocationContext often refers to an object that was used to invoke some code, which doesn't appear to fit. It's also not obvious from the method name it's utilizing the current context.

[anuraaga]

I think I'm misunderstanding something, as I thought what you describe is what it does. The current context is passed in such a way that it's active during the callback on the Executor.

The context that should be used isn't the one that is current when executeWithCurrentContext is called - it should be one that is current when e.g., ExecutorService.execute, ExecutorService.submit are called. But this method name feels like it is talking about the current context when executeWithCurrentContext is called, at least to me.

[kenfinnigan]

Thanks @anuraaga, I'd missed that.

I agree that the names I've suggested don't really fit the delayed execution meaning. I think one of your original ideas works, wrapWithCurrentContext.

[bogdandrutu]

final :)

[bogdandrutu]

Naming is the hardest problem in eng, maybe consider wrapCurrent just in case we will add equivalent for wrapping with current Runnable/Callable and would be also consistent with current wrap helpers?

[anuraaga]

maybe consider wrapCurrent just in case we will add equivalent for wrapping with current Runnable/Callable and would be also consistent with current wrap helpers?

I don't think we'll be adding those since it's very easy to call Context.current().wrap but even if we did, I precisely don't want consistency with it :) This method uses the current context when calling ExecutorService.execute, not when calling this method so it's quite different.

[anuraaga]

@kenfinnigan Any thoughts on this other idea I just came up with, wrapTasks? We're wrapping tasks given to the executor with the current context.

[trask]

I'm good with wrapTasks, just throwing out a couple others as options:

• Context.propagating(Executor)
• Context.wrapping(Executor)
• Context.taskWrapping(Executor)

[anuraaga]

@trask Thanks, liked taskWrapping best so far.

[kenfinnigan]

Ooh, I really like taskWrapping

[anuraaga]

@bogdandrutu This look ok? Will go ahead and merge soon

[bogdandrutu]

Throwing more ideas:

Context.forwarding(Executor)
Context.forward(Executor)

I am also ok with taskWrapping

[anuraaga]

Sounds like taskWrapping has multiple votes so going to go with it. Thanks!