Add bool to manage whether a tracer has been registered in GlobalTracer #114
Conversation
- add s_isRegistered bool - set s_isregistered when Registering a tracer - add unit test to verify registering a NoopTracer set the s_isRegistered bool - move ResetTracer logic to GlobalTracer as internal function instead of test utility for easier reusability
src/OpenTracing/Util/GlobalTracer.cs
Outdated
| @@ -82,12 +98,16 @@ public static void Register(ITracer tracer) | |||
| lock (s_lock) | |||
| { | |||
| if (tracer == s_instance._tracer) | |||
| { | |||
| _isRegistered = true; | |||
There was a problem hiding this comment.
Any reason why this is required? The s_instance._tracer & _isRegistered assignation below works atomically, no?
There was a problem hiding this comment.
We need this because the default tracer is a NoopTracer, so if you intentionally register a NoopTracer we need to set the s_isRegistered bool to true too.
There was a problem hiding this comment.
Right, right. Had forgotten this detail ;)
|
Overall nice, I like (with the only doubt mentioned in the code ;) ) My personal advice would be to not merge it till we have it working/approved in Java as well. @MikeGoldsmith if you are low on cycles, I can cook the PR for Java - else, feel free to cook it as well :) |
|
Thanks @carlosalberto and @austinlparker - I agree Java should be approved before merging to ensure we're consistent. So we also plan changes in the other implementations to track when a tracer has been registered and prevent subsequent calls? ie you said Python doesn't do that check and I don't think Go does either |
|
@MikeGoldsmith So IIRC it was discussed that languages that may likely have a collision initializing the global tracer (from different threads at startup time) should have this check. Java and C# fall into this category - and for Python this is not often the case, so it was decided we wouldn't do this until truly required (the same applies to Javascript?). I think Java + C# is a good start for now, in any case ;) |
|
After seeing the case of keeping the Tracer reset logic in the GlobalTracerTestUtil in Java, I've re-added it back here too. |
|
@austinlparker @carlosalberto - not sure why AppVeyor is failing with unit tests. They all successfully pass for me locally. |
|
From looking at the test code, it seems like there could be a race condition since there's a bunch of async stuff going on. I'm not sure if it's a great test the way it's written. |
|
I think I need different repo permissions to be able to re-run the appveyor job, lemme look into it. |
|
@austinlparker had any luck with getting AppVeyor permissions to re-run the CI? |
|
Ah, sorry, completely slipped my mind. Let me check today or I'll bring it up tomorrow at the OTSC call. |
add s_isRegistered bool
set s_isregistered when Registering a tracer
add unit test to verify registering a NoopTracer set the s_isRegistered bool
move ResetTracer logic to GlobalTracer as internal function instead of test utility for easier reusability
Addresses #113