skill(apm-integrations): listener/interceptor-SPI discovery guidance

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

@@ -41,6 +41,94 @@
 
   ✅ `public String[] triggerClasses() { return new String[]{"com.example.Foo"}; }`
 
+### Before hand-writing method advice, check for an official interception SPI
+
+Before instrumenting a client library by matching and advising its methods directly, check whether the library — or an official companion library in its ecosystem — exposes a first-class **interception / listener SPI**: a listener interface, an interceptor-registration hook, or a proxy/wrapper factory built for observability. Many client libraries provide one, and it is almost always the better hook point than hand-written method advice.
+
+Examples of such SPIs:
+
+- **R2DBC** → `r2dbc-proxy` (an official R2DBC project) exposes `ProxyExecutionListener` / `ProxyMethodExecutionListener`, invoked around every query via `beforeQuery(QueryExecutionInfo)` / `afterQuery(QueryExecutionInfo)`. You install it by wrapping the `ConnectionFactory`.
+- **gRPC** → `ClientInterceptor` / `ServerInterceptor`.
+- **Kafka** → `ProducerInterceptor` / `ConsumerInterceptor` (`interceptor.classes`).
+- **JAX-RS / JDBC / many others** → filter, interceptor, or proxy-driver registration hooks.
+
+**Why prefer the SPI over hand-written advice, especially for reactive/async clients:** a listener SPI is designed around the library's own execution lifecycle and delivers a single well-defined start/end (and error/**cancel**) callback per logical operation, with the operation's metadata (query text, connection info, status, timing) already assembled for you. Hand-written advice on a reactive method must instead re-implement that lifecycle by wrapping the returned `Publisher`/`Mono`/`Flux` and tracking subscribe/complete/error/**cancel** by hand — this is easy to get subtly wrong. A wrapper that finishes the span only on `onComplete`/`onError` will **leak every span that gets cancelled** (reactive pipelines routinely cancel upstreams — e.g. `take(1)`, timeouts, `DiscardOnCancel` operators), so the span is created but never finished and never exported. It is also easy to wrap at the wrong granularity and emit many spans per logical query. The library's listener already handles all of this.
+
+**Do this without forcing a new runtime dependency on the user.** Using a library's listener SPI does NOT require adding it as a normal (`implementation`/`api`) dependency:
+
+- Declare the interception library **`compileOnly`** so it is not put on the application's runtime classpath, and inject your listener implementation and any glue via the module's `helperClassNames()` (the same mechanism used for decorators and other injected helpers). The listener classes travel inside the agent, not the user's app.
+- If helper injection is impractical for a given SPI, the alternative is to **shade/bundle** the interception library's classes into the instrumentation rather than depend on it at runtime.
+- The `compileOnly` interception library must also be added to muzzle's classpath via `extraDependency` (see the worked example below) — otherwise muzzle validation fails with "missing class" for every type the listener/wrap-helper classes reference from it.
+
+Reach for hand-written method advice when no such SPI exists, or when the SPI cannot express what you need to capture. When one does exist and fits, prefer it — and note the choice (and the `compileOnly`/injection approach) in the PR so a reviewer sees the dependency was considered.
+
+**Worked example — hooking a registration/factory point instead of the query methods (R2DBC + `r2dbc-proxy`):**
+
+1. **Advice target is the factory, not the query.** Hook `io.r2dbc.spi.ConnectionFactories.find(ConnectionFactoryOptions)` — NOT `Statement.execute()` / `Batch.execute()`. Replace the returned `ConnectionFactory` using `@Advice.AssignReturned.ToReturned` on method exit:
+
+   ```java
+   @Advice.OnMethodExit(suppress = Throwable.class, inline = false)
+   @Advice.AssignReturned.ToReturned
+   public static ConnectionFactory wrap(
+       @Advice.Return ConnectionFactory factory,
+       @Advice.Argument(0) ConnectionFactoryOptions options) {
+     return R2dbcTracingSupport.wrapConnectionFactory(factory, options);
+   }
+   ```
+
+   Do NOT use a writable `@Advice.Return(readOnly = false) Publisher<...> ...` here — binding a reactive-streams interface to a writable return against a concrete `Mono`/`Flux` result fails Byte Buddy transformation. `@Advice.AssignReturned.ToReturned` sidesteps this because it substitutes the value rather than mutating a typed slot.
+
+2. **Wrap helper installs the listener** (injected via `helperClassNames()`, `r2dbc-proxy` declared `compileOnly`):
+
+   ```java
+   public static ConnectionFactory wrapConnectionFactory(
+       ConnectionFactory factory, ConnectionFactoryOptions options) {
+     ProxyConfig cfg = new ProxyConfig();
+     cfg.addListener(new TraceProxyListener(options));
+     return ProxyConnectionFactory.builder(factory, cfg).build();
+   }
+   ```
+
+3. **Listener drives the span lifecycle** — implements `ProxyMethodExecutionListener`; stash the span on the query's own value store (the interception library already carries one per call — do not add a separate `ContextStore` keyed on the driver object):
+
+   ```java
+   public class TraceProxyListener implements ProxyMethodExecutionListener {
+     @Override
+     public void beforeQuery(QueryExecutionInfo qei) {
+       AgentSpan span = startSpan(...);
+       DECORATE.afterStart(span);
+       DECORATE.onStatement(span, qei);
+       qei.getValueStore().put(SPAN_KEY, span);
+     }
+     @Override
+     public void afterQuery(QueryExecutionInfo qei) {
+       AgentSpan span = qei.getValueStore().get(SPAN_KEY, AgentSpan.class);
+       if (qei.getThrowable() != null) DECORATE.onError(span, qei.getThrowable());
+       DECORATE.beforeFinish(span);
+       span.finish();
+     }
+   }
+   ```
+
+   `beforeQuery`/`afterQuery` fire once per logical operation and `r2dbc-proxy` itself owns completion/error/**cancel** — you do not hand-roll a `Publisher` wrapper to catch those.
+
+4. **Muzzle needs an `extraDependency` for the `compileOnly` interception library.** Muzzle only puts the module's pinned primary dependency (here, `r2dbc-spi`) on its validation classpath by default. Since the wrap helper and listener classes reference the interception library's own types directly (`ProxyConnectionFactory`, `ProxyMethodExecutionListener`, `QueryExecutionInfo`, ...), muzzle reports them as "missing class" unless you add the interception library explicitly:
+
+   ```groovy
+   muzzle {
+     pass {
+       group = "io.r2dbc"
+       module = "r2dbc-spi"
+       versions = "[1.0.0.RELEASE,)"
+       extraDependency 'io.r2dbc:r2dbc-proxy:1.1.0.RELEASE'
+     }
+   }
+   ```
+
+   This is the same `extraDependency` directive used elsewhere for a module's secondary compile-time dependency — it is not R2DBC-specific.
+
+Net result: 1 advice (factory hook) + 1 wrap helper + 1 listener class — smaller than the method-advice alternative (which needs per-method advice on both `Statement.execute()` and `Batch.execute()`, plus a hand-rolled cancel-safe `Publisher` wrapper) and correct by construction on cancellation. The same shape applies to any other SPI in the examples list above: hook the registration/interceptor-installation point, not the client's own operational methods.
+
 ### Before writing a new module, scan for an existing one
 
 Before creating `dd-java-agent/instrumentation/$framework/$framework-$version/`, check whether `dd-java-agent/instrumentation/$framework/` already exists and what's in it.
@@ -123,7 +211,7 @@ A helper class is appropriate when multiple instrumentation classes share the sa
 For database-client integrations (`DatabaseClientDecorator` / `DBTypeProcessingDatabaseClientDecorator`), capture connection metadata (host, port, db name, user) at **connection-establishment** time and cache it in a `ContextStore` keyed on the connection object — not lazily on the first query. The canonical pattern is a dedicated instrumentation on the connect/factory method:
 
 - **JDBC** — `dd-java-agent/instrumentation/jdbc/DriverInstrumentation.java` hooks `Driver.connect(url, props)` and populates `InstrumentationContext.get(Connection.class, DBInfo.class)` at open time. Statement advice then reads the already-cached `DBInfo`.
-- **Reactive drivers with an async connect** — the equivalent connect point is the connection FACTORY, not the connection object. For R2DBC, `io.r2dbc.spi.ConnectionFactoryOptions` is the only place host/port/database/user are exposed as structured data; `io.r2dbc.spi.ConnectionMetadata` (on the live `Connection`) exposes ONLY product name/version. **But `ConnectionFactory.create()` is a zero-argument SPI method returning a `Publisher<? extends Connection>` — the options are NOT available at `create()`.** Capture them earlier, where the factory is built: hook `ConnectionFactories.get(ConnectionFactoryOptions)` (or the provider-construction path) and store the options in a `ContextStore<ConnectionFactory, ConnectionFactoryOptions>`; then, in advice on `create()`, read the stored options for that factory and thread them onto the asynchronously-emitted `Connection` (a second context store keyed on the returned `Connection`). Hooking only `Connection.createStatement()` + `ConnectionMetadata` CANNOT populate `db.name`/`peer.hostname`/`db.user`/port. (OpenTelemetry's R2DBC instrumentation does exactly this options→factory→connection threading; it is a good reference.)
+- **Reactive drivers with an async connect** — the equivalent connect point is the connection FACTORY, not the connection object. For R2DBC, `io.r2dbc.spi.ConnectionFactoryOptions` is the only place host/port/database/user are exposed as structured data; `io.r2dbc.spi.ConnectionMetadata` (on the live `Connection`) exposes ONLY product name/version. **But `ConnectionFactory.create()` is a zero-argument SPI method returning a `Publisher<? extends Connection>` — the options are NOT available at `create()`.** Capture them earlier, where the factory is built: hook `ConnectionFactories.get(ConnectionFactoryOptions)` (or the provider-construction path) and store the options in a `ContextStore<ConnectionFactory, ConnectionFactoryOptions>`; then, in advice on `create()`, read the stored options for that factory and thread them onto the asynchronously-emitted `Connection` (a second context store keyed on the returned `Connection`). Hooking only `Connection.createStatement()` + `ConnectionMetadata` CANNOT populate `db.name`/`peer.hostname`/`db.user`/port. (OpenTelemetry's R2DBC instrumentation does exactly this options→factory→connection threading; it is a good reference.) Note also — per "Before hand-writing method advice, check for an official interception SPI" above — that R2DBC has `r2dbc-proxy`, whose `ProxyMethodExecutionListener` surfaces this connection/query metadata through a listener and handles the reactive lifecycle for you; prefer it over hand-wrapping the `create()`/`execute()` publishers where it fits.
 
 Why eager-at-connect beats lazy-per-query: lazy extraction (e.g. `statement.getConnection().getMetaData().getURL()` on first execute) works for plain JDBC but (a) pays the extraction cost on every connection's first query instead of amortizing at pool-open, and (b) silently yields nothing when the metadata is not reachable from the object the query advice happens to hold — which is exactly what happens for reactive drivers whose statement/connection objects don't carry the factory options.