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

Different handling of traceparent #455

Merged
merged 19 commits into from
Nov 22, 2023
Merged

Different handling of traceparent #455

merged 19 commits into from
Nov 22, 2023

Conversation

grcevski
Copy link
Contributor

@grcevski grcevski commented Nov 20, 2023

This PR sets the stage for implementing distributed tracing, by changing how we handle the incoming traceparent header field. We process the field on the BPF side and generate the traceID, spanID there so we can let the database match our calls. With this PR the matching of parent/child traces in traces.go is removed.

TODO in this PR:

  • Unit tests for the new id generator
  • Check if we need more integration tests for server to client calls.

TODO in follow up PRs:

  • Detect if the kernel is in integrity mode
  • Add support for passing the trace parent on client calls for Go

@grcevski grcevski added the do-not-merge WIP or something that musn't be merged label Nov 20, 2023
@codecov-commenter
Copy link

codecov-commenter commented Nov 21, 2023

Codecov Report

Attention: 51 lines in your changes are missing coverage. Please review.

Comparison is base (633773d) 43.80% compared to head (14c276a) 42.64%.
Report is 11 commits behind head on main.

Files Patch % Lines
pkg/internal/ebpf/tracer_linux.go 0.00% 21 Missing ⚠️
pkg/internal/ebpf/httpfltr/httpfltr.go 0.00% 8 Missing ⚠️
pkg/internal/export/otel/id_generator.go 86.79% 5 Missing and 2 partials ⚠️
pkg/internal/export/debug/debug.go 0.00% 6 Missing ⚠️
pkg/internal/export/otel/traces.go 80.00% 2 Missing and 2 partials ⚠️
pkg/internal/ebpf/common/spanner.go 50.00% 3 Missing ⚠️
pkg/internal/discover/watcher.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #455      +/-   ##
==========================================
- Coverage   43.80%   42.64%   -1.16%     
==========================================
  Files          59       60       +1     
  Lines        4917     4877      -40     
==========================================
- Hits         2154     2080      -74     
- Misses       2629     2663      +34     
  Partials      134      134              
Flag Coverage Δ
unittests 42.64% <57.50%> (-1.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 Draft: Different handling of traceparent Different handling of traceparent Nov 22, 2023
@grcevski grcevski marked this pull request as ready for review November 22, 2023 02:56
@grcevski grcevski requested a review from mariomac as a code owner November 22, 2023 02:56
@grcevski grcevski removed the do-not-merge WIP or something that musn't be merged label Nov 22, 2023
@grcevski
Copy link
Contributor Author

I think I need to make some additional documentation around how this works now.

Copy link
Contributor

@mariomac mariomac left a comment

Choose a reason for hiding this comment

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

Mostly LGTM!! My only concern is about backwards compatibility.

Comment on lines 125 to 133
kernelMajor, kernelMinor := ebpf2.KernelVersion()
if kernelMajor > 5 || (kernelMajor == 5 && kernelMinor >= 17) {
p.log.Info("Found Linux kernel later than 5.17, enabling trace information parsing", "major", kernelMajor, "minor", kernelMinor)
loader = loadBpf_tp
if p.cfg.EBPF.BpfDebug {
loader = loadBpf_tp_debug
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

Copy link
Contributor

Choose a reason for hiding this comment

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

Will this mean that 1.0 users getting Traceparent with kernel versions < 5.17 will stop getting it?

Should we enclose all this code in an if config.TrackRequestHeaders { ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good point. I'm not sure what to do there, there's no chance we'll ever be able to implement propagation of traces with userspace parsing of the traceparent, and passing along all of the headers caused a lot of overhead.

I'll implement this suggestion with the config option, it helps us test both paths in Github without having to use VMs.

One other option is to implement an alternative bpf_strstr with a regular unrolled loop. It won't be able to go for long, but we might have partial support for traceparent if the header is seen in the first 100 bytes or so.

@grcevski grcevski merged commit 760d3ad into grafana:main Nov 22, 2023
3 checks passed
@grcevski grcevski deleted the dist branch November 22, 2023 15:40
@grcevski
Copy link
Contributor Author

I'll follow-up with a PR to restore the userspace parsing of the tracebuffers for kprobes for older kernels. That way we don't break anything, or maybe I can figure out another way that's less CPU intensive.

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