From fa0b66f03c38465246c15aeebb2ed6382aa1504c Mon Sep 17 00:00:00 2001 From: Jordan Wong Date: Thu, 16 Jul 2026 02:11:11 -0400 Subject: [PATCH 1/7] 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 2/7] =?UTF-8?q?skill(apm-integrations):=20strengthen=20Rul?= =?UTF-8?q?e=20#2=20for=20regen=20=E2=80=94=20super(),=20package,=20classe?= =?UTF-8?q?s,=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 3/7] 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 4/7] 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 5/7] 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 6/7] =?UTF-8?q?skill(apm-integrations):=20address=20Copilo?= =?UTF-8?q?t=20review=20on=20#11990=20=E2=80=94=20latestDepTest=20nuance,?= =?UTF-8?q?=20find=20vs=20ls=20glob,=20muzzle=20fail=20scope,=20ContextSto?= =?UTF-8?q?re=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 7/7] =?UTF-8?q?skill(apm-integrations):=20address=20Codex?= =?UTF-8?q?=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`.