-
Notifications
You must be signed in to change notification settings - Fork 350
skill(apm-integrations): database-category rule sharpening (SPI-first, eager connect metadata, muzzle) #12114
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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(<SpiType>.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. | ||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Generated integrations can omit required vendor-only advice, causing missing or unfinished spans for affected libraries. Assertion details
Suggested change
Was this helpful? React 👍 or 👎 |
||||||||||||||
| - 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("<name>"' 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 `<existing-family-dir>/<family>-<version>/`; 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. | ||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When multiple frameworks intentionally share an enablement name, this rule sends a new module to the wrong family or tells the agent to stop: AGENTS.md reference: AGENTS.md:L61-L62 Useful? React with 👍 / 👎. |
||||||||||||||
|
|
||||||||||||||
| **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. | ||||||||||||||
|
Comment on lines
+51
to
+53
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Following the rule can place generated code under an unrelated family or halt a valid integration. Assertion details
Suggested change
Was this helpful? React 👍 or 👎 |
||||||||||||||
|
|
||||||||||||||
| ### Module constructor: choose names based on sibling structure | ||||||||||||||
|
|
||||||||||||||
| Each name passed to `super(...)` becomes a distinct `DD_TRACE_<NAME>_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: | ||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a JDBC connection is not created through the instrumented Useful? React with 👍 / 👎. |
||||||||||||||
|
|
||||||||||||||
| - **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.) | ||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For R2DBC, Useful? React with 👍 / 👎. |
||||||||||||||
|
|
||||||||||||||
| 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: | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a concrete target class appears only as the string returned by Useful? React with 👍 / 👎. |
||
|
|
||
| ## Muzzle range must exclude incompatible major versions | ||
|
|
||
| If the library you are instrumenting has a major version break where a newer major version | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When an integration needs vendor-specific behavior that the shared SPI advice cannot provide, implementing an SPI does not mean the concrete type is already covered. For example,
DBMCompatibleConnectionInstrumentation.java:39-98deliberately matches concrete PostgreSQL and other JDBC connection classes—even though they implementjava.sql.Connection—to add DBM-specific prepare behavior absent from the generic SPI instrumentation. Scope this prohibition to advice that is behaviorally redundant; otherwise the skill will reject valid concrete instrumentation and silently omit requested features.Useful? React with 👍 / 👎.