Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add asynchronous tracing for Java 8 CompletableFuture in WithSpanAdvice #2530

Merged
merged 23 commits into from
Mar 22, 2021

Conversation

HaloFour
Copy link
Contributor

@HaloFour HaloFour commented Mar 8, 2021

Adds basic support for asynchronous tracing with the OTel annotation WithSpanAdvice. Only supports Java 8 CompletionStage<T> and CompletableFuture<T> but the MethodSpanStrategy interface should be able to support most/any promise-like return types.

@HaloFour HaloFour marked this pull request as ready for review March 8, 2021 18:27
Copy link
Contributor

@iNikem iNikem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely an interesting proposal :) But it needs some documentation. E.g. package-info.java in io.opentelemetry.instrumentation.api.tracer.async.

@HaloFour
Copy link
Contributor Author

HaloFour commented Mar 9, 2021

Thanks for the feedback! I hope "interesting" is a good thing. 😁

@HaloFour
Copy link
Contributor Author

HaloFour commented Mar 9, 2021

I will park this for feedback again.

I've moved some more of the machinery to instrumentation-api to a new abstract class derived from BaseTracer called BaseMethodTracer. This will likely be reused by WithSpanAdvice and MethodAdvice, but I didn't think it necessarily belonged in BaseTracer as it wouldn't apply to most instrumentation, only those designed specifically to intercept method invocation.

I changed the MethodSpanStrategies to be a singleton instance instead of static. This is kind of a placeholder class and I'm not sure what the best route would be to handle all of the reactive/future-like types that can be supported by a given framework. Right now it would require manually registering additional MethodSpanStrategy implementations in code for each reactive type in shared mutable state. I imagine a goal would be that when using the Java agent that it would know which types exist and to automatically register supporting strategies.

Names and other conventions are certainly up for discussion.

Thank you!

@anuraaga
Copy link
Contributor

Thanks @HaloFour! To be honest, I was only expecting a change in https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/instrumentation/opentelemetry-annotations-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/otelannotations because I forgot about the spring aspect :) It does seem like we'd want to make something generic to apply to both, but does it make sense to first create something non-generic in just otelannotations (our primary supported use case right now) and then make things generic from there after we understand the implementation fully?

@HaloFour
Copy link
Contributor Author

@anuraaga

Done and done, shifted all of the potentially sharable bits over to the otelannotations instrumentation module. I targeted the Spring version as the projects I've been working on are Spring WebFlux.

I also see a MethodAdvice instrumentation which uses ByteBuddy but without using the WithSpan annotation? Surprised there isn't an AspectJ flavor also. I can certainly understand any hesitation about having this much specialized code in instrumentation-api. Perhaps it could live in a separate instrumentation-aop module or the like if this is accepted and we want to share the machinery across the different instrumentation modules?

@HaloFour HaloFour changed the title Add asynchronous tracing for Java 8 CompletableFuture in Spring WithSpanAspect Add asynchronous tracing for Java 8 CompletableFuture in WithSpanAdvice Mar 10, 2021
private boolean endSynchronously(
CompletableFuture<?> future, BaseTracer tracer, Context context) {

if (future.isDone()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: you can return early instead:

if (!future.isDone()) {
  return false;
}

@mateuszrzeszutek
Copy link
Member

LGTM 👍

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😎

Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for delay, just a small comment but this mostly looks ready

*/
private CompletionStage<?> endWhenComplete(
CompletionStage<?> stage, BaseTracer tracer, Context context) {
return stage.whenComplete(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the stage is already complete I think this is guaranteed to be synchronous - I guess we can remove endSynchronously?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I double checked to be sure and yes, whenComplete should always be synchronous, at least given how it's implemented in CompletableFuture<T>. Checking and completing synchronously was more about optimizing away the need for the callback and the extra allocations that requires. It's not observable, but it is cheaper/faster. But if that's not worth the complexity I can remove it.

@trask
Copy link
Member

trask commented Mar 21, 2021

I sent a PR to your PR with a couple of suggestions HaloFour#1

@trask trask merged commit 4168c0b into open-telemetry:main Mar 22, 2021
@trask
Copy link
Member

trask commented Mar 22, 2021

thanks for the new feature @HaloFour 🎉

@HaloFour
Copy link
Contributor Author

HaloFour commented Mar 22, 2021

Excellent news! Glad I could contribute.

From here what might the plan be to extend this to other aspect-based instrumentation, especially WithSpanAspect/WithSpanAspectTracer? The actual implementation within the instrumentation classes seems simple enough but the strategy approach could be shared, if that were to live in instrumentation-api or a new module.

Then there is expanding the list of strategies to other asynchronous types, like Reactor, RxJava, Guava, etc.

I'd love to be involved with both but I can understand wanting to let this "soak in" first.

@HaloFour HaloFour deleted the withspan-spring-async branch March 22, 2021 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants