diff --git a/.agents/skills/apm-integrations/SKILL.md b/.agents/skills/apm-integrations/SKILL.md new file mode 100644 index 00000000000..0975cdd38c5 --- /dev/null +++ b/.agents/skills/apm-integrations/SKILL.md @@ -0,0 +1,195 @@ +--- +name: apm-integrations +description: Write a new library instrumentation end-to-end. Use when the user ask to add a new APM integration or a library instrumentation. +context: fork +allowed-tools: + - Bash + - Read + - Write + - Edit + - Glob + - Grep +--- + +Write a new APM end-to-end integration for dd-trace-java, based on library instrumentations, following all project conventions. + +## Step 1 – Read the authoritative docs and sync this skill (mandatory, always first) + +Before writing any code, read all three files in full: + +1. [`docs/how_instrumentations_work.md`](docs/how_instrumentations_work.md) — full reference (types, methods, advice, helpers, context stores, decorators) +2. [`docs/add_new_instrumentation.md`](docs/add_new_instrumentation.md) — step-by-step walkthrough +3. [`docs/how_to_test.md`](docs/how_to_test.md) — test types and how to run them + +These files are the single source of truth. Reference them while implementing. + +## Step 2 – Clarify the task + +If the user has not already provided all of the following, ask before proceeding: + +- **Framework name** and **minimum supported version** (e.g. `okhttp-3.0`) +- **Target class(es) and method(s)** to instrument (fully qualified class names preferred) +- **Target system**: one of `Tracing`, `Profiling`, `AppSec`, `Iast`, `CiVisibility`, `Usm`, `ContextTracking` +- **Whether this is a bootstrap instrumentation** (affects allowed imports) + +## Step 3 – Find a reference instrumentation + +Search `dd-java-agent/instrumentation/` for a structurally similar integration: +- Same target system +- Comparable type-matching strategy (single type, hierarchy, known types) + +Read the reference integration's `InstrumenterModule`, Advice, Decorator, and test files to understand the established +pattern before writing new code. Use it as a template. + +## Step 4 – Set up the module + +1. Create directory: `dd-java-agent/instrumentation/$framework/$framework-$minVersion/` +2. Under it, create the standard Maven source layout: + - `src/main/java/` — instrumentation code + - `src/test/groovy/` — Groovy/Spock instrumentation tests (see Step 9.1) +3. Create `build.gradle` with: + - `compileOnly` dependencies for the target framework + - `testImplementation` dependencies for tests + - `muzzle { pass { } }` directives (see Step 9.2) +4. Register the new module in `settings.gradle.kts` in **alphabetical order** +5. Register all integration names in `metadata/supported-configurations.json` — **read [Supported Configurations](references/supported-configurations.md)** for the exact key shapes and CI checks involved. Declaring several names (`super("a", "b")`) means one entry each. + +**See [Naming Conventions](references/naming-conventions.md) — module directory name must end with a version or an allowed suffix (`-common`, `-stubs`, `-iast`). Java filename and `public class` name MUST match character-for-character including acronym casing (CRITICAL — see § "Java naming consistency").** + +## Step 4.1 – Span-creating vs context-tracking instrumentation + +Read [Context-Tracking Instrumentation](references/context-tracking.md) and decide whether the library needs `InstrumenterModule.Tracing` (I/O operations that create spans) or `InstrumenterModule.ContextTracking` (async-boundary bridging, no spans). + +## Step 5 – Write the InstrumenterModule + +**Read [InstrumenterModule Guidance](references/instrumenter-module.md).** + +## Step 6 – Write the Decorator + +- Extend the most specific available base decorator: + - `HttpClientDecorator`, `DatabaseClientDecorator`, `ServerDecorator`, `MessagingClientDecorator`, etc. +- One `public static final DECORATE` instance +- Define `UTF8BytesString` constants for the component name and operation name +- Keep all tag/naming/error logic here — not in the Advice class +- Override `spanType()`, `component()`, `spanKind()` as appropriate +- Override `instrumentationNames()` to return the primary integration name without a version suffix: `return new String[] {"jedis"};` not `"jedis-3.0"`. +- `BaseDecorator` uses those names to resolve analytics settings (`DD_TRACE__ANALYTICS_ENABLED`, `DD_TRACE__ANALYTICS_SAMPLE_RATE`). Search `metadata/supported-configurations.json` for each returned name — if analytics keys are absent, add them (see [Supported Configurations](references/supported-configurations.md) for the JSON shape). + +## Step 7 – Write the Advice class (highest-risk step) + +**Read [Writing the Advice Class](references/advice-class.md) — the highest-risk step.** Pay particular attention to: `@Advice.OnMethodEnter/Exit` annotations; `CallDepthThreadLocalMap` reentrancy guarding; span lifecycle order; and the "Must NOT do" list. + +## Step 8 – Add SETTER/GETTER adapters (if applicable) + +For context propagation to and from upstream services, like HTTP headers, +implement `AgentPropagation.Setter` / `AgentPropagation.Getter` adapters that wrap the framework's specific header API. +Place them in the helpers package, declare them in `helperClassNames()`. + +## Step 9 – Write tests + +Cover all mandatory test types: + +### 1. Instrumentation test (mandatory) + +**Read [Writing Tests](references/tests.md).** Instrumentation tests are Groovy/Spock (`src/test/groovy/`) — add the `tag: override groovy enforcement` label to suppress the `Enforce Groovy Migration` CI check (which blocks new `.groovy` files by default — instrumentation tests are intentionally Groovy/Spock). Must cover error/exception scenarios. When adding new integration names, register them per [Supported Configurations](references/supported-configurations.md). When `compileOnly` and `testImplementation` use different versions, comment the specific class that requires the higher version. Include sibling version modules as `testImplementation` dependencies for mutual-exclusion tests. + +### 2. Muzzle directives (mandatory) + +**Read [Muzzle Directives](references/muzzle.md)** — it covers all three valid patterns and their `assertInverse` rules. Search adjacent module `build.gradle` files for `skipVersions` before declaring a new version-bounded module's muzzle directives. + +### 3. Latest dependency test (mandatory) + +Use `latestDepTestImplementation` in `build.gradle` to pin the latest available version. Run with: +```bash +./gradlew :dd-java-agent:instrumentation:$framework:$framework-$version:latestDepTest +``` + +**`latestDepTestImplementation` version range must match the instrumented range.** If your module instruments version `2.x`, use `2.+` as the version constraint, not `3.+`: + +```groovy +// WRONG — latestDep tests against 3.x but the module only instruments 2.x +latestDepTestImplementation group: 'commons-httpclient', name: 'commons-httpclient', version: '3.+' + +// CORRECT — latestDep tests against the highest 2.x release +latestDepTestImplementation group: 'commons-httpclient', name: 'commons-httpclient', version: '2.+' +``` + +Using `3.+` for a `2.x` instrumentation means `latestDepTest` runs against an incompatible API version and will either fail or silently test nothing. + +### 4. Smoke test (optional) + +Add a smoke test in `dd-smoke-tests/` only if the framework warrants a full end-to-end demo-app test. + +## Step 10 – Build and verify + +Run these commands in order and fix any failures before proceeding: + +```bash +./gradlew :dd-java-agent:instrumentation:$framework:$framework-$version:muzzle +./gradlew :dd-java-agent:instrumentation:$framework:$framework-$version:test +./gradlew :dd-java-agent:instrumentation:$framework:$framework-$version:latestDepTest +./gradlew checkInstrumenterModuleConfigurations +./gradlew checkDecoratorAnalyticsConfigurations +./gradlew spotlessApply +./gradlew :dd-java-agent:updateAgentJarIntegrationsGoldenFile +``` + +After `updateAgentJarIntegrationsGoldenFile` runs, commit the updated `metadata/agent-jar-checks.properties` file alongside your instrumentation changes. The `verifyAgentJarIntegrations` check runs automatically in CI and fails if this file is out of date. + +**If muzzle fails:** +- Missing helper class names in `helperClassNames()` — the most common cause; add any missing inner, anonymous, or enum synthetic classes. +- Wrong version range — the declared `versions` in `build.gradle` doesn't cover the versions actually used by tests; adjust the bounds. +- API mismatch — the instrumented class or method doesn't exist in the declared version; check the library's changelog and narrow the `compileOnly` version or the muzzle range. + +**If `checkInstrumenterModuleConfigurations` fails:** an integration name from `super(...)` is missing +(or mismatched) in `metadata/supported-configurations.json` — see [Supported Configurations](references/supported-configurations.md). + +**If `checkDecoratorAnalyticsConfigurations` fails:** a name returned by the decorator's `instrumentationNames()` is missing `DD_TRACE__ANALYTICS_ENABLED` / `DD_TRACE__ANALYTICS_SAMPLE_RATE` entries in `metadata/supported-configurations.json` — add them per [Supported Configurations](references/supported-configurations.md). + +**If tests fail:** verify span lifecycle order (start → activate → error → close → finish), helper registration, +and `contextStore()` map entries match actual usage. + +## Step 11 – Checklist before finishing + +Output this checklist and confirm each item is satisfied: + +- [ ] `settings.gradle.kts` entry added in alphabetical order +- [ ] `metadata/supported-configurations.json` has a `DD_TRACE__ENABLED` entry (+ the two aliases) for every name passed to `super(...)` +- [ ] `metadata/supported-configurations.json` has `DD_TRACE__ANALYTICS_ENABLED` and `DD_TRACE__ANALYTICS_SAMPLE_RATE` entries for every name returned by the decorator's `instrumentationNames()` +- [ ] `build.gradle` has `compileOnly` deps and `muzzle` directives +- [ ] Muzzle pattern is correct (see [Muzzle Directives](references/muzzle.md)) +- [ ] `latestDepTestImplementation` version range matches the instrumented version range (e.g. `2+` not `3+` for a `2.x` module) +- [ ] `@AutoService(InstrumenterModule.class)` annotation present on the module class +- [ ] `helperClassNames()` lists ALL referenced helpers (including inner, anonymous, and enum synthetic classes) +- [ ] Advice methods are `static` with `@Advice.OnMethodEnter` / `@Advice.OnMethodExit` annotations +- [ ] `@Advice.OnMethodEnter(suppress = Throwable.class)` on enter; `@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)` on exit (omit `onThrowable` when hooking a constructor) +- [ ] No static constants holding return values of one-shot instrumenter methods (`triggerClasses()`, `contextStore()`, etc.) +- [ ] No logger field in the Advice class or InstrumenterModule class +- [ ] No `inline=false` left in production code +- [ ] No `java.util.logging.*` / `java.nio.file.*` / `javax.management.*` in bootstrap path +- [ ] `metadata/agent-jar-checks.properties` updated via `./gradlew :dd-java-agent:updateAgentJarIntegrationsGoldenFile` and committed +- [ ] Span lifecycle order is correct: startSpan → afterStart → activateSpan (enter); onError → beforeFinish → close → finish (exit) +- [ ] All Step 10 verification commands passed with no failures + +## Step 12 – Retrospective: update this skill with what was learned + +After the instrumentation is complete (or abandoned), review the full session and improve this skill for future use. + +**Collect lessons from four sources:** + +1. **Build/test failures** — did any Gradle task fail with an error that this skill did not anticipate or gave wrong + guidance for? (e.g. a muzzle failure that wasn't caused by missing helpers, a test pattern that didn't work) +2. **Docs vs. skill gaps** — did Step 1's sync miss anything? Did you consult the docs for something not captured here? +3. **Reference instrumentation insights** — did the reference integration use a pattern, API, or convention not + reflected in any step of this skill? +4. **User corrections** — did the user correct an output, override a decision, or point out a mistake? + +**For each lesson identified**, edit this file (`.agents/skills/apm-integrations/SKILL.md`) or its referenced files +using the `Edit` tool: +- Wrong rule → fix it in place +- Missing rule → add it to the most relevant step +- Wrong failure guidance → update the relevant "If X fails" section in Step 10 +- Misleading or obsolete content → remove it + +Keep each change minimal and targeted. Do not rewrite sections that worked correctly. +After editing, confirm to the user which improvements were made to the skill. diff --git a/.agents/skills/apm-integrations/references/advice-class.md b/.agents/skills/apm-integrations/references/advice-class.md new file mode 100644 index 00000000000..f0db92887d2 --- /dev/null +++ b/.agents/skills/apm-integrations/references/advice-class.md @@ -0,0 +1,109 @@ +# Writing the Advice Class + +> Referenced from `SKILL.md` Step 7. The highest-risk step — every rule in this file exists because someone's PR broke on it. + +## Must do + +- Advice methods **must** be `static` +- Annotate enter: `@Advice.OnMethodEnter(suppress = Throwable.class)` +- Annotate exit: `@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)` + - **Exception**: do NOT use `onThrowable` when hooking a constructor — if the constructor throws, `@Advice.This` is a partially initialized object +- Use `@Advice.Local("...")` for values shared between enter and exit (span, scope) +- Use the correct parameter annotations: + - `@Advice.This` — the receiver object + - `@Advice.Argument(N)` — a method argument by index + - `@Advice.Return` — the return value (exit only) + - `@Advice.Thrown` — the thrown exception (exit only) + - `@Advice.Enter` — the return value of the enter method (exit only) +- Use `CallDepthThreadLocalMap` to guard against recursive instrumentation of the same method +- **Instrument the single delegate method, not all overloads**: when a library has multiple overloads of the same operation (e.g. `executeMethod(String)`, `executeMethod(HostConfig)`, `executeMethod(HostConfig, HttpMethod)`), check if they all delegate to a single internal method. If yes, instrument ONLY the delegate — not each overload. Instrumenting all overloads without a proper reentrancy guard creates **duplicate spans per request** (one per overload in the call chain) and injects context propagation headers multiple times. Use `CallDepthThreadLocalMap` when you must instrument at a higher level. + +## Span lifecycle (in order) + +Enter method: +1. `AgentSpan span = startSpan(DECORATE.operationName(), ...)` +2. `DECORATE.afterStart(span)` + set domain-specific tags +3. `AgentScope scope = activateSpan(span)` — return or store via `@Advice.Local` + +Exit method: +4. `DECORATE.onError(span, throwable)` — only if throwable is non-null +5. `DECORATE.beforeFinish(span)` +6. `scope.close()` +7. `span.finish()` + +### onExit handling when the target method throws + +The `onThrowable = Throwable.class` attribute on `@Advice.OnMethodExit` controls whether the exit advice fires when the **instrumented target method** throws. You **must** set it explicitly to `Throwable.class` for any exit advice that closes a scope or finishes a span — the default skips exceptional termination, which leaks active scopes when the instrumented method throws. + +```java +// Standard pattern — exit fires whether the target method returned or threw +@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) +public static void exit( + @Advice.Enter final AgentScope scope, + @Advice.Thrown final Throwable thrown) { + if (scope != null) { + AgentSpan span = scope.span(); + DECORATE.onError(span, thrown); + DECORATE.beforeFinish(span); + scope.close(); + span.finish(); + } +} +``` + +**`onThrowable` does NOT compensate for `onEnter` throwing.** Per `docs/how_instrumentations_work.md`: "If the `Advice.OnMethodEnter` method throws an exception, the `Advice.OnMethodExit` method is not invoked" — this is unconditional. To keep `onEnter` from throwing in the first place, use `suppress = Throwable.class` on the enter advice. + +When using `CallDepthThreadLocalMap`, only the outermost call (the one where `incrementCallDepth` returned 0) should reset the counter. Recursive inner calls that returned early on enter must also return early on exit without resetting — otherwise an inner exit clears the counter while the outer call is still active, allowing subsequent nested calls to create duplicate spans. The exit guard must mirror the enter guard exactly. + +### Specify charset explicitly when converting byte[] to String + +```java +// WRONG — uses platform default charset +String cmd = new String(commandBytes); + +// CORRECT — explicit charset +import java.nio.charset.StandardCharsets; +String cmd = new String(commandBytes, StandardCharsets.UTF_8); +``` + +### Do NOT catch `NullPointerException`; use null-check guards instead + +Catching `NullPointerException` is always a sign of an unguarded precondition — fix the root cause with an explicit null check instead. dd-trace-java enforces this via SpotBugs rule `DCN_NULLPOINTER_EXCEPTION`; violations fail `:spotbugsMain` and block the PR. + +```java +// WRONG — SpotBugs DCN_NULLPOINTER_EXCEPTION +@Override +protected int status(final HttpMethod httpMethod) { + try { + return httpMethod.getStatusCode(); + } catch (NullPointerException e) { + // getStatusCode() throws NPE when statusLine is null + return 0; + } +} + +// CORRECT — null-check guard (this is the canonical master pattern) +@Override +protected int status(final HttpMethod httpMethod) { + final StatusLine statusLine = httpMethod.getStatusLine(); + return statusLine == null ? 0 : statusLine.getStatusCode(); +} +``` + +**How to discover**: when implementing a method that calls library code which may NPE on null internal state, READ the master module's analogous method for the canonical null-check pattern. The master typically exposes the nullable intermediate (e.g. `getStatusLine()`) so you can guard it. + +## Multiple advice classes and `@AppliesOn` + +If your instrumentation needs to apply multiple advices to the same method (e.g. separate context-tracking from tracing logic), use `applyAdvices()` inside `methodAdvice()`. Use the `@AppliesOn` annotation to control which target systems each advice applies to. + +See the `@AppliesOn Annotation` section of `docs/how_instrumentations_work.md` for the full API and examples. + +## Must NOT do + +- **No logger fields** in the Advice class or the Instrumentation class (loggers only in helpers/decorators) +- **No code in the Advice constructor** — it is never called +- **Do not use lambdas in advice methods** — they create synthetic classes that will be missing from helper declarations +- **No references** to other methods in the same Advice class or in the InstrumenterModule class +- **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 diff --git a/.agents/skills/apm-integrations/references/context-tracking.md b/.agents/skills/apm-integrations/references/context-tracking.md new file mode 100644 index 00000000000..9004ee0811f --- /dev/null +++ b/.agents/skills/apm-integrations/references/context-tracking.md @@ -0,0 +1,62 @@ +# Context-Tracking Instrumentation + +> Referenced from `SKILL.md` Step 4.1. Read this BEFORE picking instrumentation targets. + +## Which kind of instrumentation to write + +Before picking instrumentation targets, decide which kind of instrumentation the library needs. dd-trace-java has two: + +**Span-creating instrumentation** — the module extends `InstrumenterModule.Tracing`. Advice creates spans around the library's operations. This is the common case: HTTP clients/servers, DB clients, messaging clients, RPC frameworks. Targets are methods that perform the I/O (e.g. `Connection.sendCommand`, `Client.execute`). Tests assert spans exist with correct tags. + +**Context-tracking instrumentation** — the module extends `InstrumenterModule.ContextTracking`. Advice captures the active trace context at a boundary and restores it when work crosses that boundary. The module creates **no spans** itself; it only bridges trace context for spans created by other instrumentations or by user code. This is the case for reactive libraries (RxJava, Reactor), async/executor libraries (`CompletableFuture`, `ListenableFuture`, executors), coroutine/actor libraries (Kotlin coroutines, pekko/akka), and any code that schedules a callback to run later or on another thread. + +- Typical libraries: RxJava, Reactor, Mutiny, CompletableFuture, ListenableFuture, executors, pekko/akka, lettuce async-command queue, ZIO, virtual threads +- Typical targets: the boundary-crossing type's constructor (capture context) + its subscribe/execute/schedule method (restore context around the callback) +- For RxJava-shaped libraries, one instrumentation per reactive type (e.g. `Observable`, `Flowable`, `Single`, `Maybe`, `Completable`) +- For executor-shaped libraries, one instrumentation for the executor's submit/execute methods wrapping `Runnable`/`Callable` + +## Reference implementation + +**`dd-java-agent/instrumentation/rxjava/rxjava-2.0/`** — the canonical context-tracking module. It uses `InstrumenterModule.ContextTracking`. Contents: 5 type-instrumenters (`ObservableInstrumentation`, `FlowableInstrumentation`, `SingleInstrumentation`, `MaybeInstrumentation`, `CompletableInstrumentation`), 5 wrapper classes (`TracingObserver`, `TracingSubscriber`, `TracingSingleObserver`, `TracingMaybeObserver`, `TracingCompletableObserver`), 1 `RxJavaModule.java`, 1 `RxJavaAsyncResultExtension.java`. Read this before writing a new context-tracking module. + +## What a context-tracking instrumentation captures + +For each boundary-crossing type in the library, the instrumentation needs to identify: + +- **The boundary type** — the FQN of the type that gets subscribed to / executed / scheduled (e.g. `io.reactivex.rxjava3.core.Observable`). +- **The capture point** — where to grab the active trace context. Usually the constructor (`` or `isConstructor()`). +- **The restore point** — where the captured context needs to be reactivated. Usually the `subscribe(...)` / `execute(...)` / `schedule(...)` method that runs the user-provided callback. +- **The wrapped argument type** — the user's callback interface that must be wrapped in a tracing wrapper (e.g. `io.reactivex.rxjava3.core.Observer`, `java.lang.Runnable`). +- **The context store key class** — the FQN used to key the `contextStore` (almost always the boundary type itself). +- **The tracing wrapper** — a new class implementing the callback interface that reattaches the captured context before delegating (e.g. `TracingObserver` implements `Observer`). +- **The wrapper's methods to guard** — the callback methods that must reattach context before running user code (e.g. `onNext`, `onError`, `onComplete` for RxJava observers). + +## Tests for context-tracking instrumentations + +Context-tracking tests assert that **a span created by user code inside the wrapped callback becomes a child of the parent span active when the boundary was created**: + +```groovy +runUnderTrace("parent") { + constructBoundary() // capture happens here + .subscribe({ item -> userCodeStartsAChildSpan() }) // restore happens around the closure +} +// Assert: 1 trace, 2 spans — child has parent's spanId as parentId. +``` + +Do NOT assert span kinds, operation names, span tags, or span types on the *target* methods — there are no spans on those methods. + +## Choosing between method overloads + +When a boundary type exposes multiple overloads of the subscribe / invoke method — e.g. `Flowable.subscribe(Subscriber)` vs `Flowable.subscribe(FlowableSubscriber)`, or `Mono.subscribe(Subscriber)` vs `Mono.subscribe(CoreSubscriber)` — hook the **most specific framework-internal interface**, not the public wrapper. + +**Why:** the public overload typically delegates to the internal one. Hooking the public overload causes double-wrapping (every subscription flows through the wrapper twice) and the internal overload sees a wrapped argument of the wrong runtime type. + +**How to identify the right overload:** read the framework source. The public method usually calls a `subscribeActual(...)` or similar protected method that takes the framework-internal interface. If the framework documents one of the overloads as "for internal use only" or marks it `public final`, that's the implementation method — hook it. + +**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. + +## 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. + +Hybrid libraries that BOTH coordinate work AND perform I/O usually get one span-creating instrumentation for the I/O path and (optionally) one context-tracking instrumentation for the coordination path. `lettuce-5.0` is an example: there is a span-creating instrumentation for Redis commands and a separate context-tracking instrumentation for the async command queue. diff --git a/.agents/skills/apm-integrations/references/instrumenter-module.md b/.agents/skills/apm-integrations/references/instrumenter-module.md new file mode 100644 index 00000000000..cb35c7d7cf0 --- /dev/null +++ b/.agents/skills/apm-integrations/references/instrumenter-module.md @@ -0,0 +1,90 @@ +# InstrumenterModule Guidance + +> Referenced from `SKILL.md` Step 5. Everything needed to write the `InstrumenterModule` class correctly. + +## Conventions to enforce + +- Add `@AutoService(InstrumenterModule.class)` annotation — required for auto-discovery +- Extend the correct `InstrumenterModule.*` subclass (never the bare abstract class) +- 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. + - 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` + - **JDBC**: `java.sql.*` — interfaces like `Driver`, `Connection`, `Statement`, `PreparedStatement` + - **JCache**: `javax.cache:cache-api` + - **Bean Validation**: `jakarta.validation:jakarta.validation-api` + - **JAX-RS**: `jakarta.ws.rs:jakarta.ws.rs-api` + - **JAX-WS**: `jakarta.xml.ws:jakarta.xml.ws-api` + - **Servlet**: `jakarta.servlet:jakarta.servlet-api` + - **DO NOT classify interface-only API JARs as not_applicable.** They ARE instrumentable via `implementsInterface()`. +- Add `classLoaderMatcher()` if a sentinel class identifies the framework on the classpath +- Declare **all** helper class names in `helperClassNames()`: + - Include inner classes (`Foo$Bar`), anonymous classes (`Foo$1`), and enum synthetic classes — for enums, each constant with an anonymous body generates its own synthetic class (`MyEnum$1`, `MyEnum$2`, …), each must be listed individually +- Declare `contextStore()` entries if context stores are needed (key class → value class) +- **Null-check before every `ContextStore` key** — `ContextStore` does not support null keys. Always guard with a null check before calling `store.put(obj, ...)` or `store.get(obj)`. Passing null throws at runtime; with `suppress = Throwable.class` this silently drops the span. +- Keep method matchers as narrow as possible (name, parameter types, visibility) + +## Must NOT do in InstrumenterModule + +- **Do not extract one-shot method return values into static constants.** + Methods like `triggerClasses()`, `contextStore()`, `classLoaderMatcher()`, and `methodAdvice()` + are called **once** by `AgentInstaller` / the framework wiring. Extracting their return value + into a `private static final` constant provides no performance benefit and needlessly bloats + the constant pool of the instrumentation class. + + ❌ `private static final String[] TRIGGER_CLASSES = new String[]{"com.example.Foo"};` + `public String[] triggerClasses() { return TRIGGER_CLASSES; }` + + ✅ `public String[] triggerClasses() { return new String[]{"com.example.Foo"}; }` + +### Module constructor: new modules add a version alias; existing modules preserve existing names + +**New module**: pass a generic name AND a version-qualified alias so users can enable/disable this version independently: + +```java +// CORRECT — generic + version alias +public JedisInstrumentation() { + super("jedis", "jedis-3.0"); +} +``` + +The version alias (e.g. `"jedis-3.0"`) maps to `DD_TRACE_JEDIS_3_0_ENABLED`. Do NOT add version aliases to the decorator's `instrumentationNames()` — that method is for analytics keys only. + +**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. + +### 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: + +```java +// WRONG — pointless wrapper when only one type is instrumented +public class GsonHelper { + public static boolean shouldSkip() { + return CallDepthThreadLocalMap.incrementCallDepth(GsonHelper.class) > 0; + } + public static void reset() { + CallDepthThreadLocalMap.reset(GsonHelper.class); + } +} + +// CORRECT — use a target library class as key (instrumentation/module classes are not +// helper-injected and must not appear as literals in inlined advice) +if (CallDepthThreadLocalMap.incrementCallDepth(Gson.class) > 0) return; +// ... in exit: +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. + +## 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: + +- Must extend a `TargetSystem` subclass and have `@AutoService(InstrumenterModule.class)` +- Must implement `typeInstrumentations()` returning a `List` +- Must **not** implement an `Instrumenter` interface +- Member instrumentations must **not** carry `@AutoService` and must **not** extend `TargetSystem` subclasses + +See `docs/how_instrumentations_work.md` section "Grouping Instrumentations" for details. diff --git a/.agents/skills/apm-integrations/references/muzzle.md b/.agents/skills/apm-integrations/references/muzzle.md new file mode 100644 index 00000000000..1ca23275f03 --- /dev/null +++ b/.agents/skills/apm-integrations/references/muzzle.md @@ -0,0 +1,137 @@ +# Muzzle Directives + +> Referenced from `SKILL.md` Step 9.2. Everything about `muzzle { pass { … } fail { … } }` blocks and the traps that fail CI. + +## Muzzle directives (mandatory) + +In `build.gradle`, add `muzzle` blocks. **There are three valid patterns** — choose based on whether your version range is open-ended or bounded, and if bounded, why. + +**Pattern A — Open-ended range** (your instrumentation supports `[minVersion, ∞)` with no upper bound). Use `assertInverse = true` — the plugin auto-asserts that versions below `minVersion` fail muzzle: + +```groovy +muzzle { + pass { + group = "com.example" + module = "framework" + versions = "[$minVersion,)" + assertInverse = true + } +} +``` + +**Pattern B — Bounded range, sibling module takes over at `maxVersion`** (a separate module in `dd-java-agent/instrumentation/` covers `[maxVersion, ∞)`). **Do NOT use `assertInverse = true`** — the plugin can pick sibling-covered versions as inverse-test targets, causing unexpected failures: + +``` +> Task :muzzle-AssertFail-redis.clients-jedis-jedis-3.6.2 FAILED +MUZZLE PASSED JedisInstrumentation BUT FAILURE WAS EXPECTED +``` + +To avoid this, declare the pass range AND an explicit fail range below `$minVersion` (the sibling module covers versions above `$maxVersion`): + +```groovy +muzzle { + pass { + group = "com.example" + module = "framework" + versions = "[$minVersion,$maxVersion)" + } + fail { + group = "com.example" + module = "framework" + versions = "[,$minVersion)" + } +} +``` + +**Pattern C — Bounded range, incompatible major version above `maxVersion`** (the same `group:module` coordinates republish a completely different API at `maxVersion`). Use `assertInverse = true` — versions above `maxVersion` genuinely fail muzzle because the API is incompatible: + +```groovy +muzzle { + pass { + group = "commons-httpclient" + module = "commons-httpclient" + versions = "[2.0,4.0)" + assertInverse = true + } +} +``` + +## Do NOT use `assertInverse = true` unless the declared min is the actual minimum compatible version + +The `assertInverse = true` directive tells muzzle to auto-test versions below the declared minimum and assert they fail. If your instrumentation is actually compatible with versions below the declared minimum (a common case when only ONE of several instrumentation classes requires the new feature), this auto-assertion will fail with: + +``` +> Task :module:muzzle-AssertFail-group-module-X.Y.Z FAILED +MUZZLE PASSED FeignClientInstrumentation BUT FAILURE WAS EXPECTED +``` + +**Rule**: If you can't verify (via running `./gradlew muzzle` against a sweep of older versions) that the declared min version is truly the minimum compatible version, **omit `assertInverse = true`**: + +```groovy +// Conservative — passes muzzle without asserting unprovable inverse claims +muzzle { + pass { + group = "io.github.openfeign" + module = "feign-core" + versions = "[10.8,)" + // NO assertInverse here — we don't know the true minimum + } +} +``` + +Add `assertInverse = true` only when you've empirically verified the min via local muzzle sweep. Otherwise, leaving it off is correct. + +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. + +## Muzzle range must exclude incompatible major versions + +If the library you are instrumenting has a major version break where a newer major version +is published under the **same** `group:module` coordinates but with a completely different API, +your muzzle `pass` range must have an explicit upper bound to exclude the incompatible major: + +```groovy +// WRONG — open range accidentally covers 4.x (different API, same coordinates) +muzzle { + pass { + group = "com.example" + module = "framework" + versions = "[2.0,)" // 4.x would also match but has completely different API + assertInverse = true + } +} + +// CORRECT — bounded range excludes 4.x +muzzle { + pass { + group = "com.example" + module = "framework" + versions = "[2.0,4.0)" + assertInverse = true + } +} +``` + +**How to check**: search Maven Central for the `group:module` coordinates and look for versions +that are clearly a different generation (major API rewrites). Look at the existing dd-trace-java +module for the same library to see how it bounds its range. + +## Library-specific muzzle quirks: skipVersions + +Some libraries have **malformed release versions** in Maven Central (e.g., a version literally named `jedis-3.6.2` with a `jedis-` prefix). These break the muzzle plugin's version-resolution algorithm — the task name becomes `muzzle-AssertFail-redis.clients-jedis-jedis-3.6.2` (doubled "jedis") and the muzzle plan can include them in the wrong directive. + +The fix is `skipVersions` in the affected directive: + +```groovy +muzzle { + fail { + group = "redis.clients" + module = "jedis" + versions = "[,3.0.0)" + skipVersions += "jedis-3.6.2" // bad release version ("jedis-" prefix) + } +} +``` + +**How to discover these**: when verify fails with a task name that has a doubled module name (e.g., `redis.clients-jedis-jedis-3.6.2`), check the existing production module for the same library at another version. If it has `skipVersions` entries, copy them. This is library-specific tribal knowledge that lives in the existing modules. + +When in doubt, **search adjacent module build.gradle files for `skipVersions`** before declaring a new version-bounded module's muzzle directives. diff --git a/.agents/skills/apm-integrations/references/naming-conventions.md b/.agents/skills/apm-integrations/references/naming-conventions.md new file mode 100644 index 00000000000..ef089b9c147 --- /dev/null +++ b/.agents/skills/apm-integrations/references/naming-conventions.md @@ -0,0 +1,27 @@ +# Naming Conventions + +> Referenced from `SKILL.md` Step 4 (module directory name and Java class/file naming). + +## Module directory name must end with a version OR an allowed suffix + +dd-trace-java's `dd-gitlab/check-instrumentation-naming` plugin +(`buildSrc/.../naming/InstrumentationNamingPlugin.kt`) enforces: + +> Module name must end with a version (e.g., `2.0`, `3.1`) OR one of: `-common`, `-stubs`, `-iast` + +Pick a directory name like `$framework-$minVersion` (e.g. `okhttp-3.0`, `jedis-3.0`). For shared +helpers/stubs/iast support code factored out across version-specific modules, use the documented +suffixes above. + +## Java naming consistency (CRITICAL — non-negotiable) + +The filename and the declared `public class` name MUST match exactly, character-for-character including case. Java will not compile a file where they differ. + +**Pick one canonical name per class, then use that exact string everywhere:** + +- The filename (e.g., `JMSDecorator.java`) +- The class declaration inside (e.g., `public class JMSDecorator`) +- Every `import static ..` across all other files in the module +- Every reference in the form `.` or `.class` + +**Convention in dd-trace-java**: acronym casing is NOT uniform across the codebase. Some libraries use uppercase acronyms (`JMSDecorator`, `JDBCDecorator`) and others use title case (`GrpcClientDecorator`, `HttpServletDecorator`). **When in doubt, match a reference instrumentation's casing exactly** (see Step 3) — do not invent a casing convention. diff --git a/.agents/skills/apm-integrations/references/supported-configurations.md b/.agents/skills/apm-integrations/references/supported-configurations.md new file mode 100644 index 00000000000..815a672520c --- /dev/null +++ b/.agents/skills/apm-integrations/references/supported-configurations.md @@ -0,0 +1,56 @@ +# Supported Configurations Registry + +> Referenced from `SKILL.md` Steps 4, 6, and 9.1. How to register integration names in `metadata/supported-configurations.json` so CI linters pass. + +## Two distinct CI checks — different names, different key shapes + +- **`super(...)` args in `InstrumenterModule`** → validated by `dd-gitlab/config-inversion-linter` as `DD_TRACE__ENABLED` entries. Every name passed to the constructor must have a `_ENABLED` entry. +- **`instrumentationNames()` in the decorator** → validated by `checkDecoratorAnalyticsConfigurations` as `DD_TRACE__ANALYTICS_ENABLED` / `DD_TRACE__ANALYTICS_SAMPLE_RATE` entries. + +## `_ENABLED` entry (for each `super(...)` arg) + +For each new integration name `` (uppercase, dashes/dots replaced with underscores — `couchbase-3` → `DD_TRACE_COUCHBASE_3_ENABLED`), add: + +```json +"DD_TRACE__ENABLED": [ + { + "version": "A", + "type": "boolean", + "default": "true", + "aliases": ["DD_TRACE_INTEGRATION__ENABLED", "DD_INTEGRATION__ENABLED"] + } +], +``` + +**Default value:** the `_ENABLED` default mirrors the module's `defaultEnabled()` return value — `"true"` for typical integrations. Set `"default": "false"` only if the module overrides `defaultEnabled()` to return `false` (e.g. OpenTelemetry, Hazelcast, sparkjava). For new integrations this is always `"true"`; when modifying an existing module, verify with: `grep -A2 "defaultEnabled" dd-java-agent/instrumentation/*`. + +## Analytics entries (for each decorator `instrumentationNames()` return value) + +```json +"DD_TRACE__ANALYTICS_ENABLED": [ + { + "version": "A", + "type": "boolean", + "default": "false", + "aliases": ["DD__ANALYTICS_ENABLED"] + } +], +"DD_TRACE__ANALYTICS_SAMPLE_RATE": [ + { + "version": "A", + "type": "decimal", + "default": "1.0", + "aliases": ["DD__ANALYTICS_SAMPLE_RATE"] + } +], +``` + +## Rules + +**Place entries alphabetically** in the JSON file. + +**Type names — match existing conventions**: use `"boolean"`, `"string"`, `"int"`, `"decimal"` (for floating-point — NOT `"double"`). The `dd-gitlab/validate_supported_configurations_v2_local_file` CI job will fail with non-canonical type names. + +The Gradle checks (`checkInstrumenterModuleConfigurations`, `checkDecoratorAnalyticsConfigurations`) validate the JSON automatically — they will fail with a parse error if the file is malformed. + +**How to discover whether entries are missing**: after writing the instrumentation, search `metadata/supported-configurations.json` for each name used in `super(...)` and decorator `instrumentationNames()`. If any is absent, add it. Do not assume master already has it — version-specific integration names (e.g. `sparkjava-2.3` vs `sparkjava-2.4`) are not interchangeable. diff --git a/.agents/skills/apm-integrations/references/tests.md b/.agents/skills/apm-integrations/references/tests.md new file mode 100644 index 00000000000..b83a6410c4f --- /dev/null +++ b/.agents/skills/apm-integrations/references/tests.md @@ -0,0 +1,85 @@ +# Writing Tests + +> Referenced from `SKILL.md` Step 9.1 (Instrumentation test). For muzzle directives (Step 9.2), see `muzzle.md` in this directory. + +## 1. Instrumentation test (mandatory) + +**Write Groovy/Spock tests for instrumentation tests** (per `AGENTS.md`: "Only use Groovy / Spock tests for instrumentation and smoke tests"). Full Java instrumentation test support is not yet available. Adding new `.groovy` files to a PR will trigger the `Enforce Groovy Migration` bot — add the `tag: override groovy enforcement` label to bypass it. + +- Groovy/Spock test class in `src/test/groovy/datadog/trace/instrumentation//` +- Verify: spans created, tags set, errors propagated, resource names correct +- Use `assertTraces(N) { trace(N) { span { ... } } }` for span assertions (Spock DSL from `InstrumentationSpecification`) +- Use `TEST_WRITER.waitForTraces(N)` for setup/teardown flushing (not for assertions) +- Use `runUnderTrace("root") { ... }` from `TraceUtils` for synchronous code (trailing Groovy closure) + +**Tests must cover error/exception scenarios, not just the happy path.** At minimum, add a test that exercises an exception or error condition and asserts the span's error tags (`error.type`, `error.message`, `error.stack`) are set correctly: + +```groovy +// Example error test (Groovy/Spock) +def "exception sets error tags"() { + when: + client.execute(badRequest) + + then: + thrown(SomeException) + assertTraces(1) { + trace(1) { + span { + errored true + errorTags(SomeException, "expected error message") + } + } + } +} +``` + +For tests that need a separate JVM, suffix the test class with `ForkedTest` and run via the `forkedTest` task. + +### Register new integration names in `metadata/supported-configurations.json` + +See [Supported Configurations](supported-configurations.md) for the key shapes, CI checks, and JSON format. + +### compileOnly and testImplementation may use different versions — explain why + +When `compileOnly` and `testImplementation` use different versions of the same library, add a +comment that explains the specific API or class that requires the higher version, and why. +Do not just state the fact — state the reason. + +```groovy +// WRONG — states the fact without explaining why +// compileOnly=2.3, testImplementation=2.4 +compileOnly group: 'com.sparkjava', name: 'spark-core', version: '2.3' +testImplementation group: 'com.sparkjava', name: 'spark-core', version: '2.4' + +// CORRECT — explains the specific class and why it requires the higher version +// compileOnly=2.3 (module targets this version) but testImplementation=2.4: +// JettyHandler, which Spark uses internally to dispatch HTTP requests to route handlers, +// is not accessible as a public class in 2.3 — it was exposed starting in 2.4. +// The instrumentation hooks into JettyHandler via Jetty's existing instrumentation, +// so tests require 2.4 at minimum to exercise the code path. +compileOnly group: 'com.sparkjava', name: 'spark-core', version: '2.3' +testImplementation group: 'com.sparkjava', name: 'spark-core', version: '2.4' +``` + +**How to discover this during development**: install the library at the `compileOnly` version and run +your instrumentation test. If a specific class raises `ClassNotFoundException` or is inaccessible, that +class is the reason — check when it became public and use that version for `testImplementation`. +Name the class in the comment. + +### Include sibling version modules in testImplementation for mutual exclusion + +When two modules instrument the same library at non-overlapping version ranges, each module should include the other as a `testImplementation` dependency to confirm they don't double-instrument. The rule is symmetric — both the older and the newer module should carry this dependency: + +```groovy +// jedis-3.0/build.gradle +dependencies { + testImplementation project(':dd-java-agent:instrumentation:jedis:jedis-1.4') +} + +// jedis-1.4/build.gradle +dependencies { + testImplementation project(':dd-java-agent:instrumentation:jedis:jedis-3.0') +} +``` + +This ensures `:test` in each module validates that only the correct module fires for its version range. diff --git a/.claude/skills/migrate-groovy-to-java/QUALITY_RULES.md b/.agents/skills/migrate-groovy-to-java/QUALITY_RULES.md similarity index 100% rename from .claude/skills/migrate-groovy-to-java/QUALITY_RULES.md rename to .agents/skills/migrate-groovy-to-java/QUALITY_RULES.md diff --git a/.claude/skills/migrate-groovy-to-java/SKILL.md b/.agents/skills/migrate-groovy-to-java/SKILL.md similarity index 100% rename from .claude/skills/migrate-groovy-to-java/SKILL.md rename to .agents/skills/migrate-groovy-to-java/SKILL.md diff --git a/.claude/skills/migrate-junit-source-to-tabletest/SKILL.md b/.agents/skills/migrate-junit-source-to-tabletest/SKILL.md similarity index 100% rename from .claude/skills/migrate-junit-source-to-tabletest/SKILL.md rename to .agents/skills/migrate-junit-source-to-tabletest/SKILL.md diff --git a/.claude/skills/review-groovy-migration/SKILL.md b/.agents/skills/review-groovy-migration/SKILL.md similarity index 100% rename from .claude/skills/review-groovy-migration/SKILL.md rename to .agents/skills/review-groovy-migration/SKILL.md diff --git a/.claude/skills/techdebt/SKILL.md b/.agents/skills/techdebt/SKILL.md similarity index 100% rename from .claude/skills/techdebt/SKILL.md rename to .agents/skills/techdebt/SKILL.md diff --git a/.claude/skills/add-apm-integrations/SKILL.md b/.claude/skills/add-apm-integrations/SKILL.md deleted file mode 100644 index 6b0e57414b0..00000000000 --- a/.claude/skills/add-apm-integrations/SKILL.md +++ /dev/null @@ -1,258 +0,0 @@ ---- -name: add-apm-integrations -description: Write a new library instrumentation end-to-end. Use when the user ask to add a new APM integration or a library instrumentation. -context: fork -allowed-tools: - - Bash - - Read - - Write - - Edit - - Glob - - Grep ---- - -Write a new APM end-to-end integration for dd-trace-java, based on library instrumentations, following all project conventions. - -## Step 1 – Read the authoritative docs and sync this skill (mandatory, always first) - -Before writing any code, read all three files in full: - -1. [`docs/how_instrumentations_work.md`](docs/how_instrumentations_work.md) — full reference (types, methods, advice, helpers, context stores, decorators) -2. [`docs/add_new_instrumentation.md`](docs/add_new_instrumentation.md) — step-by-step walkthrough -3. [`docs/how_to_test.md`](docs/how_to_test.md) — test types and how to run them - -These files are the single source of truth. Reference them while implementing. - -**After reading the docs, sync this skill with them:** - -Compare the content of the three docs against the rules encoded in Steps 2–11 of this skill file. Look for: -- Patterns, APIs, or conventions described in the docs but absent or incorrect here -- Steps that are out of date relative to the current docs (e.g. renamed methods, new base classes) -- Advice constraints or test requirements that have changed - -For every discrepancy found, edit this file (`.claude/skills/apm-integrations/SKILL.md`) to correct it using the -`Edit` tool before continuing. Keep changes targeted: fix what diverged, add what is missing, remove what is wrong. -Do not touch content that already matches the docs. - -## Step 2 – Clarify the task - -If the user has not already provided all of the following, ask before proceeding: - -- **Framework name** and **minimum supported version** (e.g. `okhttp-3.0`) -- **Target class(es) and method(s)** to instrument (fully qualified class names preferred) -- **Target system**: one of `Tracing`, `Profiling`, `AppSec`, `Iast`, `CiVisibility`, `Usm`, `ContextTracking` -- **Whether this is a bootstrap instrumentation** (affects allowed imports) - -## Step 3 – Find a reference instrumentation - -Search `dd-java-agent/instrumentation/` for a structurally similar integration: -- Same target system -- Comparable type-matching strategy (single type, hierarchy, known types) - -Read the reference integration's `InstrumenterModule`, Advice, Decorator, and test files to understand the established -pattern before writing new code. Use it as a template. - -## Step 4 – Set up the module - -1. Create directory: `dd-java-agent/instrumentation/$framework/$framework-$minVersion/` -2. Under it, create the standard Maven source layout: - - `src/main/java/` — instrumentation code - - `src/test/groovy/` — Spock tests -3. Create `build.gradle` with: - - `compileOnly` dependencies for the target framework - - `testImplementation` dependencies for tests - - `muzzle { pass { } }` directives (see Step 9) -4. Register the new module in `settings.gradle.kts` in **alphabetical order** -5. Register the integration name in `metadata/supported-configurations.json`, or - `checkInstrumenterModuleConfigurations` fails. The name in `super(...)` maps to env var - `DD_TRACE__ENABLED` (`.` and `-` become `_`, uppercased — `couchbase-3` → - `DD_TRACE_COUCHBASE_3_ENABLED`). Add a `"type": "boolean"` entry, in alphabetical order, with - aliases `DD_TRACE_INTEGRATION__ENABLED` and `DD_INTEGRATION__ENABLED`. Set `default` - to the module's real default — `"true"`, or `"false"` if it overrides `defaultEnabled()` (e.g. - OpenTelemetry, Hazelcast). Declaring several names (`super("a", "b")`) means one entry each. - -## Step 5 – Write the InstrumenterModule - -Conventions to enforce: - -- Add `@AutoService(InstrumenterModule.class)` annotation — required for auto-discovery -- Extend the correct `InstrumenterModule.*` subclass (never the bare abstract class) -- Implement the **narrowest** `Instrumenter` interface possible: - - Prefer `ForSingleType` > `ForKnownTypes` > `ForTypeHierarchy` -- Add `classLoaderMatcher()` if a sentinel class identifies the framework on the classpath -- Declare **all** helper class names in `helperClassNames()`: - - Include inner classes (`Foo$Bar`), anonymous classes (`Foo$1`), and enum synthetic classes -- Declare `contextStore()` entries if context stores are needed (key class → value class) -- Keep method matchers as narrow as possible (name, parameter types, visibility) - -### Must NOT do in InstrumenterModule - -- **Do not extract one-shot method return values into static constants.** - Methods like `triggerClasses()`, `contextStore()`, `classLoaderMatcher()`, and `methodAdvice()` - are called **once** by `AgentInstaller` / the framework wiring. Extracting their return value - into a `private static final` constant provides no performance benefit and needlessly bloats - the constant pool of the instrumentation class. - - ❌ `private static final String[] TRIGGER_CLASSES = new String[]{"com.example.Foo"};` - `public String[] triggerClasses() { return TRIGGER_CLASSES; }` - - ✅ `public String[] triggerClasses() { return new String[]{"com.example.Foo"}; }` - -## Step 6 – Write the Decorator - -- Extend the most specific available base decorator: - - `HttpClientDecorator`, `DatabaseClientDecorator`, `ServerDecorator`, `MessagingClientDecorator`, etc. -- One `public static final DECORATE` instance -- Define `UTF8BytesString` constants for the component name and operation name -- Keep all tag/naming/error logic here — not in the Advice class -- Override `spanType()`, `component()`, `spanKind()` as appropriate - -## Step 7 – Write the Advice class (highest-risk step) - -### Must do - -- Advice methods **must** be `static` -- Annotate enter: `@Advice.OnMethodEnter(suppress = Throwable.class)` -- Annotate exit: `@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)` - - **Exception**: do NOT use `suppress` when hooking a constructor -- Use `@Advice.Local("...")` for values shared between enter and exit (span, scope) -- Use the correct parameter annotations: - - `@Advice.This` — the receiver object - - `@Advice.Argument(N)` — a method argument by index - - `@Advice.Return` — the return value (exit only) - - `@Advice.Thrown` — the thrown exception (exit only) - - `@Advice.Enter` — the return value of the enter method (exit only) -- Use `CallDepthThreadLocalMap` to guard against recursive instrumentation of the same method - -### Span lifecycle (in order) - -Enter method: -1. `AgentSpan span = startSpan(DECORATE.operationName(), ...)` -2. `DECORATE.afterStart(span)` + set domain-specific tags -3. `AgentScope scope = activateSpan(span)` — return or store via `@Advice.Local` - -Exit method: -4. `DECORATE.onError(span, throwable)` — only if throwable is non-null -5. `DECORATE.beforeFinish(span)` -6. `span.finish()` -7. `scope.close()` - -### Must NOT do - -- **No logger fields** in the Advice class or the Instrumentation class (loggers only in helpers/decorators) -- **No code in the Advice constructor** — it is never called -- **Do not use lambdas in advice methods** — they create synthetic classes that will be missing from helper declarations -- **No references** to other methods in the same Advice class or in the InstrumenterModule class -- **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.*`, or `javax.management.*`** in bootstrap instrumentations - -## Step 8 – Add SETTER/GETTER adapters (if applicable) - -For context propagation to and from upstream services, like HTTP headers, -implement `AgentPropagation.Setter` / `AgentPropagation.Getter` adapters that wrap the framework's specific header API. -Place them in the helpers package, declare them in `helperClassNames()`. - -## Step 9 – Write tests - -Cover all mandatory test types: - -### 1. Instrumentation test (mandatory) - -- Spock spec extending `InstrumentationSpecification` -- Place in `src/test/groovy/` -- Verify: spans created, tags set, errors propagated, resource names correct -- Use `TEST_WRITER.waitForTraces(N)` for assertions -- Use `runUnderTrace("root") { ... }` for synchronous code - -For tests that need a separate JVM, suffix the test class with `ForkedTest` and run via the `forkedTest` task. - -### 2. Muzzle directives (mandatory) - -In `build.gradle`, add `muzzle` blocks: -```groovy -muzzle { - pass { - group = "com.example" - module = "framework" - versions = "[$minVersion,)" - assertInverse = true // ensures versions below $minVersion fail muzzle - } -} -``` - -### 3. Latest dependency test (mandatory) - -Use the `latestDepTestLibrary` helper in `build.gradle` to pin the latest available version. Run with: -```bash -./gradlew :dd-java-agent:instrumentation:$framework-$version:latestDepTest -``` - -### 4. Smoke test (optional) - -Add a smoke test in `dd-smoke-tests/` only if the framework warrants a full end-to-end demo-app test. - -## Step 10 – Build and verify - -Run these commands in order and fix any failures before proceeding: - -```bash -./gradlew :dd-java-agent:instrumentation:$framework-$version:muzzle -./gradlew :dd-java-agent:instrumentation:$framework-$version:test -./gradlew :dd-java-agent:instrumentation:$framework-$version:latestDepTest -./gradlew checkInstrumenterModuleConfigurations -./gradlew spotlessCheck -``` - -**If muzzle fails:** check for missing helper class names in `helperClassNames()`. - -**If `checkInstrumenterModuleConfigurations` fails:** an integration name from `super(...)` is missing -(or mismatched) in `metadata/supported-configurations.json` — see Step 4, item 5. - -**If tests fail:** verify span lifecycle order (start → activate → error → finish → close), helper registration, -and `contextStore()` map entries match actual usage. - -**If spotlessCheck fails:** run `./gradlew spotlessApply` to auto-format, then re-check. - -## Step 11 – Checklist before finishing - -Output this checklist and confirm each item is satisfied: - -- [ ] `settings.gradle.kts` entry added in alphabetical order -- [ ] `metadata/supported-configurations.json` has a `DD_TRACE__ENABLED` entry (+ the two aliases) for every name passed to `super(...)` -- [ ] `build.gradle` has `compileOnly` deps and `muzzle` directives with `assertInverse = true` -- [ ] `@AutoService(InstrumenterModule.class)` annotation present on the module class -- [ ] `helperClassNames()` lists ALL referenced helpers (including inner, anonymous, and enum synthetic classes) -- [ ] Advice methods are `static` with `@Advice.OnMethodEnter` / `@Advice.OnMethodExit` annotations -- [ ] `suppress = Throwable.class` on enter/exit (unless the hooked method is a constructor) -- [ ] No static constants holding return values of one-shot instrumenter methods (`triggerClasses()`, `contextStore()`, etc.) -- [ ] No logger field in the Advice class or InstrumenterModule class -- [ ] No `inline=false` left in production code -- [ ] No `java.util.logging.*` / `java.nio.*` / `javax.management.*` in bootstrap path -- [ ] Span lifecycle order is correct: startSpan → afterStart → activateSpan (enter); onError → beforeFinish → finish → close (exit) -- [ ] Muzzle passes -- [ ] Instrumentation tests pass -- [ ] `latestDepTest` passes -- [ ] `spotlessCheck` passes - -## Step 12 – Retrospective: update this skill with what was learned - -After the instrumentation is complete (or abandoned), review the full session and improve this skill for future use. - -**Collect lessons from four sources:** - -1. **Build/test failures** — did any Gradle task fail with an error that this skill did not anticipate or gave wrong - guidance for? (e.g. a muzzle failure that wasn't caused by missing helpers, a test pattern that didn't work) -2. **Docs vs. skill gaps** — did Step 1's sync miss anything? Did you consult the docs for something not captured here? -3. **Reference instrumentation insights** — did the reference integration use a pattern, API, or convention not - reflected in any step of this skill? -4. **User corrections** — did the user correct an output, override a decision, or point out a mistake? - -**For each lesson identified**, edit this file (`.claude/skills/apm-integrations/SKILL.md`) using the `Edit` tool: -- Wrong rule → fix it in place -- Missing rule → add it to the most relevant step -- Wrong failure guidance → update the relevant "If X fails" section in Step 10 -- Misleading or obsolete content → remove it - -Keep each change minimal and targeted. Do not rewrite sections that worked correctly. -After editing, confirm to the user which improvements were made to the skill. diff --git a/.claude/skills/apm-integrations b/.claude/skills/apm-integrations new file mode 120000 index 00000000000..89f300ce53a --- /dev/null +++ b/.claude/skills/apm-integrations @@ -0,0 +1 @@ +../../.agents/skills/apm-integrations \ No newline at end of file diff --git a/.claude/skills/migrate-groovy-to-java b/.claude/skills/migrate-groovy-to-java new file mode 120000 index 00000000000..9c9cfbe3f62 --- /dev/null +++ b/.claude/skills/migrate-groovy-to-java @@ -0,0 +1 @@ +../../.agents/skills/migrate-groovy-to-java \ No newline at end of file diff --git a/.claude/skills/migrate-junit-source-to-tabletest b/.claude/skills/migrate-junit-source-to-tabletest new file mode 120000 index 00000000000..e6e828679b7 --- /dev/null +++ b/.claude/skills/migrate-junit-source-to-tabletest @@ -0,0 +1 @@ +../../.agents/skills/migrate-junit-source-to-tabletest \ No newline at end of file diff --git a/.claude/skills/review-groovy-migration b/.claude/skills/review-groovy-migration new file mode 120000 index 00000000000..12d03bb6ebc --- /dev/null +++ b/.claude/skills/review-groovy-migration @@ -0,0 +1 @@ +../../.agents/skills/review-groovy-migration \ No newline at end of file diff --git a/.claude/skills/techdebt b/.claude/skills/techdebt new file mode 120000 index 00000000000..673cef85d58 --- /dev/null +++ b/.claude/skills/techdebt @@ -0,0 +1 @@ +../../.agents/skills/techdebt \ No newline at end of file diff --git a/AGENTS.md b/AGENTS.md index e6c0ce4ec90..69e6e48aac3 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -75,7 +75,7 @@ docs/ Developer documentation (see below) Code running in the agent's `premain` phase must **not** use: - `java.util.logging.*` — locks in log manager before app configures it -- `java.nio.*` — triggers premature provider initialization +- `java.nio.file.*` — triggers premature provider initialization - `javax.management.*` — causes class loading issues See [docs/bootstrap_design_guidelines.md](docs/bootstrap_design_guidelines.md) for details and alternatives. diff --git a/docs/add_new_instrumentation.md b/docs/add_new_instrumentation.md index 27a851dd198..e845661039a 100644 --- a/docs/add_new_instrumentation.md +++ b/docs/add_new_instrumentation.md @@ -17,18 +17,17 @@ named `google-http-client`. (see [Naming](./how_instrumentations_work.md#naming) ## Configuring Gradle -Add the new instrumentation to [`settings.gradle`](../settings.gradle) +Add the new instrumentation to [`settings.gradle.kts`](../settings.gradle.kts) in alpha order with the other instrumentations in this format: -```groovy -include ':dd-java-agent:instrumentation:$framework?:$framework-$minVersion' +```kotlin +include(":dd-java-agent:instrumentation:$framework:$framework-$minVersion") ``` -In this case -we [added](https://github.com/DataDog/dd-trace-java/blob/297b575f0f265c1dc78f9958e7b4b9365c80d1f9/settings.gradle#L209C3-L209C3): +For example, the jedis 3.x instrumentation appears as: -```groovy -include ':dd-java-agent:instrumentation:google-http-client' +```kotlin +include(":dd-java-agent:instrumentation:jedis:jedis-3.0") ``` ## Create the Instrumentation class @@ -59,7 +58,8 @@ include ':dd-java-agent:instrumentation:google-http-client' ```java @AutoService(InstrumenterModule.class) -public class GoogleHttpClientInstrumentation extends InstrumenterModule.Tracing implements Instrumenter.ForSingleType { +public class GoogleHttpClientInstrumentation extends InstrumenterModule.Tracing + implements Instrumenter.ForSingleType, Instrumenter.HasMethodAdvice { public GoogleHttpClientInstrumentation() { super("google-http-client"); } @@ -102,11 +102,11 @@ public HttpResponse execute() throws IOException {/* */} ``` Target the method using [appropriate Method Matchers](./how_instrumentations_work.md#method-matching) and include the -name String to be used for the Advice class when calling `transformation.applyAdvice()`: +name String to be used for the Advice class when calling `transformer.applyAdvice()`: ```java -public void adviceTransformations(AdviceTransformation transformation) { - transformation.applyAdvice( +public void methodAdvice(MethodTransformer transformer) { + transformer.applyAdvice( isMethod() .and(isPublic()) .and(named("execute")) @@ -121,8 +121,8 @@ public void adviceTransformations(AdviceTransformation transformation) { If you need to apply multiple advice classes to the same method (for example, to separate context tracking from tracing logic), you can pass multiple advice class names to `applyAdvices()`: ```java -public void adviceTransformations(AdviceTransformation transformation) { - transformation.applyAdvices( +public void methodAdvice(MethodTransformer transformer) { + transformer.applyAdvices( named("service") .and(takesArgument(0, named("org.apache.coyote.Request"))) .and(takesArgument(1, named("org.apache.coyote.Response"))), @@ -252,7 +252,7 @@ public String[] helperClassNames() { ## Add Advice class 1. Add a new static class to the Instrumentation class. The name must match what was passed to - the `adviceTransformations()` method earlier, here `GoogleHttpClientAdvice.` + the `methodAdvice()` method earlier, here `GoogleHttpClientAdvice.` 2. Create two static methods named whatever you like. `methodEnter` and `methodExit` are good choices. These **must** be static. 3. With `methodEnter:` @@ -360,8 +360,8 @@ public static class TracingAdvice { Then apply both advices: ```java -public void adviceTransformations(AdviceTransformation transformation) { - transformation.applyAdvice( +public void methodAdvice(MethodTransformer transformer) { + transformer.applyAdvices( named("service"), getClass().getName() + "$ContextTrackingAdvice", // Only for CONTEXT_TRACKING getClass().getName() + "$TracingAdvice" // Only for TRACING diff --git a/docs/bootstrap_design_guidelines.md b/docs/bootstrap_design_guidelines.md index 8cefe8195a6..24329034e06 100644 --- a/docs/bootstrap_design_guidelines.md +++ b/docs/bootstrap_design_guidelines.md @@ -47,12 +47,12 @@ import org.slf4j.LoggerFactory; private static final Logger log = LoggerFactory.getLogger(MyClass.class); ``` -### 2. Java NIO (`java.nio.*`) +### 2. Java NIO (`java.nio.file.*`) **Why to avoid:** - Calling `FileSystems.getDefault()` during premain can break applications that set the `java.nio.file.spi.DefaultFileSystemProvider` system property in their `main` method - Some applications expect to control the default filesystem implementation, and premature initialization locks in the wrong provider -- Loading `java.nio` also triggers native library initialization (`pthread` on Linux) which can introduce a race condition +- Loading `java.nio.file` also triggers native library initialization (`pthread` on Linux) which can introduce a race condition **Reference:** [Java NIO FileSystems Documentation](https://docs.oracle.com/javase/8/docs/api/java/nio/file/FileSystems.html#getDefault--) @@ -63,7 +63,7 @@ private static final Logger log = LoggerFactory.getLogger(MyClass.class); **Example from issue #9780:** ```java -// BAD - Triggers java.nio initialization in premain +// BAD - Triggers java.nio.file initialization in premain FileSystems.getDefault(); Path.of("/some/path"); @@ -110,6 +110,6 @@ Loading a class during premain can have unintended side effects: **Guideline:** Be aware of native library loading and initialization Some Java classes trigger native library loading: -- `java.nio` - triggers pthread initialization on Linux and may create a [race condition (race conditionJDK-8345810)](https://bugs.openjdk.org/browse/JDK-8345810) +- `java.nio.file` - triggers pthread initialization on Linux and may create a [race condition (race conditionJDK-8345810)](https://bugs.openjdk.org/browse/JDK-8345810) - Socket operations - may trigger native networking libraries - File system operations - platform-specific native code diff --git a/docs/how_instrumentations_work.md b/docs/how_instrumentations_work.md index 3c4aa7a9053..8a52dbf7f09 100644 --- a/docs/how_instrumentations_work.md +++ b/docs/how_instrumentations_work.md @@ -241,7 +241,7 @@ or [`UrlInstrumentation`](https://github.com/DataDog/dd-trace-java/blob/3e81c006 ### Method Matching After the type is selected, the type’s target members(e.g., methods) must next be selected using the Instrumentation -class’s `adviceTransformations()` method. +class’s `methodAdvice()` method. ByteBuddy’s [`ElementMatchers`](https://javadoc.io/doc/net.bytebuddy/byte-buddy/1.4.17/net/bytebuddy/matcher/ElementMatchers.html) are used to describe the target members to be instrumented. Datadog’s [`DDElementMatchers`](../dd-java-agent/agent-installer/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/DDElementMatchers.java) @@ -262,8 +262,8 @@ Here, any public `execute()` method taking no arguments will have `PreparedState ```java @Override -public void adviceTransformations(AdviceTransformation transformation) { - transformation.applyAdvice( +public void methodAdvice(MethodTransformer transformer) { + transformer.applyAdvice( nameStartsWith("execute") .and(takesArguments(0)) .and(isPublic()), @@ -276,8 +276,8 @@ Here, any matching `connect()` method will have `DriverAdvice` applied: ```java @Override -public void adviceTransformations(AdviceTransformation transformation) { - transformation.applyAdvice( +public void methodAdvice(MethodTransformer transformer) { + transformer.applyAdvice( nameStartsWith("connect") .and(takesArgument(0, String.class)) .and(takesArgument(1, Properties.class)) @@ -292,8 +292,8 @@ The `applyAdvices` method supports applying multiple advice classes to the same ```java @Override -public void adviceTransformations(AdviceTransformation transformation) { - transformation.applyAdvices( +public void methodAdvice(MethodTransformer transformer) { + transformer.applyAdvices( named("service") .and(takesArgument(0, named("org.apache.coyote.Request"))) .and(takesArgument(1, named("org.apache.coyote.Response"))), @@ -391,10 +391,10 @@ Decorator class names should end in _Decorator._ ## Advice Classes Byte Buddy injects compiled bytecode at runtime to wrap existing methods, so they communicate with Datadog at entry or exit. -These modifications are referred to as _advice transformation_ or just _advice_. +These modifications are referred to as _method advice_ or just _advice_. -Instrumenters register advice transformations by calling `AdviceTransformation.applyAdvice(ElementMatcher, String)` -and Methods are matched by the instrumentation's `adviceTransformations()` method. +Instrumenters register advice transformations by calling `MethodTransformer.applyAdvice(ElementMatcher, String)` +and methods are matched by the instrumentation's `methodAdvice()` method. The Advice is injected into the type so Advice can only refer to those classes on the bootstrap class-path or helpers injected into the application class-loader. @@ -491,8 +491,8 @@ In the Tomcat instrumentation, we apply both context tracking and tracing advice ```java @Override -public void adviceTransformations(AdviceTransformation transformation) { - transformation.applyAdvices( +public void methodAdvice(MethodTransformer transformer) { + transformer.applyAdvices( named("service") .and(takesArgument(0, named("org.apache.coyote.Request"))) .and(takesArgument(1, named("org.apache.coyote.Response"))), @@ -525,9 +525,8 @@ The opposite would be either no `suppress` annotation or equivalently `suppress allow exceptions in Advice code to surface and is usually undesirable. > [!NOTE] -> Don't use `suppress` on an advice hooking a constructor. -> For older JVMs that do not support [flexible constructor bodies](https://openjdk.org/jeps/513), you can't decorate the -> mandatory self or parent constructor call with try/catch, as it must be the first call from the constructor body. +> Don't use `onThrowable` on exit advice hooking a constructor. +> If the constructor throws, `@Advice.This` is a partially initialized object, making exit advice unsafe to run. If the [`Advice.OnMethodEnter`](https://javadoc.io/static/net.bytebuddy/byte-buddy/1.10.2/net/bytebuddy/asm/Advice.OnMethodEnter.html)