Skip to content

feat: nested-scope context save/restore via ScopeStack#496

Open
jbachorik wants to merge 19 commits intomainfrom
muse/impl-20260422-163430
Open

feat: nested-scope context save/restore via ScopeStack#496
jbachorik wants to merge 19 commits intomainfrom
muse/impl-20260422-163430

Conversation

@jbachorik
Copy link
Copy Markdown
Collaborator

@jbachorik jbachorik commented Apr 22, 2026

What does this PR do?:

Adds a bulk save/restore path for the per-thread profiling context, so dd-trace-java's ProfilingScope can propagate trace/span/LRS + custom-attribute state across nested scopes with one memcpy per transition.

  • ScopeStack — per-thread stack of context snapshots, with a contiguous fast-path buffer for the first 6 depths and lazily allocated 12-slot chunks beyond.
  • ThreadContext.snapshot(byte[], int) / restore(byte[], int) — bulk memcpy over the full record + sidecar, gated by detach/attach so concurrent signal handlers never observe partial state. Preserves the pre-snapshot valid byte: a cleared (valid=0) record stays unpublished across save/restore.
  • The three previously-separate ByteBuffers (record, sidecar, combined) collapse into a single 688-byte ctxBuffer that javaApi.cpp now returns to Java. Used for both per-field access and bulk memcpy.
  • Oversized attribute values (>255 UTF-8 bytes) are rejected at the Java entry point before touching the native Dictionary, so the write-only Dictionary never gets orphan entries.

Motivation:

In 1.41.0, setContextValue(int, int) and encode(String) were removed (correctly — the int-only path left OTEP attrs_data stale). That also removed the restore path that ProfilingScope.close() relied on, so profiling samples after a nested scope closed were attributed to stale context values from the finished scope.

Additional Notes:

  • ThreadContext.readContextAttribute is retained purely as a test-only read path exercised by TagContextTest. Its Javadoc calls this out; do not add a readback cache unless a production consumer appears.
  • setContextValue(int, int) is still not restored — the int-only path was the original inconsistency source.

How to test the change?:

  • ScopeStackTest (new, pure Java — no native lib required): depth accounting, underflow, fast-path and chunked-path round-trips, reuse after full unwind, and the clear-doesn't-republish regression.
  • TagContextTest.testSnapshotRestore / testAttrsDataOverflow / testPutClearsCustomSlots / testCrossSlotIsolation: native-backed coverage of set/clear/overwrite cycles and attrs_data overflow behavior.
  • Existing TagContextTest.test() exercises the full profiling path with custom attributes.

For Datadog employees:

  • If this PR touches code that signs or publishes builds or packages, or handles credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
  • This PR doesn't touch any of that.
  • JIRA: PROF-14338

Unsure? Have a question? Request a review!

@jbachorik jbachorik added the AI label Apr 22, 2026
@jbachorik
Copy link
Copy Markdown
Collaborator Author

@codex review

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 30456f08f9

ℹ️ 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".

Comment thread ddprof-lib/src/main/java/com/datadoghq/profiler/ThreadContext.java Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Restores ContextSetter snapshot/restore support needed by dd-trace-java profiling scopes by reintroducing a read-back API for custom context attributes and optimizing the read path to avoid allocations after writes.

Changes:

  • Add ContextSetter.readContextValue(int) / readContextValue(String) for reading current thread’s attribute values.
  • Add JavaProfiler.readContextAttribute(int) bridge and implement a per-slot value cache in ThreadContext to make reads allocation-free on the warm path.
  • Add TagContextTest.testSnapshotRestore() covering set/read/overwrite/restore and clear cycles.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
ddprof-test/src/test/java/com/datadoghq/profiler/context/TagContextTest.java Adds a unit test validating snapshot/restore behavior via the new read API.
ddprof-lib/src/main/java/com/datadoghq/profiler/ThreadContext.java Introduces indexedValueCache and updates readContextAttribute to use it for warm-path reads.
ddprof-lib/src/main/java/com/datadoghq/profiler/JavaProfiler.java Adds package-private bridge readContextAttribute(int) delegating to TLS ThreadContext.
ddprof-lib/src/main/java/com/datadoghq/profiler/ContextSetter.java Adds public read-back methods symmetric with existing setContextValue overloads.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread ddprof-lib/src/main/java/com/datadoghq/profiler/ThreadContext.java Outdated
@dd-octo-sts
Copy link
Copy Markdown
Contributor

dd-octo-sts Bot commented Apr 22, 2026

CI Test Results

Run: #24889214046 | Commit: abbaa94 | Duration: 30m 44s (longest job)

All 32 test jobs passed

Status Overview

JDK glibc-aarch64/debug glibc-amd64/debug musl-aarch64/debug musl-amd64/debug
8 - - -
8-ibm - - -
8-j9 - -
8-librca - -
8-orcl - - -
11 - - -
11-j9 - -
11-librca - -
17 - -
17-graal - -
17-j9 - -
17-librca - -
21 - -
21-graal - -
21-librca - -
25 - -
25-graal - -
25-librca - -

Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled

Summary: Total: 32 | Passed: 32 | Failed: 0


Updated: 2026-04-24 12:56:01 UTC

- Return null when valid=0 (after clearContext, attrs_data_size is
  intentionally stale — scanning it would return cleared attributes)
- Cache cold-path result in indexedValueCache to avoid repeated scans
  after resetThreadContext()

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Restores snapshot/restore support for custom profiling context attributes by adding allocation-free readback APIs, enabling dd-trace-java ProfilingScope to correctly restore prior context values when nested scopes close.

Changes:

  • Add ContextSetter.readContextValue(int) / readContextValue(String) for snapshotting current custom attribute values.
  • Introduce a per-slot indexedValueCache in ThreadContext and use it in readContextAttribute() to avoid scanning/allocations on the warm path.
  • Add testSnapshotRestore() in TagContextTest to validate snapshot/overwrite/restore behavior and name-based reads.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
ddprof-test/src/test/java/com/datadoghq/profiler/context/TagContextTest.java Adds snapshot/restore unit test coverage for the new readback API.
ddprof-lib/src/main/java/com/datadoghq/profiler/ThreadContext.java Adds per-slot readback caching and enhances readContextAttribute() to support snapshot/restore.
ddprof-lib/src/main/java/com/datadoghq/profiler/JavaProfiler.java Exposes package-level readContextAttribute(int) delegating to thread-local ThreadContext.
ddprof-lib/src/main/java/com/datadoghq/profiler/ContextSetter.java Adds public readContextValue(...) APIs used by nested scope snapshot/restore logic.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread ddprof-lib/src/main/java/com/datadoghq/profiler/ThreadContext.java Outdated
Co-Authored-By: muse <muse@noreply>
Comment thread ddprof-lib/src/main/java/com/datadoghq/profiler/ThreadContext.java Fixed
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Restores snapshot/restore support for custom profiling context attributes by introducing allocation-free read-back of attribute values from thread-local profiling context, enabling nested scope closures (e.g., ProfilingScope.close()) to correctly restore previous values.

Changes:

  • Add ContextSetter.readContextValue(int) and ContextSetter.readContextValue(String) to read current custom attribute values for snapshot/restore.
  • Introduce a per-slot indexedValueCache in ThreadContext and update readContextAttribute() to use it (warm-path allocation-free; cold-path scans attrs_data once and caches results, including “absent”).
  • Add JavaProfiler.readContextAttribute(int) and a new TagContextTest.testSnapshotRestore() to validate snapshot/restore behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
ddprof-test/src/test/java/com/datadoghq/profiler/context/TagContextTest.java Adds snapshot/restore test coverage for readContextValue() behavior.
ddprof-lib/src/main/java/com/datadoghq/profiler/ThreadContext.java Adds per-slot read-back cache and updates read path to support snapshot/restore efficiently.
ddprof-lib/src/main/java/com/datadoghq/profiler/JavaProfiler.java Exposes package-private readContextAttribute(int) delegating to thread-local ThreadContext.
ddprof-lib/src/main/java/com/datadoghq/profiler/ContextSetter.java Adds readContextValue(...) API for callers to snapshot/restore custom attributes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Co-Authored-By: muse <muse@noreply>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Restores context snapshot/restore support for custom profiling context attributes by adding allocation-free readback of the current attribute value from thread-local context, enabling nested scope close to correctly restore prior values.

Changes:

  • Add ContextSetter.readContextValue(int) and readContextValue(String) to support snapshot/restore semantics.
  • Add a per-slot indexedValueCache in ThreadContext (with ABSENT sentinel) to make readContextAttribute() warm-path allocation-free and avoid repeated attrs_data scans.
  • Expose JavaProfiler.readContextAttribute(int) as a package-private bridge to thread-local ThreadContext.readContextAttribute().

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
ddprof-test/src/test/java/com/datadoghq/profiler/context/TagContextTest.java Adds testSnapshotRestore() validating snapshot/overwrite/restore and null/clear behavior.
ddprof-lib/src/main/java/com/datadoghq/profiler/ThreadContext.java Introduces per-slot readback cache and updates clear/write paths + readContextAttribute() logic.
ddprof-lib/src/main/java/com/datadoghq/profiler/JavaProfiler.java Adds readContextAttribute(int) to read TLS ThreadContext attribute values.
ddprof-lib/src/main/java/com/datadoghq/profiler/ContextSetter.java Adds public readContextValue(...) APIs for restoring context values on scope close.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread ddprof-lib/src/main/java/com/datadoghq/profiler/ThreadContext.java Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Restores snapshot/restore support for custom profiling context attributes by adding string read-back APIs in ContextSetter, backed by a per-slot cache in ThreadContext to keep readContextAttribute() allocation-free on the warm path. This enables dd-trace-java’s ProfilingScope to correctly restore prior attribute values when nested scopes close.

Changes:

  • Add ContextSetter.readContextValue(int) and readContextValue(String) to enable snapshot/restore of custom attributes.
  • Add a per-slot indexedValueCache (with ABSENT sentinel) in ThreadContext.readContextAttribute() to avoid rescanning attrs_data and allocating on repeated reads.
  • Add TagContextTest.testSnapshotRestore() to cover null/set/overwrite/restore/clear cycles and the String-key overload.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
ddprof-test/src/test/java/com/datadoghq/profiler/context/TagContextTest.java Adds a focused snapshot/restore regression test for custom context attributes.
ddprof-lib/src/main/java/com/datadoghq/profiler/ThreadContext.java Implements per-slot read-back caching for custom attribute reads and keeps it in sync across set/clear/reset paths.
ddprof-lib/src/main/java/com/datadoghq/profiler/JavaProfiler.java Exposes a package-private TLS read helper delegating to ThreadContext.readContextAttribute().
ddprof-lib/src/main/java/com/datadoghq/profiler/ContextSetter.java Adds public read APIs used for snapshot/restore within nested profiling scopes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- Add Javadoc to JavaProfiler.readContextAttribute and ContextSetter.readContextValue(String)
- Document valid=0 invariant and ABSENT ClassLoader-singleton requirement in ThreadContext
- Expand readContextAttribute Javadoc (warm/cold path, allocation contract)
- Document orphan Dictionary encoding in setContextAttribute Javadoc
- Add tests: cold-path scan, attrs_data overflow, put() cache clear, cross-slot isolation
- Fix raw-offset assumption in testSnapshotRestore (use offsetOf instead of hardcoded 0)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Restores snapshot/restore support for custom profiling context attributes by adding read-back APIs to ContextSetter, backed by an allocation-free warm-path cache in ThreadContext. This enables dd-trace-java nested ProfilingScope instances to correctly restore prior attribute values when scopes close.

Changes:

  • Add ContextSetter.readContextValue(int) / readContextValue(String) to read the current attribute value from same-thread TLS.
  • Add ThreadContext.indexedValueCache + ABSENT sentinel and implement ThreadContext.readContextAttribute(int) with warm-path O(1) reads and cold-path attrs_data scan.
  • Add JavaProfiler.readContextAttribute(int) TLS delegate and expand TagContextTest with snapshot/restore + cold-path + overflow + isolation coverage.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
ddprof-test/src/test/java/com/datadoghq/profiler/context/TagContextTest.java Adds targeted tests for snapshot/restore behavior and cache/cold-path correctness.
ddprof-lib/src/main/java/com/datadoghq/profiler/ThreadContext.java Introduces per-slot string cache and read-back API, keeping sidecar/OTEP views consistent on overflow/clear.
ddprof-lib/src/main/java/com/datadoghq/profiler/JavaProfiler.java Adds TLS delegate to expose ThreadContext.readContextAttribute() within the package.
ddprof-lib/src/main/java/com/datadoghq/profiler/ContextSetter.java Adds read APIs to support snapshot/restore patterns for nested scopes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

jbachorik and others added 2 commits April 23, 2026 13:23
…ries

On musl (no JVM fork) the per-thread attrCacheKeys persists across
@RetryingTest retries. Once "0"-"9" are cached in the native Dictionary
and the Java cache, subsequent retries hit the cache, skip
registerConstant0(), and leave dictionary_context_keys=0.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Restores snapshot/restore support for custom profiling context attributes by adding read-back APIs in ContextSetter and an allocation-free warm-path cache in ThreadContext, ensuring nested scopes can correctly restore prior attribute values.

Changes:

  • Add ContextSetter.readContextValue(int) / readContextValue(String) to enable snapshot/restore patterns in callers (e.g., nested scopes).
  • Add ThreadContext.indexedValueCache with an ABSENT sentinel and cold-path scan caching to make same-thread reads O(1) on the warm path.
  • Expand TagContextTest with targeted tests for snapshot/restore, cold-path scan behavior, overflow handling, and cache-clearing semantics.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
ddprof-test/src/test/java/com/datadoghq/profiler/context/TagContextTest.java Adds/adjusts tests to validate snapshot/restore behavior and cache semantics across warm/cold/overflow paths.
ddprof-lib/src/main/java/com/datadoghq/profiler/ThreadContext.java Introduces per-slot read-back cache and updates write/clear paths to keep it consistent; enhances readContextAttribute() with warm/cold behavior.
ddprof-lib/src/main/java/com/datadoghq/profiler/JavaProfiler.java Exposes TLS-delegated readContextAttribute(int) for same-thread read-back.
ddprof-lib/src/main/java/com/datadoghq/profiler/ContextSetter.java Adds read-back APIs (readContextValue) to support snapshot/restore patterns for nested scopes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread ddprof-lib/src/main/java/com/datadoghq/profiler/ThreadContext.java Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Comment thread ddprof-lib/src/main/java/com/datadoghq/profiler/ThreadContext.java Fixed
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Restores same-thread snapshot/restore for custom context attributes (needed by dd-trace-java ProfilingScope) by adding read-back APIs to ContextSetter, backed by an allocation-free warm-path cache in ThreadContext.

Changes:

  • Add ContextSetter.readContextValue(int) and ContextSetter.readContextValue(String) for snapshot/restore of custom attribute values.
  • Add ThreadContext.indexedValueCache (with an ABSENT sentinel) and implement warm/cold read-back in ThreadContext.readContextAttribute(int).
  • Expand TagContextTest with new tests covering snapshot/restore behavior, cold-path scanning, attrs_data overflow handling, cache clearing on setContext, and cross-slot isolation; adjust the existing retrying test to use session-unique strings.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
ddprof-test/src/test/java/com/datadoghq/profiler/context/TagContextTest.java Adds targeted tests for snapshot/restore and cache behaviors; makes retry attempts register fresh dictionary values.
ddprof-lib/src/main/java/com/datadoghq/profiler/ThreadContext.java Introduces per-slot value cache + sentinel and uses it to provide warm-path, allocation-free read-back (with cold-path scan fallback).
ddprof-lib/src/main/java/com/datadoghq/profiler/JavaProfiler.java Adds TLS-delegating readContextAttribute(int) entry point for ContextSetter.
ddprof-lib/src/main/java/com/datadoghq/profiler/ContextSetter.java Exposes public read APIs (readContextValue) to support snapshot/restore in nested scopes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Adds a 688-byte DirectByteBuffer aliasing record+sidecar for one-memcpy
save/restore of nested scope state. New ScopeStack provides tiered scratch
(6-slot eager + lazy 12-slot chunks) for allocation-free per-thread use.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment thread ddprof-lib/src/main/java/com/datadoghq/profiler/ThreadContext.java Fixed
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR restores same-thread snapshot/restore support for custom context attributes by adding read-back APIs on ContextSetter, plus introduces allocation-free warm-path reads in ThreadContext via a per-slot string cache. It also adds an optional bulk snapshot/restore mechanism (ThreadContext.snapshot/restore + ScopeStack) to efficiently save/restore full context state for nested scopes.

Changes:

  • Add ContextSetter.readContextValue(int) / readContextValue(String) to support restoring previous custom attribute values when nested scopes close.
  • Add ThreadContext.indexedValueCache + warm/cold read logic to make read-back O(1) and allocation-free on the warm path, with tests covering snapshot/restore, cold-path scan, overflow, and cache clearing.
  • Extend TLS initialization to return a third DirectByteBuffer (“combined” record+sidecar) and add ThreadContext.snapshot/restore + new ScopeStack for bulk memcpy-based scope transitions.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
ddprof-test/src/test/java/com/datadoghq/profiler/context/TagContextTest.java Adds tests for snapshot/restore read-back behavior, cold-path scan, overflow handling, cache clearing, and cross-slot isolation; also makes retry attempts register unique dictionary values.
ddprof-lib/src/main/java/com/datadoghq/profiler/ThreadContext.java Adds per-slot string cache and read-back API, plus new bulk snapshot/restore support over a combined buffer.
ddprof-lib/src/main/java/com/datadoghq/profiler/ScopeStack.java Introduces a per-thread snapshot stack for nested-scope bulk save/restore.
ddprof-lib/src/main/java/com/datadoghq/profiler/JavaProfiler.java Adds TLS read-back delegate and updates TLS initialization to construct ThreadContext from 3 buffers.
ddprof-lib/src/main/java/com/datadoghq/profiler/ContextSetter.java Adds public read-back methods (readContextValue) for restoring prior attribute values.
ddprof-lib/src/main/cpp/thread.h Documents/enforces contiguous layout expectations for combined buffer usage.
ddprof-lib/src/main/cpp/javaApi.cpp Updates JNI to return 3 DirectByteBuffers and creates the combined contiguous buffer view.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread ddprof-lib/src/main/java/com/datadoghq/profiler/ThreadContext.java Outdated
Comment thread ddprof-lib/src/main/java/com/datadoghq/profiler/ThreadContext.java
Comment thread ddprof-lib/src/main/java/com/datadoghq/profiler/ThreadContext.java Outdated
jbachorik and others added 4 commits April 23, 2026 19:20
Java readers of custom attrs are test-only — the profiler signal handler
uses sidecar encodings and OTEL eBPF reads attrs_data directly. Drop the
per-slot String cache and scan attrs_data on the test-only read path.
Reject values whose UTF-8 encoding exceeds 255 bytes at setContextAttribute
to keep the OTEP view consistent and avoid orphan Dictionary entries.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Drop dead Math.min truncation in replaceOtepAttribute (unreachable
  after up-front rejection); document the caller contract
- Fix stale "via cache" assertion message in testAttrsDataOverflow
- Refresh MAX_VALUE_BYTES comment to match the new enforcement point
- Move ScopeStackTest to com.datadoghq.profiler so it can reference
  ThreadContext.SNAPSHOT_SIZE / OTEL_MAX_RECORD_SIZE instead of
  duplicating layout constants

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ffer

initializeContextTLS0 returns a single 688-byte DirectByteBuffer; metadata[5]
is the absolute LRS offset in that buffer. Tag-encoding writes use
TAG_ENCODINGS_OFFSET (=OTEL_MAX_RECORD_SIZE). One less JNI call per thread,
one less field per ThreadContext, identical memory layout.

Also fixes Java 8 CI: cast ByteBuffer to Buffer before calling position(int)
so bytecode targets the Java 8 Buffer method signature rather than the
JDK 9+ covariant ByteBuffer override.

Update doc/architecture/TLSContext.md to match the unified-buffer API.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
snapshot() used to unconditionally re-attach after the bulk read. If the
record was intentionally left unpublished (valid=0 after the all-zero clear
path in setContextDirect), snapshot would flip valid=1 over stale attrs_data.
Now both snapshot and restore consult the pre-snapshot valid byte (stored in
scratch[validOffset]) and only re-publish when the captured state was valid.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Restores nested-scope snapshot/restore of custom context attributes by adding read APIs to ContextSetter and enabling bulk save/restore of the per-thread OTEP record + DD sidecar state (now exposed as a single contiguous DirectByteBuffer).

Changes:

  • Switch TLS context initialization to return one unified 688B DirectByteBuffer (record + tag encodings + LRS) and update Java/C++ plumbing accordingly.
  • Add ThreadContext.snapshot()/restore() and introduce ScopeStack + a pure-Java unit test for stack behavior.
  • Add ContextSetter.readContextValue(...) and expand tests around snapshot/restore and attrs_data overflow behavior.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
doc/architecture/TLSContext.md Updates architecture docs to reflect unified ctxBuffer layout and snapshot/restore semantics.
ddprof-test/src/test/java/com/datadoghq/profiler/context/TagContextTest.java Adds snapshot/restore, overflow, and isolation tests for context attributes.
ddprof-test/src/test/java/com/datadoghq/profiler/ScopeStackTest.java New pure-Java unit tests for ScopeStack behavior (no native lib required).
ddprof-lib/src/main/java/com/datadoghq/profiler/ThreadContext.java Unifies buffers, adds bulk snapshot/restore, enforces value size limit, and adjusts attrs_data write paths.
ddprof-lib/src/main/java/com/datadoghq/profiler/ScopeStack.java New per-thread stack for nested-scope snapshot/restore storage.
ddprof-lib/src/main/java/com/datadoghq/profiler/JavaProfiler.java Updates JNI contract to single buffer + adds TLS-delegated attribute read method.
ddprof-lib/src/main/java/com/datadoghq/profiler/ContextSetter.java Adds readContextValue(int) / readContextValue(String) to support snapshot/restore patterns.
ddprof-lib/src/main/cpp/thread.h Enforces/clarifies contiguity and gating invariants for unified TLS context layout.
ddprof-lib/src/main/cpp/javaApi.cpp Returns a single contiguous DirectByteBuffer and updates metadata offsets accordingly.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread ddprof-test/src/test/java/com/datadoghq/profiler/ScopeStackTest.java Outdated
Comment thread ddprof-lib/src/main/java/com/datadoghq/profiler/ThreadContext.java
jbachorik and others added 3 commits April 24, 2026 13:22
- ScopeStackTest: drop unused assertNull import (3137117966)
- ThreadContext.readContextAttribute: replace stale "test-only path"
  Javadoc with accurate description — method now backs
  ContextSetter.readContextValue used for scope snapshot/restore
  (3137117980)

Co-Authored-By: muse <muse@noreply>
Previous Javadoc revision suggested it was reachable from production
scope snapshot/restore; in fact ContextSetter.readContextValue is only
used from tests. Mark the method test-only, state that allocation is
acceptable, and caution against adding a readback cache without a real
production consumer.

Co-Authored-By: muse <muse@noreply>
dd-trace-java will not call readContextValue — the only caller chain
was TagContextTest -> ContextSetter.readContextValue -> JavaProfiler
.readContextAttribute -> ThreadContext.readContextAttribute. Drop the
two middle hops and let the test reach ThreadContext directly via
JavaProfiler.getThreadContext(), mirroring OtelContextStorageModeTest.

- Remove ContextSetter.readContextValue(int) / (String)
- Remove JavaProfiler.readContextAttribute(int)
- TagContextTest uses a local readTag helper that goes through
  profiler.getThreadContext().readContextAttribute(offset)
- Strengthen ThreadContext.readContextAttribute Javadoc to reflect
  that the sole caller is now a test

Co-Authored-By: muse <muse@noreply>
@jbachorik jbachorik marked this pull request as ready for review April 24, 2026 12:21
@jbachorik jbachorik requested a review from a team as a code owner April 24, 2026 12:21
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0aefdc55b2

ℹ️ 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".

Comment on lines +299 to +300
if (utf8.length > MAX_VALUE_BYTES) {
return false;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Clear stale tag when rejecting oversized values

When this slot already has a value and a caller tries to replace it with a string whose UTF-8 encoding exceeds 255 bytes, this early return leaves the old sidecar encoding and attrs_data entry published. The method reports failure, but subsequent samples still carry the previous tag value; the other failure paths below clear the slot, so this size check should do the same before returning false.

Useful? React with 👍 / 👎.

@jbachorik jbachorik changed the title fix: restore ContextSetter snapshot/restore via readContextValue() feat: nested-scope context save/restore via ScopeStack Apr 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants