From fa0b66f03c38465246c15aeebb2ed6382aa1504c Mon Sep 17 00:00:00 2001 From: Jordan Wong Date: Thu, 16 Jul 2026 02:11:11 -0400 Subject: [PATCH 01/11] skill(apm-integrations): library-native context-map pattern + wrap-placement guidance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds two subsections to context-tracking.md driven by reactor-core-3.1 blind regen findings (dd-trace-java PR #11940): 1. **Library-native context maps — the *ContextBridge pattern** (RI-6). Prescribes preservation/regeneration of the Reactor-style context-bridge helper for libraries with a first-class Context concept (Reactor, Kotlin coroutines, JAX-RS, Vert.x). The subscriber-wrapping pattern alone is insufficient — regenerating a reactive module without the bridge silently breaks Spring WebFlux, Spring Kafka reactive, and related downstream libraries. 2. **Wrap placement and context-store lifecycle** (from @mcculls' review of PR #11940). Boundary-only wrapping, Subscriber-lifecycle stores, avoid double-wrapping. Prevents the "increased memory use" pattern the eval output demonstrated. Motivating incident: dd-trace-java PR #11940 eval dropped 5 of 7 master reactor-core-3.1 instrumentation classes including ReactorContextBridge, which caused spring-messaging-4.0's KafkaBatchListenerCoroutineTest to time out. Sources: - @mcculls on PR #11940 FluxInstrumentation.java:20 - docs/eval-research/cycles/2026-07-14-async-cycle-report.md RI-5, RI-6 - Master reference: dd-java-agent/instrumentation/reactor-core-3.1/ --- .../references/context-tracking.md | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/.agents/skills/apm-integrations/references/context-tracking.md b/.agents/skills/apm-integrations/references/context-tracking.md index d94272beb61..6912d3a6d3d 100644 --- a/.agents/skills/apm-integrations/references/context-tracking.md +++ b/.agents/skills/apm-integrations/references/context-tracking.md @@ -55,6 +55,45 @@ When a boundary type exposes multiple overloads of the subscribe / invoke method **Reference:** dd-trace-java's `rxjava-2.0` module hooks the single-argument `subscribe(...)` method (matcher: `named("subscribe").and(takesArguments(1))`) with the argument typed as the base callback interface (e.g. `Observer` for `Observable`, `Subscriber` for `Flowable`). Read the module source to see the exact matcher — pattern-match on this rather than copying overload names. +## Library-native context maps — the `*ContextBridge` pattern + +Some libraries expose their own request-scoped context map that users write to explicitly. Examples: + +- **Reactor** — `reactor.util.context.Context` (immutable per-subscription map); users pass a span via `.contextWrite(ctx -> ctx.put("dd.span", span))`. +- **Kotlin coroutines** — `CoroutineContext` with custom `CoroutineContextElement`. +- **JAX-RS** — `ContainerRequestContext` for per-request state. +- **Vert.x** — `Context.putLocal(...)`. + +When a library has a first-class context-map concept, the observer/subscriber wrapping pattern documented above is **not sufficient by itself** — it only handles the "capture the currently-active trace context at subscribe time" case. It does NOT handle the case where a user has placed a span in the library's own context map and expects that span to propagate through the operator chain. + +**A context-tracking module for such a library MUST include a `*ContextBridge`-style helper** that: + +1. Reads a well-known key (Datadog convention: `"dd.span"`) from the library-native context. +2. Adapts the retrieved object (which implements `WithAgentSpan` or is an `AgentSpan`) into a `datadog.context.Context`. +3. Stores that context in the toolkit-native `ContextStore` keyed by the library's reactive-streams primitives (`Publisher`, `Subscriber`). +4. Activates the stored context at the right lifecycle boundaries: on subscribe, on signal delivery, on blocking, and on internal subscriber handoff (e.g. Reactor's fused-operator path). + +**Reference:** `dd-java-agent/instrumentation/reactor-core-3.1/` — master has `ReactorContextBridge.java` plus four supporting instrumentations that hook the specific lifecycle points: `BlockingPublisherInstrumentation` (for `.block()` / `.blockFirst()` / `.blockLast()` on the calling thread), `ContextWritingSubscriberInstrumentation` (for `.contextWrite(...)` subscribers at subscribe time), `CorePublisherInstrumentation` (for the base publisher interface handing off to downstream subscribers), and `OptimizableOperatorInstrumentation` (for Reactor's internal fused-operator optimization path). Regenerating this module without preserving these classes silently breaks downstream libraries — Spring WebFlux, Spring Kafka reactive `@KafkaListener suspend fun`, resilience4j-reactor, reactor-netty — that rely on the `dd.span` propagation path. + +**How to detect the pattern in an unfamiliar library:** search the library's public API for a `Context` type with `put`/`get`/`hasKey` methods that user code can write to (as opposed to a Datadog-internal context). If such a type exists, the library has a native context map and needs a `*ContextBridge` helper. + +**When regenerating an existing reactive module:** enumerate every `*Instrumentation.java` in the master module and account for each one. Dropping any without a documented reason is always a Rule #2 (regen-preservation) violation. Also preserve the master's `*ContextBridge`-style helper verbatim, or produce equivalent semantics — do not drop it in favor of the subscriber-wrapping pattern alone. + +## Wrap placement and context-store lifecycle — memory considerations + +Context-tracking instrumentations allocate two kinds of long-lived state that add up on hot reactive paths: + +- **Wrapper instances** — one `TracingObserver`/`TracingSubscriber`/etc. per subscribe call. +- **Context-store entries** — one `ContextStore.put(subscriber, context)` per subscribe call, held until the subscription completes/cancels. + +Place both at the narrowest scope that preserves correctness: + +- **Wrap only at chain boundaries** — subscribe, `.contextWrite(...)`, `.block()` — not at every internal operator. Operator-level wrapping causes N-fold allocations for an N-operator chain and does not add propagation coverage that boundary wrapping doesn't already provide. +- **Use Subscriber-lifecycle context stores** — the store should drop entries when the subscription completes or cancels. `ContextStore` implementations in dd-trace-java are weakly-keyed; make sure the key is the subscriber (which has a bounded lifetime) rather than the publisher (which may be shared and long-lived). +- **Avoid double-wrapping** — when a downstream operator already carries the context via the library-native context map (e.g., Reactor's `Context` flows through the whole operator chain by construction), do not add a per-operator wrap on top. + +**Why this matters:** a 10-operator Reactor chain with per-operator wrapping allocates 10× the wrappers of a boundary-only implementation, and every allocation must be reclaimed when the subscription completes. In steady-state reactive services, that's the difference between context-tracking being invisible in profiling and being a measurable overhead. + ## When NOT to write a context-tracking instrumentation If the library DOES perform I/O — sends HTTP requests, runs DB queries, makes RPC calls, talks to a broker, reads/writes a cache — write a **span-creating instrumentation** (`InstrumenterModule.Tracing`) instead. Context-tracking is only for libraries that coordinate work; the moment there's actual I/O to observe, you want spans around it. From ccdebdfecfb8e2716573099fed40797640e107e5 Mon Sep 17 00:00:00 2001 From: Jordan Wong Date: Thu, 16 Jul 2026 02:12:24 -0400 Subject: [PATCH 02/11] =?UTF-8?q?skill(apm-integrations):=20strengthen=20R?= =?UTF-8?q?ule=20#2=20for=20regen=20=E2=80=94=20super(),=20package,=20clas?= =?UTF-8?q?ses,=20ordering?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds five prescriptions to instrumenter-module.md driven by async cycle iteration-1 findings (docs/eval-research/cycles/2026-07-14-async-cycle-report.md) plus @ygree review feedback on rxjava-3.0 PR #11939. 1. **super() alias strengthening (RI-1)** — concrete rxjava-3.0 example showing DD_TRACE_RXJAVA_3_ENABLED breakage when the version alias is dropped. Existing "preserve super() verbatim" rule was too abstract. 2. **Package layout preservation on regen (RI-4)** — concrete reactor-core-3.1 example showing graal-native-image cross-module reference breakage when the eval renamed reactor.core -> reactorcore. 3. **Enumerate all master *Instrumentation.java classes on regen (RI-5)** — the reactor-core-3.1 regen kept 2 of 7 master classes, silently dropping ReactorContextBridge and 4 others. Rule prescribes an explicit pre-generation enumeration + post-generation diff check. 4. **Preserve declarative-array ordering (NR-1)** — @ygree flagged "meaningless reshuffling" in helperClassNames(). Rule: preserve master's ordering unless a semantic change requires reordering. 5. **Hoist repeated Class.getName() in contextStore() (NR-2)** — @ygree flagged five inlined copies of Context.class.getName() as a regression from master's hoist pattern. Sources: - docs/eval-research/cycles/2026-07-14-async-cycle-report.md RI-1, RI-4, RI-5 - @ygree PR #11939 comments 3591691401, 3591695153 --- .../references/instrumenter-module.md | 112 ++++++++++++++++++ 1 file changed, 112 insertions(+) diff --git a/.agents/skills/apm-integrations/references/instrumenter-module.md b/.agents/skills/apm-integrations/references/instrumenter-module.md index c6ecda9fd0c..fc7c7519eee 100644 --- a/.agents/skills/apm-integrations/references/instrumenter-module.md +++ b/.agents/skills/apm-integrations/references/instrumenter-module.md @@ -87,6 +87,118 @@ Do NOT add version aliases to the decorator's `instrumentationNames()` — that **When rewriting or refactoring an existing module, preserve every override the master version has.** Not just `super(...)` — also `defaultEnabled()`, `helperClassNames()`, `contextStore()`, `orderPriority()`, `muzzleDirective()`, and any other overridden method. Read the current file on master before writing; carry each override forward verbatim unless there's a documented reason to change it. Silent loss of `defaultEnabled() = false` (or similar opt-in flags) ships an integration with a different default than users expected. +**Concrete failure pattern (from dd-trace-java PR #11939, rxjava-3.0 regen):** master has + +```java +public RxJavaModule() { + super("rxjava", "rxjava-3"); // ← two args: family name + version alias +} +``` + +An eval regenerated this as + +```java +public RxJavaModule() { + super("rxjava"); // ❌ dropped "rxjava-3" version alias +} +``` + +Impact: customers who set `DD_TRACE_RXJAVA_3_ENABLED=false` to opt out silently lose that opt-out — the flag stops being recognized. No CI check catches it; the regression is only visible when a customer tries to disable the integration. **When regenerating a module that already exists, both the number of arguments AND the exact string values of `super(...)` must be preserved verbatim.** + +### Package layout must be preserved verbatim on regen + +When regenerating an existing module, use the exact same Java package for every production class. Master's package is authoritative; do not rename, consolidate, or shorten. + +**Concrete failure pattern (from dd-trace-java PR #11940, reactor-core-3.1 regen):** master's production classes live under `datadog.trace.instrumentation.reactor.core`. An eval regenerated the module under `datadog.trace.instrumentation.reactorcore` (concatenated form, chosen by analogy with `datadog.trace.instrumentation.rxjava3`). + +Consequences: +- Silently breaks fully-qualified class references from other modules. In this case, `dd-java-agent/instrumentation/graal/graal-native-image-20.0/` has a `NativeImageGeneratorRunnerInstrumentation` that lists `datadog.trace.instrumentation.reactor.core.ReactorAsyncResultExtension` in its build-time classlist by name — the rename breaks Graal native-image builds that depend on that classlist entry. +- Confuses reviewers and merge-conflict resolution when comparing eval output to master. +- Doesn't reduce the diff size or improve any measurable outcome — it's a purely stylistic choice the model made without prompting. + +**Rule:** when regenerating an existing module, the top-level Java package of every production class MUST match master exactly. If master uses dotted style (`datadog.trace.instrumentation.reactor.core`), preserve it; if master uses concatenated style (`datadog.trace.instrumentation.rxjava3`), preserve it. Master is the invariant. + +### Enumerate all master `*Instrumentation.java` classes on regen + +When regenerating an existing module, list every `*Instrumentation.java`, `*Bridge.java`, and helper class currently in master's `src/main/java/.../` directory. Every one of those classes must be either preserved verbatim in the output OR explicitly replaced with an equivalent class. **Dropping a master class without a documented reason is always a Rule #2 violation.** + +**Concrete failure pattern (from PR #11940):** master's `reactor-core-3.1` has 7 production classes: + +``` +ReactorCoreModule.java +ReactorContextBridge.java (context-map bridge — see context-tracking.md) +ReactorAsyncResultExtension.java +BlockingPublisherInstrumentation.java +ContextWritingSubscriberInstrumentation.java +CorePublisherInstrumentation.java +OptimizableOperatorInstrumentation.java +``` + +The eval output kept only 2 (`ReactorCoreModule`, `ReactorAsyncResultExtension`) and added 3 new ones (`FluxInstrumentation`, `MonoInstrumentation`, `TracingCoreSubscriber`). Net effect: 5 master classes silently dropped, including `ReactorContextBridge` — which is what breaks Spring WebFlux, Spring Kafka reactive, and other downstream Reactor-based libraries. No CI check on the target module catches it; the regression only surfaces when sibling-module tests fail. + +**How to apply this rule:** before generating, run `ls dd-java-agent/instrumentation//src/main/java/**/` and record every filename. After generating, diff the list of classes in your output against that record. Any master class not present in the output must be explicitly justified in the PR description. + +### Preserve declarative-array ordering (`helperClassNames`, `contextStore` keys) + +When regenerating an existing `InstrumenterModule`, preserve the exact order of entries in `helperClassNames()` and the exact order of `store.put(...)` calls in `contextStore()`. Reordering entries with no semantic change produces noisy diffs that reviewers correctly reject as "meaningless reshuffling." + +**Concrete failure pattern (from PR #11939, @ygree review):** the eval output re-sorted `helperClassNames()` — same set of entries, different order. This adds review burden (reviewer must verify the set is unchanged) with zero benefit. + +```java +// ❌ Reordered without reason +return new String[] { + packageName + ".TracingObserver", + packageName + ".TracingSubscriber", + packageName + ".TracingSingleObserver", + packageName + ".TracingMaybeObserver", + packageName + ".TracingCompletableObserver", // was first in master + packageName + ".RxJavaAsyncResultExtension", +}; + +// ✅ Preserve master's ordering +return new String[] { + packageName + ".TracingCompletableObserver", + packageName + ".TracingObserver", + packageName + ".TracingSubscriber", + packageName + ".TracingSingleObserver", + packageName + ".TracingMaybeObserver", + packageName + ".RxJavaAsyncResultExtension", +}; +``` + +Only reorder when adding or removing an entry for a semantic reason (a helper is being introduced or retired). + +### Hoist repeated `Class.getName()` calls in `contextStore()` + +When multiple `store.put(...)` calls in `contextStore()` use the same value-class FQN, hoist `SomeClass.class.getName()` into a single local variable. This is the idiomatic pattern in existing dd-trace-java modules; inlining the same call five times is treated as a regression by reviewers. + +```java +// ❌ Inlined at every put site +public Map contextStore() { + final Map store = new HashMap<>(); + store.put("io.reactivex.rxjava3.core.Observable", Context.class.getName()); + store.put("io.reactivex.rxjava3.core.Flowable", Context.class.getName()); + store.put("io.reactivex.rxjava3.core.Single", Context.class.getName()); + store.put("io.reactivex.rxjava3.core.Maybe", Context.class.getName()); + store.put("io.reactivex.rxjava3.core.Completable", Context.class.getName()); + return store; +} + +// ✅ Hoisted once +public Map contextStore() { + String contextClass = Context.class.getName(); + final Map store = new HashMap<>(); + store.put("io.reactivex.rxjava3.core.Observable", contextClass); + store.put("io.reactivex.rxjava3.core.Flowable", contextClass); + store.put("io.reactivex.rxjava3.core.Single", contextClass); + store.put("io.reactivex.rxjava3.core.Maybe", contextClass); + store.put("io.reactivex.rxjava3.core.Completable", contextClass); + return store; +} +``` + +Source: @ygree review on PR #11939. + ### Do not create a helper class just for CallDepthThreadLocalMap when only one type is instrumented When only one type is being instrumented, use `CallDepthThreadLocalMap` directly in the Advice class. A separate helper class that just wraps `CallDepthThreadLocalMap.incrementCallDepth` / `decrementCallDepth` adds indirection without value: From b84681f1a5b6425075440cb83986462966fd3c02 Mon Sep 17 00:00:00 2001 From: Jordan Wong Date: Thu, 16 Jul 2026 02:13:19 -0400 Subject: [PATCH 03/11] skill(apm-integrations): namespace-isolation fail block + dep version parity Adds three prescriptions to muzzle.md from async cycle iteration-1 findings and @mcculls review feedback. 1. **Namespace-isolation fail block for major-version siblings (RI-2).** rxjava-3.0 master has `muzzle { fail { name = "rxjava2-must-not-match" } }` to assert rxjava3 advice never matches rxjava2 coordinates. Eval dropped the block. Rule prescribes preservation for any module with a prior-major sibling module in the repo. 2. **compileOnly dep version parity on regen (RI-3).** rxjava-3.0 master uses reactive-streams 1.0.3; eval regenerated as 1.0.0, silently narrowing the tested API surface. Rule extends the existing testImplementation parity rule to compileOnly. 3. **Test-scope build.gradle dep preservation (NR-5, @mcculls PR #11940).** reactor-core-3.1 eval dropped 8 testImplementation and latestDepTest deps that back annotation-driven tests and cross-module interop tests. Rule prescribes superset-of-master semantics for test deps. --- .../apm-integrations/references/muzzle.md | 80 +++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/.agents/skills/apm-integrations/references/muzzle.md b/.agents/skills/apm-integrations/references/muzzle.md index e1f281630cd..9eee0a32ac1 100644 --- a/.agents/skills/apm-integrations/references/muzzle.md +++ b/.agents/skills/apm-integrations/references/muzzle.md @@ -172,3 +172,83 @@ muzzle { **How to discover these**: when verify fails with a task name that has a doubled module name (e.g., `redis.clients-jedis-jedis-3.6.2`), check the existing production module for the same library at another version. If it has `skipVersions` entries, copy them. This is library-specific tribal knowledge that lives in the existing modules. When in doubt, **search adjacent module build.gradle files for `skipVersions`** before declaring a new version-bounded module's muzzle directives. + +## Namespace-isolation `fail` blocks for major-version siblings + +When a library has multiple major versions published under **different** `group:module` coordinates but under the same brand (e.g., `io.reactivex.rxjava2:rxjava` and `io.reactivex.rxjava3:rxjava`), and the two versions cannot share advice (the instrumentation must never resolve against the wrong major), master modules explicitly assert namespace isolation with a `muzzle { fail { ... } }` block. + +The block is defense-in-depth: it catches accidental cross-version advice matching that would otherwise pass silently. When regenerating a module that has such a block, preserve it verbatim. + +**Concrete failure pattern (from dd-trace-java PR #11939, rxjava-3.0 regen):** master's `rxjava-3.0/build.gradle` has: + +```groovy +muzzle { + pass { + group = "io.reactivex.rxjava3" + module = "rxjava" + versions = "[3.0.0,)" + } + // Assert the rxjava3 advice never resolves against rxjava2 — the two namespaces + // must not overlap. rxjava3 references io.reactivex.rxjava3.core.*, absent from + // the rxjava2 artifact, so muzzle must fail to match it. + fail { + name = "rxjava2-must-not-match" + group = "io.reactivex.rxjava2" + module = "rxjava" + versions = "[2.0.0,)" + } +} +``` + +An eval regenerated this WITHOUT the `fail` block. Muzzle would still likely fail naturally on rxjava2 (the FQNs don't exist in that artifact), but the explicit assertion is what catches the failure at CI time with a specific error message rather than a generic muzzle mismatch. + +**Rule:** for any module whose brand has a prior major version published under different Maven coordinates in the same repo, check master for a `muzzle { fail { name = "..." } }` block. If present, preserve verbatim on regen. When creating a new module for a library that has a prior-major sibling module in the repo, add such a fail block to assert non-overlap. + +Common cases where this applies: `rxjava-2.0` ↔ `rxjava-3.0`, `okhttp-2.0` ↔ `okhttp-3.0`, `jedis-1.4` ↔ `jedis-3.0` ↔ `jedis-4.0`, `jetty-server-7.0` ↔ `jetty-server-9.0.4` ↔ `jetty-server-11.0` (etc.). + +## Preserve `compileOnly` dependency versions on regen + +When regenerating an existing module, preserve the exact version of every `compileOnly` dependency in `build.gradle`. Silently narrowing a compileOnly version reduces the tested API surface without any warning — the module still compiles and tests still pass because the older version is a subset. + +**Concrete failure pattern (from dd-trace-java PR #11939, rxjava-3.0 regen):** + +```groovy +// Master +dependencies { + compileOnly group: 'org.reactivestreams', name: 'reactive-streams', version: '1.0.3' + compileOnly group: 'io.reactivex.rxjava3', name: 'rxjava', version: '3.0.0' +} + +// Eval regenerated as +dependencies { + compileOnly group: 'org.reactivestreams', name: 'reactive-streams', version: '1.0.0' // ❌ regressed + compileOnly group: 'io.reactivex.rxjava3', name: 'rxjava', version: '3.0.0' +} +``` + +`reactive-streams 1.0.3` adds APIs and behavioral clarifications over `1.0.0` that dd-trace-java's advice may rely on. Downgrading silently removes them from the tested surface. + +**Rule:** all `compileOnly` versions must match master verbatim on regen. This complements the existing `testImplementation` version parity rule — the same principle applies at the compile scope. + +## Preserve test-scope build.gradle dependencies on regen + +When regenerating an existing module, preserve every `testImplementation`, `latestDepTestImplementation`, and `forkedTestImplementation` dependency verbatim unless the corresponding test file is also being removed. Do not drop cross-module test dependencies — they back annotation-driven and cross-tracer interop tests that silently fail to compile or run without them. + +**Concrete failure pattern (from dd-trace-java PR #11940, reactor-core-3.1 regen):** the eval dropped these test dependencies: + +```groovy +testImplementation project(':dd-java-agent:instrumentation:reactive-streams-1.0') +testImplementation project(path: ':dd-java-agent:agent-otel:otel-bootstrap', configuration: 'shadow') +testImplementation project(':dd-java-agent:instrumentation:opentelemetry:opentelemetry-1.4') +testImplementation project(':dd-java-agent:instrumentation:opentelemetry:opentelemetry-annotations-1.20') +testImplementation project(':dd-java-agent:instrumentation:opentracing:opentracing-0.32') +testImplementation group: 'io.opentelemetry.instrumentation', name: 'opentelemetry-instrumentation-annotations', version: '1.28.0' +testImplementation group: 'io.opentracing', name: 'opentracing-util', version: '0.32.0' +latestDepTestImplementation group: 'io.micrometer', name: 'micrometer-core', version: '1.+' +``` + +These deps back annotation-driven tests (`@WithSpan`, `@Traced`), cross-module interop tests (`reactive-streams-1.0`), and version-drift workaround deps (`micrometer-core` required by newer Reactor versions). @mcculls flagged all of these as required. + +**Rule:** the set of `testImplementation` and `latestDepTestImplementation` entries in a regenerated `build.gradle` must be a superset of master's. If master has cross-module `project(':dd-java-agent:instrumentation:...')` deps, they must be present in the eval output. If master has `latestDepTestImplementation` workaround deps (e.g. `micrometer-core` for Reactor), they must be present. + +Source: @mcculls PR #11940 build.gradle review. From 82be43f5f8c8220774c689c653fe375f4bdb6fa7 Mon Sep 17 00:00:00 2001 From: Jordan Wong Date: Thu, 16 Jul 2026 02:14:00 -0400 Subject: [PATCH 04/11] skill(apm-integrations): latestDepTest source set + no banner comments in tests Adds two prescriptions to tests.md from async cycle iteration-1 findings and @ygree review feedback. 1. **latestDepTest source-set preservation (RI-7).** reactor-core-3.1 master has src/latestDepTest/groovy/ for version-sensitive tests. The eval collapsed all tests into src/test/java/, which caused Schedulers.elastic() (removed in Reactor 3.4+) to break :latestDepTest compilation across all JVM shards. Rule prescribes preserving the split when master has it, and using it for libraries with breaking API changes across recent minor versions. 2. **No banner comments in test files (NR-4).** @ygree flagged `// --------- Successful completion ---------` style banners in RxJava3ResultExtensionTest.java as "distracting and don't offer much value because their scope is unclear." Rule: omit or extract into separate test classes with focused Javadoc. --- .../apm-integrations/references/tests.md | 71 +++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/.agents/skills/apm-integrations/references/tests.md b/.agents/skills/apm-integrations/references/tests.md index 2aad929bfe8..c70440f3d09 100644 --- a/.agents/skills/apm-integrations/references/tests.md +++ b/.agents/skills/apm-integrations/references/tests.md @@ -113,3 +113,74 @@ State the isolation reason in a comment on the `ForkedTest` class. ### Do not add default jvmArgs to test tasks `dd.trace.enabled=true` is the default; adding `jvmArgs '-Ddd.trace.enabled=true'` to a `Test` task in `build.gradle` is noise. Only add jvmArgs that meaningfully diverge from defaults (e.g. enabling a specific integration that's off by default, or a debug flag). If you're tempted to copy a `jvmArgs` block from a sibling module, check whether each flag is actually needed for this module. + +## Version-sensitive tests belong in a separate `latestDepTest` source set + +When regenerating an existing module, check if master has a `src/latestDepTest/` source set (declared via `addTestSuite('latestDepTest')` or `addTestSuiteForDir('latestDepTest', ...)` in `build.gradle`). If so, preserve that split — do NOT collapse latestDep-specific tests into the base `src/test/` directory. + +**Why this matters:** for libraries whose API surface changes across minor versions (Reactor deprecates and removes APIs; Netty changes signatures; gRPC evolves generated code), the base test directory compiles against `testImplementation` (pinned to the module's declared min version) AND against `latestDepTestImplementation` (which resolves to the latest published version). If a test uses an API that was removed after the declared min, putting it in the base directory causes a compile failure in `latestDepTest` even though the test itself is intended to run against the older version. + +Master's solution: put version-sensitive tests in `src/latestDepTest/` where they only compile against `latestDepTestImplementation` and can freely use the current API. When the latest version removes an API, only the `latestDepTest` copy needs updating. + +**Concrete failure pattern (from dd-trace-java PR #11940, reactor-core-3.1 regen):** master has `src/latestDepTest/groovy/ReactorCoreTest.groovy` that uses `Schedulers.boundedElastic()` (the current API). The eval collapsed all tests into `src/test/java/` and used `Schedulers.elastic()` (removed in Reactor 3.4+). Result: `:latestDepTest` compilation failure blocking `:check` on every JVM shard. + +**Rule:** +- Before generating tests, `ls src/latestDepTest/` in master's module. If it exists, the regen must include the equivalent source set. +- If master's `build.gradle` has `addTestSuiteForDir('latestDepTest', ...)` or `addTestSuite('latestDepTest')`, preserve that declaration verbatim. +- When generating tests for a library that has deprecated or removed APIs across recent minor versions, use `latestDepTest/` for tests that exercise those APIs and `test/` for tests that exercise stable APIs. +- Common libraries where this split matters: Reactor (`Schedulers.elastic()` removed in 3.4+), Netty (channel handler API changes across 4.x), gRPC (generated-code shape evolves), Kafka clients (consumer API changed 3.0), Cassandra driver (3.x vs 4.x are largely incompatible). + +Source: master's `dd-java-agent/instrumentation/reactor-core-3.1/src/latestDepTest/groovy/ReactorCoreTest.groovy`; failure pattern documented in `docs/eval-research/cycles/2026-07-14-async-cycle-report.md` RI-7. + +## No banner/separator comments in test files + +Do NOT insert banner-style separator comments (e.g. `// --------- Successful completion ---------`) inside test files to group related test methods. Banner comments have unclear scope, don't render usefully in IDEs, and add review burden without a benefit that justifies the noise. + +**If a group of related tests warrants its own heading**, extract them into a separate test class with a focused class-level Javadoc: + +```java +// ❌ Banner comments +class RxJava3ResultExtensionTest extends AbstractInstrumentationTest { + // --------------------------------------------------------------------------- + // Successful async completion: span finishes when reactive type completes + // --------------------------------------------------------------------------- + @ParameterizedTest + void successfulCompletion(...) { ... } + + // --------------------------------------------------------------------------- + // Error paths: span records error and finishes + // --------------------------------------------------------------------------- + @ParameterizedTest + void errorPath(...) { ... } +} + +// ✅ Either omit the banner +class RxJava3ResultExtensionTest extends AbstractInstrumentationTest { + @ParameterizedTest + void successfulCompletion(...) { ... } + + @ParameterizedTest + void errorPath(...) { ... } +} + +// OR extract into focused classes with class Javadoc +/** + * Successful async completion — verifies the extension finishes the span + * when the reactive type emits a terminal signal. + */ +class RxJava3ResultExtensionSuccessTest extends AbstractInstrumentationTest { + @ParameterizedTest + void successfulCompletion(...) { ... } +} + +/** + * Error paths — verifies the extension records the error and finishes the span + * when the reactive type emits an onError signal. + */ +class RxJava3ResultExtensionErrorTest extends AbstractInstrumentationTest { + @ParameterizedTest + void errorPath(...) { ... } +} +``` + +Source: @ygree review on PR #11939. From b0d4736e66862288b6bb326f3ab8640c83414e6a Mon Sep 17 00:00:00 2001 From: Jordan Wong Date: Thu, 16 Jul 2026 02:14:32 -0400 Subject: [PATCH 05/11] skill(apm-integrations): no inline explanatory comments in Advice methods Adds NR-3 from @ygree review of PR #11939. Advice bodies are typically short; the wrapper/helper class is where reviewers look to understand semantics. Duplicating the explanation as a `//`-comment at the advice call site inflates the diff and drifts out of sync with the wrapper's Javadoc. Source: @ygree, PR #11939 comment on SingleInstrumentation.java:55. --- .../references/advice-class.md | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/.agents/skills/apm-integrations/references/advice-class.md b/.agents/skills/apm-integrations/references/advice-class.md index b75cde27623..f92b8df6ffb 100644 --- a/.agents/skills/apm-integrations/references/advice-class.md +++ b/.agents/skills/apm-integrations/references/advice-class.md @@ -152,3 +152,36 @@ For route-only enrichers: no `AgentScope`, no `startSpan()`, no `decorator.after ### Advice classes must not declare non-constant static fields `*Advice.java` classes are inlined at instrumentation sites; non-constant static fields (fields that aren't `static final` primitives or string literals) get pulled into every instrumented callsite and violate muzzle's assumptions. Keep only `static final` constants — no logger references, no cached decorators, no state. If you need shared state, put it on a helper class registered via `helperClassNames()`, not on the advice. + +## No inline explanatory comments in Advice methods + +Do NOT add narrative `//`-comments inside `@Advice.OnMethodEnter` / `@Advice.OnMethodExit` methods to explain *why* wrapping, subscription capture, or span activation happens. If the target helper class (e.g. `TracingSingleObserver`, `TracingRunnable`) already has a class-level Javadoc documenting the intent, duplicating the explanation at the call site is treated as noise by reviewers. + +```java +// ❌ Redundant inline comment (TracingSingleObserver's Javadoc already explains this) +@Advice.OnMethodEnter(suppress = Throwable.class) +public static void enter( + @Advice.Argument(value = 0, readOnly = false) SingleObserver observer) { + AgentSpan parentSpan = activeSpan(); + if (parentSpan != null) { + // wrap the observer so spans from its events treat the captured span as their parent + observer = new TracingSingleObserver<>(observer, parentSpan); + } +} + +// ✅ Wrapper's Javadoc carries the explanation +@Advice.OnMethodEnter(suppress = Throwable.class) +public static void enter( + @Advice.Argument(value = 0, readOnly = false) SingleObserver observer) { + AgentSpan parentSpan = activeSpan(); + if (parentSpan != null) { + observer = new TracingSingleObserver<>(observer, parentSpan); + } +} +``` + +Advice bodies are typically short (5–15 lines); the wrapper/helper class is where reviewers look to understand semantics. Inline `//`-comments in advice methods duplicate that documentation, inflate the diff, and drift out of sync with the wrapper's Javadoc. + +**When an inline comment IS appropriate:** to explain a non-obvious constraint or hidden invariant the reader cannot recover from the wrapper's Javadoc (e.g. "must run before the delegate's own advice runs because …" — an ordering fact not visible from either class alone). These are rare; the default is no inline comment. + +Source: @ygree review on PR #11939 (SingleInstrumentation.java). From 0338411570aca119478a6f20ad316bb47d664faa Mon Sep 17 00:00:00 2001 From: Jordan Wong Date: Mon, 20 Jul 2026 00:51:46 -0400 Subject: [PATCH 06/11] =?UTF-8?q?skill(apm-integrations):=20address=20Copi?= =?UTF-8?q?lot=20review=20on=20#11990=20=E2=80=94=20latestDepTest=20nuance?= =?UTF-8?q?,=20find=20vs=20ls=20glob,=20muzzle=20fail=20scope,=20ContextSt?= =?UTF-8?q?ore=20implementation=20accuracy?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../skills/apm-integrations/references/context-tracking.md | 2 +- .../skills/apm-integrations/references/instrumenter-module.md | 2 +- .agents/skills/apm-integrations/references/muzzle.md | 2 +- .agents/skills/apm-integrations/references/tests.md | 4 ++-- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.agents/skills/apm-integrations/references/context-tracking.md b/.agents/skills/apm-integrations/references/context-tracking.md index 6912d3a6d3d..f29fd48edd9 100644 --- a/.agents/skills/apm-integrations/references/context-tracking.md +++ b/.agents/skills/apm-integrations/references/context-tracking.md @@ -89,7 +89,7 @@ Context-tracking instrumentations allocate two kinds of long-lived state that ad Place both at the narrowest scope that preserves correctness: - **Wrap only at chain boundaries** — subscribe, `.contextWrite(...)`, `.block()` — not at every internal operator. Operator-level wrapping causes N-fold allocations for an N-operator chain and does not add propagation coverage that boundary wrapping doesn't already provide. -- **Use Subscriber-lifecycle context stores** — the store should drop entries when the subscription completes or cancels. `ContextStore` implementations in dd-trace-java are weakly-keyed; make sure the key is the subscriber (which has a bounded lifetime) rather than the publisher (which may be shared and long-lived). +- **Use Subscriber-lifecycle context stores** — the store should drop entries when the subscription completes or cancels. `ContextStore` implementations in dd-trace-java use field injection on the key instance when possible (the common fast path), or a weak-map fallback when field injection is unavailable. In either case, choose a key with a bounded lifetime: use the subscriber (scoped to one subscription) rather than the publisher (which may be shared and long-lived across many subscriptions). - **Avoid double-wrapping** — when a downstream operator already carries the context via the library-native context map (e.g., Reactor's `Context` flows through the whole operator chain by construction), do not add a per-operator wrap on top. **Why this matters:** a 10-operator Reactor chain with per-operator wrapping allocates 10× the wrappers of a boundary-only implementation, and every allocation must be reclaimed when the subscription completes. In steady-state reactive services, that's the difference between context-tracking being invisible in profiling and being a measurable overhead. diff --git a/.agents/skills/apm-integrations/references/instrumenter-module.md b/.agents/skills/apm-integrations/references/instrumenter-module.md index fc7c7519eee..93234c19024 100644 --- a/.agents/skills/apm-integrations/references/instrumenter-module.md +++ b/.agents/skills/apm-integrations/references/instrumenter-module.md @@ -136,7 +136,7 @@ OptimizableOperatorInstrumentation.java The eval output kept only 2 (`ReactorCoreModule`, `ReactorAsyncResultExtension`) and added 3 new ones (`FluxInstrumentation`, `MonoInstrumentation`, `TracingCoreSubscriber`). Net effect: 5 master classes silently dropped, including `ReactorContextBridge` — which is what breaks Spring WebFlux, Spring Kafka reactive, and other downstream Reactor-based libraries. No CI check on the target module catches it; the regression only surfaces when sibling-module tests fail. -**How to apply this rule:** before generating, run `ls dd-java-agent/instrumentation//src/main/java/**/` and record every filename. After generating, diff the list of classes in your output against that record. Any master class not present in the output must be explicitly justified in the PR description. +**How to apply this rule:** before generating, enumerate every `.java` file in the existing module — `find dd-java-agent/instrumentation//src/main/java -name "*.java"` — and record each filename. After generating, diff that list against the classes in your output. Any master class not present in the output must be explicitly justified in the PR description. ### Preserve declarative-array ordering (`helperClassNames`, `contextStore` keys) diff --git a/.agents/skills/apm-integrations/references/muzzle.md b/.agents/skills/apm-integrations/references/muzzle.md index 9eee0a32ac1..22f07565e43 100644 --- a/.agents/skills/apm-integrations/references/muzzle.md +++ b/.agents/skills/apm-integrations/references/muzzle.md @@ -175,7 +175,7 @@ When in doubt, **search adjacent module build.gradle files for `skipVersions`** ## Namespace-isolation `fail` blocks for major-version siblings -When a library has multiple major versions published under **different** `group:module` coordinates but under the same brand (e.g., `io.reactivex.rxjava2:rxjava` and `io.reactivex.rxjava3:rxjava`), and the two versions cannot share advice (the instrumentation must never resolve against the wrong major), master modules explicitly assert namespace isolation with a `muzzle { fail { ... } }` block. +When a library has multiple major versions that must never be instrumented by the same advice — whether published under different `group:module` coordinates (e.g., `io.reactivex.rxjava2:rxjava` vs `io.reactivex.rxjava3:rxjava`) or under the same coordinates at incompatible major versions (e.g., `org.springframework:spring-webflux` at major 5 vs 6) — master modules explicitly assert namespace isolation with a `muzzle { fail { ... } }` block. The block is defense-in-depth: it catches accidental cross-version advice matching that would otherwise pass silently. When regenerating a module that has such a block, preserve it verbatim. diff --git a/.agents/skills/apm-integrations/references/tests.md b/.agents/skills/apm-integrations/references/tests.md index c70440f3d09..e5fe30bb5f2 100644 --- a/.agents/skills/apm-integrations/references/tests.md +++ b/.agents/skills/apm-integrations/references/tests.md @@ -118,7 +118,7 @@ State the isolation reason in a comment on the `ForkedTest` class. When regenerating an existing module, check if master has a `src/latestDepTest/` source set (declared via `addTestSuite('latestDepTest')` or `addTestSuiteForDir('latestDepTest', ...)` in `build.gradle`). If so, preserve that split — do NOT collapse latestDep-specific tests into the base `src/test/` directory. -**Why this matters:** for libraries whose API surface changes across minor versions (Reactor deprecates and removes APIs; Netty changes signatures; gRPC evolves generated code), the base test directory compiles against `testImplementation` (pinned to the module's declared min version) AND against `latestDepTestImplementation` (which resolves to the latest published version). If a test uses an API that was removed after the declared min, putting it in the base directory causes a compile failure in `latestDepTest` even though the test itself is intended to run against the older version. +**Why this matters:** for libraries whose API surface changes across minor versions (Reactor deprecates and removes APIs; Netty changes signatures; gRPC evolves generated code), tests in `src/test/` compile against both `testImplementation` (pinned to min) and `latestDepTestImplementation` (latest) when the module uses `addTestSuiteForDir('latestDepTest', 'test')` — the common pattern that reuses `src/test/` sources for both suites. For modules that use `addTestSuite('latestDepTest')` instead, the `latestDepTest` suite has its own sources at `src/latestDepTest/` and `src/test/` is not compiled against `latestDepTestImplementation`. In both cases: if a test uses an API removed in a later version, it will cause a `latestDepTest` compile failure. Master's solution: put version-sensitive tests in `src/latestDepTest/` where they only compile against `latestDepTestImplementation` and can freely use the current API. When the latest version removes an API, only the `latestDepTest` copy needs updating. @@ -130,7 +130,7 @@ Master's solution: put version-sensitive tests in `src/latestDepTest/` where the - When generating tests for a library that has deprecated or removed APIs across recent minor versions, use `latestDepTest/` for tests that exercise those APIs and `test/` for tests that exercise stable APIs. - Common libraries where this split matters: Reactor (`Schedulers.elastic()` removed in 3.4+), Netty (channel handler API changes across 4.x), gRPC (generated-code shape evolves), Kafka clients (consumer API changed 3.0), Cassandra driver (3.x vs 4.x are largely incompatible). -Source: master's `dd-java-agent/instrumentation/reactor-core-3.1/src/latestDepTest/groovy/ReactorCoreTest.groovy`; failure pattern documented in `docs/eval-research/cycles/2026-07-14-async-cycle-report.md` RI-7. +Source: master's `dd-java-agent/instrumentation/reactor-core-3.1/src/latestDepTest/groovy/ReactorCoreTest.groovy`. ## No banner/separator comments in test files From 9e8d28be61e2f6f708008a69368dff2370cd7563 Mon Sep 17 00:00:00 2001 From: Jordan Wong Date: Tue, 21 Jul 2026 10:43:09 -0400 Subject: [PATCH 07/11] =?UTF-8?q?skill(apm-integrations):=20address=20Code?= =?UTF-8?q?x=20review=20on=20#11990=20=E2=80=94=205=20P2=20comments?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - context-tracking.md: narrow ContextStore key rule — Reactor uses Publisher/Subscriber; JAX-RS uses ContainerRequestContext; Vert.x uses its own Context; coroutines use Continuation/CoroutineContext. Do not force Reactor's key type onto libraries that don't expose Publisher / Subscriber. Also broaden lifecycle-boundary examples per library shape. - tests.md: distinguish latest-only APIs (belong in src/latestDepTest/) from removed-in-latest APIs (belong in src/test/, or use replacement API in latestDepTest). The Reactor Schedulers.elastic() removal is the removed-in-latest case, not the latest-only case. - muzzle.md #1 (fail-block scope): explicitly show same-coordinate cases (jedis/okhttp/jetty-server) alongside different-coordinate cases (rxjava, jms api). The bounded 'versions' range in the fail block is what asserts non-overlap for same-coordinate siblings. - muzzle.md #2 (test-dep preservation): extend the preservation list to cover testRuntimeOnly / latestDepTestRuntimeOnly / forkedTestRuntimeOnly. Runtime-only test deps do NOT trigger compile failures if dropped, so losing them silently removes cross-instrumentation coexistence coverage (e.g. rxjava-3.0's testRuntimeOnly on rxjava-2.0). - instrumenter-module.md: broaden the pre-regen source-file enumeration from `src/main/java` to every production source set: src/main/java17, src/main/java11, src/main/groovy, src/main/scala, src/main/kotlin. Kafka-clients-3.8 and jetty-server-12.0 keep classes under java17; a `find` limited to `src/main/java` misses them. All five findings are P2 severity per Codex classification; each corrects a case where the iter-2 rule was too narrow and could mislead a regen. --- .../references/context-tracking.md | 4 ++-- .../references/instrumenter-module.md | 17 ++++++++++++++++- .../apm-integrations/references/muzzle.md | 18 +++++++++++++++--- .../apm-integrations/references/tests.md | 4 +++- 4 files changed, 36 insertions(+), 7 deletions(-) diff --git a/.agents/skills/apm-integrations/references/context-tracking.md b/.agents/skills/apm-integrations/references/context-tracking.md index f29fd48edd9..b83f45664fc 100644 --- a/.agents/skills/apm-integrations/references/context-tracking.md +++ b/.agents/skills/apm-integrations/references/context-tracking.md @@ -70,8 +70,8 @@ When a library has a first-class context-map concept, the observer/subscriber wr 1. Reads a well-known key (Datadog convention: `"dd.span"`) from the library-native context. 2. Adapts the retrieved object (which implements `WithAgentSpan` or is an `AgentSpan`) into a `datadog.context.Context`. -3. Stores that context in the toolkit-native `ContextStore` keyed by the library's reactive-streams primitives (`Publisher`, `Subscriber`). -4. Activates the stored context at the right lifecycle boundaries: on subscribe, on signal delivery, on blocking, and on internal subscriber handoff (e.g. Reactor's fused-operator path). +3. Stores that context in the toolkit-native `ContextStore` keyed by the appropriate library-specific lifecycle type. **For Reactor / Reactive-Streams libraries**, the key is `Publisher` or `Subscriber`. **For non-Reactive-Streams libraries**, key by the library's own lifecycle primitive: JAX-RS → `ContainerRequestContext`; Vert.x → `io.vertx.core.Context`; Kotlin coroutines → the `Continuation` or `CoroutineContext` element. Do not force a Reactor-style `Publisher`/`Subscriber` key onto a library that doesn't expose those types. +4. Activates the stored context at the right lifecycle boundaries. For Reactive-Streams: on subscribe, on signal delivery, on blocking, and on internal subscriber handoff (e.g. Reactor's fused-operator path). For other shapes, activate at whatever boundaries the library exposes (request-start / request-end for JAX-RS; coroutine resume/suspend for Kotlin; verticle handler entry for Vert.x). **Reference:** `dd-java-agent/instrumentation/reactor-core-3.1/` — master has `ReactorContextBridge.java` plus four supporting instrumentations that hook the specific lifecycle points: `BlockingPublisherInstrumentation` (for `.block()` / `.blockFirst()` / `.blockLast()` on the calling thread), `ContextWritingSubscriberInstrumentation` (for `.contextWrite(...)` subscribers at subscribe time), `CorePublisherInstrumentation` (for the base publisher interface handing off to downstream subscribers), and `OptimizableOperatorInstrumentation` (for Reactor's internal fused-operator optimization path). Regenerating this module without preserving these classes silently breaks downstream libraries — Spring WebFlux, Spring Kafka reactive `@KafkaListener suspend fun`, resilience4j-reactor, reactor-netty — that rely on the `dd.span` propagation path. diff --git a/.agents/skills/apm-integrations/references/instrumenter-module.md b/.agents/skills/apm-integrations/references/instrumenter-module.md index 93234c19024..1f21ad30747 100644 --- a/.agents/skills/apm-integrations/references/instrumenter-module.md +++ b/.agents/skills/apm-integrations/references/instrumenter-module.md @@ -136,7 +136,22 @@ OptimizableOperatorInstrumentation.java The eval output kept only 2 (`ReactorCoreModule`, `ReactorAsyncResultExtension`) and added 3 new ones (`FluxInstrumentation`, `MonoInstrumentation`, `TracingCoreSubscriber`). Net effect: 5 master classes silently dropped, including `ReactorContextBridge` — which is what breaks Spring WebFlux, Spring Kafka reactive, and other downstream Reactor-based libraries. No CI check on the target module catches it; the regression only surfaces when sibling-module tests fail. -**How to apply this rule:** before generating, enumerate every `.java` file in the existing module — `find dd-java-agent/instrumentation//src/main/java -name "*.java"` — and record each filename. After generating, diff that list against the classes in your output. Any master class not present in the output must be explicitly justified in the PR description. +**How to apply this rule:** before generating, enumerate every production source file in the existing module — **not just `src/main/java`**. Several modules keep production classes in version-specific or JVM-specific source sets: + +- `src/main/java17` — Java 17-only APIs (`kafka-clients-3.8`, `jetty-server-12.0`) +- `src/main/java11` — Java 11-only APIs (some HTTP-client modules) +- `src/main/groovy` — Groovy production classes (rare, but present in some modules) +- `src/main/scala` — Scala production classes (Play framework, some Akka modules) + +A regen that walks only `src/main/java` will silently miss version-specific instrumentation classes and lose them from the output. Use this command instead: + +``` +find dd-java-agent/instrumentation//src/main \ + \( -name "*.java" -o -name "*.groovy" -o -name "*.scala" -o -name "*.kt" \) \ + -type f +``` + +Record each filename. After generating, diff that list against the classes in your output. Any master class not present in the output must be explicitly justified in the PR description. ### Preserve declarative-array ordering (`helperClassNames`, `contextStore` keys) diff --git a/.agents/skills/apm-integrations/references/muzzle.md b/.agents/skills/apm-integrations/references/muzzle.md index 22f07565e43..55bb8f09824 100644 --- a/.agents/skills/apm-integrations/references/muzzle.md +++ b/.agents/skills/apm-integrations/references/muzzle.md @@ -202,9 +202,14 @@ muzzle { An eval regenerated this WITHOUT the `fail` block. Muzzle would still likely fail naturally on rxjava2 (the FQNs don't exist in that artifact), but the explicit assertion is what catches the failure at CI time with a specific error message rather than a generic muzzle mismatch. -**Rule:** for any module whose brand has a prior major version published under different Maven coordinates in the same repo, check master for a `muzzle { fail { name = "..." } }` block. If present, preserve verbatim on regen. When creating a new module for a library that has a prior-major sibling module in the repo, add such a fail block to assert non-overlap. +**Rule:** for any module whose brand has a prior major version — **whether published under different Maven coordinates or under the same coordinates at an incompatible major** — check master for a `muzzle { fail { name = "..." } }` block. If present, preserve verbatim on regen. When creating a new module for a library that has a prior-major sibling module in the repo, add such a fail block to assert non-overlap. -Common cases where this applies: `rxjava-2.0` ↔ `rxjava-3.0`, `okhttp-2.0` ↔ `okhttp-3.0`, `jedis-1.4` ↔ `jedis-3.0` ↔ `jedis-4.0`, `jetty-server-7.0` ↔ `jetty-server-9.0.4` ↔ `jetty-server-11.0` (etc.). +Common cases where this applies: + +- **Different Maven coordinates:** `rxjava-2.0` (`io.reactivex.rxjava2:rxjava`) ↔ `rxjava-3.0` (`io.reactivex.rxjava3:rxjava`); `javax-jms-*` ↔ `jakarta-jms-*`; `spring-webflux-5.0` (`org.springframework:spring-webflux`) ↔ `spring-webflux-6.0` (`org.springframework:spring-webflux`, but Jakarta EE 9+ package rename). +- **Same Maven coordinates, incompatible majors:** `okhttp-2.0` ↔ `okhttp-3.0` (both `com.squareup.okhttp*`, package renamed at v3); `jedis-1.4` ↔ `jedis-3.0` ↔ `jedis-4.0` (all `redis.clients:jedis`, API changed across majors); `jetty-server-7.0` ↔ `jetty-server-9.0.4` ↔ `jetty-server-11.0` (all `org.eclipse.jetty:jetty-server`, `javax.servlet` → `jakarta.servlet` at 11+). + +For same-coordinate cases, the `fail` block still applies: it asserts the older major's version range does NOT match the newer module's advice, even though the coordinates are identical. Look for `versions = "[,3.0.0)"` bounded ranges in the master's `fail` block, not a coordinate difference. ## Preserve `compileOnly` dependency versions on regen @@ -232,7 +237,14 @@ dependencies { ## Preserve test-scope build.gradle dependencies on regen -When regenerating an existing module, preserve every `testImplementation`, `latestDepTestImplementation`, and `forkedTestImplementation` dependency verbatim unless the corresponding test file is also being removed. Do not drop cross-module test dependencies — they back annotation-driven and cross-tracer interop tests that silently fail to compile or run without them. +When regenerating an existing module, preserve every test-scope dependency verbatim — **including runtime-only scopes** — unless the corresponding test file is also being removed. The full set to preserve is: + +- `testImplementation`, `latestDepTestImplementation`, `forkedTestImplementation` (compile-scope test deps) +- `testRuntimeOnly`, `latestDepTestRuntimeOnly`, `forkedTestRuntimeOnly` (runtime-only test deps — **these do NOT show up as compile failures if dropped**) + +Do not drop cross-module test dependencies — they back annotation-driven, cross-tracer interop, and cross-instrumentation-coexistence tests that silently fail to compile or run without them. + +**Why runtime-only scopes matter:** modules like `rxjava-3.0` declare `testRuntimeOnly project(':dd-java-agent:instrumentation:rxjava:rxjava-2.0')` to load the rxjava2 instrumenter into the test JVM. This is what verifies rxjava3 tests are NOT accidentally being served by rxjava2's advice (the mutual-exclusion coverage that pairs with the `muzzle { fail { ... } }` block above). Dropping the `testRuntimeOnly` dep still compiles cleanly — but the test JVM no longer has rxjava2's instrumenter loaded, so the mutual-exclusion coverage silently disappears with no CI signal. **Concrete failure pattern (from dd-trace-java PR #11940, reactor-core-3.1 regen):** the eval dropped these test dependencies: diff --git a/.agents/skills/apm-integrations/references/tests.md b/.agents/skills/apm-integrations/references/tests.md index e5fe30bb5f2..be022c6af9e 100644 --- a/.agents/skills/apm-integrations/references/tests.md +++ b/.agents/skills/apm-integrations/references/tests.md @@ -127,7 +127,9 @@ Master's solution: put version-sensitive tests in `src/latestDepTest/` where the **Rule:** - Before generating tests, `ls src/latestDepTest/` in master's module. If it exists, the regen must include the equivalent source set. - If master's `build.gradle` has `addTestSuiteForDir('latestDepTest', ...)` or `addTestSuite('latestDepTest')`, preserve that declaration verbatim. -- When generating tests for a library that has deprecated or removed APIs across recent minor versions, use `latestDepTest/` for tests that exercise those APIs and `test/` for tests that exercise stable APIs. +- **Route each test to the source set whose classpath actually satisfies its imports.** Two distinct scenarios drive the split: + - **API is only in the latest version** (added after the pinned min) → the test uses that API, must go in `src/latestDepTest/`. `src/test/` compiles against `testImplementation` (pinned min), where the API is absent — the test would fail to compile there. + - **API is only in the pinned min** (removed in a later version, e.g. Reactor's `Schedulers.elastic()` removed in 3.4+) → the test uses the removed API, must go in `src/test/` and NOT in `latestDepTest/`. Placing it in `src/latestDepTest/` (where the classpath resolves to the latest release) would produce a compile failure like the concrete failure pattern above. Prefer testing the equivalent replacement API (`Schedulers.boundedElastic()`) in `latestDepTest/` instead. - Common libraries where this split matters: Reactor (`Schedulers.elastic()` removed in 3.4+), Netty (channel handler API changes across 4.x), gRPC (generated-code shape evolves), Kafka clients (consumer API changed 3.0), Cassandra driver (3.x vs 4.x are largely incompatible). Source: master's `dd-java-agent/instrumentation/reactor-core-3.1/src/latestDepTest/groovy/ReactorCoreTest.groovy`. From aae41473b1f12462cc0ab04d5124e714523dcdc9 Mon Sep 17 00:00:00 2001 From: Jordan Wong Date: Thu, 23 Jul 2026 14:36:24 -0400 Subject: [PATCH 08/11] skill(apm-integrations): condense regen-preservation content, wire new rules into SKILL.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Feedback from @mcculls on this PR and separately from a senior engineer: async is a hard area where AI output should be a starting point for human+AI iteration, not held to a "perfect on first try" bar. Most of this PR's new content was regen-specific "keep things stable when rewriting" directives, disproportionate for a skill whose SKILL.md is otherwise written entirely for the common case (writing a new integration). 8 of the 12 new sections fell into this category. Collapsed all 8 into a single "editing an existing module" note per file (3 files), each a few sentences: read the current file, preserve what's there unless you have a reason to change it, applies to super()/package/class-set/ordering/dependencies/test-source-sets alike. Kept exactly one concrete example (ReactorContextBridge — the highest-stakes case, since dropping it silently breaks sibling modules that reference the class by FQN) instead of one narrated failure story per rule. Net: ~365 lines added by the PR, ~200 removed here. The 4 sections that are genuine new async-instrumentation content (library-native context maps / *ContextBridge pattern, wrap-placement memory considerations, no-inline-advice-comments, no-banner-comments) are unchanged — those aren't regen-specific, they apply to any reactive library instrumentation, new or edited. Also addresses the Datadog Autotest P1 finding (2026-07-21): the new rules existed in reference files but SKILL.md's per-step pointers were generic ("read this file") rather than pointing at the specific new subsections. Added explicit pointers at Steps 4.1, 5, 7, and 9.2/9.3 so a code-gen LLM sees "read the ContextBridge section" / "add the namespace-isolation fail block" inline in its step-by-step flow instead of needing to discover them by skimming a 200+ line reference file. Co-Authored-By: Claude Sonnet 5 --- .agents/skills/apm-integrations/SKILL.md | 10 +- .../references/context-tracking.md | 2 +- .../references/instrumenter-module.md | 133 +----------------- .../apm-integrations/references/muzzle.md | 71 +--------- .../apm-integrations/references/tests.md | 19 +-- 5 files changed, 19 insertions(+), 216 deletions(-) diff --git a/.agents/skills/apm-integrations/SKILL.md b/.agents/skills/apm-integrations/SKILL.md index 0975cdd38c5..8be12f64815 100644 --- a/.agents/skills/apm-integrations/SKILL.md +++ b/.agents/skills/apm-integrations/SKILL.md @@ -60,9 +60,11 @@ pattern before writing new code. Use it as a template. Read [Context-Tracking Instrumentation](references/context-tracking.md) and decide whether the library needs `InstrumenterModule.Tracing` (I/O operations that create spans) or `InstrumenterModule.ContextTracking` (async-boundary bridging, no spans). +**If you picked `ContextTracking`:** the same file's "Library-native context maps" section tells you whether the library needs a `*ContextBridge`-style helper (Reactor, Kotlin coroutines, JAX-RS, Vert.x all do) in addition to the subscriber-wrapping pattern — read it before moving to Step 5. Its "Wrap placement" section covers where to allocate wrappers and context-store entries to avoid per-operator overhead on hot reactive paths. + ## Step 5 – Write the InstrumenterModule -**Read [InstrumenterModule Guidance](references/instrumenter-module.md).** +**Read [InstrumenterModule Guidance](references/instrumenter-module.md).** If you're editing an existing module rather than writing a new one, its "Editing an existing module" note applies — read the current file first and change only what the task requires. ## Step 6 – Write the Decorator @@ -77,7 +79,7 @@ Read [Context-Tracking Instrumentation](references/context-tracking.md) and deci ## Step 7 – Write the Advice class (highest-risk step) -**Read [Writing the Advice Class](references/advice-class.md) — the highest-risk step.** Pay particular attention to: `@Advice.OnMethodEnter/Exit` annotations; `CallDepthThreadLocalMap` reentrancy guarding; span lifecycle order; and the "Must NOT do" list. +**Read [Writing the Advice Class](references/advice-class.md) — the highest-risk step.** Pay particular attention to: `@Advice.OnMethodEnter/Exit` annotations; `CallDepthThreadLocalMap` reentrancy guarding; span lifecycle order; and the "Must NOT do" list. If the target wraps or delegates to another async client, its "Do not double-span async HTTP clients" section covers whether to open a second span or rely on context-propagation-only advice. If a returned `CompletableFuture`/`CompletionStage` needs a completion callback, see [Context-Tracking Instrumentation](references/context-tracking.md)'s "Preserving cancellation" section for the read-only-return + named-callback pattern — do not reassign the future with `future = future.whenComplete(...)`. ## Step 8 – Add SETTER/GETTER adapters (if applicable) @@ -95,10 +97,12 @@ Cover all mandatory test types: ### 2. Muzzle directives (mandatory) -**Read [Muzzle Directives](references/muzzle.md)** — it covers all three valid patterns and their `assertInverse` rules. Search adjacent module `build.gradle` files for `skipVersions` before declaring a new version-bounded module's muzzle directives. +**Read [Muzzle Directives](references/muzzle.md)** — it covers all three valid patterns and their `assertInverse` rules. Search adjacent module `build.gradle` files for `skipVersions` before declaring a new version-bounded module's muzzle directives. **If a prior-major-version sibling module already exists in the repo** (e.g. you're writing `rxjava-3.0` and `rxjava-2.0` exists), add the "Namespace-isolation `fail` block" that section describes — it's not optional, it's how CI catches accidental cross-version advice matching. ### 3. Latest dependency test (mandatory) +If the library's API surface changes across minor versions (deprecated/removed methods, changed signatures), see [Writing Tests](references/tests.md)'s "Version-sensitive tests belong in a separate `latestDepTest` source set" section for which tests belong in `src/test/` vs `src/latestDepTest/`. + Use `latestDepTestImplementation` in `build.gradle` to pin the latest available version. Run with: ```bash ./gradlew :dd-java-agent:instrumentation:$framework:$framework-$version:latestDepTest diff --git a/.agents/skills/apm-integrations/references/context-tracking.md b/.agents/skills/apm-integrations/references/context-tracking.md index b83f45664fc..ae42c28fd49 100644 --- a/.agents/skills/apm-integrations/references/context-tracking.md +++ b/.agents/skills/apm-integrations/references/context-tracking.md @@ -77,7 +77,7 @@ When a library has a first-class context-map concept, the observer/subscriber wr **How to detect the pattern in an unfamiliar library:** search the library's public API for a `Context` type with `put`/`get`/`hasKey` methods that user code can write to (as opposed to a Datadog-internal context). If such a type exists, the library has a native context map and needs a `*ContextBridge` helper. -**When regenerating an existing reactive module:** enumerate every `*Instrumentation.java` in the master module and account for each one. Dropping any without a documented reason is always a Rule #2 (regen-preservation) violation. Also preserve the master's `*ContextBridge`-style helper verbatim, or produce equivalent semantics — do not drop it in favor of the subscriber-wrapping pattern alone. +**Editing an existing reactive module:** account for every `*Instrumentation.java` class already present, including the `*ContextBridge` helper — don't drop it in favor of the subscriber-wrapping pattern alone. A dropped bridge silently breaks any sibling module that depends on it (see `instrumenter-module.md`). ## Wrap placement and context-store lifecycle — memory considerations diff --git a/.agents/skills/apm-integrations/references/instrumenter-module.md b/.agents/skills/apm-integrations/references/instrumenter-module.md index 1f21ad30747..b765157c8ed 100644 --- a/.agents/skills/apm-integrations/references/instrumenter-module.md +++ b/.agents/skills/apm-integrations/references/instrumenter-module.md @@ -79,141 +79,10 @@ Users can then set `DD_TRACE_OKHTTP_ENABLED=false` (group off) OR `DD_TRACE_OKHT **New module you expect will soon have a sibling** — add the alias upfront and document why in the commit message. If no sibling appears, drop the alias in a follow-up. -**Existing module** (modifying, refactoring, or splitting): read the existing module's `super(...)` and copy it verbatim. Integration names are public config API — renaming one silently breaks customer `DD_TRACE_*_ENABLED` settings. - -Before choosing, list the `dd-java-agent/instrumentation/$framework/` directory — the contents are the ground truth for whether siblings exist. +**Editing an existing module:** read it first, then change only what the task requires. Copy `super(...)` verbatim — integration names are public config API, and silently changing the number of arguments or a string value breaks customer `DD_TRACE_*_ENABLED` settings. The same "read before touching, preserve unless you have a reason to change it" rule covers everything else on the class: overridden methods (`defaultEnabled()`, `helperClassNames()`, `contextStore()`, `orderPriority()`), the Java package, the set and order of production classes, and array/map entry ordering. Don't rename, reorder, drop, or restructure any of it as a side effect of regenerating the file — every one of those looks harmless in isolation but reads as an unexplained regression to a reviewer, and some (like a dropped `*ContextBridge` helper — see `context-tracking.md`) silently break sibling modules that reference the class by FQN. Do NOT add version aliases to the decorator's `instrumentationNames()` — that method is for analytics keys only. -**When rewriting or refactoring an existing module, preserve every override the master version has.** Not just `super(...)` — also `defaultEnabled()`, `helperClassNames()`, `contextStore()`, `orderPriority()`, `muzzleDirective()`, and any other overridden method. Read the current file on master before writing; carry each override forward verbatim unless there's a documented reason to change it. Silent loss of `defaultEnabled() = false` (or similar opt-in flags) ships an integration with a different default than users expected. - -**Concrete failure pattern (from dd-trace-java PR #11939, rxjava-3.0 regen):** master has - -```java -public RxJavaModule() { - super("rxjava", "rxjava-3"); // ← two args: family name + version alias -} -``` - -An eval regenerated this as - -```java -public RxJavaModule() { - super("rxjava"); // ❌ dropped "rxjava-3" version alias -} -``` - -Impact: customers who set `DD_TRACE_RXJAVA_3_ENABLED=false` to opt out silently lose that opt-out — the flag stops being recognized. No CI check catches it; the regression is only visible when a customer tries to disable the integration. **When regenerating a module that already exists, both the number of arguments AND the exact string values of `super(...)` must be preserved verbatim.** - -### Package layout must be preserved verbatim on regen - -When regenerating an existing module, use the exact same Java package for every production class. Master's package is authoritative; do not rename, consolidate, or shorten. - -**Concrete failure pattern (from dd-trace-java PR #11940, reactor-core-3.1 regen):** master's production classes live under `datadog.trace.instrumentation.reactor.core`. An eval regenerated the module under `datadog.trace.instrumentation.reactorcore` (concatenated form, chosen by analogy with `datadog.trace.instrumentation.rxjava3`). - -Consequences: -- Silently breaks fully-qualified class references from other modules. In this case, `dd-java-agent/instrumentation/graal/graal-native-image-20.0/` has a `NativeImageGeneratorRunnerInstrumentation` that lists `datadog.trace.instrumentation.reactor.core.ReactorAsyncResultExtension` in its build-time classlist by name — the rename breaks Graal native-image builds that depend on that classlist entry. -- Confuses reviewers and merge-conflict resolution when comparing eval output to master. -- Doesn't reduce the diff size or improve any measurable outcome — it's a purely stylistic choice the model made without prompting. - -**Rule:** when regenerating an existing module, the top-level Java package of every production class MUST match master exactly. If master uses dotted style (`datadog.trace.instrumentation.reactor.core`), preserve it; if master uses concatenated style (`datadog.trace.instrumentation.rxjava3`), preserve it. Master is the invariant. - -### Enumerate all master `*Instrumentation.java` classes on regen - -When regenerating an existing module, list every `*Instrumentation.java`, `*Bridge.java`, and helper class currently in master's `src/main/java/.../` directory. Every one of those classes must be either preserved verbatim in the output OR explicitly replaced with an equivalent class. **Dropping a master class without a documented reason is always a Rule #2 violation.** - -**Concrete failure pattern (from PR #11940):** master's `reactor-core-3.1` has 7 production classes: - -``` -ReactorCoreModule.java -ReactorContextBridge.java (context-map bridge — see context-tracking.md) -ReactorAsyncResultExtension.java -BlockingPublisherInstrumentation.java -ContextWritingSubscriberInstrumentation.java -CorePublisherInstrumentation.java -OptimizableOperatorInstrumentation.java -``` - -The eval output kept only 2 (`ReactorCoreModule`, `ReactorAsyncResultExtension`) and added 3 new ones (`FluxInstrumentation`, `MonoInstrumentation`, `TracingCoreSubscriber`). Net effect: 5 master classes silently dropped, including `ReactorContextBridge` — which is what breaks Spring WebFlux, Spring Kafka reactive, and other downstream Reactor-based libraries. No CI check on the target module catches it; the regression only surfaces when sibling-module tests fail. - -**How to apply this rule:** before generating, enumerate every production source file in the existing module — **not just `src/main/java`**. Several modules keep production classes in version-specific or JVM-specific source sets: - -- `src/main/java17` — Java 17-only APIs (`kafka-clients-3.8`, `jetty-server-12.0`) -- `src/main/java11` — Java 11-only APIs (some HTTP-client modules) -- `src/main/groovy` — Groovy production classes (rare, but present in some modules) -- `src/main/scala` — Scala production classes (Play framework, some Akka modules) - -A regen that walks only `src/main/java` will silently miss version-specific instrumentation classes and lose them from the output. Use this command instead: - -``` -find dd-java-agent/instrumentation//src/main \ - \( -name "*.java" -o -name "*.groovy" -o -name "*.scala" -o -name "*.kt" \) \ - -type f -``` - -Record each filename. After generating, diff that list against the classes in your output. Any master class not present in the output must be explicitly justified in the PR description. - -### Preserve declarative-array ordering (`helperClassNames`, `contextStore` keys) - -When regenerating an existing `InstrumenterModule`, preserve the exact order of entries in `helperClassNames()` and the exact order of `store.put(...)` calls in `contextStore()`. Reordering entries with no semantic change produces noisy diffs that reviewers correctly reject as "meaningless reshuffling." - -**Concrete failure pattern (from PR #11939, @ygree review):** the eval output re-sorted `helperClassNames()` — same set of entries, different order. This adds review burden (reviewer must verify the set is unchanged) with zero benefit. - -```java -// ❌ Reordered without reason -return new String[] { - packageName + ".TracingObserver", - packageName + ".TracingSubscriber", - packageName + ".TracingSingleObserver", - packageName + ".TracingMaybeObserver", - packageName + ".TracingCompletableObserver", // was first in master - packageName + ".RxJavaAsyncResultExtension", -}; - -// ✅ Preserve master's ordering -return new String[] { - packageName + ".TracingCompletableObserver", - packageName + ".TracingObserver", - packageName + ".TracingSubscriber", - packageName + ".TracingSingleObserver", - packageName + ".TracingMaybeObserver", - packageName + ".RxJavaAsyncResultExtension", -}; -``` - -Only reorder when adding or removing an entry for a semantic reason (a helper is being introduced or retired). - -### Hoist repeated `Class.getName()` calls in `contextStore()` - -When multiple `store.put(...)` calls in `contextStore()` use the same value-class FQN, hoist `SomeClass.class.getName()` into a single local variable. This is the idiomatic pattern in existing dd-trace-java modules; inlining the same call five times is treated as a regression by reviewers. - -```java -// ❌ Inlined at every put site -public Map contextStore() { - final Map store = new HashMap<>(); - store.put("io.reactivex.rxjava3.core.Observable", Context.class.getName()); - store.put("io.reactivex.rxjava3.core.Flowable", Context.class.getName()); - store.put("io.reactivex.rxjava3.core.Single", Context.class.getName()); - store.put("io.reactivex.rxjava3.core.Maybe", Context.class.getName()); - store.put("io.reactivex.rxjava3.core.Completable", Context.class.getName()); - return store; -} - -// ✅ Hoisted once -public Map contextStore() { - String contextClass = Context.class.getName(); - final Map store = new HashMap<>(); - store.put("io.reactivex.rxjava3.core.Observable", contextClass); - store.put("io.reactivex.rxjava3.core.Flowable", contextClass); - store.put("io.reactivex.rxjava3.core.Single", contextClass); - store.put("io.reactivex.rxjava3.core.Maybe", contextClass); - store.put("io.reactivex.rxjava3.core.Completable", contextClass); - return store; -} -``` - -Source: @ygree review on PR #11939. - ### Do not create a helper class just for CallDepthThreadLocalMap when only one type is instrumented When only one type is being instrumented, use `CallDepthThreadLocalMap` directly in the Advice class. A separate helper class that just wraps `CallDepthThreadLocalMap.incrementCallDepth` / `decrementCallDepth` adds indirection without value: diff --git a/.agents/skills/apm-integrations/references/muzzle.md b/.agents/skills/apm-integrations/references/muzzle.md index 55bb8f09824..659ec6ce676 100644 --- a/.agents/skills/apm-integrations/references/muzzle.md +++ b/.agents/skills/apm-integrations/references/muzzle.md @@ -175,11 +175,7 @@ When in doubt, **search adjacent module build.gradle files for `skipVersions`** ## Namespace-isolation `fail` blocks for major-version siblings -When a library has multiple major versions that must never be instrumented by the same advice — whether published under different `group:module` coordinates (e.g., `io.reactivex.rxjava2:rxjava` vs `io.reactivex.rxjava3:rxjava`) or under the same coordinates at incompatible major versions (e.g., `org.springframework:spring-webflux` at major 5 vs 6) — master modules explicitly assert namespace isolation with a `muzzle { fail { ... } }` block. - -The block is defense-in-depth: it catches accidental cross-version advice matching that would otherwise pass silently. When regenerating a module that has such a block, preserve it verbatim. - -**Concrete failure pattern (from dd-trace-java PR #11939, rxjava-3.0 regen):** master's `rxjava-3.0/build.gradle` has: +When a library has multiple major versions that must never be instrumented by the same advice — whether published under different `group:module` coordinates (e.g., `io.reactivex.rxjava2:rxjava` vs `io.reactivex.rxjava3:rxjava`) or under the same coordinates at incompatible major versions (e.g., `org.springframework:spring-webflux` at major 5 vs 6) — assert namespace isolation with a `muzzle { fail { ... } }` block: ```groovy muzzle { @@ -200,67 +196,10 @@ muzzle { } ``` -An eval regenerated this WITHOUT the `fail` block. Muzzle would still likely fail naturally on rxjava2 (the FQNs don't exist in that artifact), but the explicit assertion is what catches the failure at CI time with a specific error message rather than a generic muzzle mismatch. - -**Rule:** for any module whose brand has a prior major version — **whether published under different Maven coordinates or under the same coordinates at an incompatible major** — check master for a `muzzle { fail { name = "..." } }` block. If present, preserve verbatim on regen. When creating a new module for a library that has a prior-major sibling module in the repo, add such a fail block to assert non-overlap. - -Common cases where this applies: - -- **Different Maven coordinates:** `rxjava-2.0` (`io.reactivex.rxjava2:rxjava`) ↔ `rxjava-3.0` (`io.reactivex.rxjava3:rxjava`); `javax-jms-*` ↔ `jakarta-jms-*`; `spring-webflux-5.0` (`org.springframework:spring-webflux`) ↔ `spring-webflux-6.0` (`org.springframework:spring-webflux`, but Jakarta EE 9+ package rename). -- **Same Maven coordinates, incompatible majors:** `okhttp-2.0` ↔ `okhttp-3.0` (both `com.squareup.okhttp*`, package renamed at v3); `jedis-1.4` ↔ `jedis-3.0` ↔ `jedis-4.0` (all `redis.clients:jedis`, API changed across majors); `jetty-server-7.0` ↔ `jetty-server-9.0.4` ↔ `jetty-server-11.0` (all `org.eclipse.jetty:jetty-server`, `javax.servlet` → `jakarta.servlet` at 11+). - -For same-coordinate cases, the `fail` block still applies: it asserts the older major's version range does NOT match the newer module's advice, even though the coordinates are identical. Look for `versions = "[,3.0.0)"` bounded ranges in the master's `fail` block, not a coordinate difference. - -## Preserve `compileOnly` dependency versions on regen - -When regenerating an existing module, preserve the exact version of every `compileOnly` dependency in `build.gradle`. Silently narrowing a compileOnly version reduces the tested API surface without any warning — the module still compiles and tests still pass because the older version is a subset. - -**Concrete failure pattern (from dd-trace-java PR #11939, rxjava-3.0 regen):** - -```groovy -// Master -dependencies { - compileOnly group: 'org.reactivestreams', name: 'reactive-streams', version: '1.0.3' - compileOnly group: 'io.reactivex.rxjava3', name: 'rxjava', version: '3.0.0' -} - -// Eval regenerated as -dependencies { - compileOnly group: 'org.reactivestreams', name: 'reactive-streams', version: '1.0.0' // ❌ regressed - compileOnly group: 'io.reactivex.rxjava3', name: 'rxjava', version: '3.0.0' -} -``` - -`reactive-streams 1.0.3` adds APIs and behavioral clarifications over `1.0.0` that dd-trace-java's advice may rely on. Downgrading silently removes them from the tested surface. - -**Rule:** all `compileOnly` versions must match master verbatim on regen. This complements the existing `testImplementation` version parity rule — the same principle applies at the compile scope. - -## Preserve test-scope build.gradle dependencies on regen - -When regenerating an existing module, preserve every test-scope dependency verbatim — **including runtime-only scopes** — unless the corresponding test file is also being removed. The full set to preserve is: - -- `testImplementation`, `latestDepTestImplementation`, `forkedTestImplementation` (compile-scope test deps) -- `testRuntimeOnly`, `latestDepTestRuntimeOnly`, `forkedTestRuntimeOnly` (runtime-only test deps — **these do NOT show up as compile failures if dropped**) - -Do not drop cross-module test dependencies — they back annotation-driven, cross-tracer interop, and cross-instrumentation-coexistence tests that silently fail to compile or run without them. - -**Why runtime-only scopes matter:** modules like `rxjava-3.0` declare `testRuntimeOnly project(':dd-java-agent:instrumentation:rxjava:rxjava-2.0')` to load the rxjava2 instrumenter into the test JVM. This is what verifies rxjava3 tests are NOT accidentally being served by rxjava2's advice (the mutual-exclusion coverage that pairs with the `muzzle { fail { ... } }` block above). Dropping the `testRuntimeOnly` dep still compiles cleanly — but the test JVM no longer has rxjava2's instrumenter loaded, so the mutual-exclusion coverage silently disappears with no CI signal. - -**Concrete failure pattern (from dd-trace-java PR #11940, reactor-core-3.1 regen):** the eval dropped these test dependencies: - -```groovy -testImplementation project(':dd-java-agent:instrumentation:reactive-streams-1.0') -testImplementation project(path: ':dd-java-agent:agent-otel:otel-bootstrap', configuration: 'shadow') -testImplementation project(':dd-java-agent:instrumentation:opentelemetry:opentelemetry-1.4') -testImplementation project(':dd-java-agent:instrumentation:opentelemetry:opentelemetry-annotations-1.20') -testImplementation project(':dd-java-agent:instrumentation:opentracing:opentracing-0.32') -testImplementation group: 'io.opentelemetry.instrumentation', name: 'opentelemetry-instrumentation-annotations', version: '1.28.0' -testImplementation group: 'io.opentracing', name: 'opentracing-util', version: '0.32.0' -latestDepTestImplementation group: 'io.micrometer', name: 'micrometer-core', version: '1.+' -``` +Muzzle would likely fail naturally on the sibling major anyway (the FQNs usually don't exist in that artifact), but the explicit assertion catches it at CI time with a specific error message instead of a generic muzzle mismatch. Add this block whenever a prior-major sibling module already exists in the repo — whether that sibling uses different Maven coordinates (`rxjava-2.0` vs `rxjava-3.0`, `javax-jms-*` vs `jakarta-jms-*`) or the same coordinates at an incompatible major (`okhttp-2.0` vs `okhttp-3.0`, `jedis-1.4`/`jedis-3.0`/`jedis-4.0`). **Editing an existing module that already has one: preserve it verbatim** — don't drop it as a side effect of regenerating the file. -These deps back annotation-driven tests (`@WithSpan`, `@Traced`), cross-module interop tests (`reactive-streams-1.0`), and version-drift workaround deps (`micrometer-core` required by newer Reactor versions). @mcculls flagged all of these as required. +## `compileOnly` and test-scope dependencies -**Rule:** the set of `testImplementation` and `latestDepTestImplementation` entries in a regenerated `build.gradle` must be a superset of master's. If master has cross-module `project(':dd-java-agent:instrumentation:...')` deps, they must be present in the eval output. If master has `latestDepTestImplementation` workaround deps (e.g. `micrometer-core` for Reactor), they must be present. +`compileOnly` pins the API surface your advice compiles against; test scopes (`testImplementation`, `latestDepTestImplementation`, `forkedTestImplementation`, and their `*RuntimeOnly` counterparts) pin what your tests exercise. When writing a new module, choose these deliberately — the version constraint and dependency list are part of the instrumentation's contract, not incidental build config. -Source: @mcculls PR #11940 build.gradle review. +**Editing an existing module: preserve every dependency version and entry unless you have a reason to change it.** This includes runtime-only scopes, which don't show up as compile failures if dropped — e.g. `rxjava-3.0` declares `testRuntimeOnly project(':dd-java-agent:instrumentation:rxjava:rxjava-2.0')` to load the sibling instrumenter into the test JVM and verify the two don't cross-fire (the coexistence coverage that pairs with the namespace-isolation `fail` block above). It also includes cross-module `project(...)` test deps that back annotation-driven tests (`@WithSpan`, `@Traced`) and version-drift workaround deps (e.g. a `latestDepTestImplementation` pin needed only because a newer library version requires it). None of these produce a compile error when silently dropped — they just remove coverage. diff --git a/.agents/skills/apm-integrations/references/tests.md b/.agents/skills/apm-integrations/references/tests.md index be022c6af9e..42fd95c08c5 100644 --- a/.agents/skills/apm-integrations/references/tests.md +++ b/.agents/skills/apm-integrations/references/tests.md @@ -116,23 +116,14 @@ State the isolation reason in a comment on the `ForkedTest` class. ## Version-sensitive tests belong in a separate `latestDepTest` source set -When regenerating an existing module, check if master has a `src/latestDepTest/` source set (declared via `addTestSuite('latestDepTest')` or `addTestSuiteForDir('latestDepTest', ...)` in `build.gradle`). If so, preserve that split — do NOT collapse latestDep-specific tests into the base `src/test/` directory. +For libraries whose API surface changes across minor versions (Reactor deprecates and removes APIs; Netty changes handler signatures; gRPC's generated code evolves), route each test to the source set whose classpath actually satisfies its imports: -**Why this matters:** for libraries whose API surface changes across minor versions (Reactor deprecates and removes APIs; Netty changes signatures; gRPC evolves generated code), tests in `src/test/` compile against both `testImplementation` (pinned to min) and `latestDepTestImplementation` (latest) when the module uses `addTestSuiteForDir('latestDepTest', 'test')` — the common pattern that reuses `src/test/` sources for both suites. For modules that use `addTestSuite('latestDepTest')` instead, the `latestDepTest` suite has its own sources at `src/latestDepTest/` and `src/test/` is not compiled against `latestDepTestImplementation`. In both cases: if a test uses an API removed in a later version, it will cause a `latestDepTest` compile failure. +- **API only exists in the latest version** (added after your pinned min) → put the test in `src/latestDepTest/`, which compiles against `latestDepTestImplementation`. `src/test/` compiles against the pinned min and doesn't have the API. +- **API only exists at the pinned min** (removed in a later version — e.g. Reactor's `Schedulers.elastic()`, removed in 3.4+) → put the test in `src/test/`, NOT `latestDepTest/`. Test the replacement API (`Schedulers.boundedElastic()`) in `latestDepTest/` instead. -Master's solution: put version-sensitive tests in `src/latestDepTest/` where they only compile against `latestDepTestImplementation` and can freely use the current API. When the latest version removes an API, only the `latestDepTest` copy needs updating. +Common libraries where this split matters: Reactor, Netty, gRPC, Kafka clients (consumer API changed at 3.0), Cassandra driver (3.x vs 4.x largely incompatible). -**Concrete failure pattern (from dd-trace-java PR #11940, reactor-core-3.1 regen):** master has `src/latestDepTest/groovy/ReactorCoreTest.groovy` that uses `Schedulers.boundedElastic()` (the current API). The eval collapsed all tests into `src/test/java/` and used `Schedulers.elastic()` (removed in Reactor 3.4+). Result: `:latestDepTest` compilation failure blocking `:check` on every JVM shard. - -**Rule:** -- Before generating tests, `ls src/latestDepTest/` in master's module. If it exists, the regen must include the equivalent source set. -- If master's `build.gradle` has `addTestSuiteForDir('latestDepTest', ...)` or `addTestSuite('latestDepTest')`, preserve that declaration verbatim. -- **Route each test to the source set whose classpath actually satisfies its imports.** Two distinct scenarios drive the split: - - **API is only in the latest version** (added after the pinned min) → the test uses that API, must go in `src/latestDepTest/`. `src/test/` compiles against `testImplementation` (pinned min), where the API is absent — the test would fail to compile there. - - **API is only in the pinned min** (removed in a later version, e.g. Reactor's `Schedulers.elastic()` removed in 3.4+) → the test uses the removed API, must go in `src/test/` and NOT in `latestDepTest/`. Placing it in `src/latestDepTest/` (where the classpath resolves to the latest release) would produce a compile failure like the concrete failure pattern above. Prefer testing the equivalent replacement API (`Schedulers.boundedElastic()`) in `latestDepTest/` instead. -- Common libraries where this split matters: Reactor (`Schedulers.elastic()` removed in 3.4+), Netty (channel handler API changes across 4.x), gRPC (generated-code shape evolves), Kafka clients (consumer API changed 3.0), Cassandra driver (3.x vs 4.x are largely incompatible). - -Source: master's `dd-java-agent/instrumentation/reactor-core-3.1/src/latestDepTest/groovy/ReactorCoreTest.groovy`. +**Editing an existing module:** check for `src/latestDepTest/` and the `addTestSuite('latestDepTest')` / `addTestSuiteForDir('latestDepTest', ...)` declaration in `build.gradle` before touching tests, and preserve the split — collapsing it into `src/test/` produces a `latestDepTest` compile failure the moment a test uses a since-removed API. ## No banner/separator comments in test files From e3c3f17db9598e002c11339501a69bb318f2dc0e Mon Sep 17 00:00:00 2001 From: Jordan Wong <61242306+jordan-wong@users.noreply.github.com> Date: Thu, 23 Jul 2026 15:13:21 -0400 Subject: [PATCH 09/11] heading fix Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .agents/skills/apm-integrations/references/advice-class.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.agents/skills/apm-integrations/references/advice-class.md b/.agents/skills/apm-integrations/references/advice-class.md index f92b8df6ffb..ad140b8d94e 100644 --- a/.agents/skills/apm-integrations/references/advice-class.md +++ b/.agents/skills/apm-integrations/references/advice-class.md @@ -153,7 +153,7 @@ For route-only enrichers: no `AgentScope`, no `startSpan()`, no `decorator.after `*Advice.java` classes are inlined at instrumentation sites; non-constant static fields (fields that aren't `static final` primitives or string literals) get pulled into every instrumented callsite and violate muzzle's assumptions. Keep only `static final` constants — no logger references, no cached decorators, no state. If you need shared state, put it on a helper class registered via `helperClassNames()`, not on the advice. -## No inline explanatory comments in Advice methods +### No inline explanatory comments in Advice methods Do NOT add narrative `//`-comments inside `@Advice.OnMethodEnter` / `@Advice.OnMethodExit` methods to explain *why* wrapping, subscription capture, or span activation happens. If the target helper class (e.g. `TracingSingleObserver`, `TracingRunnable`) already has a class-level Javadoc documenting the intent, duplicating the explanation at the call site is treated as noise by reviewers. From 22d970969a3a054d8a141a3743ba3755b6cd01ad Mon Sep 17 00:00:00 2001 From: Jordan Wong <61242306+jordan-wong@users.noreply.github.com> Date: Thu, 23 Jul 2026 15:14:23 -0400 Subject: [PATCH 10/11] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .agents/skills/apm-integrations/references/tests.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.agents/skills/apm-integrations/references/tests.md b/.agents/skills/apm-integrations/references/tests.md index 42fd95c08c5..3693b06c9c4 100644 --- a/.agents/skills/apm-integrations/references/tests.md +++ b/.agents/skills/apm-integrations/references/tests.md @@ -119,7 +119,7 @@ State the isolation reason in a comment on the `ForkedTest` class. For libraries whose API surface changes across minor versions (Reactor deprecates and removes APIs; Netty changes handler signatures; gRPC's generated code evolves), route each test to the source set whose classpath actually satisfies its imports: - **API only exists in the latest version** (added after your pinned min) → put the test in `src/latestDepTest/`, which compiles against `latestDepTestImplementation`. `src/test/` compiles against the pinned min and doesn't have the API. -- **API only exists at the pinned min** (removed in a later version — e.g. Reactor's `Schedulers.elastic()`, removed in 3.4+) → put the test in `src/test/`, NOT `latestDepTest/`. Test the replacement API (`Schedulers.boundedElastic()`) in `latestDepTest/` instead. +- **API only exists at the pinned min** (removed in a later version — e.g. Reactor's `Schedulers.elastic()`, removed in 3.5+) → put the test in `src/test/`, NOT `latestDepTest/`. Test the replacement API (`Schedulers.boundedElastic()`) in `latestDepTest/` instead. Common libraries where this split matters: Reactor, Netty, gRPC, Kafka clients (consumer API changed at 3.0), Cassandra driver (3.x vs 4.x largely incompatible). From 273129371af17cbe3376718c4af361c66ddab188 Mon Sep 17 00:00:00 2001 From: Jordan Wong Date: Thu, 23 Jul 2026 15:40:06 -0400 Subject: [PATCH 11/11] =?UTF-8?q?skill(apm-integrations):=20fix=204=20fact?= =?UTF-8?q?ual=20errors=20from=20Codex=20review=20=E2=80=94=20verified=20a?= =?UTF-8?q?gainst=20dd-trace-java=20source?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex flagged (2026-07-23) that the previous fix broadening the ContextBridge rule beyond Reactor invented plausible-sounding but factually wrong specifics for 3 libraries. Verified each claim against actual dd-trace-java modules before fixing: - JAX-RS: SKILL.md listed it as needing a ContextTracking bridge, but AbstractRequestContextInstrumentation extends InstrumenterModule.Tracing — it's span-owning code, not context-tracking. Removed the JAX-RS mention from SKILL.md and context-tracking.md entirely. - Kotlin coroutines: told agents to key a ContextStore by Continuation/CoroutineContext, but KotlinCoroutinesModule has no contextStore() at all — it uses ThreadContextElement (DatadogThreadContextElement implements ThreadContextElement). Corrected to point at the actual pattern. - Vert.x: told agents to key by io.vertx.core.Context, but that type is shared across many handler executions on the same Vert.x context — existing vertx-web instrumentation keys per-request state on RoutingContext instead. Keying by core Context would cause cross-request parentage. Corrected to point at RoutingContext. - Reactor wrap-placement: the "prefer subscriber over publisher key" guidance implied these were alternatives, but master's ReactorCoreModule.contextStore() keys BOTH Publisher (via HandoffContext, read before a subscriber exists) AND Subscriber (via Context, for the wrapping pattern) — they're complementary, not a choice. Reworded so the publisher-keyed store is called out as exempt from the "prefer bounded lifetime" optimization. Also fixed a latestDepTest gap Codex found: the prior wording said to put removed-in-latest-version tests in src/test/, but that's only safe when the module uses addTestSuite('latestDepTest') with separate sources. Modules using addTestSuiteForDir('latestDepTest', 'test') reuse src/test/ for both suites, so a removed-API test placed there still gets compiled against latestDepTestImplementation and fails the same way. Split the guidance by which build.gradle wiring the module uses. Root cause: the earlier fix (responding to a prior Codex comment about Reactor-only bias) generalized to other libraries from category-level reasoning ("JAX-RS surely uses a context map like Reactor does") rather than reading those libraries' actual dd-trace-java modules first. Fixed per the java-eval-research skill's canonical-first rule: don't publish a claim about library-specific behavior without checking the source. Co-Authored-By: Claude Sonnet 5 --- .agents/skills/apm-integrations/SKILL.md | 2 +- .../references/context-tracking.md | 17 ++++++----------- .../skills/apm-integrations/references/tests.md | 8 ++++---- 3 files changed, 11 insertions(+), 16 deletions(-) diff --git a/.agents/skills/apm-integrations/SKILL.md b/.agents/skills/apm-integrations/SKILL.md index 8be12f64815..5bacedebeaa 100644 --- a/.agents/skills/apm-integrations/SKILL.md +++ b/.agents/skills/apm-integrations/SKILL.md @@ -60,7 +60,7 @@ pattern before writing new code. Use it as a template. Read [Context-Tracking Instrumentation](references/context-tracking.md) and decide whether the library needs `InstrumenterModule.Tracing` (I/O operations that create spans) or `InstrumenterModule.ContextTracking` (async-boundary bridging, no spans). -**If you picked `ContextTracking`:** the same file's "Library-native context maps" section tells you whether the library needs a `*ContextBridge`-style helper (Reactor, Kotlin coroutines, JAX-RS, Vert.x all do) in addition to the subscriber-wrapping pattern — read it before moving to Step 5. Its "Wrap placement" section covers where to allocate wrappers and context-store entries to avoid per-operator overhead on hot reactive paths. +**If you picked `ContextTracking`:** the same file's "Library-native context maps" section tells you whether the library needs a `*ContextBridge`-style helper (Reactor is the canonical example) in addition to the subscriber-wrapping pattern — read it before moving to Step 5. Its "Wrap placement" section covers where to allocate wrappers and context-store entries to avoid per-operator overhead on hot reactive paths. ## Step 5 – Write the InstrumenterModule diff --git a/.agents/skills/apm-integrations/references/context-tracking.md b/.agents/skills/apm-integrations/references/context-tracking.md index ae42c28fd49..6c2b94f91b8 100644 --- a/.agents/skills/apm-integrations/references/context-tracking.md +++ b/.agents/skills/apm-integrations/references/context-tracking.md @@ -57,25 +57,20 @@ When a boundary type exposes multiple overloads of the subscribe / invoke method ## Library-native context maps — the `*ContextBridge` pattern -Some libraries expose their own request-scoped context map that users write to explicitly. Examples: +Some Reactive-Streams libraries expose their own request-scoped context map that users write to explicitly — Reactor's `reactor.util.context.Context` (immutable per-subscription map) is the canonical example; users pass a span via `.contextWrite(ctx -> ctx.put("dd.span", span))`. -- **Reactor** — `reactor.util.context.Context` (immutable per-subscription map); users pass a span via `.contextWrite(ctx -> ctx.put("dd.span", span))`. -- **Kotlin coroutines** — `CoroutineContext` with custom `CoroutineContextElement`. -- **JAX-RS** — `ContainerRequestContext` for per-request state. -- **Vert.x** — `Context.putLocal(...)`. - -When a library has a first-class context-map concept, the observer/subscriber wrapping pattern documented above is **not sufficient by itself** — it only handles the "capture the currently-active trace context at subscribe time" case. It does NOT handle the case where a user has placed a span in the library's own context map and expects that span to propagate through the operator chain. +When a library has a first-class context-map concept like this, the observer/subscriber wrapping pattern documented above is **not sufficient by itself** — it only handles the "capture the currently-active trace context at subscribe time" case. It does NOT handle the case where a user has placed a span in the library's own context map and expects that span to propagate through the operator chain. **A context-tracking module for such a library MUST include a `*ContextBridge`-style helper** that: 1. Reads a well-known key (Datadog convention: `"dd.span"`) from the library-native context. 2. Adapts the retrieved object (which implements `WithAgentSpan` or is an `AgentSpan`) into a `datadog.context.Context`. -3. Stores that context in the toolkit-native `ContextStore` keyed by the appropriate library-specific lifecycle type. **For Reactor / Reactive-Streams libraries**, the key is `Publisher` or `Subscriber`. **For non-Reactive-Streams libraries**, key by the library's own lifecycle primitive: JAX-RS → `ContainerRequestContext`; Vert.x → `io.vertx.core.Context`; Kotlin coroutines → the `Continuation` or `CoroutineContext` element. Do not force a Reactor-style `Publisher`/`Subscriber` key onto a library that doesn't expose those types. -4. Activates the stored context at the right lifecycle boundaries. For Reactive-Streams: on subscribe, on signal delivery, on blocking, and on internal subscriber handoff (e.g. Reactor's fused-operator path). For other shapes, activate at whatever boundaries the library exposes (request-start / request-end for JAX-RS; coroutine resume/suspend for Kotlin; verticle handler entry for Vert.x). +3. Stores that context in the toolkit-native `ContextStore`. For Reactor specifically, master keys **both** `Publisher` (via `HandoffContext`, for the explicit `dd.span` bridge — read by the blocking/subscribe advices before a subscriber may exist) **and** `Subscriber` (via `Context`, for the wrapping pattern) — these are complementary, not alternatives. Read the target library's own context-propagation surface before assuming Reactor's keying scheme transfers directly; a library with a different context/lifecycle shape (e.g. one that carries context via a thread-local element rather than a per-subscription map) needs its own keying, not a forced `Publisher`/`Subscriber` pair. +4. Activates the stored context at the right lifecycle boundaries: on subscribe, on signal delivery, on blocking, and on internal subscriber handoff (e.g. Reactor's fused-operator path). **Reference:** `dd-java-agent/instrumentation/reactor-core-3.1/` — master has `ReactorContextBridge.java` plus four supporting instrumentations that hook the specific lifecycle points: `BlockingPublisherInstrumentation` (for `.block()` / `.blockFirst()` / `.blockLast()` on the calling thread), `ContextWritingSubscriberInstrumentation` (for `.contextWrite(...)` subscribers at subscribe time), `CorePublisherInstrumentation` (for the base publisher interface handing off to downstream subscribers), and `OptimizableOperatorInstrumentation` (for Reactor's internal fused-operator optimization path). Regenerating this module without preserving these classes silently breaks downstream libraries — Spring WebFlux, Spring Kafka reactive `@KafkaListener suspend fun`, resilience4j-reactor, reactor-netty — that rely on the `dd.span` propagation path. -**How to detect the pattern in an unfamiliar library:** search the library's public API for a `Context` type with `put`/`get`/`hasKey` methods that user code can write to (as opposed to a Datadog-internal context). If such a type exists, the library has a native context map and needs a `*ContextBridge` helper. +**How to detect the pattern in an unfamiliar library:** search the library's public API for a `Context` type with `put`/`get`/`hasKey` methods that user code can write to (as opposed to a Datadog-internal context). If such a type exists, the library has a native context map and needs a `*ContextBridge` helper. If the library instead carries context via a thread-local mechanism (e.g. Kotlin coroutines' `ThreadContextElement`) or a per-request object already used for span ownership elsewhere in the codebase (e.g. Vert.x web's `RoutingContext`), follow that library's existing pattern instead of introducing a `ContextStore`-backed bridge — read the sibling instrumentation module for that library first. **Editing an existing reactive module:** account for every `*Instrumentation.java` class already present, including the `*ContextBridge` helper — don't drop it in favor of the subscriber-wrapping pattern alone. A dropped bridge silently breaks any sibling module that depends on it (see `instrumenter-module.md`). @@ -89,7 +84,7 @@ Context-tracking instrumentations allocate two kinds of long-lived state that ad Place both at the narrowest scope that preserves correctness: - **Wrap only at chain boundaries** — subscribe, `.contextWrite(...)`, `.block()` — not at every internal operator. Operator-level wrapping causes N-fold allocations for an N-operator chain and does not add propagation coverage that boundary wrapping doesn't already provide. -- **Use Subscriber-lifecycle context stores** — the store should drop entries when the subscription completes or cancels. `ContextStore` implementations in dd-trace-java use field injection on the key instance when possible (the common fast path), or a weak-map fallback when field injection is unavailable. In either case, choose a key with a bounded lifetime: use the subscriber (scoped to one subscription) rather than the publisher (which may be shared and long-lived across many subscriptions). +- **Prefer subscriber-lifecycle context stores when the store's only purpose is the wrapping pattern** — the store should drop entries when the subscription completes or cancels. `ContextStore` implementations in dd-trace-java use field injection on the key instance when possible (the common fast path), or a weak-map fallback when field injection is unavailable. Prefer a key with a bounded lifetime — the subscriber (scoped to one subscription) — over the publisher (which may be shared and long-lived across many subscriptions), **unless the library-native context map requires a publisher-keyed store for its own reasons** (e.g. Reactor's `Publisher → HandoffContext` bridge is read before a subscriber exists — see "Library-native context maps" above; that store is not a candidate for this optimization). - **Avoid double-wrapping** — when a downstream operator already carries the context via the library-native context map (e.g., Reactor's `Context` flows through the whole operator chain by construction), do not add a per-operator wrap on top. **Why this matters:** a 10-operator Reactor chain with per-operator wrapping allocates 10× the wrappers of a boundary-only implementation, and every allocation must be reclaimed when the subscription completes. In steady-state reactive services, that's the difference between context-tracking being invisible in profiling and being a measurable overhead. diff --git a/.agents/skills/apm-integrations/references/tests.md b/.agents/skills/apm-integrations/references/tests.md index 3693b06c9c4..c1be9b8cc07 100644 --- a/.agents/skills/apm-integrations/references/tests.md +++ b/.agents/skills/apm-integrations/references/tests.md @@ -116,14 +116,14 @@ State the isolation reason in a comment on the `ForkedTest` class. ## Version-sensitive tests belong in a separate `latestDepTest` source set -For libraries whose API surface changes across minor versions (Reactor deprecates and removes APIs; Netty changes handler signatures; gRPC's generated code evolves), route each test to the source set whose classpath actually satisfies its imports: +For libraries whose API surface changes across minor versions (Reactor deprecates and removes APIs; Netty changes handler signatures; gRPC's generated code evolves), route each test to the source set whose classpath actually satisfies its imports. First check how the module wires `latestDepTest` in `build.gradle`: -- **API only exists in the latest version** (added after your pinned min) → put the test in `src/latestDepTest/`, which compiles against `latestDepTestImplementation`. `src/test/` compiles against the pinned min and doesn't have the API. -- **API only exists at the pinned min** (removed in a later version — e.g. Reactor's `Schedulers.elastic()`, removed in 3.5+) → put the test in `src/test/`, NOT `latestDepTest/`. Test the replacement API (`Schedulers.boundedElastic()`) in `latestDepTest/` instead. +- **`addTestSuite('latestDepTest')`** — `latestDepTest` has its own sources at `src/latestDepTest/`, separate from `src/test/`. In this shape: put latest-only APIs (added after your pinned min) in `src/latestDepTest/`; put removed-in-latest APIs (e.g. Reactor's `Schedulers.elastic()`, removed in 3.5+) in `src/test/`, testing the replacement API (`Schedulers.boundedElastic()`) in `latestDepTest/` instead. +- **`addTestSuiteForDir('latestDepTest', 'test')`** — `latestDepTest` reuses `src/test/`'s sources and compiles them against the latest classpath too. In this shape, `src/test/` is NOT a safe place for a removed-in-latest-API test — it still gets compiled against `latestDepTestImplementation` and will fail the same way. A test that exercises a removed API needs the module to declare a real separate `latestDepTest` source set instead (switch to `addTestSuite('latestDepTest')`), or the test needs to avoid the removed API entirely (e.g. call the replacement API and assert equivalent behavior). Common libraries where this split matters: Reactor, Netty, gRPC, Kafka clients (consumer API changed at 3.0), Cassandra driver (3.x vs 4.x largely incompatible). -**Editing an existing module:** check for `src/latestDepTest/` and the `addTestSuite('latestDepTest')` / `addTestSuiteForDir('latestDepTest', ...)` declaration in `build.gradle` before touching tests, and preserve the split — collapsing it into `src/test/` produces a `latestDepTest` compile failure the moment a test uses a since-removed API. +**Editing an existing module:** check for `src/latestDepTest/` and the exact `addTestSuite(...)`/`addTestSuiteForDir(...)` declaration in `build.gradle` before touching tests, and preserve whichever shape master uses. ## No banner/separator comments in test files