BraveScopeManager not thread safe?

[mpetazzoni]

I'm trying to pass a Span from one thread to another, as described in the README's "Deferring asynchronous work" section. Unfortunately, I think I'm running into a thread safety bug in BraveScopeManager, in particular in getOrEstablishActiveSpan().

In the following code:

// In thread A
try (Scope s1 = tracer.buildSpan("foo").startActive(false)) {
    final Span span = tracer.activeSpan();
    executor.submit(() -> {
        // In thread B
        try (Scope s2 = tracer.scopeManager().activate(span, true)) {
            logger.info("scope: {}, active scope: {}", s2, tracer.scopeManager().active());
        }
    });
}

The active scope seen in thread B is sometimes null (while I expect it to be s2). I believe this happens if thread B activates the span before thread A closes its scope s1, leading to s2 == s1 in thread B.

This of course does not happen if I simply create a child span in thread B with tracer.buildSpan("bar").asChildOf(span), because that gets a different span ID and therefore a different entry in the activeSpans map of the BraveScopeManager:

// In thread A
try (Scope s1 = tracer.buildSpan("foo").startActive(true)) {
    final Span span = tracer.activeSpan();
    executor.submit(() -> {
        // In thread B
        try (Scope s2 = tracer.buildSpan("bar").asChildOf(span).startActive(true)) {
            logger.info("scope: {}, active scope: {}", s2, tracer.scopeManager().active());
        }
    });
}

I'm not sure what the best practice is here, or even whether this is a bug or a feature of the BraveScopeManager. But as-is, I believe the example in the README isn't a reliable way to pass tracing context to the async routine.

(Thanks @isaachier for the help looking into this!)

[codefromthecrypt]

which readme are you referring to? Since there are no base test classes for us to inherit, I suppose making a test based on your example is one way to figure out what is going on. Regardless I'd expect that the executor would need to be instrumented to propagate the calling scope. In normal brave this is CurrentTraceContext.wrap(blah). Maybe you can ask @pavolloffay @CodingFabian or someone else from opentracing to clarify what this readme is saying and why it isn't wrapping the executor service (assuming it isn't)

[isaachier]

@adriancole I am pretty sure @mpetazzoni is referring to the OpenTracing Java project's README here.

[codefromthecrypt]

thanks.. I notice this at the bottom, which should be at the top as the README encourages something usually not done manually "In practice, all of this is most fluently accomplished through the use of an OpenTracing-aware ExecutorServiceand/or Runnable/Callable adapter; they factor out most of the typing." At any rate thanks, for the context on this.

[mpetazzoni]

@adriancole I was indeed talking about opentracing-java's README file, which I realize maybe doesn't mean much for Zipkin/Brave. But if brave-opentracing intends to make using OpenTracing instrumentation (and patterns) possible, it should correctly support its documented best practices.

BraveScopeManager returns an existing Scope instance from getOrEstablishActiveSpan(), which seems to be a different behavior than what CurrentTraceContext.newScope() would do (always returns a distinct Scope instance).

Feels to me like BraveScopeManager should be made a bit more thread-aware (since scopes are not supposed to be shared across threads anyway), and make BraveScopeManager.activeSpans a thread local, or something like that?

[codefromthecrypt]

I don't think for a second what they document is a best practice.. it is simply what they document. Anyway, I will look later to attempt to ensure documented malpractice works.

[codefromthecrypt]

I've opened opentracing/opentracing.io#258 to remove Zipkin from the supported tracer list as the people writing these READMEs (and the code in question) and promote opentracing do not help, and in lieu of that leave burden for others to clean it up

[codefromthecrypt]

#71 yanks the faulty implementation

[codefromthecrypt]

v0.30.0 on the way with this

[mpetazzoni]

Awesome, thanks! I'll check it out.