Skip to content
This repository has been archived by the owner on May 23, 2023. It is now read-only.

Add ScopeManager#activate(SpanContext) #319

Open
wants to merge 2 commits into
base: v0.32.0
Choose a base branch
from

Conversation

felixbarny
Copy link
Contributor

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 of Tracer.activeSpan(). The EarlySpanFinishTest demonstrates this.

It also simplifies tracer implementations as with this change tracers don't have to guard against racy calls to Span#finish() and ScopeManager#activate(Span). This is really painful to do if the tracer implementation relies on no methods being called on the span after finish() (for example when the tracer recycles the span object).

This addresses the problems laid out in #312

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
io.opentracing.contrib.concurrent.TracedRunnable)
instead of the user of Tracer.activeSpan().

It also simplifies tracer implementations as with this change tracers
don't have to guard against racy calls to Span#finish() and
ScopeManager#activate(Span).
@coveralls
Copy link

coveralls commented Oct 30, 2018

Coverage Status

Coverage decreased (-2.4%) to 74.245% when pulling ee709d0 on felixbarny:span-context-activation into b6f6324 on opentracing:v0.32.0.

@yurishkuro
Copy link
Member

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

@felixbarny
Copy link
Contributor Author

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.

Copy link
Contributor

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

returning null part is the only questionable bit. While I understand that you might not want to interact with finished spans, this puts a tracking burden on this function which is only needed when one creates bugs. I know users routinely use Span interface in opentracing as it lacks a SpanCustomizer which would prevent them from accidentally closing things, but I think this part is digging that hole deeper.

If the null part was removed, I would be happy with the change, as it ends up being mostly the same as Brave and we know for years and a lot of experience that this design works and is needed.

An alternative could be that implementations can be free to return a NoOp scope.. you'd never know anyway.

@felixbarny
Copy link
Contributor Author

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.

@carlosalberto
Copy link
Collaborator

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 ;) )

@felixbarny
Copy link
Contributor Author

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.

* @return a {@link Scope} instance to control the end of the active period for the {@link Span}. It is a
* programming error to neglect to call {@link Scope#close()} on the returned instance.
*/
Scope activate(SpanContext spanContext);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@felixbarny specifically what I mean here is that it is strange to have something that is meant for try/finally and returns null. Wouldn't you expect a NPE here because someone looks at something made for try/finally, but returns null?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Scope would always be non null. But the scope can hold either a Span or just a SpanContext.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my mistake. I misread this and I don't think others would misread it

* That means you must make sure that no other thread calls {@link Span#finish()}
* while the scope is still active.
* If you can't guarantee that, use {@link #activate(SpanContext)} instead.
*
* @param span the {@link Span} that should become the {@link #activeSpan()}
* @return a {@link Scope} instance to control the end of the active period for the {@link Span}. It is a
* programming error to neglect to call {@link Scope#close()} on the returned instance.
*/
Scope activate(Span span);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be nice to deprecate this, but I expect this to not be a popular suggestion

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should have a separate discussion about this

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be (initially) against it ;) And yeah, this can definitely happen on a different discussion.

Copy link
Contributor

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a couple asks, but positive overall

* One example of that is when performing an activation in the {@link Runnable#run()}
* method of a traced {@link Runnable} wrapper which is executed by an
* {@link java.util.concurrent.ExecutorService}.
*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add example in code? So far doesn't explicitly mention try/finally. I think this will reinforce it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added an example and copied over the try-with-resources paragraph

* Because both {@link #active()} and {@link #activeSpan()} reference the current
* active state, they both will be either null or non-null.
* Note that {@link #activeSpan()} can return {@code null} while {@link #active()} is non-null
* in case of a {@linkplain #activate(SpanContext) span context activation}.
*
* @return the {@link Span active span}, or null if none could be found.
*/
Span activeSpan();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similarly prefer this to be deprecated

/**
* @return the active {@link SpanContext}. This is a shorthand for {@code Tracer.scopeManager().activeSpanContext()}.
*/
SpanContext activeSpanContext();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

personally don't think these shortcuts should be here. just use the scope manager, and change docs of the activateSpan stuff to mention it internally uses the same. I don't know if Tracer.toSpan(SpanContext) has been done yet, but it would clear all of this up, possibly addressing @wu-sheng's gripe about not knowing when a span is made present on another thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the interest of scoping this pr (see what I did there?) I think it's better to move this discussion to a separate issue. I added this to be in line with the current conventions. I have not heard of Tracer.toSpan(SpanContext) yet. What are Wu's gripes you are referring to?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this topic is indeed separately addressable, though should be done! I may have misunderstood your SELF link issue as the same end functionality as Tracer.toSpan(SpanContext)

Copy link
Contributor

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approving as progress, but not if this is the last progress!

in other words, yes, do merge this. it is helpful, but don't stop fixing things as this in isolation of other work leaves more apis and others that should be cleaned up.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants