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

End span on cancellation of subscription to reactive publishers #3153

Merged
merged 12 commits into from
Jun 3, 2021

Conversation

HaloFour
Copy link
Contributor

@HaloFour HaloFour commented May 31, 2021

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.

@HaloFour HaloFour changed the title Stop span on cancellation of subscription to reactive publisher End span on cancellation of subscription to reactive publishers May 31, 2021
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.

thanks, and great tests as always!

@@ -57,6 +62,10 @@ public EndOnFirstNotificationConsumer(BaseTracer tracer, Context context) {
accept(null);
}

public void onCancel() {
accept(new CancellationException());
Copy link
Member

Choose a reason for hiding this comment

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

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

💯 👍

@HaloFour HaloFour force-pushed the withspan-reactive-cancellation branch from 4c9216d to 7a94c9b Compare June 1, 2021 15:59
Comment on lines +27 to +29
public static ReactorAsyncSpanEndStrategyBuilder newBuilder() {
return new ReactorAsyncSpanEndStrategyBuilder();
}
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

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.

thanks!

@trask trask merged commit 1402d11 into open-telemetry:main Jun 3, 2021
@HaloFour HaloFour deleted the withspan-reactive-cancellation branch June 3, 2021 02:32
robododge pushed a commit to robododge/opentelemetry-java-instrumentation that referenced this pull request Jun 17, 2021
…-telemetry#3153)

* Stop span on cancellation of subscription to reactive publisher

* Add semantic attribute on cancelation of reactive publisher

* Change TracingOperator and TracingAssembly to accept configuration from Javaagent
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.

3 participants