BraveScopeManager throws if Span is not a BraveSpan

[dschulten]

BraveScopeManager.activate assumes that the given Span is a BraveSpan and wants to cast it, throwing otherwise. However, the Span interface can be wrapped by other wrappers, e.g. an ApiExtensionsSpan. Possibly stick to the Span interface in subsequent code or rewrap the incoming Span if it's not a BraveSpan.

[codefromthecrypt]

can you make a failing unit test and/or a fix PR? I'm not aware of the wrapping and how it generally affects propagation. please use the actual wrapping library.

…

On Sat, Feb 1, 2020, 12:45 AM Dietrich Schulten ***@***.***> wrote: BraveScopeManager.activate assumes that the given Span is a BraveSpan and wants to cast it, throwing otherwise. However, the Span interface can be wrapped by other wrappers, e.g. an ApiExtensionsSpan. Possibly stick to the Span interface in subsequent code or rewrap the incoming Span if it's not a BraveSpan. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#99?email_source=notifications&email_token=AAAPVV66XV5DZUAJECY4VPDRARIR7A5CNFSM4KOJ7OW2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4IKGDPXQ>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAPVV4SQYEUE4CB6BQOTPTRARIR7ANCNFSM4KOJ7OWQ> .

[dschulten]

I have looked into it and found that it is not as easy, I'll push a sample project.
Related: opentracing-contrib/java-api-extensions#16

[dschulten]

related: opentracing-contrib/java-api-extensions#16

[codefromthecrypt]

ok so the unit test would use this library I guess. can you do that? main thing is to strictly show why we are permitting this, and also when it can be stopped.

[splatch]

I've found an case which is fairly simple. I used opentracing-jdbc with zipkin bridge and it works fine as long as I do not modify jdbc tracing to skip datasource pool validation queries.
The error message I get comes from same check:
Span must be an instance of brave.opentracing.BraveSpan, but was class io.opentracing.noop.NoopSpanImpl

I believe that this is simpler to fix, as it seems to be an gap in coverage of bridging.

[codefromthecrypt]

opentracing is a spec, can you please at least raise an issue in their spec repo as it can't be a gap in bridging if it was never said that accepting other types was a requirement.

I think this specific "extension" randomly decided something. It is only fair for them to own that decision by formalizing it.