skill(apm-integrations): async cycle iteration-2 — 13 rules from #11939/#11940 review feedback

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

.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/<module>/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<String, String> contextStore() {
+  final Map<String, String> 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<String, String> contextStore() {
+  String contextClass = Context.class.getName();
+  final Map<String, String> 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: