opentracing-contrib / java-reactor

OpenTracing instrumentation for Reactor
Apache License 2.0
12 stars 7 forks source link

No active span with onError and onComplete #9

Closed NHebrard closed 4 years ago

NHebrard commented 4 years ago

Hello!

Is there any reason to not wrap TracedSubscriber.onError() and TracedSubscriber.onComplete() within the active span?

I would like to access the span in both cases in order to have a clean log correlation.

Happy to contribute if needed! Thanks.

jam01 commented 4 years ago

Hey @NHebrard yeah, I don't see a reason why we can't wrap those as well. If you can make that PR and some coverage I can merge it in.

NHebrard commented 4 years ago

Hello @jam01 Thank you for the quick feedback, I will take care of it during the upcoming week.

chengchen commented 4 years ago

@NHebrard Good catch for these 2 👍

Here are 2 test cases which can reproduce the bugs:

    @Test
    public void activeSpanShouldBeAccessibleInOnCompleteCallback() {
        MockSpan initSpan = tracer.buildSpan("foo").start();

        try (Scope ws = tracer.scopeManager().activate(initSpan)) {
            Flux.range(1, 5)
                .flatMap(i -> Mono.fromCallable(() -> i * 2).subscribeOn(elastic()))
                .doOnComplete(() -> assertNotNull(tracer.activeSpan()))
                .then()
                .block();
        } finally {
            initSpan.finish();
        }
    }

    @Test
    public void activeSpanShouldBeAccessibleInOnErrorCallback() {
        MockSpan initSpan = tracer.buildSpan("foo").start();

        try (Scope ws = tracer.scopeManager().activate(initSpan)) {
            Mono.error(RuntimeException::new)
                .subscribeOn(elastic())
                .doOnError(e -> assertNotNull(tracer.activeSpan()))
                .onErrorResume(RuntimeException.class, e -> Mono.just("fallback"))
                .block();
        } finally {
            initSpan.finish();
        }
    }
jam01 commented 4 years ago

Thanks @NHebrard @chengchen ! The changes should be public under v0.2.0. Let me know if they are not.