search

opentracing / opentracing-java

OpenTracing API for Java. 🛑 This library is DEPRECATED! https://github.com/opentracing/specification/issues/163
http://opentracing.io
Apache License 2.0
1.68k stars 344 forks source link

Should NoopSpanBuilder still extend NoopSpanContext? #255

Closed cwe1ss closed 6 years ago

cwe1ss commented 6 years ago

Hi! I'm working on the C# version of opentracing-noop and I've seen that NoopSpanBuilder extends NoopSpanContext.

Is there still any use for this or is this no longer necessary due to #55, #121 ?

carlosalberto commented 6 years ago

Hey @cwe1ss

It looks to me like #121 included a lot of changes after refactoring (19 files in total), and my bet is that NoopSpanBuilder was forgotten all along (I think this because in the PR/Issue there's no mention of NoopSpanBuilder).

I tried updating the code to remove that implements SpanContext + minor tuning, and everything works just fine, so I could finish the patch up and send it as a final fix piece.

Does it make sense to you guys? @yurishkuro @bhs (author of #121) You may remember it better ;)

bhs commented 6 years ago

@carlosalberto @cwe1ss sorry for the delay! I don't know why that inheritance relationship would still be necessary. If everything compiles without it, we should def remove. Thanks for asking.