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

Enabling NOOP classes to work with pre-existing production code #263

Closed natehart closed 6 years ago

natehart commented 6 years ago

In special circumstances, code may automatically generate 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 methods are accessed via 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.

Signed-off-by: Nate Hart nhart@tableau.com

natehart commented 6 years ago

@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.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-8.1%) to 73.368% when pulling cc63126b772c81995b3919ea0e59801c7381ebdb on natehart:nhart/npe_fix into 806b0266a18c7bf036bcdf6a7b538bba22e1543e on opentracing:master.

natehart commented 6 years ago

@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.