opentracing-contrib / java-spring-web

OpenTracing Spring Web instrumentation
Apache License 2.0
107 stars 59 forks source link

Span not finished when subscription is cancelled #133

Closed NHebrard closed 4 years ago

NHebrard commented 4 years ago

Hello!

In the TracingOperator component is there any good reason the not finish the span when the wrapped source has been canceled?

I would change

class TracingOperator extends MonoOperator<Void, Void> {

    @Override
    public void subscribe(final CoreSubscriber<? super Void> subscriber) {
        // ...
        try (final Scope scope = tracer.scopeManager().activate(span)) {
            exchange.getAttributes().put(TracingWebFilter.SERVER_SPAN_CONTEXT, span.context());
            source.subscribe(new TracingSubscriber(subscriber, exchange, context, span, spanDecorators));
        }
    }

To

class TracingOperator extends MonoOperator<Void, Void> {

    @Override
    public void subscribe(final CoreSubscriber<? super Void> subscriber) {
        // ...
        try (final Scope scope = tracer.scopeManager().activate(span)) {
            exchange.getAttributes().put(TracingWebFilter.SERVER_SPAN_CONTEXT, span.context());
            source.doOnCancel(() -> span.finish())
               .subscribe(new TracingSubscriber(subscriber, exchange, context, span, spanDecorators));
        }
    }

Happy to contribute if needed!

Thanks.

geoand commented 4 years ago

Hi,

that seems reasonable to me

NHebrard commented 4 years ago

@geoand Would you mind triggering a release?

geoand commented 4 years ago

I actually want the dust to setttle regarding the previous release, so why don't you ping me again please towards the end of the week?

NHebrard commented 4 years ago

This bug fix is somehow important for my team as we would like to have a complete trace visualization in the Jaeger UI. But, we can wait a few days more as the number of impacted spans is low.

Thanks!

geoand commented 4 years ago

Cool :)

NHebrard commented 4 years ago

Hello, Sorry to bother you again. Can we plan a release if there is no issue on your side?

Thanks!

geoand commented 4 years ago

Thanks for reminding me! Just launched it

NHebrard commented 4 years ago

Thanks for the new release!