diff --git a/.agents/skills/apm-integrations/references/instrumenter-module.md b/.agents/skills/apm-integrations/references/instrumenter-module.md index b765157c8ed..67d3ce8df05 100644 --- a/.agents/skills/apm-integrations/references/instrumenter-module.md +++ b/.agents/skills/apm-integrations/references/instrumenter-module.md @@ -9,6 +9,7 @@ - Implement the **narrowest** `Instrumenter` interface possible: - Prefer `ForSingleType` > `ForKnownTypes` > `ForTypeHierarchy` - **EXCEPTION — API specification / interface-only libraries**: when the target library is a specification JAR containing only interfaces (no concrete classes), `ForSingleType` does not work because there are no concrete types to instrument directly. You MUST use `ForTypeHierarchy` with `implementsInterface(named("the.interface.Fqn"))`. This is how vendor implementations of the specification (ActiveMQ, IBM MQ, EclipseLink, Hibernate, etc.) get instrumented through the common interface contract. + - **EXCEPTION applies even when you are handed a CONCRETE implementation, not the spec jar.** The trigger is "does this type implement a shared JDK/spec SPI that other vendors also implement?" — NOT "is the coordinate an interface-only jar?" If you are given a single concrete driver (e.g. `org.postgresql:postgresql`, whose `org.postgresql.jdbc.PgStatement` implements `java.sql.Statement`), you MUST still hook the SPI interface via `ForTypeHierarchy` + `implementsInterface(named("java.sql.Statement"))`, NOT the concrete class via `ForSingleType(named("org.postgresql.jdbc.PgStatement"))`. Hooking the concrete class (a) covers only that one vendor while the SPI hook covers all conforming drivers with one module, and (b) collides at runtime with the existing SPI module that already instruments the same interface — both fire on the same object and mutually suppress spans via the shared `CallDepthThreadLocalMap.incrementCallDepth(.class)` guard. Before instrumenting any concrete class, check whether it implements a type already listed below; if so, the existing SPI module already covers it — do not generate a parallel per-vendor module. - Common API JARs that REQUIRE `ForTypeHierarchy` + `implementsInterface`: - **JMS**: `javax.jms:javax.jms-api`, `jakarta.jms:jakarta.jms-api` — see `dd-java-agent/instrumentation/jms/javax-jms-1.1/` for the canonical example. Targets `MessageProducer`, `MessageConsumer`, `Message`, `MessageListener` interfaces. - **JPA**: `javax.persistence:javax.persistence-api`, `jakarta.persistence:jakarta.persistence-api` @@ -47,6 +48,10 @@ If an existing module covers the same framework at a compatible version, **modif If the existing module targets a genuinely different version range (e.g. existing `foo-1.0/` and you're adding `foo-3.0/`), a version-sibling is correct — but confirm by reading the existing module's muzzle range first. +**The integration name you are given may NOT match the existing family directory — and if it doesn't, the directory wins, not the name.** Before creating a module, grep the whole tree for your intended `super(...)` name: `grep -rn 'super(""' dd-java-agent/instrumentation/`. If ANY existing module already declares that name — including version-sibling modules you are not touching — your module MUST join that family's directory as `/-/`; it must NOT become a new top-level module under a different slug. Placement and name are ONE decision: a taken name dictates the directory. + +**Concrete failure (Cassandra regen, R-DB-1):** the eval was given the integration slug `cassandra`, but dd-trace-java's family directory is `datastax-cassandra/` with siblings `datastax-cassandra-3.0/`, `-3.8/`, `-4.0/`, all declaring `super("cassandra")`. The agent created a new top-level `instrumentation/cassandra/` module that also declared `super("cassandra")`, producing two `@AutoService(InstrumenterModule.class)` registrations for the same name. Result: a **silent tracing outage** — ByteBuddy advice failed to apply, zero spans, all tests timed out, and there was no build error to catch it. This is especially dangerous under the blind protocol: if the same-version master module was deleted, "modify it in place" has no target — but the surviving siblings still hold the name, so grepping for the name (not looking for a same-version directory) is what tells you where the module belongs. When the name is taken and the correct family directory differs from the slug you were handed, place the module in the family directory and match the siblings' `super(...)` exactly; if there is genuinely no correct home without colliding, STOP and surface it rather than shipping a parallel registration. + ### Module constructor: choose names based on sibling structure Each name passed to `super(...)` becomes a distinct `DD_TRACE__ENABLED` flag. Choose the number of names based on whether version-specific siblings exist (or are imminent): @@ -107,6 +112,15 @@ CallDepthThreadLocalMap.reset(Gson.class); A helper class is appropriate when multiple instrumentation classes share the same depth counter — use the shared sentinel class as the key in that case. +### Database clients: populate connection metadata EAGERLY at connect time, not lazily per query + +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` (available at `ConnectionFactory.create()` / `ConnectionFactories.find(...)`) 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. Hooking `Connection.createStatement()` + `ConnectionMetadata` therefore CANNOT populate `db.name`/`peer.hostname`/`db.user`/port — you must hook the factory and thread the captured options forward. (OpenTelemetry's R2DBC instrumentation does exactly this; it is a good reference.) + +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. + ## Advanced: Grouping multiple instrumentations under one module For complex frameworks with multiple version-specific or feature-specific instrumentations, you can group them under a single `InstrumenterModule` (file ending in `Module.java`). The module class: diff --git a/.agents/skills/apm-integrations/references/muzzle.md b/.agents/skills/apm-integrations/references/muzzle.md index 659ec6ce676..97f6e25127d 100644 --- a/.agents/skills/apm-integrations/references/muzzle.md +++ b/.agents/skills/apm-integrations/references/muzzle.md @@ -120,6 +120,8 @@ Add `assertInverse = true` only when you've empirically verified the min via loc This is common whenever any instrumentation class in the module is compatible with versions below the declared min — `assertInverse` then contradicts that class's compatibility. +**Especially avoid defaulting `assertInverse = true` when hooking a concrete driver class** (as opposed to a JDK SPI — but note you usually should NOT be hooking a concrete driver at all; see instrumenter-module.md). Concrete driver classes tend to be structurally stable across a much wider version range than the `compileOnly`/`testImplementation` coordinate you happened to pin. Example: a PostgreSQL module declared `versions = "[42.0.0,)"` + `assertInverse = true`, but `org.postgresql.jdbc.PgStatement` is unchanged back through 9.2 (2013), so muzzle passed on 9.2/9.3/9.4 and the inverse-assertion failed for six old releases. The declared floor matched the pinned dependency, not any real API-shape boundary. Do not set `assertInverse` unless you can point to a specific API change at the declared minimum; otherwise omit it. + ## Muzzle range must exclude incompatible major versions If the library you are instrumenting has a major version break where a newer major version