Merge branch 'master' into dougqh/metrics-arbitrary-tags

Author: dougqh

.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

.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<Object> 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<Object> 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).

.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.

.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:

.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.