Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always return NoopScope / NoopSpan for ScopeManager.Active / Tracer.ActiveSpan #84

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cwe1ss
Copy link
Member

@cwe1ss cwe1ss commented Mar 30, 2018

This brings opentracing/opentracing-java#263 to this repository.

Description from there:

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.

We have a separate NoopTests class in our repo so I had to adjust these tests. I also changed everything to AssertSame(Noop*.Instance, ...) instead of Assert.IsType<Noop*>(...) as this is a better check for our use case IMO.

As I changed these tests as well, I'll keep this PR open for a few days.

@cwe1ss cwe1ss added this to the 0.12.0 milestone Mar 30, 2018
@cwe1ss
Copy link
Member Author

cwe1ss commented Apr 26, 2018

Please file any objections until May 2nd - I'll merge afterwards if there's none.

@cwe1ss
Copy link
Member Author

cwe1ss commented Apr 26, 2018

Found my own concerns 😄 and filed them here opentracing/specification#116. I'll put this on hold until that issue is resolved.

@cwe1ss cwe1ss mentioned this pull request May 19, 2018
@cwe1ss cwe1ss modified the milestones: 0.12.0, 0.13.0 May 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants