-
Notifications
You must be signed in to change notification settings - Fork 445
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
Opentelemetry Baggage Propagation #3126
Conversation
Datadog ReportBranch report: ✅ 0 Failed, 5246 Passed, 73 Skipped, 2m 1.97s Total Time |
BenchmarksBenchmark execution time: 2025-02-13 19:45:19 Comparing candidate commit 3c6ea6d in PR branch Found 1 performance improvements and 6 performance regressions! Performance is the same for 55 metrics, 0 unstable metrics. scenario:BenchmarkHttpServeTrace-24
scenario:BenchmarkSetTagMetric-24
scenario:BenchmarkStartRequestSpan-24
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the right track, but the new code in StartRequestSpan is causing us to do tracer.Extract twice when we could just do it once. At the same place where we process span links after trace extract (line 127 on your branch, currently), we can also populate a baggagemap with the baggage items found from the returned spanParentCtx. Then, we can push those items onto the request context that we are going to propagate after line 141.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Approving this, given @mtoffl01 is going to be applied too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, but with the following notes:
- Update the PR title
- Update the PR description to specify what exactly is in scope in this PR. In short, you've added support for propagating baggage with HTTP headers in net/http client and all http integrations that use httptrace.go to generate their spans. More info in this little doc I put together: https://docs.google.com/document/d/1Te1L6QKzQpbtNMqe_KD3YXPTzcP-AZCSUJ44NlVNHII/edit?tab=t.0
/merge |
View all feedbacks in Devflow UI.
The median merge time in
mergequeue build completed successfully, but the github api returned an error while merging the pr.
DetailsError: PUT https://api.github.com/repos/DataDog/dd-trace-go/pulls/3126/merge: 405 5 of 5 required status checks are expected. [] FullStacktrace: |
What does this PR do?
Motivation
Reviewer's Checklist
v2-dev
branch and reviewed by @DataDog/apm-go.Unsure? Have a question? Request a review!