End span on cancellation of subscription to reactive publishers

[HaloFour]

Updates the AsyncSpanEndStrategy for Reactor, RxJava 2 and RxJava 3 so that the Span is ended if the subscription to the publisher is canceled by the consumer. Optionally a semantic attribute will also be added to the Span to indicate that the subscription was canceled.

[trask]

thanks, and great tests as always!

[trask]

The grpc instrumentation adds a "canceled" attribute instead, which seems like a good compromise between failure and success:

opentelemetry-java-instrumentation/instrumentation/grpc-1.6/library/src/main/java/io/opentelemetry/instrumentation/grpc/v1_6/TracingServerInterceptor.java

Lines 130 to 141 in f755b3f

	public void onCancel() {
	try {
	delegate().onCancel();
	if (captureExperimentalSpanAttributes) {
	Span.fromContext(context).setAttribute("grpc.canceled", true);
	}
	} catch (Throwable e) {
	instrumenter.end(context, request, null, e);
	throw e;
	}
	instrumenter.end(context, request, null, null);
	}

Unfortunately there isn't a semantic attribute for such a canceled attribute, so we'd need to go with something like:

• reactor.canceled
• rxjava.canceled

And unfortunately we hide non-semantic attributes behind "experimental" flags (search for captureExperimentalSpanAttributes and CAPTURE_EXPERIMENTAL_SPAN_ATTRIBUTES), which would probably mean not using singletons for the strategies anymore so that flag can be injected.

[HaloFour]

That approach makes sense and I can change the strategies to adopt that pattern. It does force a breaking change but it's probably better (and more future-proof) to adopt the same create/builder strategy anyway. I'll amend the PR.

[HaloFour]

Should we do the same with the Java 8 and Guava strategies? They do propagate cancellation through completing exceptionally but that is something we can detect and end the span successfully with a similar optional semantic attribute. It'd be an excuse to change the singleton implementations of those strategies to also follow the create/builder pattern. I could also submit that as a separate PR if we want to keep this one from getting too complicated.

[mateuszrzeszutek]

Should we do the same with the Java 8 and Guava strategies?

If it's possible -- sure, why not.

I could also submit that as a separate PR if we want to keep this one from getting too complicated.

💯 👍

[mateuszrzeszutek]

Hmm, do we need the builder for this class? Is it supposed to be possible to use it without TracingOperator?
If we can we can treat it as an internal part of the library instrumentation then we can make it package-private and remove the builder and use a simple ReactorAsyncSpanEndStrategy(boolean) constructor instead.

[HaloFour]

Hm, I had not considered them as being internal to the instrumentation. Would it be problematic to allow them to be used directly? They were already public even though they were singletons.

As of right now I am using the ReactorAsyncSpanEndStrategy without the TracingOperator in my Spring WebFlux projects. I've been slowly migrating away from our own instrumentation but I've not yet considered replacing our own Reactor operators, partially because they do more than propagate the OTel Context. I've combined several operations into a single operator because every individual operator effectively doubles the stack trace as they are added for every reactive operation.

[mateuszrzeszutek]

Hm, I had not considered them as being internal to the instrumentation. Would it be problematic to allow them to be used directly? They were already public even though they were singletons.

Hmm, it would be a bit better to expose less API surface, and they seemed like they could be internal. I wouldn't say that it's a hard requirement for them to be internal; I feel like they should be, but not have to. If you're using them right now then it's probably fine to keep them public for a while.

[trask]

thanks!