Port AsyncSpanEndStrategy to Instrumenter API #3262
Conversation
mateuszrzeszutek
requested review from
anuraaga,
iNikem,
jkwatson,
laurit,
pavolloffay,
trask and
tylerbenson
as code owners
...src/main/java/io/opentelemetry/instrumentation/api/instrumenter/async/AsyncInstrumenter.java
Outdated
Show resolved
Hide resolved
...rc/main/java/io/opentelemetry/instrumentation/api/instrumenter/async/AsyncEndStrategies.java
Outdated
Show resolved
Hide resolved
|
I think it looks good. As a minor refactor of the async span strategies would you want me to follow this up with a refactor of the implementations of those strategies in the other instrumentation modules? |
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package io.opentelemetry.instrumentation.api.instrumenter.async; |
There was a problem hiding this comment.
Does this need to be in instrumentation API? It seems to only really be needed by one instrumentation, the annotations Instrumentation, isn't it?
There was a problem hiding this comment.
This would replace the AsyncSpanEndStrategy pattern already in instrumentation-api and implemented by tracers in both opentelemetry-annotations-1.0 and spring-boot-autoconfigure modules, as well as with strategy implementations in rxjava2, rxjava3, reactor and guava for different asynchronous types.
I did suggest at that time that maybe this was a little too specialized for instrumentation-api and maybe it would make sense to have a separate shared module for AOP concerns?
There was a problem hiding this comment.
Yep, we need to keep it in some shared place.
I did suggest at that time that maybe this was a little too specialized for instrumentation-api and maybe it would make sense to have a separate shared module for AOP concerns?
Hmm, I wonder about that - I fear that introducing yet another module may only increase the cognitive load of implementing an instrumentation.
@anuraaga WDYT about marking the whole package as @UnstableApi?
There was a problem hiding this comment.
Cognitive load is precisely why we might want to find some split for it. I think async return value instrumentation is far less common than others so having a package in our instrumentation API could be too discoverable for our generally expected user. Just because a server framework is async, returning CompletableFuture, doesn't mean it needs to care about this package but someone might think so just by the word async, I would have I think.
Marking as UnstableApi seems like a good idea, can we maybe name the package annotationsasyncsupport or something similar to tie it closer to the instrumentation?
There was a problem hiding this comment.
Hmm, I wonder about that - I fear that introducing yet another module may only increase the cognitive load of implementing an instrumentation.
I guess it depends on what instrumentation we expect to rely on these classes. If it's just the AOP instrumentation (opentelemetry-annotations-1.0 and spring-boot-autoconfigure) as well as the different reactive frameworks (guava,rxjava2, rxjava3 and reactor, with maybe a couple more being added over time), I don't think it's too problematic to spit it into a separate module as it's use will be fairly specialized.
However, if it's expected that instrumentation for various reactive client libraries (e.g. lettuce, spring-webflux-5.0) might also want to reuse the strategies, then it probably makes more sense for them to live in instrumentation-api. That said I don't think the strategies are geared towards that use, especially now that some of them are configuration driven and there's an expectation that they are registered with the reactive framework they represent. Although nothing would stop that instrumentation module from creating its own instance of the strategy rather than bouncing through AsyncEndStrategies.
There was a problem hiding this comment.
having a package in our instrumentation API could be too discoverable for our generally expected user. Just because a server framework is async, returning CompletableFuture, doesn't mean it needs to care about this package but someone might think so just by the word async, I would have I think.
That's very true 👍 💯
we maybe name the package annotationsasyncsupport or something similar to tie it closer to the instrumentation?
Done.
You've completely convinced me about the split (it'd make instrumentation-api much cleaner!), now I'm starting to think if we have any other scenarios like this one that can benefit from it 😄
| * A wrapper over {@link Instrumenter} that is able to defer {@link Instrumenter#end(Context, | ||
| * Object, Object, Throwable)} until asynchronous computation finishes. | ||
| * | ||
| * <p>This class is not able to extract {@code RESPONSE} from the asynchronous computation value it |
There was a problem hiding this comment.
I don't quite follow - why is this? Is it because some return types don't have one response (e.g., a reactive stream that isn't a mono)? Can we at least resolve it for the mono-like cases (especially CompletableFuture)?
There was a problem hiding this comment.
Is it because some return types don't have one response (e.g., a reactive stream that isn't a mono)?
That was one reason; the other "reason" (well, "excuse" would be more correct 😅 ) was that all those strategies pretty much operate on raw/unchecked types, before passing the response to the Instrumenter I'd have to check if it's an instance of RESPONSE type - I'd need to pass Class<RESPONSE> somewhere to do that, and I wanted to keep those strategies as simple as possible.
I'll work on that; multi-result reactive types should be the only thing that's not passed to the instrumenter.
There was a problem hiding this comment.
Having gone down the Java generics and reflection rabbit hole recently this sounds like it would be very tricky to really get right. Would the strategy itself be responsible for trying to negotiate the actual type parameters of the returned generic type?
There was a problem hiding this comment.
I thought to make it as simple as possible, just use a Class<RESPONSE> instance to check type and cast, without delving into Java generics weirdness. All Instrumenters in this project that would use the async support would use Void as the response type, so we'd never actually need to pass anything there - at least in this repo.
There was a problem hiding this comment.
Maybe I'm not understanding. Is the goal to be able to derive that RESPONSE is String and to pass a Class<String> to the instrumentation if the annotated method returns CompletableFuture<String> or Mono<String> or the like? Or is this for non-AOP instrumentation?
There was a problem hiding this comment.
I went with the simplest possible approach:
public static <REQUEST, RESPONSE> AsyncOperationEndSupport<REQUEST, RESPONSE> create(
Instrumenter<REQUEST, RESPONSE> syncInstrumenter,
Class<RESPONSE> responseType,
Class<?> asyncType)You have to manually pass the expected response type. Extracting it from the passed Instrumenter is impossible, it will never have type parameters in the runtime.
...io/opentelemetry/instrumentation/api/asyncannotationsupport/AsyncOperationEndStrategies.java
Outdated
Show resolved
Hide resolved
mateuszrzeszutek
force-pushed
the
async-instrumenter
branch
from
4e8996a
to
d7758b6
Compare
Oh, sorry @HaloFour , I haven't noticed your comment before, it got overshadowed by all the other ones 🙇 If you only find some time for that, yes please! That'd be really helpful. |
Can do, should I start a branch off of this one or would it be better to wait until this is approved/merged? |
This PR ports #2530 to Instrumenter API - we need this to be able to port
opentelemetry-annotationsandspring-boot-autoconfigureto Instrumenter API.CC @HaloFour (I can't add you as a reviewer for some reason)