[PROF-14883] Publish thread context attribute keys at process start#576
Conversation
![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: 5c28a04d41
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
CI Test ResultsRun: #27430935426 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-06-12 17:27:01 UTC |
| @@ -93,91 +92,24 @@ public void testNativeReadBackFunctionality() { | |||
| String tracerVersion = "3.5.0"; | |||
There was a problem hiding this comment.
🟡 MEDIUM · completeness [SPECIALIST]
testStartAttributesAutoRegistersKeys — the only test asserting that start,attributes=... auto-publishes the OTEP attribute_key_map without a separate setProcessContext call — is deleted with no replacement. The behaviour change in profiler.cpp is now untested; a regression would go undetected by the surviving test suite.
Suggestion: Add a test asserting the intended new behaviour of the attributes=... start path — either explicitly verifying the key map is absent (if the two-step contract is now required), or providing an equivalent end-to-end test that covers the same scenario.
Matched lens: Tests missing or dropped for new code.
There was a problem hiding this comment.
I don't agree with the AI -- it's suggesting a negative test (Y doesn't happen when X) and I don't see the value of such a negative tests. (E.g. I also don't assert that "free nachos every friday" doesn't happen when X happens -- negative tests are weird)
There was a problem hiding this comment.
This is related to #576 (comment) - if the context attributes are no longer autpublished and one needs to call setProcessContext to actually publish them, then the test, as described here, is not needed.
But - since setProcessContext is now setting the OTEL process context as well as making sure the custom attribute names are actually published, I wonder if it should not be renamed to something better reflecting the span of operations it does.
There was a problem hiding this comment.
Hmmm, that's a good point. Perhaps setProcessAndThreadContext? Any suggestions?
There was a problem hiding this comment.
Maybe initializeAllContext() and add a small javadoc mention of the custom attributes setup being done as part of this? Also, that one has to call this method to make sure the custom attributes are setup properly.
There was a problem hiding this comment.
Renamed and added big comment in 79cc3cc; also updated the downstream DataDog/dd-trace-java#11558 to match.
|
|
/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
|
**What does this PR do?** This PR unifies the `registerAttributeKeys` mechanism used for supporting the "OTel thread context" with `setProcessContext` so that they get both published together at process start. **Motivation:** Beyond the code simplification (most of this PR is deletes), the big advantage of this approach is that `registerAttributeKeys` happened at profiler start, and now this is all moved to process start. This was meh because profiler start is by default delayed (afaik up to 70 seconds in practice) which means that the "thread context" information would be missing for the same period of time, which was super confusing, and would mean an outside reader would be missing this data for that period. **Additional Notes:** This PR will pair with one on the dd-trace-java side to provide the needed info when setting the process context. **How to test the change?** This change includes test coverage + on the dd-trace-java coverage we'll add a few tests too.
…it's needed + why it's important
ivoanjo
force-pushed
the
ivoanjo/prof-14883-early-thread-context
branch
from
5df97f0 to
75be380
Compare
|
I did a trivial rebase on top of main + force push; to get over the "commits were not signed" above (I had authored this in a workspace, need to check what's up there with missing signatures) |
gh-worker-dd-mergequeue-cf854d
Bot
merged commit cbceb7a
into
main
gh-worker-dd-mergequeue-cf854d
Bot
deleted the
ivoanjo/prof-14883-early-thread-context
branch
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: done
and removed
mergequeue-status: in_progress
labels
2 participants
What does this PR do?
This PR unifies the
registerAttributeKeysmechanism used for supporting the "OTel thread context" withsetProcessContextso that they get both published together at process start.Motivation:
Beyond the code simplification (most of this PR is deletes), the big advantage of this approach is that
registerAttributeKeyshappened at profiler start, and now this is all moved to process start.This was meh because profiler start is by default delayed (afaik up to 70 seconds in practice) which means that the "thread context" information would be missing for the same period of time, which was super confusing, and would mean an outside reader would be missing this data for that period.
Additional Notes:
This PR will pair with one on the dd-trace-java side to provide the needed info when setting the process context.
How to test the change?
This change includes test coverage + on the dd-trace-java coverage we'll add a few tests too.
For Datadog employees:
credentials of any kind, I've requested a review from
@DataDog/security-design-and-guidance.