diff --git a/.agents/skills/apm-integrations/SKILL.md b/.agents/skills/apm-integrations/SKILL.md index 0975cdd38c5..5bacedebeaa 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 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 -**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/advice-class.md b/.agents/skills/apm-integrations/references/advice-class.md index b75cde27623..ad140b8d94e 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..6c2b94f91b8 100644 --- a/.agents/skills/apm-integrations/references/context-tracking.md +++ b/.agents/skills/apm-integrations/references/context-tracking.md @@ -55,6 +55,40 @@ 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 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))`. + +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`. 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. 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`). + +## 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. +- **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. + ## 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..b765157c8ed 100644 --- a/.agents/skills/apm-integrations/references/instrumenter-module.md +++ b/.agents/skills/apm-integrations/references/instrumenter-module.md @@ -79,14 +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. - ### 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..659ec6ce676 100644 --- a/.agents/skills/apm-integrations/references/muzzle.md +++ b/.agents/skills/apm-integrations/references/muzzle.md @@ -172,3 +172,34 @@ 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) — assert namespace isolation with a `muzzle { fail { ... } }` block: + +```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,)" + } +} +``` + +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. + +## `compileOnly` and test-scope dependencies + +`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. + +**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 2aad929bfe8..c1be9b8cc07 100644 --- a/.agents/skills/apm-integrations/references/tests.md +++ b/.agents/skills/apm-integrations/references/tests.md @@ -113,3 +113,67 @@ 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 + +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`: + +- **`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 exact `addTestSuite(...)`/`addTestSuiteForDir(...)` declaration in `build.gradle` before touching tests, and preserve whichever shape master uses. + +## 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.