fix(otel-thread-ctx): put the threadlocal attributes at the right place in the context#2167
Conversation
yannham
force-pushed
the
yannham/fix-context-process-setup
branch
from
d266efc to
591a444
Compare
Clippy Allow Annotation ReportComparing clippy allow annotations between branches:
Summary by Rule
Annotation Counts by File
Annotation Stats by Crate
About This ReportThis report tracks Clippy allow annotations for specific rules, showing how they've changed in this PR. Decreasing the number of these annotations generally improves code quality. |
yannham
changed the title
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 957c8f3 | Docs | Datadog PR Page | Give us feedback! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d266efc1e2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
ivoanjo
left a comment
ivoanjo
left a comment
There was a problem hiding this comment.
👍 LGTM. CI is unhappy, but other than that + some very small suggestions, this looks fine! I completely missed that we were putting these attributes in the wrong place too 😅
yannham
force-pushed
the
yannham/fix-context-process-setup
branch
from
71a5fff to
9dccfc5
Compare
|
/merge |
|
View all feedbacks in Devflow UI.
This pull request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
The expected merge time in
The merge request has been interrupted because the build 682695468372423724 took longer than expected. The current limit for the base branch 'main' is 120 minutes. |
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: rejected
and removed
mergequeue-status: in_progress
labels
|
The merge queue timed out. I am not skilled in investigating MQ failures, and I was not able to figure out why. I merged in main in hopes that it will work this time. |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
gh-worker-dd-mergequeue-cf854d
Bot
deleted the
yannham/fix-context-process-setup
branch
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: done
and removed
mergequeue-status: in_progress
labels
#2167 moved the threadlocal.* attributes from Resource.attributes into ProcessContext.extra_attributes and renamed the test helper from find_attr to find_extra_attr. Our tests, added before that fix landed, still used the old name and lookup surface. Rename to match.
4 participants
What does this PR do?
This PR stores the
threadlocal*attributes in theattributesfield of the process context, instead of theresourcefield.Additionally, fix an unrelated issue (remove undue
#[cfg(..)]gate) that was introduced in the otel thread feature-gating, preventing normal tracer metadata from being published in the context when theotel-thread-ctxfeature is active.Motivation
After experimenting more with OTel thread context sharing, I found out the way we set up the process context is incorrect. The
threadlocal*attributes should go in theattributes/extra_attributespart of the process context, not theresourcepart. Tested that it now works with the context reader locally.Additional Notes
N/A
How to test the change?
I'm using this version in a prototype integration in dd-trace-rs, using the context reader. This is how I uncovered the issue initially. Now I'm seeing context properly read with this fix.