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

.agents/skills/apm-integrations/references/advice-class.md

@@ -107,3 +107,36 @@ See the `@AppliesOn Annotation` section of `docs/how_instrumentations_work.md` f
 - **No `InstrumentationContext.get()`** outside of Advice code
 - **No `inline=false`** in production code (only for debugging; must be removed before committing)
 - **No `java.util.logging.*`, `java.nio.file.*`, or `javax.management.*`** in bootstrap instrumentations
+
+## 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 library's reactive-streams primitives (`Publisher`, `Subscriber`).
+4. Activates the stored context at the right lifecycle boundaries: on subscribe, on signal delivery, on blocking, and on internal subscriber handoff (e.g. Reactor's fused-operator path).
+
+**Reference:** `dd-java-agent/instrumentation/reactor-core-3.1/` — master has `ReactorContextBridge.java` plus four supporting instrumentations that hook the specific lifecycle points: `BlockingPublisherInstrumentation` (for `.block()` / `.blockFirst()` / `.blockLast()` on the calling thread), `ContextWritingSubscriberInstrumentation` (for `.contextWrite(...)` subscribers at subscribe time), `CorePublisherInstrumentation` (for the base publisher interface handing off to downstream subscribers), and `OptimizableOperatorInstrumentation` (for Reactor's internal fused-operator optimization path). Regenerating this module without preserving these classes silently breaks downstream libraries — Spring WebFlux, Spring Kafka reactive `@KafkaListener suspend fun`, resilience4j-reactor, reactor-netty — that rely on the `dd.span` propagation path.
+
+**How to detect the pattern in an unfamiliar library:** search the library's public API for a `Context` type with `put`/`get`/`hasKey` methods that user code can write to (as opposed to a Datadog-internal context). If such a type exists, the library has a native context map and needs a `*ContextBridge` helper.
+
+**When regenerating an existing reactive module:** enumerate every `*Instrumentation.java` in the master module and account for each one. Dropping any without a documented reason is always a Rule #2 (regen-preservation) violation. Also preserve the master's `*ContextBridge`-style helper verbatim, or produce equivalent semantics — do not drop it in favor of the subscriber-wrapping pattern alone.
+
+## Wrap placement and context-store lifecycle — memory considerations
+
+Context-tracking instrumentations allocate two kinds of long-lived state that add up on hot reactive paths:
+
+- **Wrapper instances** — one `TracingObserver`/`TracingSubscriber`/etc. per subscribe call.
+- **Context-store entries** — one `ContextStore.put(subscriber, context)` per subscribe call, held until the subscription completes/cancels.
+
+Place both at the narrowest scope that preserves correctness:
+
+- **Wrap only at chain boundaries** — subscribe, `.contextWrite(...)`, `.block()` — not at every internal operator. Operator-level wrapping causes N-fold allocations for an N-operator chain and does not add propagation coverage that boundary wrapping doesn't already provide.
+- **Use Subscriber-lifecycle context stores** — the store should drop entries when the subscription completes or cancels. `ContextStore` implementations in dd-trace-java are weakly-keyed; make sure the key is the subscriber (which has a bounded lifetime) rather than the publisher (which may be shared and long-lived).
+- **Avoid double-wrapping** — when a downstream operator already carries the context via the library-native context map (e.g., Reactor's `Context` flows through the whole operator chain by construction), do not add a per-operator wrap on top.
+
+**Why this matters:** a 10-operator Reactor chain with per-operator wrapping allocates 10× the wrappers of a boundary-only implementation, and every allocation must be reclaimed when the subscription completes. In steady-state reactive services, that's the difference between context-tracking being invisible in profiling and being a measurable overhead.
+
 ## When NOT to write a context-tracking instrumentation
 
 If the library DOES perform I/O — sends HTTP requests, runs DB queries, makes RPC calls, talks to a broker, reads/writes a cache — write a **span-creating instrumentation** (`InstrumenterModule.Tracing`) instead. Context-tracking is only for libraries that coordinate work; the moment there's actual I/O to observe, you want spans around it.

.agents/skills/apm-integrations/references/instrumenter-module.md

@@ -54,6 +54,118 @@ The version alias (e.g. `"jedis-3.0"`) maps to `DD_TRACE_JEDIS_3_0_ENABLED`. Do
 
 **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.
 
+**Concrete failure pattern (from dd-trace-java PR #11939, rxjava-3.0 regen):** master has
+
+```java
+public RxJavaModule() {
+  super("rxjava", "rxjava-3");   // ← two args: family name + version alias
+}
+```
+
+An eval regenerated this as
+
+```java
+public RxJavaModule() {
+  super("rxjava");   // ❌ dropped "rxjava-3" version alias
+}
+```
+
+Impact: customers who set `DD_TRACE_RXJAVA_3_ENABLED=false` to opt out silently lose that opt-out — the flag stops being recognized. No CI check catches it; the regression is only visible when a customer tries to disable the integration. **When regenerating a module that already exists, both the number of arguments AND the exact string values of `super(...)` must be preserved verbatim.**
+
+### Package layout must be preserved verbatim on regen
+
+When regenerating an existing module, use the exact same Java package for every production class. Master's package is authoritative; do not rename, consolidate, or shorten.
+
+**Concrete failure pattern (from dd-trace-java PR #11940, reactor-core-3.1 regen):** master's production classes live under `datadog.trace.instrumentation.reactor.core`. An eval regenerated the module under `datadog.trace.instrumentation.reactorcore` (concatenated form, chosen by analogy with `datadog.trace.instrumentation.rxjava3`).
+
+Consequences:
+- Silently breaks fully-qualified class references from other modules. In this case, `dd-java-agent/instrumentation/graal/graal-native-image-20.0/` has a `NativeImageGeneratorRunnerInstrumentation` that lists `datadog.trace.instrumentation.reactor.core.ReactorAsyncResultExtension` in its build-time classlist by name — the rename breaks Graal native-image builds that depend on that classlist entry.
+- Confuses reviewers and merge-conflict resolution when comparing eval output to master.
+- Doesn't reduce the diff size or improve any measurable outcome — it's a purely stylistic choice the model made without prompting.
+
+**Rule:** when regenerating an existing module, the top-level Java package of every production class MUST match master exactly. If master uses dotted style (`datadog.trace.instrumentation.reactor.core`), preserve it; if master uses concatenated style (`datadog.trace.instrumentation.rxjava3`), preserve it. Master is the invariant.
+
+### Enumerate all master `*Instrumentation.java` classes on regen
+
+When regenerating an existing module, list every `*Instrumentation.java`, `*Bridge.java`, and helper class currently in master's `src/main/java/.../` directory. Every one of those classes must be either preserved verbatim in the output OR explicitly replaced with an equivalent class. **Dropping a master class without a documented reason is always a Rule #2 violation.**
+
+**Concrete failure pattern (from PR #11940):** master's `reactor-core-3.1` has 7 production classes:
+
+```
+ReactorCoreModule.java
+ReactorContextBridge.java                 (context-map bridge — see context-tracking.md)
+ReactorAsyncResultExtension.java
+BlockingPublisherInstrumentation.java
+ContextWritingSubscriberInstrumentation.java
+CorePublisherInstrumentation.java
+OptimizableOperatorInstrumentation.java
+```
+
+The eval output kept only 2 (`ReactorCoreModule`, `ReactorAsyncResultExtension`) and added 3 new ones (`FluxInstrumentation`, `MonoInstrumentation`, `TracingCoreSubscriber`). Net effect: 5 master classes silently dropped, including `ReactorContextBridge` — which is what breaks Spring WebFlux, Spring Kafka reactive, and other downstream Reactor-based libraries. No CI check on the target module catches it; the regression only surfaces when sibling-module tests fail.
+
+**How to apply this rule:** before generating, run `ls dd-java-agent/instrumentation/<module>/src/main/java/**/` and record every filename. After generating, diff the list of classes in your output against that record. Any master class not present in the output must be explicitly justified in the PR description.
+
+### Preserve declarative-array ordering (`helperClassNames`, `contextStore` keys)
+
+When regenerating an existing `InstrumenterModule`, preserve the exact order of entries in `helperClassNames()` and the exact order of `store.put(...)` calls in `contextStore()`. Reordering entries with no semantic change produces noisy diffs that reviewers correctly reject as "meaningless reshuffling."
+
+**Concrete failure pattern (from PR #11939, @ygree review):** the eval output re-sorted `helperClassNames()` — same set of entries, different order. This adds review burden (reviewer must verify the set is unchanged) with zero benefit.
+
+```java
+// ❌ Reordered without reason
+return new String[] {
+  packageName + ".TracingObserver",
+  packageName + ".TracingSubscriber",
+  packageName + ".TracingSingleObserver",
+  packageName + ".TracingMaybeObserver",
+  packageName + ".TracingCompletableObserver",   // was first in master
+  packageName + ".RxJavaAsyncResultExtension",
+};
+
+// ✅ Preserve master's ordering
+return new String[] {
+  packageName + ".TracingCompletableObserver",
+  packageName + ".TracingObserver",
+  packageName + ".TracingSubscriber",
+  packageName + ".TracingSingleObserver",
+  packageName + ".TracingMaybeObserver",
+  packageName + ".RxJavaAsyncResultExtension",
+};
+```
+
+Only reorder when adding or removing an entry for a semantic reason (a helper is being introduced or retired).
+
+### Hoist repeated `Class.getName()` calls in `contextStore()`
+
+When multiple `store.put(...)` calls in `contextStore()` use the same value-class FQN, hoist `SomeClass.class.getName()` into a single local variable. This is the idiomatic pattern in existing dd-trace-java modules; inlining the same call five times is treated as a regression by reviewers.
+
+```java
+// ❌ Inlined at every put site
+public Map<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: