add isRegistered boolean to GlobalTracer #321
Conversation
- 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
|
@yurishkuro do you know where the ticket you're referencing is? I'm unfamiliar with it. |
| * </dependency> | ||
| * </code></pre> | ||
| */ | ||
| public class GlobalTracerTestUtil { |
There was a problem hiding this comment.
Removing this class breaks backward compatibility for folks using the test-jar dependency!
There was a problem hiding this comment.
I can revert this change, it's not essential to the fix. I should be able to update the isRegistered boolean in the same way the tracer is reset to a NoopTracer.
There was a problem hiding this comment.
Instead of updating two variables, have you considered a constant UNREGISTERED_TRACER and check with == if a tracer was registered?
|
@yurishkuro doesn't the synchronized keyword on |
|
@MikeGoldsmith GitHib mobile wasn't showing that part - looks good. |
| @@ -59,6 +59,11 @@ public static void setGlobalTracerUnconditionally(Tracer tracer) { | |||
| globalTracerField.setAccessible(true); | |||
| globalTracerField.set(null, tracer); | |||
| globalTracerField.setAccessible(false); | |||
|
|
|||
| Field isRegisteredField = GlobalTracer.class.getDeclaredField("isRegistered"); | |||
There was a problem hiding this comment.
This does something that is not according to the method name.
Setting unconditionally would intuitively leave isRegistered to true.
Resetting should clear it though.
There was a problem hiding this comment.
I agree with @sjoerdtalsma - IHMO resetting should be the only one touching isRegistered (an alternative would be to receive a isRegistered parameter, but it's probably not something needed at the moment, and thus could be added later if needed).
|
@sjoerdtalsma Sorry for the delay in addressing your comments. I've updated |
|
Hey @MikeGoldsmith Left a comment but overall this looks good ;) |
|
I think discarding the boolean (as it's just state) and replacing that with an I won't object this PR though. |
| public static synchronized boolean isRegistered() { | ||
| return !(GlobalTracer.tracer instanceof NoopTracer); | ||
| } | ||
| public static synchronized boolean isRegistered() { return isRegistered; } |
There was a problem hiding this comment.
Since isRegistered is volatile, we can remove the synchronized here.
Ported from C# fix where the bug was originally reported in
opentracing/opentracing-csharp#113