feat(context): all-native OTEL context write API — Phase 1 (PROF-15271)#631
Merged
Conversation
Captures the UAF problem, options A-E, the JMH benchmark campaign and its governing principle, the combined-API design, the consumer audit, and the expand -> migrate -> contract migration plan. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add native setTraceContext0/clearTraceContext0 (combined per-scope activate/deactivate) and setContextValue0/clearContextValue0 (single attribute), plus attrs_data compaction and LRS helpers, in javaApi.cpp, with matching private native declarations on JavaProfiler. Each resolves the current carrier's OtelThreadContextRecord via ProfiledThread::current(). A JNI native frame pins a mounted virtual thread to its carrier for the duration of the call, so the resolved record is guaranteed live and cannot migrate mid-write -- there is no cached per-thread DirectByteBuffer to dangle. Writes follow the same detach -> mutate -> attach valid-flag protocol as ThreadContext, keeping the async-signal-handler sampler (ContextApi::get) coherent. Native layer only; the public Java API and value cache built on top follow in a later commit. The DirectByteBuffer path is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-15271) Add the public all-native context API on JavaProfiler (provisional names, pending dd-trace-java coordination): setTraceContext/clearTraceContext (combined per-scope activate/deactivate) and setContextValue/clearContextValue (single attribute), each backed by the native primitives. Relocate the value->(encoding,utf8) memoization out of the per-thread ThreadContext into a new process-wide ContextValueCache. Because the encoding is a process-global immutable ID (registerConstant0), a shared lock-free cache is correct: immutable entries published via a single AtomicReferenceArray slot store (no torn reads), a miss re-registers idempotently, and collisions self-heal. This avoids duplicating the table across every carrier/virtual thread and needs no CarrierThreadLocal. registerConstant0 is made package-private for reuse (relocates in phase 3). Coexists with the DirectByteBuffer path (both write the same native record); the DBB path is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…5271) Mark the DBB write/read surface `@Deprecated` pointing at the all-native API: JavaProfiler.setContext/clearContext/setContextAttribute/clearContextAttribute/ setContextAttributesByIdAndBytes/copyTags/getThreadContext, and the DBB-path helper classes ThreadContext, ScopeStack, and ContextSetter. Non-breaking (annotations only) -- signals the migration for dd-trace-java (phase 2) ahead of removal (phase 3). Java has no -Werror, so the deprecation notes are informational; the DBB path still functions unchanged during coexistence. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…PROF-15271) setTraceContext0/clearTraceContext0 now write the fixed LRS entry header (key_index=0, length=16) in addition to its hex value, instead of relying on the ThreadContext constructor to have initialized it. This makes the all-native path correct on a record that only saw the ProfiledThread zero-init -- i.e. when no DirectByteBuffer/ThreadContext was ever created (the phase-2 pure-native case), where attrs_data[1] would otherwise be 0 and mis-frame the LRS entry. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
AllNativeContextTest exercises setTraceContext/clearTraceContext/setContextValue/ clearContextValue: round-trip readback, equivalence with the DirectByteBuffer path, clear semantics, span-transition attribute reset, single-attribute set/clear, attrs_data overflow, rejected (null/oversized) values, DBB<->native coexistence on one thread, and coherence of native writes from many mounted virtual threads (the migration-safe path). Read-back uses the ThreadContext DBB read methods as an oracle -- a view over the same native record the all-native path writes. All 9 pass on JDK 21 (debug); the existing context suite (Otel/Carrier/Tag/ProcessContext) is unregressed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ROF-15271) Point the design note appendix at branch context-storage-benchmark-experiment (pushed to origin), which preserves the full JMH campaign + native prototypes for reproducibility, rather than describing scaffolding that lives only on that branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…le (PROF-15271) ContextCombinedBenchmark compares the production per-scope activate+deactivate cycle: the combined native calls (setTraceContext + clearTraceContext) vs the deprecated DirectByteBuffer sequence as dd-trace-java actually drives it (profiler.setContext + 2x setContextAttribute + clear...), on platform + mounted virtual threads, both storage modes. Measured (JDK 21, debug, carrier): native ~136 ns vs DBB ~189 ns -- native ~28% faster, mode-independent, and a win on vthreads too (~147 vs ~193 ns). The DBB baseline here is the faithful production cost: each of its 6 calls pays a currentContext()/ CarrierThreadLocal lookup, which the 2-call combined native cycle avoids. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
CI Test ResultsRun: #29021930572 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-07-09 13:56:02 UTC |
Fixes check-javadoc "no comment" warnings for setTraceContext, setContextValue, and clearContextValue introduced in this PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
- Convert `// @deprecated` line comments into proper `@deprecated` javadoc tags on the deprecated JavaProfiler methods and the ThreadContext / ScopeStack / ContextSetter classes, so the rationale renders in javadoc and `-Xlint` stops warning about the missing tag. - Reject out-of-range slots (`slot >= MAX_CONTEXT_SLOTS`) in setContextValue and resolveContextValue before resolving, so an invalid slot no longer registers an orphan entry in the permanent native Dictionary (matches the DBB ThreadContext.setContextAttribute guard and the existing javadoc). - Reconcile the perf figures in the design note: explain that ~7-10% is the prototype microbench baseline and ~28% is the production ContextCombinedBenchmark baseline (six real currentContext()-resolving calls), so they no longer read as contradictory. - Clarify the otelWriteLrsEntry comment: only setTraceContext0/clearTraceContext0 establish the LRS entry; setContextValue0 does not, which is harmless. - Add an assert(len <= 255) in otelReadUtf8 documenting the caller contract, keeping the clamp as release-build defense. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![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: aa4e7cbd1d
ℹ️ 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".
jbachorik
reviewed
Jul 7, 2026
- P1 (Codex/jbachorik): the all-native write entry points never initialized OTEL TLS, so a thread that used only setTraceContext/setContextValue wrote a record the sampler (ContextApi::get / wallClock) ignored and eBPF could not discover. setTraceContext0 / setContextValue0 now call ContextApi::initializeContextTLS(thrd) on first use, matching the DBB path. Added AllNativeContextSamplingTest: a wall-clock JFR test on a thread that never calls getThreadContext(), asserting a sample carries the natively written context (fails if the record is not published to the sampler). - jbachorik: ContextValueCache assumed process-lifetime-immutable encodings, but a fresh start resets the native Dictionary (clearAll), reassigning IDs. Added ContextValueCache.clear(), invoked from execute() on a 'start' action, and corrected the class javadoc invariant. - jbachorik: MAX_CONTEXT_SLOTS duplicated native DD_TAGS_CAPACITY with only a comment binding them. Added native accessor maxContextSlots0() and MaxContextSlotsTest to catch drift from DD_TAGS_CAPACITY / ThreadContext.MAX_CUSTOM_SLOTS at test time. - jbachorik nits: added tests for the native null byte[] guard in otelReadUtf8, the otelCompactAttr 2-byte (zero-length value) boundary, and the setContextValue slot == MAX_CONTEXT_SLOTS boundary. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kaahos
reviewed
Jul 8, 2026
kaahos
left a comment
kaahos
left a comment
Contributor
There was a problem hiding this comment.
really nice changes, thanks! I only have few minor questions you can find below.
- #1/#2 (ContextValueCache invalidation): replace the Java command-string parsing (isStartAction) — which didn't match arguments.cpp's tokenization — with a native, authoritative signal. Profiler::start sets a _context_value_dict_reset flag in the reset block where _context_value_map.clearAll() runs (reached only for ACTION_START), and JavaProfiler.execute consumes it via consumeContextDictionaryReset0() to drop the cache. No command re-parsing in Java. This does not fully close the concurrent race window (resolve() during the reset), which is no worse than the deprecated DirectByteBuffer path; fully closing it is tracked as a follow-up (see PR discussion / Jira). - #3 (clearContextValue0 validity): capture and restore the prior valid flag instead of unconditionally re-attaching with valid=1, so clearing one attribute cannot resurrect a record that clearTraceContext0 intentionally left deactivated. Added AllNativeContextTest.clearContextValuePreservesInvalidState. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Reliability & Chaos Results❌ 1 failure(s) detected Pipeline: https://gitlab.ddbuild.io/DataDog/java-profiler/-/pipelines/123758963 ❌ chaos: profiler tcmalloc amd64 21 0 3 temXchaos |
kaahos
reviewed
Jul 9, 2026
Apply the same guard as clearContextValue0: capture the record's valid flag up
front and restore it instead of hardcoding valid=1. Writing a single attribute
must not resurrect a record that clearTraceContext0 intentionally deactivated —
republishing it would expose a span-less record (zero trace/span) as valid.
Mirrors the DBB path's guard in ThreadContext.setContextAttributesByIdAndBytes
("never resurrect a cleared, span-less record").
Added AllNativeContextTest.setContextValuePreservesInvalidState.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kaahos
approved these changes
Jul 9, 2026
kaahos
left a comment
kaahos
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the modifications! it looks good to me.
… API Distinguish caller programming errors (fail loud) from runtime/data conditions (soft signal), instead of silently swallowing the former: - setTraceContext rejects spanId==0 with IllegalArgumentException — it is the activation path and requires a real span; clearing is clearTraceContext. This drops the DBB path's conflation of setContext(0,0,0,0) with a clear and never publishes a span-less record. The native setTraceContext0 primitive asserts spanId!=0 as debug-only defense (the wrapper validates first). - Out-of-range custom-attribute slots now throw IllegalArgumentException in setTraceContext (non-negative slot >= MAX_CONTEXT_SLOTS; negative stays the documented skip sentinel), setContextValue, and clearContextValue — instead of being silently skipped / returning false / no-op'ing. Data conditions stay soft (not exceptions), matching normal control flow: a null or oversized value, or a full Dictionary, still returns false (setContextValue) or skips (setTraceContext). Native keeps its hard slot range guards (load-bearing for memory safety and to defend direct-JNI calls). Tests: added setTraceContextRejectsZeroSpanId; expanded slotBoundaryIsRejected to assert IllegalArgumentException across setContextValue/clearContextValue/ setTraceContext and that a negative activation slot still skips without throwing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Reverse the earlier "preserve prior valid" change on setContextValue0. Setting a value means "make it visible": an attribute must be observable to the sampler even when no span is active (zero trace/span). This is dd-trace-java's app-context-independent-of-span model (PR #11646) — app-owned attributes such as http.route, and the reapply-after-deactivation path, must stay visible between spans. It mirrors the DBB single-attribute setter (ThreadContext.setContextAttributeDirect), which always re-attaches; the earlier change had wrongly modeled it on the *bulk* setter's valid==0 guard. clearContextValue0 keeps preserving the prior valid flag: removing a value must not resurrect a deactivated record. So the asymmetry is intentional and matches the DBB single-vs-bulk behavior the tracer's reapply logic was built around: set publishes, clear does not. Test renamed/rewritten: setContextValuePublishesAppContextWithoutSpan asserts a value set after clearTraceContext republishes (valid=1, span=0, value observable). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
jbachorik
approved these changes
Jul 9, 2026
jbachorik
left a comment
jbachorik
left a comment
Collaborator
There was a problem hiding this comment.
Looks really good.
Could you, for peace of my mind, run the benchmarks also on Java 8 and 11 and add the numbers to the PR?
I expect maybe slight degradation, but we won't try and optimize for old runtimes anyway ...
kaahos
approved these changes
Jul 9, 2026
Add JavaProfiler.copyContextTags(int[]) + native copyContextTags0: reads the current thread's custom-attribute sidecar tag encodings directly from the record (ProfiledThread::current()), with no ThreadContext / DirectByteBuffer. Unlike the deprecated DBB copyTags — which goes through ThreadContext, whose ctor resets the record — this observes encodings written via the all-native setContextValue path and does not clobber them. This is the read the dd-trace-java Phase 2 migration needs for its (test-only) snapshot() oracle once writes go all-native. Also adds the Phase 2 plan doc (dd-trace-java migration onto the all-native API). Test: AllNativeContextTest.copyContextTagsReadsNativeEncodings (never calls getThreadContext, proving the native read observes native writes). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The dd-trace-java Phase 2 bridge sizes its app-offset arrays from the context attribute count. It previously read that as snapshotTags().length — a DBB read that creates a ThreadContext. Expose size() (attributes.size(), pure Java) so the bridge can size without touching the DBB path; offsetOf + size are the only ContextSetter methods it still uses. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@jbachorik ran the Only the
thread mode, ns/op (± 99.9% CI), 9 measurements each:
So the combined 2-JNI-call cycle beats the 6-call DBB sequence by ~2.3–2.5× on both — consistent with the JDK 21 result, no slight degradation observed. (Caveat: the reliable signal is the native-vs-dbb ratio within each JVM; the absolute 8-vs-11 gap is cross-JVM and noisier, as JMH itself warns.) |
Collaborator
|
@rkennke Cool, thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 1 of eliminating the virtual-thread use-after-free in OTEL trace-context storage
(PROF-15271), by moving context writes to an all-native path that resolves the current
carrier's OTEP record inside the JNI call — where the mounted virtual thread is pinned to its
carrier by the native frame, so the write is race-free by construction with no cached
DirectByteBufferto dangle.This PR is purely additive: it adds the new API alongside the existing
DirectByteBuffer(DBB) path (now
@Deprecated) and changes no behavior. The crash fix itself lands in Phase 2,when dd-trace-java switches to the new API; until then carrier-scoping (#625 /
94686da) remainsthe mitigation.
Full rationale, options considered (A–E), the JMH campaign, the consumer audit, and the
expand → migrate → contract plan are in the design note:
doc/plans/2026-07-02-all-native-context-storage-design.md.What's in this PR
javaApi.cpp):setTraceContext0/clearTraceContext0(combinedper-scope activate/deactivate) and
setContextValue0/clearContextValue0(single attribute),using the same
detach → mutate → attachvalid-flag protocol asThreadContextso theasync-signal-handler sampler (
ContextApi::get) stays coherent. The native path self-initializesthe LRS
attrs_dataentry (no dependency on theThreadContextctor).JavaProfiler,ContextValueCache): provisional namessetTraceContext/clearTraceContext/setContextValue/clearContextValue. Thevalue → (encoding, utf8)memoization moves out of the per-threadThreadContextinto aprocess-wide lock-free cache (safe because the encoding is a process-global immutable ID).
ThreadContext/ScopeStack/ContextSetterhelper classes are marked
@Deprecated(non-breaking) pointing at the new API.AllNativeContextTest, 9): round-trip, DBB-equivalence, clear semantics,span-transition attribute reset, single-attr set/clear, overflow, rejected values,
DBB↔native coexistence, and coherence of native writes from 512 mounted virtual threads.
ContextCombinedBenchmark).Results
Tests: all 9 new tests pass on JDK 21 (debug); the existing context suite (Otel / Carrier /
Tag / ProcessContext) is unregressed.
Perf (JDK 21, production per-scope activate+deactivate cycle, carrier mode): all-native
~136 ns vs DBB ~189 ns — ~28% faster (~24% thread mode), a win on virtual threads too, and
mode-independent. The DBB baseline is the faithful production cost (each of its 6 calls pays a
currentContext()/CarrierThreadLocallookup the 2-call native cycle avoids).Perf on old runtimes (Java 8 & 11): same
ContextCombinedBenchmarkand JMH config as theJDK 21 campaign (3 forks × 3×1s warmup × 3×3s measurement,
AverageTime, one op = one fullactivate+deactivate cycle). Only
platform_* / threadapplies on 8/11 —carriermode and thevthread_*benchmarks are JDK 21+ only (they needjdk.internal.misc.CarrierThreadLocal/Thread.startVirtualThread).setTraceContext+clearTraceContext)setContext+2×attr, ×2)1.8.0_492)11.0.31)No degradation on old runtimes — the all-native cycle wins by ~2.3–2.5×, consistent with JDK 21.
(Reliable signal is the native-vs-dbb ratio within each JVM; the cross-JVM absolute gap is
noisier, as JMH itself warns.)
Migration (expand → migrate → contract)
DatadogProfilingIntegrationto the combined API. Crashfix + perf win land here.
CarrierThreadLocal/OtelContextStoragesubsystem. Breaking, gated on all-consumers-migrated.
Reviewer notes
team so Phase 2 is a clean swap.
detach/attach+ release-fence protocol vsthe sampler's acquire load of
valid(ContextApi::get). Verify on aarch64 (trivial on x86).context-storage-benchmark-experiment.else is test/benchmark. Open item: confirm
ddprof-libstandalone Maven publication (externalconsumers) before the Phase-3 removal.
🤖 Generated with Claude Code