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

Store span ids in profiling events #1043

Merged
merged 4 commits into from
May 14, 2020
Merged

Conversation

jd
Copy link
Contributor

@jd jd commented Sep 3, 2019

feat(tracer): add a on_start_span hook

This allows to register a function to be called when a new span is being
started by a Tracer.

chore(black): blackify ddtrace/settings

feat(profiling/stack): collect span ids for running thread and stack frames

@jd jd requested a review from a team as a code owner September 3, 2019 08:09
ddtrace/tracer.py Outdated Show resolved Hide resolved

span2 = self.span2
del self.span2
assert self.tracer.thread_to_span[th.ident] == span2
Copy link
Member

Choose a reason for hiding this comment

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

If we call span2.finish(), th.ident should be removed from self.tracer.thread_to_span correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not necessarily; it'll be removed as soon there's no ref to span2 and once the GC has run.

@brettlangdon brettlangdon added this to the 0.30.0 milestone Sep 3, 2019
@majorgreys majorgreys modified the milestones: 0.30.0, 0.31.0 Oct 11, 2019
@majorgreys majorgreys modified the milestones: 0.31.0, 0.32.0 Oct 31, 2019
@majorgreys majorgreys modified the milestones: 0.32.0, 0.33.0 Dec 19, 2019
@majorgreys majorgreys modified the milestones: 0.33.0, 0.34.0 Jan 22, 2020
@majorgreys majorgreys removed this from the 0.34.0 milestone Feb 19, 2020
@jd jd marked this pull request as draft April 29, 2020 08:33
@jd jd requested a review from majorgreys April 29, 2020 08:33
Copy link
Collaborator

@majorgreys majorgreys left a comment

Choose a reason for hiding this comment

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

The approach for linking threads and spans looks good so far!

Comment on lines 36 to 43
for span in set(spans):
if not span.finished:
try:
spans.remove(span._parent)
except KeyError:
pass

return {span for span in spans if not span.finished}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Reuse the other function and use set difference.

Suggested change
for span in set(spans):
if not span.finished:
try:
spans.remove(span._parent)
except KeyError:
pass
return {span for span in spans if not span.finished}
active_spans = self.get_active_spans_from_thread_id(thread_id)
return active_spans - {span._parent for span in active_spans}

@jd jd changed the title tracer: store per-thread last created span tracer: add start span hook May 4, 2020
@jd jd changed the title tracer: add start span hook Store span ids in profiling events May 6, 2020
@jd jd marked this pull request as ready for review May 6, 2020 11:39
@jd jd force-pushed the threads-to-trace-id branch 2 times, most recently from 782d4ee to 0deb7ca Compare May 6, 2020 12:39
@Kyle-Verhoog
Copy link
Member

Could we move the hooks changes to a separate PR? Not super necessary, just a tad cleaner 😄

Copy link
Member

@Kyle-Verhoog Kyle-Verhoog left a comment

Choose a reason for hiding this comment

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

Looks good! Did a first pass 🙂.

Didn't review the hook changes yet, I will do so here if you'd like to keep those changes in this PR.

ddtrace/profiling/collector/stack.pyx Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
ddtrace/profiling/collector/stack.pyx Show resolved Hide resolved
ddtrace/profiling/collector/stack.pyx Outdated Show resolved Hide resolved
ddtrace/profiling/collector/stack.pyx Show resolved Hide resolved
ddtrace/profiling/collector/stack.pyx Outdated Show resolved Hide resolved
ddtrace/profiling/profiler.py Show resolved Hide resolved
ddtrace/profiling/collector/stack.pyx Show resolved Hide resolved
ddtrace/profiling/collector/stack.pyx Show resolved Hide resolved
@jd
Copy link
Contributor Author

jd commented May 12, 2020

The hooks changed have being pulled out in #1428

@jd jd force-pushed the threads-to-trace-id branch 2 times, most recently from 6b19054 to 9144b60 Compare May 13, 2020 07:56
jd added 3 commits May 13, 2020 15:05
This allows to register a function to be called when a new span is being
started by a Tracer.
Copy link
Member

@Kyle-Verhoog Kyle-Verhoog left a comment

Choose a reason for hiding this comment

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

Just one last comment but I think otherwise it's good to go!

ddtrace/profiling/collector/stack.pyx Show resolved Hide resolved
Copy link
Member

@Kyle-Verhoog Kyle-Verhoog left a comment

Choose a reason for hiding this comment

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

🥳 this is a super cool feature!

@jd jd merged commit b4f7013 into DataDog:master May 14, 2020
@jd jd deleted the threads-to-trace-id branch May 14, 2020 09:12
@Kyle-Verhoog Kyle-Verhoog added this to the 0.38.0 milestone May 18, 2020
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.

None yet

4 participants