-
Notifications
You must be signed in to change notification settings - Fork 375
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
Add active_root_span to Tracer #503
Conversation
@palazzem Are we happy with this implementation? We could alternatively implement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
I think we need to change the criteria to include |
lib/ddtrace/context.rb
Outdated
@@ -77,6 +83,7 @@ def add_span(span) | |||
return | |||
end | |||
set_current_span(span) | |||
@current_root_span = span if @trace.empty? && span.parent_id.zero? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
span.parent_id
is not 0
for distributed traces. In that case, people should tag the local root span (not the global one) so we should always return the root span for this context (and not for the global distributed trace).
Consider that to make it work with Trace Search & Analytics, any span can be tagged, so this "find the root span" is only a helper to retrieve the start of the request, start of the task.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really? You'd like the "local root span"? Even if it isn't the root span for the trace?
I thought this would have been considered a bug, since you might end up with any span on the trace (depending on how many services the distributed trace is composed of), which makes the function a bit arbitrary.
If that's what you want, I can revert these changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes the helper should help finding the root span for that local trace. It's not needed for Trace Search & Analytics, but it's very helpful in most of the cases (i.e. retrieving anywhere the Rack span). It's not meant to find the "global" root span of a distributed trace, just to help people in finding root spans created by our OOTB instrumentation.
since you might end up with any span on the trace
But it will be always the first root span created in this trace / local trace, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@palazzem Yes, in the sense that:
Span 1 --> Span 2 ---[Service call]---> Span 3 --> Span 4 --> #active_root_span
Will yield Span 3
. So if that's what you'd like, all I have to do is revert those changes. I had considered that a bug, and made it return nil
with my most recent change.
Overview
To aid the traversal of active spans (for example to tag a trace with tags), this pull request introduces the
active_root_span
function to the Tracer, which works like theactive_span
function.Usage