add isRegistered boolean to GlobalTracer

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

add isRegistered boolean to GlobalTracer #321

Closed MikeGoldsmith closed 5 years ago

MikeGoldsmith commented 5 years ago

add isRegistered boolean
set isRegistered when Registering a tracer
add unit test to verify registering a NoopTracer set the isRegistered bool
move resetTracer logic to GlobalTracer as private function instead of test utility for easier reusability

Ported from C# fix where the bug was originally reported in opentracing/opentracing-csharp#113

coveralls commented 5 years ago

Coverage increased (+0.1%) to 75.258% when pulling 4784d0931cd04d0b03e08138458d0858b5e4882b on MikeGoldsmith:global-tracer-bool into 76d7b8df4e26e6444a9a7ea19f700cb380a03434 on opentracing:master.

austinlparker commented 5 years ago

@yurishkuro do you know where the ticket you're referencing is? I'm unfamiliar with it.

MikeGoldsmith commented 5 years ago

@yurishkuro doesn't the synchronized keyword on registerIfAbsent ensure both the tracer and boolean are updated in a thread-safe way?

yurishkuro commented 5 years ago

@MikeGoldsmith GitHib mobile wasn't showing that part - looks good.

MikeGoldsmith commented 5 years ago

@sjoerdtalsma Sorry for the delay in addressing your comments.

I've updated resetGlobalTracer and setGlobalTracerUnconditionally to correctly manage the isRegistered property.

carlosalberto commented 5 years ago

Hey @MikeGoldsmith

Left a comment but overall this looks good ;)

sjoerdtalsma commented 5 years ago

I think discarding the boolean (as it's just state) and replacing that with an == comparison with a unique no-op constant tracer gets you what you want (ability to set the NoopTracer) and reduces the complexity of your solution.

I won't object this PR though.