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

Add active_root_span to Tracer #503

Merged
merged 6 commits into from
Aug 7, 2018

Conversation

delner
Copy link
Contributor

@delner delner commented Jul 31, 2018

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 the active_span function.

Usage

# e.g. adding tag to active root span
current_root_span = Datadog.tracer.active_root_span
current_root_span.set_tag('my_tag', 'my_value') unless current_root_span.nil?

@delner delner added core Involves Datadog core libraries feature Involves a product feature labels Jul 31, 2018
@delner delner self-assigned this Jul 31, 2018
@delner delner requested a review from palazzem July 31, 2018 00:23
@delner
Copy link
Contributor Author

delner commented Jul 31, 2018

@palazzem Are we happy with this implementation? We could alternatively implement current_root_span by just using the first span on @trace, or change the criteria of if @trace.empty? to if span.parent.nil?, which might yield better results. I didn't do that in this first pass because of how context flushing might cause side effects with this logic. Let me know what you think.

@delner delner added this to the 0.14.0 milestone Jul 31, 2018
Copy link
Contributor

@pawelchcki pawelchcki left a comment

Choose a reason for hiding this comment

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

👍

@delner
Copy link
Contributor Author

delner commented Aug 6, 2018

I think we need to change the criteria to include if span.parent.nil? because otherwise we might get some funny behavior with distributed traces: the second service in the distributed trace would return some non-parent span as the root, when it probably should return nil.

@@ -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?
Copy link
Contributor

@palazzem palazzem Aug 7, 2018

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

@delner delner Aug 7, 2018

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.

@delner delner dismissed palazzem’s stale review August 7, 2018 19:01

Changes made as requested.

@delner delner merged commit c4995e1 into 0.14-dev Aug 7, 2018
@delner delner deleted the feature/add_active_root_span_to_tracer branch August 7, 2018 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Involves Datadog core libraries feature Involves a product feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants