WithSpan aspect and asynchronous/reactive programming

[HaloFour]

I am working on a Spring WebFlux project that has adopted OpenTelemetry. Quite some time back we developed our own version of the aspect that creates spans around methods annotated with @WithSpan, but we extended it in a number of ways to make it work in a reactive application. Specifically the aspect is capable of recognizing that the method returns a value representing an asynchronous operation and leaves the span open until that operation completes. I've got the verbal approval of my employer to look into contributing our code and I'd like to gauge interest in doing so either as modifications to the existing aspect or as something entirely new.

[HaloFour]

Here are the two major bits of functionality that the aspect mentioned above brings to the table. These features are totally orthogonal and can be considered independently from one another.

Asynchronous Tracing

When the aspect first intercepts a call to an annotated method it will inspect the return type of that method to determine if it is asynchronous. The application can register any number of types and offer a strategy for how to handle tracing those methods, so the approach can be flexible over different frameworks (like Reactor, RxJava, Guava, Java 8, Scala, etc.). That strategy is responsible for handling the return value of the method and composing any notification required to end the span after the method has returned.

@WithSpan
public Mono<String> someUpstreamRequest() {
    return client.get()
            .uri("http://www.example.com")
            .retrieve()
            .bodyToMono(String.class);
}

When the above method is traced the span is kept open until the Mono<T> that is returned from the method signals that it is complete which will include the duration of the HTTP request.

Parameters as Attributes

Parameters of an annotated method can also be annotated so that when the method is called the values of those parameters are added to the span as attributes.

@WithSpan
public String someMethod(@Trace.Attribute("foo") String foo, @Trace.Attribute("bar") int bar) {
    return "";
}

[anuraaga]

Thanks for the idea @HaloFour! Updating WithSpan to support asynchronous return types seems quite cool (and challenging :D). Mapping params to attributes also seems cool. These would be nice additions.

[HaloFour]

Cool beans, final approval just came through so I'll look to open a PR when time permits.

[jkwatson]

I'm not convinced of the value of mapping spans to attributes. It seems easier and more java-idiomatic to just let people write some java code, rather than adding annotation to parameters, which have a tendency to make code nearly unreadable (in my opinion).

[anuraaga]

I've seen the pattern used so much in REST frameworks, e.g., @Param, that it seems fairly idiomatic to me. Or similarly, a user could always inject the request object and call request.getParam instead of using those annotations, but I think people do use them and like them. Armeria added a whole annotated service, something I mostly never use for some of the same reasons you mention, but people seem to love it.

So giving an annotation option seems fine to me and shouldn't be too complex.

[anuraaga]

By the way this reminded me that when proposing new annotations, we need to make sure to involve both SDK maintainers and instrumentation maintainers since it's a cross cutting concern (can we use baggage?). Just had not come to mind at all initially but will do so going forward.

[HaloFour]

I've created the following draft PR:

#2090

It's very raw, it's basically a dump of the source we are currently using with our own aspect implementation. Rather than trying to refactor it to fit into the existing project structure I thought I'd post what we have for the sake of discussion and figure out how to best fit the parts we want to adopt into the project.

[HaloFour]

In other news does anyone know how I can change the CLA associated with a PR? I expected to have to go through the EasyCLA check and I need to use my work email address but the check automatically passed, probably because I've submitted a PR to another repo under the open-telemetry project in the past.

[HaloFour]

Sounds like this isn't a terribly big deal on my side so I don't think we need to worry it.

[Camsteack]

Hi @HaloFour ! Just wanted to know if there was any news on that feature ? That would be really useful for us as all of our services are reactive and mostly using reactors. Being able to use @WithSpan to trace our service would be great !

[HaloFour]

Hey @Camsteack ! Unfortunately not much news to report. I did dump the Spring aspect I'm using in my projects in the draft PR above but it looks like it will require some effort to make that fit with the MethodTracer approach used in the instrumentation libraries and I haven't had a lot of time for extracurricular activities lately :(

[HaloFour]

I've put up a new draft PR which implements basic async span support for only the WithSpanAspectTracer under Spring autoconfigure so that the approach can be explored without trying to apply it to the core instrumentation modules.

#2530

It only handles Java 8 CompletionStage<T> and CompletableFuture<T> but through a MethodSpanStrategy which hides that detail from the WithSpanAspectTracer.

[HaloFour]

Awesome, now that the machinery is in place I'd like to discuss adding more implementations of AsyncSpanEndStrategy to support other libraries with async facilities, e.g.:

• Reactor (Flux<T>, Mono<T>, Publisher<T>)
• RxJava (1, 2, 3) (Completable, Single<T>, Observable<T>, Flowable<T>, Publisher<T>)
• Guava (ListenableFuture<T>)
• Spring (it's own flavor of ListenableFuture<T>, AsyncResult<T>)
• JDK 9 (Publisher<T>)
• Scala (Future[T])
• Kotlin (Flow<T>)

My questions would be:

1. Where should such implementations live? Presumably alongside any instrumentation APIs for those frameworks. Or they could be compileOnly dependencies of instrumentation-api, although that feels a bit excessive.
2. What mechanism, if any, should be used to discover these implementations? Right now you have to manually register them via AsyncSpanEndStrategies#registerStrategy but IMO if you're using instrumentation for a specific framework then it feels like this should happen automatically. Should this be driven by SPI or discovered by attempting to resolve class names on the path or left as a manual configuration?

/cc @trask

[mateuszrzeszutek]

They could live in separate library instrumentations (likerxjava-2.0:library for rxjava2), enabling instrumentation would also register the span end strategy.

Usually we have javaagent instrumentations enable library instrumentations, so all those strategies would get registered automatically.

For example, for rxjava2 if you just added AsyncSpanEndStrategies.registerStrategy(rxjava2Strategy) somewhere in the TracingAssembly#enable() it'd automagically be picked up - and @WithSpan instrumentation would be able to handle rxjava2 return types (since agent's instrumentation-api is in the bootstrap classloader and can hold shared state)

[HaloFour]

Awesome, thanks!

[bruto1]

current Reactor instrumentation in OT has a shortcoming it inherited from opentracing-contrib (span associated with publisher is created too late for propagation to the upstream publisher in some cases)
Found out about that firsthand and fixed, then offered the fix as a contribution: https://github.com/opentracing-contrib/java-reactor/pull/12/files
There's a test that demonstrates the benefit of this approach

Feel free to copy the bits you like

[trask]

hey @bruto1! if you are seeing an issue with the OpenTelemetry instrumentation, can you open an issue and provide an example or repro? we are definitely interested in this topic