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

Detect trace conflicts in Kprobe context propagation #557

Merged
merged 7 commits into from
Jan 18, 2024

Conversation

grcevski
Copy link
Contributor

@grcevski grcevski commented Jan 17, 2024

This fix detects conflicts on traces for runtimes that use single threaded queued dispatches, e.g. NodeJS. In these situations there's one worker thread, which uses a queue to pick up work and do all of the communication. Our current kprobes support for matching server -> client spans uses the thread id as an identifier, which ends up being wrong in this scenario.

This fix essentially checks if there's an existing active server span for the same thread it that hasn't finished and if there's one, it marks it as invalid. The client spans pull the server span info, if the span is invalid, they ignore it and generate new trace id. This effectively break the server -> client chain, but at least it doesn't make the last server span a parent of all client spans.

I think we'll be able to full implement support for NodeJS by finding appropriate uprobe we can attach in the node runtime, which will allow us perhaps to find a different identifier for the server span, than the thread id. For now NodeJS will not be able to propagate context.

@codecov-commenter
Copy link

codecov-commenter commented Jan 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1e8f70c) 79.36% compared to head (7b942cd) 45.16%.

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #557       +/-   ##
===========================================
- Coverage   79.36%   45.16%   -34.20%     
===========================================
  Files          69       67        -2     
  Lines        5830     5652      -178     
===========================================
- Hits         4627     2553     -2074     
- Misses        982     2950     +1968     
+ Partials      221      149       -72     
Flag Coverage Δ
integration-test ?
unittests 45.16% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@grcevski grcevski changed the title Do not merge: Kprobe traces detect conflicts Detect trace conflicts in Kprobe context propagation Jan 18, 2024
@grcevski grcevski marked this pull request as ready for review January 18, 2024 16:33
@grcevski grcevski requested a review from mariomac as a code owner January 18, 2024 16:33
@grcevski grcevski merged commit 72a0ed9 into grafana:main Jan 18, 2024
4 checks passed
@grcevski grcevski deleted the kprobe_traces_detect_conflicts branch January 18, 2024 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants