fix: emit context attributes regardless of active span#575
Conversation
Context attributes (e.g. llm.agent.phase) were silently dropped from ExecutionSample events when no span was active, because ProfiledThread was only fetched when hasContext was true. ProfiledThread::currentSignalSafe() is a pure TLS lookup — signal-safe, never allocates — and is safe to call unconditionally. Span IDs are still zeroed when no span is active (ContextApi::get fills them only on success). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
CI Test ResultsRun: #27125460476 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-06-08 08:46:33 UTC |
![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: 2454eeb24f
ℹ️ 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".
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
This PR updates the native JFR event writer to ensure thread-local context attributes are emitted for ExecutionSample events even when there is no active span, by decoupling attribute emission from span-context availability.
Changes:
- Adjust
Recording::writeCurrentContextto no longer conditionally fetchProfiledThreadbased onContextApi::get(...)’s boolean result. - Update the inline comment to describe the “no active span” case when
ContextApi::getdoes not populate span IDs.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| size_t numAttrs = Profiler::instance()->numContextAttributes(); | ||
| ProfiledThread* thrd = hasContext ? ProfiledThread::currentSignalSafe() : nullptr; | ||
| ProfiledThread* thrd = ProfiledThread::currentSignalSafe(); | ||
| for (size_t i = 0; i < numAttrs; i++) { | ||
| buf->putVar32(thrd != nullptr ? thrd->getOtelTagEncoding(i) : 0); | ||
| } |
| // spanId/rootSpanId are initialized to 0 above; ContextApi::get() only updates them | ||
| // on success, so 0s are always written when there is no valid context. | ||
| // on success, so 0s are always written when there is no active span. | ||
| ContextApi::get(spanId, rootSpanId); |
|
Invalid |
What does this PR do?:
Context attributes (e.g.
llm.agent.phase) were silently dropped fromExecutionSampleevents when no span was active. InRecording::writeCurrentContext,ProfiledThreadwas only fetched whenhasContextwas true — buthasContextonly reflects whether a span is active, not whether thread-local context attributes exist.Motivation:
ProfiledThread::currentSignalSafe()is a pure TLS lookup: signal-safe, never allocates, returnsnullptrif the key is not yet initialized. It is safe to call unconditionally. The existing null-check in the attributes loop (thrd != nullptr ? thrd->getOtelTagEncoding(i) : 0) already handles the no-thread case correctly.Span IDs continue to be written as zero when no span is active —
ContextApi::getonly fills them on success, which is unaffected by this change.Additional Notes:
The old variable
hasContextwas renamed away (its return value is now discarded) because the name was misleading: LLM phase and other thread-local attributes ARE context, sohasContext = falsewhile attributes are set is a contradiction.How to test the change?:
AgentPhaseProfilingTestinddprof-testcovers the case: a worker thread setsllm.agent.phasewithout any active span and verifies thatExecutionSampleevents carry the value.For Datadog employees:
credentials of any kind, I've requested a review from
@DataDog/security-design-and-guidance.