Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Check behavior before rejecting concrete-driver hooks

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-98 deliberately matches concrete PostgreSQL and other JDBC connection classes—even though they implement java.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 👍 / 👎.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 SPI membership does not prove behavior is covered

Generated integrations can omit required vendor-only advice, causing missing or unfinished spans for affected libraries.

Assertion details
  • Input: A concrete SPI implementation with behavior outside the shared interface, such as Tomcat Request.recycle() or DB2-specific JDBC compatibility.
  • Expected: Reject a vendor module only when its target method and behavior are already covered by existing SPI advice; allow implementation-only lifecycle and compatibility hooks.
  • Actual: The instruction treats implementing a listed SPI as proof that existing SPI advice covers every relevant behavior. The repository contradicts this: PostgreSQL prepared statements are explicitly allowlisted, DB2 has vendor-specific JDBC modules, and Tomcat instruments concrete Request.recycle() alongside servlet SPI modules.
Suggested change
- **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.
- **EXCEPTION applies even when you are handed a CONCRETE implementation, not the spec jar.** The trigger is whether the target behavior is declared by a shared JDK/spec SPI and already covered by existing SPI advice — NOT whether the coordinate is an interface-only jar. When given a concrete driver such as `org.postgresql:postgresql`, inspect the implemented SPI and the existing instrumentation first. If the target method is already advised through `ForTypeHierarchy` + `implementsInterface(...)`, reuse or extend that SPI module rather than adding overlapping vendor advice, which can duplicate or suppress spans through a shared call-depth guard. Do not infer coverage from `implements` alone: implementation-specific methods or lifecycle hooks that are not declared and advised on the SPI may still require a concrete/vendor module.

Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest

- 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`
Expand Down Expand Up @@ -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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Do not use the super name as a unique family key

When multiple frameworks intentionally share an enablement name, this rule sends a new module to the wrong family or tells the agent to stop: super("jax-rs", ...) currently appears under the independent rs, jersey, and resteasy families. InstrumenterIndex.loadModules() also indexes module class names without deduplicating InstrumenterModule.name(), so equal super(...) names do not themselves cause the claimed registration outage. Determine placement from the framework/package and actual matcher and muzzle overlap rather than treating a config name as a unique directory key.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Shared integration names do not identify one family

Following the rule can place generated code under an unrelated family or halt a valid integration.

Assertion details
  • Input: A new module using a shared primary name such as jax-rs or ci-visibility.
  • Expected: Use name matches as evidence, then place the module by library coordinates, packages, and compatible version family; allow shared configuration names across unrelated families.
  • Actual: The prescribed grep returns three legitimate top-level families for jax-rs and nine for ci-visibility, so a matching primary name does not dictate one directory. Many existing modules also share these names without registration failure.
Suggested change
**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.
**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.
**The integration name you are given may NOT match the existing family directory.** Before creating a module, grep the whole tree for the intended primary `super(...)` name: `grep -rn 'super("<name>"' dd-java-agent/instrumentation/`. Treat matches as evidence, not a unique placement key: inspect their library coordinates, target packages, and muzzle ranges. When matches for the same library form one version family, join that family as `<existing-family-dir>/<family>-<version>/`; when a shared config name spans unrelated families, choose the family from the target library rather than the name alone.
**Concrete failure (Cassandra regen, R-DB-1):** the eval was given the integration slug `cassandra`, but the surviving `datastax-cassandra-3.0/` and `-3.8/` siblings establish `datastax-cassandra/` as the family even if the same-version module was deleted. Recreating that version under a top-level `instrumentation/cassandra/` produces a parallel implementation instead of restoring the family member. Place it with the surviving siblings and preserve their `super(...)` value. If the name search points to multiple plausible families and the target coordinates do not disambiguate them, STOP and surface the ambiguity.

Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest


### 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):
Expand Down Expand Up @@ -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:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve lazy metadata as a fallback for uncovered connect paths

When a JDBC connection is not created through the instrumented Driver.connect, requiring exclusively eager capture leaves its spans without database metadata. JDBCDecorator.parseDBInfo() explicitly handles this case (JDBCDecorator.java:167-178), and StatementInstrumentation.java:87-90 intentionally invokes that fallback rather than merely reading a pre-populated value. Recommend eager capture as the primary path, but retain lazy extraction where metadata is reachable so DataSource, proxy, and other uncovered creation paths continue to be enriched.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Capture R2DBC options before ConnectionFactory.create

For R2DBC, ConnectionFactory.create() is a zero-argument SPI method returning a Publisher, so ConnectionFactoryOptions is not available at that call as stated. Unless an earlier ConnectionFactories.get(ConnectionFactoryOptions) or provider-construction hook associates the options with the returned factory, advice on create() has nothing from which to derive host, port, database, or user, and an implementation following this guidance will reproduce the missing metadata it is meant to fix. Specify the earlier options-to-factory context-store step and the subsequent propagation to the asynchronously emitted connection.

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:
Expand Down
2 changes: 2 additions & 0 deletions .agents/skills/apm-integrations/references/muzzle.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Do not treat inverse muzzle as proof the matcher target exists

When a concrete target class appears only as the string returned by instrumentedType() or in a name matcher, muzzle passing against an old artifact does not prove that class exists there: MuzzleGenerator.generateReferences() derives references from advice bytecode and explicit additional references, not matcher strings. The JDBC implementation also lists the older PostgreSQL jdbc2/jdbc3/jdbc4 statement classes separately from the newer org.postgresql.jdbc.PgPreparedStatement, contradicting the claim that the org.postgresql.jdbc.PgStatement shape is unchanged back through 9.2. Explain this matcher blind spot and require an explicit muzzle reference or runtime coverage instead; otherwise an agent may interpret the inverse result as compatibility with releases on which its target matcher never fires.

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
Expand Down
Loading