Scope Manager: Should it return Noop-types for Scope.active / Tracer.activeSpan?

[cwe1ss]

Java has recently merged opentracing/opentracing-java#263 (and the same change is pending in C# (opentracing/opentracing-csharp#84)) which changes the behavior of the NoopTracer: It will now always return NoopScope.INSTANCE and NoopSpanImpl.INSTANCE instead of null values.

However, if people rely on these methods to never return null then this will violate the current "Scope Manager" RFC (see quote below) and it will ultimately lead to null pointer exceptions when people switch from a NoopTracer to a real tracer.

active, the Scope containing the current active Span if any, or else null/nothing.

Did I misinterpret this risk? If not, should we revert this change in Java or should we change the RFC to state that ALL scope managers should return Noop-types for Scope.active and Tracer.activeSpan?

/cc @carlosalberto (author of the RFC)
/cc @natehart @yurishkuro (people who contributed to the Java PR)
/cc @MikeGoldsmith (people wo contributed to the C# PR)

PS: @carlosalberto the RFC currently doesn't mention the convenience accessor Tracer.activeSpan - should it be added to the RFC?

[yurishkuro]

I think it was a bug in Java NoopTracer, which has been fixed. The Scope Manager concept is somewhat orthogonal to the Tracer (esp. #114), it should be possible to use a real thread-local based scope manager with a Noop tracer.

[cwe1ss]

@yurishkuro I think it was a bug in Java NoopTracer, which has been fixed.

Yes and no. It probably was a bug that scopeManager.active() returned null after a call to scopeManager.activate(). But now active() also returns a scope even if activate() was never called. And this is a behavior that "real" scope managers do not share.

[yurishkuro]

@cwe1ss I think there should be two ways of using NoopTracer, (a) with a normal ScopeManager (thread-local), and (b) with a lightweight NoopScopeManager. opentracing/opentracing-java#263 implements the option (b), and only that option. I think is should also allow option (a). Specifically, this line

https://github.com/opentracing/opentracing-java/pull/263/files#diff-b9123283a173da56a32c0717b632bb76R35

is incorrect, it should delegate to scopeManager.active() instead of defaulting to noop span instance.

[pavolloffay]

There was also this PR opentracing/opentracing-java#167 related to this topic.

Switch from "real" tracer to noop shouldn't fail on NPE.

[carlosalberto]

the RFC currently doesn't mention the convenience accessor Tracer.activeSpan - should it be added to the RFC?

I think this is more of sugar on top of the API, so I think this is not important to include in the specification itself.

[carlosalberto]

IHMO, the issue/PR @pavolloffay would be a nice thing to have, and would also address the point @yurishkuro made.

Regarding the specification: I think we need to change/update it to reflect this (and add any clarification as needed). Good thing we are still in the draft stage of it ;)

[indrekj]

// about opentracing-ruby

Returning NOOP_INSTANCE may get especially confusing with https://github.com/opentracing/specification/blob/master/rfc/trace_identifiers.md#specification-changes . E.g. a logging library may include something like

if tracer.active_span
  log_metadata[:trace_id] = tracer.active_span.context.trace_id
end

which doesn't really make sense if you have tracing disabled / working with noop instance. I'd rather not add a dummy trace_id to the logs if I'm dealing with a noop instance.