Open felixbarny opened 5 years ago
@felixbarny I understand the reasons why you want this, but I'm really troubled by the exponential increase of mental complexity of using such an API. Every time you want to use active span now you have to check for null. It makes life much harder for everyone who's using the API correctly already, ie not accessing the span after finish.
It makes life much harder for everyone who's using the API correctly already, ie not accessing the span after finish.
Not really because ScopeManager#active()
would only return null
when you don't own the lifecycle of the span. Which means that you are not allowed to call any methods on the returned span instance anyway. So when using the API correctly, you don't have to introduce any new null
checks.
this puts a tracking burden on this function which is only needed when one creates bugs.
What do you mean with tracking burden? You mean tracking when a span is finished and then return null
thereafter? This is precisely the thing I want to avoid having to do with this change.
This API addition forces instrumentation authors to think about whether the activation should allow code in scope to have access to the active span or if it has only relevance for setting parent-child relationships. They should choose the latter one in case the scope of the activation may outlive the corresponding span. That guards users against accidentally interacting with an already finished span.
An alternative could be that implementations can be free to return a NoOp scope
Not sure I follow. Do you mean a noop span? With this change, you would still have a scope but it might only hold a SpanContext and no Span.
I totally agree with @yurishkuro opinion on the complexity part, and also think that as @adriancole says, having a no-op active Span
(and related Scope
) could help.
(I think we had already talked briefly in the past about this possibility, so it's probably a good time to resurrect it ;) )
I totally agree with @yurishkuro opinion on the complexity part
Maybe there is something I'm missing but as explained, there is no additional complexity in terms of null
checks as when the using the API correctly. Because when you use the API correctly, you may only get the active span when you are allowed to, that is if there is no chance for the span to already be finished. In that case, there are no new null checks.
Whether Tracer.activeSpan()
and ScopeManager.active()
should return a noop span rather than null
is another discussion.
Similar to
ScopeManager#activate(Span)
but used in cases where the thread in which the activation is performed does not have control over the life cycle of the span.This puts the burden of being aware of the lifecycle to the one writing the instrumentation (for example
TracedRunnable
) instead of the user ofTracer.activeSpan()
. TheEarlySpanFinishTest
demonstrates this.It also simplifies tracer implementations as with this change tracers don't have to guard against racy calls to
Span#finish()
andScopeManager#activate(Span)
. This is really painful to do if the tracer implementation relies on no methods being called on the span afterfinish()
(for example when the tracer recycles the span object).This addresses the problems laid out in https://github.com/opentracing/opentracing-java/issues/312