Enabling NOOP classes to work with pre-existing production code #263
Conversation
|
@yurishkuro I made a minor change to the MockTracer to work with the type assumptions baked into the tests. Not only does this allow the tests to propagate MockSpans as expected, but it brings the Propagator constructor closer into line with the no-argument constructor's default values. You may want to put eyes on it before I merge in case I changed a very intentional design decision. |
| @@ -59,7 +59,7 @@ public MockTracer(ScopeManager scopeManager, Propagator propagator) { | |||
| * Create a new MockTracer that passes through any calls to inject() and/or extract(). | |||
| */ | |||
| public MockTracer(Propagator propagator) { | |||
| this(NoopScopeManager.INSTANCE, propagator); | |||
| this(new ThreadLocalScopeManager(), propagator); | |||
There was a problem hiding this comment.
can this be done as another PR? It's completely unrelated.
There was a problem hiding this comment.
Sure, but we'll have to merge that one first.
The MockTracer assumes that ScopeManager.active() either returns null or a scope that with a MockSpan. Since we are now returning a NoopScope (and thus a NoopSpan) from the NoopScopeManager, it tries to cast the NoopSpan to a MockSpan and blows up. We won't be able to get the Mock tests to pass until we remove the NoopScopeManager from the Mocks.
natehart
force-pushed
the
nhart/npe_fix
branch
3 times, most recently
from
7da5d5e
to
35f6009
Compare
In special circumstances, code may be written to automatically create spans, negating the need to always check if Tracer.activeSpan() is null. Under these circumstances, the Noop Tracer and ScopeManager will cause NPEs when they return null values when their `active` values are accessed from the GlobalTracer. This prevents code from running successfully when using Noop Tracers. This minor change allows users to depend that their code will genuinely Noop under all use cases.
|
@yurishkuro I've done some digging into why code coverage is dropping so drastically, but I can't figure out where the problem is. I bet there is some null check in a test somewhere that was assuming a null return, but I'll be darned if I can figure out where it is. Unfortunately, I don't have any more time to spend on this. Merge it if you think it's worthwhile, otherwise you can close the PR. |
In special circumstances, code may automatically generate spans,
negating the need to always check if
Tracer.activeSpan()isnull. Under thesecircumstances, the Noop Tracer and ScopeManager will cause NPEs when they return
nullvalues when theiractivemethods are accessed via the GlobalTracer. Thisprevents code from running successfully when using Noop Tracers.
This minor change allows users to depend that their code will genuinely Noop
under all use cases.
Signed-off-by: Nate Hart nhart@tableau.com