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). diff --git a/.agents/skills/apm-integrations/references/context-tracking.md b/.agents/skills/apm-integrations/references/context-tracking.md index d94272beb61..b83f45664fc 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 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. + +**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 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. + ## 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. diff --git a/.agents/skills/apm-integrations/references/instrumenter-module.md b/.agents/skills/apm-integrations/references/instrumenter-module.md index c6ecda9fd0c..1f21ad30747 100644 --- a/.agents/skills/apm-integrations/references/instrumenter-module.md +++ b/.agents/skills/apm-integrations/references/instrumenter-module.md @@ -87,6 +87,133 @@ 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, 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 e1f281630cd..55bb8f09824 100644 --- a/.agents/skills/apm-integrations/references/muzzle.md +++ b/.agents/skills/apm-integrations/references/muzzle.md @@ -172,3 +172,95 @@ 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 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: + +```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 — **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.+' +``` + +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. diff --git a/.agents/skills/apm-integrations/references/tests.md b/.agents/skills/apm-integrations/references/tests.md index 2aad929bfe8..be022c6af9e 100644 --- a/.agents/skills/apm-integrations/references/tests.md +++ b/.agents/skills/apm-integrations/references/tests.md @@ -113,3 +113,76 @@ 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), 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. + +**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`. + +## 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.