search

openzipkin-contrib / brave-opentracing

Bridge between OpenTracing and Brave
http://opentracing.io
Apache License 2.0
64 stars 39 forks source link

BraveScopeManager throws if Span is not a BraveSpan #99

Open dschulten opened 4 years ago

dschulten commented 4 years ago

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 commented 4 years ago

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 notifications@github.com 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 https://github.com/openzipkin-contrib/brave-opentracing/issues/99?email_source=notifications&email_token=AAAPVV66XV5DZUAJECY4VPDRARIR7A5CNFSM4KOJ7OW2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4IKGDPXQ, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAPVV4SQYEUE4CB6BQOTPTRARIR7ANCNFSM4KOJ7OWQ .

dschulten commented 4 years ago

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

dschulten commented 4 years ago

related: https://github.com/opentracing-contrib/java-api-extensions/issues/16

codefromthecrypt commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.