[PROF-14883] Publish thread context attribute keys at process start#576
[PROF-14883] Publish thread context attribute keys at process start#576ivoanjo wants to merge 7 commits into
Conversation
**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.
![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: #27134891685 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-06-08 12:05:17 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.
|
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.