fix(gotracer): respect context_propagation for Go HTTP/gRPC header in…#2292
fix(gotracer): respect context_propagation for Go HTTP/gRPC header in…#2292mohd-ibadullah wants to merge 3 commits into
Conversation
|
|
16c5173 to
fd9bc6e
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2292 +/- ##
==========================================
+ Coverage 68.83% 69.49% +0.65%
==========================================
Files 308 317 +9
Lines 40127 41228 +1101
==========================================
+ Hits 27623 28653 +1030
+ Misses 10977 10973 -4
- Partials 1527 1602 +75
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes Go tracer (gotracer) context propagation behavior so that traceparent header injection (HTTP/1, HTTP/2, gRPC) is only enabled when the configured context_propagation mode includes headers, aligning behavior with tpinjector/generictracer and addressing #2276.
Changes:
- Add
headerPropagationEnabled()and use it to gateg_bpf_header_propagationand HTTP/HTTP2 propagation uprobe registration. - Align
g_bpf_traceparent_enabledwithgenerictracer(enabled only whenTrackRequestHeadersor any context propagation is enabled). - Add unit tests covering constants and (some) probe-registration expectations across propagation modes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/internal/ebpf/gotracer/gotracer.go | Gates Go header propagation constants/probes on both kernel capability and ContextPropagation.HasHeaders(), and aligns traceparent parsing enablement with other tracers. |
| pkg/internal/ebpf/gotracer/gotracer_test.go | Adds regression tests for propagation-related constants and for conditional registration of key HTTP/HTTP2 propagation probes. |
Comments suppressed due to low confidence (1)
pkg/internal/ebpf/gotracer/gotracer.go:834
- The new header-propagation gating in GoProbes only wraps the net/http and http2 WriteHeaders probes. Several gRPC header-propagation helper probes (e.g., google.golang.org/grpc/internal/transport.(*http2Client).NewStream return probe, (*controlBuffer).executeAndPut, (*loopyWriter).originateStream) are still registered unconditionally even though their eBPF programs early-return when g_bpf_header_propagation=false. To fully “register propagation probes only when header propagation is enabled” (per PR description) and to avoid unnecessary uprobe attachment overhead, consider moving those gRPC propagation-only probes under the same headerPropagationEnabled() condition (while keeping non-propagation gRPC tracing probes always-on).
if p.headerPropagationEnabled() {
m["net/http.Header.writeSubset"] = []*ebpfcommon.ProbeDesc{{
Start: p.bpfObjects.ObiUprobeWriteSubset, // http 1.x context propagation
}}
m["golang.org/x/net/http2.(*Framer).WriteHeaders"] = []*ebpfcommon.ProbeDesc{
{ // http2 context propagation
Start: p.bpfObjects.ObiUprobeGolangHttp2FramerWriteHeaders,
End: p.bpfObjects.ObiUprobeHttp2FramerWriteHeadersReturns,
},
{ // for grpc
Start: p.bpfObjects.ObiUprobeGrpcFramerWriteHeaders,
End: p.bpfObjects.ObiUprobeGrpcFramerWriteHeadersReturns,
},
}
m["net/http.(*http2Framer).WriteHeaders"] = []*ebpfcommon.ProbeDesc{{ // http2 context propagation
Start: p.bpfObjects.ObiUprobeNetHttp2FramerWriteHeaders,
End: p.bpfObjects.ObiUprobeHttp2FramerWriteHeadersReturns,
}}
}
|
looks like a lot of tests are failing, they probably need explicit config for headers context propagation |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
pkg/internal/ebpf/gotracer/gotracer.go:113
- The CAP_SYS_ADMIN/lockdown log in
LoadSpecs()is emitted even when header propagation is not requested (e.g.,context_propagation: disabledortcp). Since this message is specifically about Go library-level header injection support, it’s misleading/noisy unless header propagation is enabled in config. Consider gating the log behindContextPropagation.HasHeaders()so users don’t see propagation warnings when they’ve explicitly disabled it (or enabled tcp-only propagation).
func (p *Tracer) LoadSpecs() ([]*ebpfcommon.SpecBundle, error) {
if !p.supportsContextPropagation() {
p.log.Info("Kernel in lockdown mode or missing CAP_SYS_ADMIN.")
}
mmat11
left a comment
There was a problem hiding this comment.
needs linter fix and copilot comment looks valid, lgtm after that
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Summary
When
context_propagationis disabled, Go HTTP/gRPC clients still injecttraceparentheaders because the Go-specific propagation path only checks kernel capabilities and ignores the configured propagation mode.This change aligns
gotracerbehavior withtpinjectorandgenerictracerby requiring header propagation to be explicitly enabled before registering propagation probes or enabling header injection.Fixes #2276
Changes
headerPropagationEnabled()g_bpf_header_propagationon both kernel support andContextPropagation.HasHeaders()g_bpf_traceparent_enabledwithgenerictracerValidation