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

Make root span trace IDs unique #505

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

c24t
Copy link
Member

@c24t c24t commented Feb 15, 2019

This PR fixes a longstanding bug: that all spans generated by a given ContextTracer share the same trace ID regardless of the context. This meant sequential root spans were reported as siblings in the same trace instead of each belonging to its own trace.

Fixes #182 and #366. See #182 (comment) for more detail.

cc @tab1293, @sduskis.

@@ -116,6 +117,8 @@ def end_span(self, *args, **kwargs):
execution_context.set_current_span(cur_span.parent_span)
else:
execution_context.set_current_span(None)
self.span_context.trace_id = generate_trace_id()
self.trace_id = self.span_context.trace_id
Copy link
Member Author

Choose a reason for hiding this comment

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

tracer.trace_id is unused, and I don't know why it's included on the tracer. Removing it is a problem for another PR.

@sduskis
Copy link

sduskis commented Mar 6, 2019

@c24t, what's the story with this PR?

@nikhaldi
Copy link
Contributor

nikhaldi commented Mar 6, 2019

I'll mention here too that this PR does not fix #182 for me while my proposed change #539 does.

@c24t
Copy link
Member Author

c24t commented Mar 6, 2019

what's the story with this PR?

I hadn't merged it because it was breaking a system test that was asserting the wrong behavior. Still TODO.

this PR does not fix #182 for me while my proposed change #539 does

This PR deals with siblings spans having the same trace ID when the parent trace ID is null, #539 changes the grpc client interceptor so that it uses the current tracer to set trace IDs instead of the tracer that was current at the time the interceptor was created. Both problems affect the google-cloud-python integration, and both cause clashing traceIDs, but each has a different root cause.

@nikhaldi does that match your understanding?

@nikhaldi
Copy link
Contributor

nikhaldi commented Mar 7, 2019

I see what you mean, thanks. I don't have enough background about how people use the API to judge if the case of a null parent trace id is valid. My reading of the code tracking down my issues was that the API requires a new tracer to be created for every individual trace. But if that's not case this PR seems reasonable.

@c24t c24t requested review from aabmass, hectorhdzg, lzchen, songy23 and a team as code owners May 13, 2021 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

trace: clashing RPCs share the same traceID
5 participants