From 16a575f883d5b3bc4b63016831cc1839043405bb Mon Sep 17 00:00:00 2001 From: Jordan Wong Date: Mon, 29 Jun 2026 00:55:54 -0400 Subject: [PATCH 01/22] skill(add-apm-integrations): R13-R33 + Cat B (context-propagation) Step 4.4 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Port reviewer-encoded rules from the toolkit's synced derivative (`apm-integrations` skill in DataDog/apm-instrumentation-toolkit) back to the canonical `add-apm-integrations` skill. The toolkit accumulated 21+ numbered reviewer rules during eval research (commits flowing through PR #11337 jedis-3.0, PR #11562 sparkjava-2.3, PR #11717 commons-httpclient, PR #11709 feign, and PR #11506 RxJava 3). These rules encode real failure modes observed during agent-generated PRs and the fixes reviewers asked for. Sections added: - **Step 4.4** — Library category: span-creating vs context-propagation. New axis that routes Cat B libraries (reactive, async, executors, futures, actors) into a `context_propagation` codegen path producing `InstrumenterModule.ContextTracking` instead of span-creating advice. Includes the Flowable subscribe(FlowableSubscriber) overload rule (hook the framework-internal overload, not the public wrapper). - **Step 4.5** — Java naming consistency (module-name conventions). R-rule placements: - Step 4: R32 (dir name must end with version OR allowed suffix) - Step 5: R13 (no single-type helper class for CallDepthThreadLocalMap) R30 (preserve master's integration name when regenerating) - Step 7: R15/R16/R17 (single delegate method, not all overloads) R33 (no NullPointerException catches; use null-check guards) - Step 9: R14 (test error/exception scenarios + spotless) R18 (muzzle excludes incompatible majors) R19 (latestDepTestImplementation range matches instrumented) R20 (Java tests only; no new .groovy files) R28 (compileOnly/testImplementation version split rationale) R29 (register names in supported-configurations.json) R31 (assertInverse only when declared min is true min) - Step 4.4: R21-R27 (the Cat A vs Cat B classification + Cat B schema) This PR pairs with toolkit-side PR DataDog/apm-instrumentation-toolkit#472 which adds the same content to the toolkit's synced derivative copy. Both copies should stay in sync — this is the canonical home. Signed-off-by: Jordan Wong --- .claude/skills/add-apm-integrations/SKILL.md | 548 ++++++++++++++++++- 1 file changed, 542 insertions(+), 6 deletions(-) diff --git a/.claude/skills/add-apm-integrations/SKILL.md b/.claude/skills/add-apm-integrations/SKILL.md index 6b0e57414b0..b0605a521ae 100644 --- a/.claude/skills/add-apm-integrations/SKILL.md +++ b/.claude/skills/add-apm-integrations/SKILL.md @@ -63,7 +63,7 @@ pattern before writing new code. Use it as a template. - `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 +5. Register the integration name in `metadata/supported-configurations.json` (see R29 below in Step 9), 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 @@ -71,6 +71,111 @@ pattern before writing new code. Use it as a template. 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. +#### R32 — 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. + +## Step 4.4 – Library category: span-creating vs context-propagation (read BEFORE picking targets) + +**Before picking instrumentation targets**, classify the library along the `target_kind` axis: + +**Category A — span-creating** (most libraries): performs I/O, makes calls, runs queries. The instrumentation creates spans around those operations. + +- HTTP clients/servers, DB clients, messaging clients, RPC frameworks +- Targets: methods that perform the I/O (e.g., `Connection.sendCommand`, `Client.execute`) +- Tests: assert spans exist with correct tags + +**Category B — context-propagation** (reactive, async, threading, executor, fiber libraries): does NOT perform I/O directly. It coordinates work that other code performs. Instrumentation captures the active trace context at boundary creation and restores it at boundary crossing — **no spans are created by the module**, it only bridges trace context for spans created by other instrumentations or by user code. + +- RxJava, Reactor, CompletableFuture, ListenableFuture, executors, pekko/akka, lettuce async-command queue, ZIO, virtual threads +- Targets: one boundary-crossing type (e.g. `Observable`, `Flowable`, `Single`, `Maybe`, `Completable` for RxJava-shaped libs; `Runnable`/`Callable` for executor-shaped libs) +- Tests: assert that a span created in operation X is still ACTIVE when a callback scheduled by Y runs (parent-child bridging of *user-created* spans, NOT span tags on a target span) + +**Reference implementation for Category B:** + +`dd-java-agent/instrumentation/rxjava/rxjava-2.0/` — uses `InstrumenterModule.ContextTracking`. 5 type-instrumenters (Observable, Flowable, Single, Maybe, Completable), 5 wrappers (Tracing{Observer,Subscriber,SingleObserver,MaybeObserver,CompletableObserver}), 1 `RxJavaModule.java`, 1 `RxJavaAsyncResultExtension.java`. ~600 LOC total. + +### Category B target shape + +For each boundary-crossing type, capture: + +- `library_class` — FQN of the boundary-crossing type (e.g. `io.reactivex.rxjava3.core.Observable`). +- `capture_method` — capture point (usually the constructor — `` or `isConstructor()`). +- `restore_method` — restore point (usually `subscribe(Observer)` / `subscribe(Subscriber)`). +- `wrapped_argument_type` — FQN of the user callback argument that must be wrapped (e.g. `io.reactivex.rxjava3.core.Observer`). +- `context_key_class` — FQN to key the `contextStore` on (almost always equals `library_class`). +- `wrapper_class_name` — class name of the wrapper to generate (e.g. `TracingObserver`). +- `wrapper_methods` — methods on the wrapped type that must reattach context before delegating (e.g. `["onNext", "onError", "onComplete"]`). + +### Tests for Category B + +Context-propagation 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**: + +```java +runUnderTrace("parent", () -> { + constructBoundary() // capture happens here + .subscribe(item -> userCodeStartsAChildSpan()); // restore happens around the lambda +}); +// 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 (Category B) + +When a reactive 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` hooks `subscribe(Observer)` (the implementation). The RxJava 3 reference instrumentation hooks `subscribe(FlowableSubscriber)`, not `subscribe(Subscriber)`. + +### When NOT to use Category B + +If the library DOES perform I/O — sends HTTP requests, runs DB queries, makes RPC calls, talks to a broker, reads/writes a cache — it is **span-creating**, not context-propagation. + +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-propagation instrumentation for the coordination path. `lettuce-5.0` is an example: there is a span-creating instrumentation for Redis commands and a separate context-propagation instrumentation for the async command queue. + +## Step 4.5 – 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 classes use **uppercase**: + +- CORRECT: `JMSDecorator`, `gRPCInstrumentation`, `JDBCConnection` +- WRONG: `JmsDecorator`, `GrpcInstrumentation`, `JdbcConnection` + +This matches the existing module conventions. When in doubt, match a reference instrumentation's casing exactly (see Step 3). + +**After generating each module, sanity-check before declaring done:** + +```bash +# Every public class declaration must match its filename +cd dd-java-agent/instrumentation/$framework/$framework-$version/src/main/java +for f in $(find . -name '*.java'); do + CLS=$(grep -oE 'public (final )?(abstract )?class [A-Za-z0-9_]+' "$f" | awk '{print $NF}') + EXPECTED=$(basename "$f" .java) + [ "$CLS" = "$EXPECTED" ] || echo "MISMATCH: $f declares '$CLS'" +done +``` + +If any MISMATCH lines print, fix them before moving on. + ## Step 5 – Write the InstrumenterModule Conventions to enforce: @@ -79,6 +184,17 @@ Conventions to enforce: - 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 @@ -98,6 +214,71 @@ Conventions to enforce: ✅ `public String[] triggerClasses() { return new String[]{"com.example.Foo"}; }` +#### instrumentationNames() must include a version-qualified alias + +```java +@Override +public String[] instrumentationNames() { + // WRONG — only generic name + return new String[]{"jedis"}; + + // CORRECT — generic + version alias + return new String[]{"jedis", "jedis-3.0"}; +} +``` + +The version alias (e.g. `"jedis-3.0"`) lets users enable/disable this specific version independently. + +#### R13 — 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 CallDepthThreadLocalMap directly with the Advice or Decorator class as key +if (CallDepthThreadLocalMap.incrementCallDepth(GsonInstrumentation.class) > 0) return; +// ... in exit: +CallDepthThreadLocalMap.reset(GsonInstrumentation.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. + +#### R30 — When regenerating an existing module, preserve master's integration name convention + +If you are modifying or regenerating instrumentation for a library that **already exists** in `dd-java-agent/instrumentation/` on master (e.g. `commons-httpclient-2.0/`), READ the existing module's `super(...)` and `instrumentationNames()` declarations and reuse them. + +```java +// Master's existing source uses dashed name: +// super("commons-http-client"); +// CORRECT (matches master, master's DD_TRACE_COMMONS_HTTP_CLIENT_* entries continue to work) +super("commons-http-client"); + +// WRONG (new name, requires adding 6 new supported-configurations.json entries) +super("commons-httpclient", "commons-httpclient-2.0"); +``` + +**Why**: dd-trace-java's `dd-gitlab/config-inversion-linter` requires registered names. Master already has registered entries for its existing convention; inventing a new convention forces a metadata change. Preserving the convention keeps the diff minimal and reviewer attention focused on the code, not boilerplate. + +### 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 an array of instrumentations +- 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. + ## Step 6 – Write the Decorator - Extend the most specific available base decorator: @@ -123,6 +304,7 @@ Conventions to enforce: - `@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 (R15/R16/R17)**: 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) @@ -137,6 +319,114 @@ Exit method: 6. `span.finish()` 7. `scope.close()` +#### onExit must be resilient to onEnter throwing + +If `onEnter` throws before the scope is set, `onExit` must still decrement the call depth. +A null-check that skips the reset leaks the ThreadLocal: + +```java +// RISKY — if onEnter threw, scope is null and reset is skipped +@Advice.OnMethodExit(suppress = Throwable.class) +public static void exit(@Advice.Enter final AgentScope scope) { + if (scope != null) scope.close(); +} + +// SAFER — onThrowable = Throwable.class ensures exit fires even on onEnter exception +@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) +public static void exit(@Advice.Enter final AgentScope scope) { + if (scope != null) { + scope.close(); + } +} +``` + +When using `CallDepthThreadLocalMap`, always decrement unconditionally in exit. + +#### 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); +``` + +#### R33 — Do NOT catch `NullPointerException`; use null-check guards instead + +dd-trace-java enforces SpotBugs rule `DCN_NULLPOINTER_EXCEPTION` (no NPE catch). Defensive `try { ... } catch (NullPointerException e) { ... }` patterns will 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. + +### Step 7.1 — 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()` instead of `applyAdvice()`: + +```java +@Override +public void adviceTransformations(AdviceTransformation transformation) { + transformation.applyAdvices( + named("someMethod") + .and(takesArgument(0, named("com.example.Request"))) + .and(takesArgument(1, named("com.example.Response"))), + getClass().getName() + "$ContextTrackingAdvice", // Applied first + getClass().getName() + "$ServiceAdvice" // Applied second + ); +} +``` + +Use the `@AppliesOn` annotation to control which target systems each advice applies to: + +```java +import datadog.trace.agent.tooling.InstrumenterModule.TargetSystem; +import datadog.trace.agent.tooling.annotation.AppliesOn; + +@AppliesOn(TargetSystem.CONTEXT_TRACKING) +public static class ContextTrackingAdvice { + @Advice.OnMethodEnter(suppress = Throwable.class) + public static void enter(@Advice.Argument(0) Request request) { + // This advice only runs when CONTEXT_TRACKING is enabled + } +} + +public static class TracingAdvice { + // Without @AppliesOn, this advice runs for the module's target system + @Advice.OnMethodEnter(suppress = Throwable.class) + public static void enter(@Advice.Argument(0) Request request) { + // Tracing-specific logic + } +} +``` + +**When to use `@AppliesOn`:** + +- Separate context-propagation logic from tracing logic +- Different target systems need different instrumentation behaviours +- Multiple advices apply to the same method with different system requirements + +See `docs/how_instrumentations_work.md` section "@AppliesOn Annotation" for complete details. + ### Must NOT do - **No logger fields** in the Advice class or the Instrumentation class (loggers only in helpers/decorators) @@ -159,28 +449,262 @@ Cover all mandatory test types: ### 1. Instrumentation test (mandatory) -- Spock spec extending `InstrumentationSpecification` -- Place in `src/test/groovy/` +**Write Java tests (JUnit 5), NOT Groovy/Spock (R20).** dd-trace-java policy +forbids new `.groovy` files — the PR bot (`checkNewGroovyFiles`) rejects any PR containing them. +All new tests must go in `src/test/java/`. + +- JUnit 5 test class in `src/test/java/datadog/trace/instrumentation//` - Verify: spans created, tags set, errors propagated, resource names correct - Use `TEST_WRITER.waitForTraces(N)` for assertions -- Use `runUnderTrace("root") { ... }` for synchronous code +- Use `runUnderTrace("root", () -> { ... })` for synchronous code (Java lambda, not Groovy closure) + +**Tests must cover error/exception scenarios, not just the happy path (R14).** 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: + +```java +// Example error test (Java) +@Test +void testExceptionSetsErrorTags() throws Exception { + assertThrows(SomeException.class, () -> { + // trigger operation that throws + client.execute(badRequest); + }); + List> traces = TEST_WRITER.waitForTraces(1); + SpanData span = traces.get(0).get(0); + assertThat(span.getAttributes().get(ERROR_TYPE)).isNotNull(); + assertThat(span.getAttributes().get(ERROR_MESSAGE)).isNotNull(); +} +``` For tests that need a separate JVM, suffix the test class with `ForkedTest` and run via the `forkedTest` task. +#### R20 — No new .groovy files (Java tests only) + +dd-trace-java forbids new `.groovy` files (bot-enforced on every PR). Write Java: + +```java +// WRONG — generates a .groovy file which the bot rejects automatically +// src/test/groovy/JedisClientTest.groovy + +// CORRECT — Java in src/test/java/ +// src/test/java/datadog/trace/instrumentation/jedis3/Jedis3ClientTest.java +@ExtendWith(AgentJUnit5Extension.class) +public class Jedis3ClientTest extends AgentInstrumentationTest { + @Test + void testCommandCreatesSpan() { + // test body + } +} +``` + +The PR bot check (`checkNewGroovyFiles`) will fail the PR if any `.groovy` file is added. + +#### R29 — Register new integration names in `metadata/supported-configurations.json` + +Every new integration name in your module (whether from `super("foo-X.Y")` in `InstrumenterModule`, or from `instrumentationNames()` in the decorator) MUST have a corresponding entry in `metadata/supported-configurations.json` at the repo root. The `dd-gitlab/config-inversion-linter` CI job fails otherwise. + +For each new integration name `` (uppercase, dashes/dots replaced with underscores), add: + +```json +"DD_TRACE__ENABLED": [ + { + "version": "A", + "type": "boolean", + "default": "false", + "aliases": ["DD_TRACE_INTEGRATION__ENABLED", "DD_INTEGRATION__ENABLED"] + } +], +``` + +If the decorator's `instrumentationNames()` returns a shared name (e.g. `"sparkjava"` covering all sparkjava versions), also add the analytics keys for the shared name: + +```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"] + } +], +``` + +**Place entries alphabetically** in the JSON file. **Verify the JSON parses** before committing (`python3 -c "import json; json.load(open('metadata/supported-configurations.json'))"`). + +**Type names — match existing conventions**: use `"boolean"`, `"string"`, `"integer"`, `"decimal"` (for floating-point — NOT `"double"`). The `dd-gitlab/validate_supported_configurations_v2_local_file` CI job will fail with non-canonical type names like `"double"`. Cross-check by grepping existing entries for similar fields. + +**How to discover whether entries are missing**: after writing the instrumentation, search `metadata/supported-configurations.json` for each name used in `super(...)` and `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. + +#### R28 — 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 `compileOnly` version and run +the sample app. 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 prior version module in testImplementation for mutual exclusion + +When two modules instrument the same library at different versions, add the prior +version's module as a `testImplementation` dependency to confirm they don't double-instrument: + +```groovy +// jedis-3.0/build.gradle +dependencies { + testImplementation project(':dd-java-agent:instrumentation:jedis:jedis-1.4') +} +``` + +This ensures `:test` validates that only the correct module fires for jedis-3.x requests. + ### 2. Muzzle directives (mandatory) -In `build.gradle`, add `muzzle` blocks: +In `build.gradle`, add `muzzle` blocks. **There are two valid patterns** — choose based on whether your version range is open-ended or bounded. + +**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 // ensures versions below $minVersion fail muzzle + assertInverse = true } } ``` +**Pattern B — Bounded range** (your instrumentation supports `[minVersion, maxVersion)` and an existing sibling module covers `[maxVersion, ∞)`). **Do NOT use `assertInverse = true`** — it can pick boundary versions inside your declared range as inverse-test targets, causing `muzzle-AssertFail-...` failures: + +``` +> Task :muzzle-AssertFail-redis.clients-jedis-jedis-3.6.2 FAILED +MUZZLE PASSED JedisInstrumentation BUT FAILURE WAS EXPECTED +``` + +Instead, declare BOTH the pass range AND the explicit fail ranges that bound it: + +```groovy +muzzle { + pass { + group = "com.example" + module = "framework" + versions = "[$minVersion,$maxVersion)" + } + fail { + group = "com.example" + module = "framework" + versions = "[,$minVersion)" + } +} +``` + +#### R31 — 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. + +**Background**: a typical greenfield generation produces a sync instrumentation class (works on older versions) and an async instrumentation class (requires a newer API). The agent picks the higher version as the declared min for muzzle, which is conservative for compileOnly. But `assertInverse = true` then auto-tests a lower version with ONLY the sync class hooks active, and that passes — creating the failure. + +#### R18 — 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 +(e.g. `commons-httpclient:commons-httpclient` 2.x vs 4.x), your muzzle `pass` range must +have an explicit upper bound to exclude the incompatible major: + +```groovy +// WRONG — open range accidentally covers commons-httpclient 4.x (different artifact family) +muzzle { + pass { + group = "commons-httpclient" + module = "commons-httpclient" + versions = "[2.0,)" // 4.x would also match but has completely different API + assertInverse = true + } +} + +// CORRECT — bounded range excludes 4.x +muzzle { + pass { + group = "commons-httpclient" + module = "commons-httpclient" + 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 (3.x → 4.x breaks, 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. + ### 3. Latest dependency test (mandatory) Use the `latestDepTestLibrary` helper in `build.gradle` to pin the latest available version. Run with: @@ -188,6 +712,18 @@ Use the `latestDepTestLibrary` helper in `build.gradle` to pin the latest availa ./gradlew :dd-java-agent:instrumentation:$framework-$version:latestDepTest ``` +**`latestDepTestImplementation` version range must match the instrumented range (R19).** 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. From 4fbb559705c48221bf37a3f6c2bb9118d4cc0c03 Mon Sep 17 00:00:00 2001 From: Jordan Wong Date: Mon, 29 Jun 2026 01:27:26 -0400 Subject: [PATCH 02/22] skill(add-apm-integrations): clean up R-numbering and sub-step naming MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two cosmetic passes on the rule encoding ported from the toolkit's synced derivative copy. Substantive content unchanged. Pass 1 — strip R-numbering. The R13-R33 numbers are toolkit-internal traceability tags that tie each rule to a specific reviewer comment on a specific generated-PR review. They are meaningful to the toolkit-side eval research workflow but meaningless to readers of the canonical skill, who have no R1-R12 context here. Removes: - '#### R — ' prefixes (14 sub-section headings) - Inline '(R<NN>)', '(R<NN>/R<NN>/R<NN>)' parentheticals on existing bullets that already convey the rule in their wording - A stale cross-reference 'see R29 below' rewritten to 'see "Register new integration names"' The toolkit-side copy keeps the R-numbering — it remains the eval-research home where the traceability matters. This is a one-way port convention. Pass 2 — renumber half-steps so the decimals make sense. Was: Step 4 → Step 4.4 → Step 4.5 → Step 5 implying missing 4.1, 4.2, 4.3. The original numbering was an artifact of an earlier toolkit-side draft that had a 4.1-4.4 enumeration which got collapsed. Renumbered to: Step 4 → Step 4.1 → Step 4.2 → Step 5 Main integer steps (1-12) unchanged. Step 7.1 (Multiple advice classes and @AppliesOn) left as-is since its decimal already makes sense relative to Step 7. Signed-off-by: Jordan Wong <jordan.wong@datadoghq.com> --- .claude/skills/add-apm-integrations/SKILL.md | 32 ++++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/.claude/skills/add-apm-integrations/SKILL.md b/.claude/skills/add-apm-integrations/SKILL.md index b0605a521ae..92a8e93d1cb 100644 --- a/.claude/skills/add-apm-integrations/SKILL.md +++ b/.claude/skills/add-apm-integrations/SKILL.md @@ -63,7 +63,7 @@ pattern before writing new code. Use it as a template. - `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` (see R29 below in Step 9), or +5. Register the integration name in `metadata/supported-configurations.json` (see "Register new integration names" in Step 9), or `checkInstrumenterModuleConfigurations` fails. The name in `super(...)` maps to env var `DD_TRACE_<NAME>_ENABLED` (`.` and `-` become `_`, uppercased — `couchbase-3` → `DD_TRACE_COUCHBASE_3_ENABLED`). Add a `"type": "boolean"` entry, in alphabetical order, with @@ -71,7 +71,7 @@ pattern before writing new code. Use it as a template. 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. -#### R32 — Module directory name must end with a version OR an allowed suffix +#### 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: @@ -82,7 +82,7 @@ Pick a directory name like `$framework-$minVersion` (e.g. `okhttp-3.0`, `jedis-3 helpers/stubs/iast support code factored out across version-specific modules, use the documented suffixes above. -## Step 4.4 – Library category: span-creating vs context-propagation (read BEFORE picking targets) +## Step 4.1 – Library category: span-creating vs context-propagation (read BEFORE picking targets) **Before picking instrumentation targets**, classify the library along the `target_kind` axis: @@ -144,7 +144,7 @@ If the library DOES perform I/O — sends HTTP requests, runs DB queries, makes 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-propagation instrumentation for the coordination path. `lettuce-5.0` is an example: there is a span-creating instrumentation for Redis commands and a separate context-propagation instrumentation for the async command queue. -## Step 4.5 – Java naming consistency (CRITICAL — non-negotiable) +## Step 4.2 – 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. @@ -229,7 +229,7 @@ public String[] instrumentationNames() { The version alias (e.g. `"jedis-3.0"`) lets users enable/disable this specific version independently. -#### R13 — Do not create a helper class just for CallDepthThreadLocalMap when only one type is instrumented +#### 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: @@ -252,7 +252,7 @@ CallDepthThreadLocalMap.reset(GsonInstrumentation.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. -#### R30 — When regenerating an existing module, preserve master's integration name convention +#### When regenerating an existing module, preserve master's integration name convention If you are modifying or regenerating instrumentation for a library that **already exists** in `dd-java-agent/instrumentation/` on master (e.g. `commons-httpclient-2.0/`), READ the existing module's `super(...)` and `instrumentationNames()` declarations and reuse them. @@ -304,7 +304,7 @@ See `docs/how_instrumentations_work.md` section "Grouping Instrumentations" for - `@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 (R15/R16/R17)**: 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. +- **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) @@ -353,7 +353,7 @@ import java.nio.charset.StandardCharsets; String cmd = new String(commandBytes, StandardCharsets.UTF_8); ``` -#### R33 — Do NOT catch `NullPointerException`; use null-check guards instead +#### Do NOT catch `NullPointerException`; use null-check guards instead dd-trace-java enforces SpotBugs rule `DCN_NULLPOINTER_EXCEPTION` (no NPE catch). Defensive `try { ... } catch (NullPointerException e) { ... }` patterns will fail `:spotbugsMain` and block the PR. @@ -449,7 +449,7 @@ Cover all mandatory test types: ### 1. Instrumentation test (mandatory) -**Write Java tests (JUnit 5), NOT Groovy/Spock (R20).** dd-trace-java policy +**Write Java tests (JUnit 5), NOT Groovy/Spock.** dd-trace-java policy forbids new `.groovy` files — the PR bot (`checkNewGroovyFiles`) rejects any PR containing them. All new tests must go in `src/test/java/`. @@ -458,7 +458,7 @@ All new tests must go in `src/test/java/`. - Use `TEST_WRITER.waitForTraces(N)` for assertions - Use `runUnderTrace("root", () -> { ... })` for synchronous code (Java lambda, not Groovy closure) -**Tests must cover error/exception scenarios, not just the happy path (R14).** 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: +**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: ```java // Example error test (Java) @@ -477,7 +477,7 @@ void testExceptionSetsErrorTags() throws Exception { For tests that need a separate JVM, suffix the test class with `ForkedTest` and run via the `forkedTest` task. -#### R20 — No new .groovy files (Java tests only) +#### No new .groovy files (Java tests only) dd-trace-java forbids new `.groovy` files (bot-enforced on every PR). Write Java: @@ -498,7 +498,7 @@ public class Jedis3ClientTest extends AgentInstrumentationTest { The PR bot check (`checkNewGroovyFiles`) will fail the PR if any `.groovy` file is added. -#### R29 — Register new integration names in `metadata/supported-configurations.json` +#### Register new integration names in `metadata/supported-configurations.json` Every new integration name in your module (whether from `super("foo-X.Y")` in `InstrumenterModule`, or from `instrumentationNames()` in the decorator) MUST have a corresponding entry in `metadata/supported-configurations.json` at the repo root. The `dd-gitlab/config-inversion-linter` CI job fails otherwise. @@ -542,7 +542,7 @@ If the decorator's `instrumentationNames()` returns a shared name (e.g. `"sparkj **How to discover whether entries are missing**: after writing the instrumentation, search `metadata/supported-configurations.json` for each name used in `super(...)` and `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. -#### R28 — compileOnly and testImplementation may use different versions — explain why +#### 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. @@ -624,7 +624,7 @@ muzzle { } ``` -#### R31 — Do NOT use `assertInverse = true` unless the declared min is the actual minimum compatible version +#### 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: @@ -651,7 +651,7 @@ Add `assertInverse = true` only when you've empirically verified the min via loc **Background**: a typical greenfield generation produces a sync instrumentation class (works on older versions) and an async instrumentation class (requires a newer API). The agent picks the higher version as the declared min for muzzle, which is conservative for compileOnly. But `assertInverse = true` then auto-tests a lower version with ONLY the sync class hooks active, and that passes — creating the failure. -#### R18 — Muzzle range must exclude incompatible major versions +#### 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 @@ -712,7 +712,7 @@ Use the `latestDepTestLibrary` helper in `build.gradle` to pin the latest availa ./gradlew :dd-java-agent:instrumentation:$framework-$version:latestDepTest ``` -**`latestDepTestImplementation` version range must match the instrumented range (R19).** If your module instruments version `2.x`, use `2+` as the version constraint, not `3+`: +**`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 From f56d160d2e500107145694b55d2d4759b1aceb2a Mon Sep 17 00:00:00 2001 From: Jordan Wong <jordan.wong@datadoghq.com> Date: Tue, 7 Jul 2026 20:26:48 -0400 Subject: [PATCH 03/22] refactor(skill): extract Category B routing to references/ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move the 62-line "Step 4.1 – Library category" section from SKILL.md into references/category-b-context-propagation.md. SKILL.md keeps a 5-line stub linking to the reference file — enough context to know when to read it, not enough to bury the rest of Step 4. Preserves all content verbatim; no wording changes. Follows the existing dd-trace-java skill convention of tracking specific per-skill files (git add -f, matching the precedent set by .claude/skills/migrate-groovy-to-java/QUALITY_RULES.md). Part 1 of a refactor to slim the 794-line SKILL.md into a routing overview (~250 lines target) with topic-oriented reference files. Signed-off-by: Jordan Wong <jordan.wong@datadoghq.com> --- .claude/skills/add-apm-integrations/SKILL.md | 62 +----------------- .../category-b-context-propagation.md | 63 +++++++++++++++++++ 2 files changed, 66 insertions(+), 59 deletions(-) create mode 100644 .claude/skills/add-apm-integrations/references/category-b-context-propagation.md diff --git a/.claude/skills/add-apm-integrations/SKILL.md b/.claude/skills/add-apm-integrations/SKILL.md index 92a8e93d1cb..b9052def165 100644 --- a/.claude/skills/add-apm-integrations/SKILL.md +++ b/.claude/skills/add-apm-integrations/SKILL.md @@ -82,67 +82,11 @@ Pick a directory name like `$framework-$minVersion` (e.g. `okhttp-3.0`, `jedis-3 helpers/stubs/iast support code factored out across version-specific modules, use the documented suffixes above. -## Step 4.1 – Library category: span-creating vs context-propagation (read BEFORE picking targets) +## Step 4.1 – Library category: span-creating vs context-propagation -**Before picking instrumentation targets**, classify the library along the `target_kind` axis: +**Read [Category B — Context Propagation](references/category-b-context-propagation.md) before picking instrumentation targets.** -**Category A — span-creating** (most libraries): performs I/O, makes calls, runs queries. The instrumentation creates spans around those operations. - -- HTTP clients/servers, DB clients, messaging clients, RPC frameworks -- Targets: methods that perform the I/O (e.g., `Connection.sendCommand`, `Client.execute`) -- Tests: assert spans exist with correct tags - -**Category B — context-propagation** (reactive, async, threading, executor, fiber libraries): does NOT perform I/O directly. It coordinates work that other code performs. Instrumentation captures the active trace context at boundary creation and restores it at boundary crossing — **no spans are created by the module**, it only bridges trace context for spans created by other instrumentations or by user code. - -- RxJava, Reactor, CompletableFuture, ListenableFuture, executors, pekko/akka, lettuce async-command queue, ZIO, virtual threads -- Targets: one boundary-crossing type (e.g. `Observable`, `Flowable`, `Single`, `Maybe`, `Completable` for RxJava-shaped libs; `Runnable`/`Callable` for executor-shaped libs) -- Tests: assert that a span created in operation X is still ACTIVE when a callback scheduled by Y runs (parent-child bridging of *user-created* spans, NOT span tags on a target span) - -**Reference implementation for Category B:** - -`dd-java-agent/instrumentation/rxjava/rxjava-2.0/` — uses `InstrumenterModule.ContextTracking`. 5 type-instrumenters (Observable, Flowable, Single, Maybe, Completable), 5 wrappers (Tracing{Observer,Subscriber,SingleObserver,MaybeObserver,CompletableObserver}), 1 `RxJavaModule.java`, 1 `RxJavaAsyncResultExtension.java`. ~600 LOC total. - -### Category B target shape - -For each boundary-crossing type, capture: - -- `library_class` — FQN of the boundary-crossing type (e.g. `io.reactivex.rxjava3.core.Observable`). -- `capture_method` — capture point (usually the constructor — `<init>` or `isConstructor()`). -- `restore_method` — restore point (usually `subscribe(Observer)` / `subscribe(Subscriber)`). -- `wrapped_argument_type` — FQN of the user callback argument that must be wrapped (e.g. `io.reactivex.rxjava3.core.Observer`). -- `context_key_class` — FQN to key the `contextStore` on (almost always equals `library_class`). -- `wrapper_class_name` — class name of the wrapper to generate (e.g. `TracingObserver`). -- `wrapper_methods` — methods on the wrapped type that must reattach context before delegating (e.g. `["onNext", "onError", "onComplete"]`). - -### Tests for Category B - -Context-propagation 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**: - -```java -runUnderTrace("parent", () -> { - constructBoundary() // capture happens here - .subscribe(item -> userCodeStartsAChildSpan()); // restore happens around the lambda -}); -// 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 (Category B) - -When a reactive 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` hooks `subscribe(Observer)` (the implementation). The RxJava 3 reference instrumentation hooks `subscribe(FlowableSubscriber)`, not `subscribe(Subscriber)`. - -### When NOT to use Category B - -If the library DOES perform I/O — sends HTTP requests, runs DB queries, makes RPC calls, talks to a broker, reads/writes a cache — it is **span-creating**, not context-propagation. - -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-propagation instrumentation for the coordination path. `lettuce-5.0` is an example: there is a span-creating instrumentation for Redis commands and a separate context-propagation instrumentation for the async command queue. +Classify the library along the `target_kind` axis: Category A (span-creating — HTTP/DB/messaging/RPC) or Category B (context-propagation — reactive/async/executor/fiber). Category B does not create spans; it bridges trace context across async boundaries. Get this wrong and you generate the wrong shape of instrumentation. ## Step 4.2 – Java naming consistency (CRITICAL — non-negotiable) diff --git a/.claude/skills/add-apm-integrations/references/category-b-context-propagation.md b/.claude/skills/add-apm-integrations/references/category-b-context-propagation.md new file mode 100644 index 00000000000..329f9003c41 --- /dev/null +++ b/.claude/skills/add-apm-integrations/references/category-b-context-propagation.md @@ -0,0 +1,63 @@ +# Category B — Context Propagation + +> Referenced from `SKILL.md` Step 4.1. Read this BEFORE picking instrumentation targets. + +**Before picking instrumentation targets**, classify the library along the `target_kind` axis: + +**Category A — span-creating** (most libraries): performs I/O, makes calls, runs queries. The instrumentation creates spans around those operations. + +- HTTP clients/servers, DB clients, messaging clients, RPC frameworks +- Targets: methods that perform the I/O (e.g., `Connection.sendCommand`, `Client.execute`) +- Tests: assert spans exist with correct tags + +**Category B — context-propagation** (reactive, async, threading, executor, fiber libraries): does NOT perform I/O directly. It coordinates work that other code performs. Instrumentation captures the active trace context at boundary creation and restores it at boundary crossing — **no spans are created by the module**, it only bridges trace context for spans created by other instrumentations or by user code. + +- RxJava, Reactor, CompletableFuture, ListenableFuture, executors, pekko/akka, lettuce async-command queue, ZIO, virtual threads +- Targets: one boundary-crossing type (e.g. `Observable`, `Flowable`, `Single`, `Maybe`, `Completable` for RxJava-shaped libs; `Runnable`/`Callable` for executor-shaped libs) +- Tests: assert that a span created in operation X is still ACTIVE when a callback scheduled by Y runs (parent-child bridging of *user-created* spans, NOT span tags on a target span) + +**Reference implementation for Category B:** + +`dd-java-agent/instrumentation/rxjava/rxjava-2.0/` — uses `InstrumenterModule.ContextTracking`. 5 type-instrumenters (Observable, Flowable, Single, Maybe, Completable), 5 wrappers (Tracing{Observer,Subscriber,SingleObserver,MaybeObserver,CompletableObserver}), 1 `RxJavaModule.java`, 1 `RxJavaAsyncResultExtension.java`. ~600 LOC total. + +## Category B target shape + +For each boundary-crossing type, capture: + +- `library_class` — FQN of the boundary-crossing type (e.g. `io.reactivex.rxjava3.core.Observable`). +- `capture_method` — capture point (usually the constructor — `<init>` or `isConstructor()`). +- `restore_method` — restore point (usually `subscribe(Observer)` / `subscribe(Subscriber)`). +- `wrapped_argument_type` — FQN of the user callback argument that must be wrapped (e.g. `io.reactivex.rxjava3.core.Observer`). +- `context_key_class` — FQN to key the `contextStore` on (almost always equals `library_class`). +- `wrapper_class_name` — class name of the wrapper to generate (e.g. `TracingObserver`). +- `wrapper_methods` — methods on the wrapped type that must reattach context before delegating (e.g. `["onNext", "onError", "onComplete"]`). + +## Tests for Category B + +Context-propagation 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**: + +```java +runUnderTrace("parent", () -> { + constructBoundary() // capture happens here + .subscribe(item -> userCodeStartsAChildSpan()); // restore happens around the lambda +}); +// 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 (Category B) + +When a reactive 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` hooks `subscribe(Observer)` (the implementation). The RxJava 3 reference instrumentation hooks `subscribe(FlowableSubscriber)`, not `subscribe(Subscriber)`. + +## When NOT to use Category B + +If the library DOES perform I/O — sends HTTP requests, runs DB queries, makes RPC calls, talks to a broker, reads/writes a cache — it is **span-creating**, not context-propagation. + +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-propagation instrumentation for the coordination path. `lettuce-5.0` is an example: there is a span-creating instrumentation for Redis commands and a separate context-propagation instrumentation for the async command queue. From 346f35f786b36acae855cd1dbe4d8bf8ac5a498e Mon Sep 17 00:00:00 2001 From: Jordan Wong <jordan.wong@datadoghq.com> Date: Tue, 7 Jul 2026 20:28:07 -0400 Subject: [PATCH 04/22] refactor(skill): extract naming conventions to references/ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move two related naming rules from SKILL.md into references/naming-conventions.md: - Step 4's module-directory-name rule (must end with version or "-common"/"-stubs"/"-iast" suffix) - Step 4.2's Java filename ↔ class-name matching rule (with the sanity-check script) They belong together because both are enforcement rules for names. SKILL.md keeps short stubs linking to the reference file. Preserves all content verbatim. Part 2 of the SKILL.md slim. Signed-off-by: Jordan Wong <jordan.wong@datadoghq.com> --- .claude/skills/add-apm-integrations/SKILL.md | 41 ++--------------- .../references/naming-conventions.md | 46 +++++++++++++++++++ 2 files changed, 49 insertions(+), 38 deletions(-) create mode 100644 .claude/skills/add-apm-integrations/references/naming-conventions.md diff --git a/.claude/skills/add-apm-integrations/SKILL.md b/.claude/skills/add-apm-integrations/SKILL.md index b9052def165..3ab0c533094 100644 --- a/.claude/skills/add-apm-integrations/SKILL.md +++ b/.claude/skills/add-apm-integrations/SKILL.md @@ -71,16 +71,7 @@ pattern before writing new code. Use it as a template. 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. -#### 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. +**See [Naming Conventions](references/naming-conventions.md) — module directory name must end with a version or an allowed suffix (`-common`, `-stubs`, `-iast`).** ## Step 4.1 – Library category: span-creating vs context-propagation @@ -90,35 +81,9 @@ Classify the library along the `target_kind` axis: Category A (span-creating — ## Step 4.2 – 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 <pkg>.<ClassName>.<member>` across all other files in the module -- Every reference in the form `<ClassName>.<member>` or `<ClassName>.class` - -**Convention in dd-trace-java**: acronym classes use **uppercase**: - -- CORRECT: `JMSDecorator`, `gRPCInstrumentation`, `JDBCConnection` -- WRONG: `JmsDecorator`, `GrpcInstrumentation`, `JdbcConnection` - -This matches the existing module conventions. When in doubt, match a reference instrumentation's casing exactly (see Step 3). - -**After generating each module, sanity-check before declaring done:** - -```bash -# Every public class declaration must match its filename -cd dd-java-agent/instrumentation/$framework/$framework-$version/src/main/java -for f in $(find . -name '*.java'); do - CLS=$(grep -oE 'public (final )?(abstract )?class [A-Za-z0-9_]+' "$f" | awk '{print $NF}') - EXPECTED=$(basename "$f" .java) - [ "$CLS" = "$EXPECTED" ] || echo "MISMATCH: $f declares '$CLS'" -done -``` +**Read [Naming Conventions](references/naming-conventions.md) § "Java naming consistency".** -If any MISMATCH lines print, fix them before moving on. +Filename and `public class` name MUST match character-for-character (including acronym casing). Use one canonical string everywhere: filename, class decl, `import static`, `ClassName.member` references. The reference file has a sanity-check script — run it before declaring done. ## Step 5 – Write the InstrumenterModule diff --git a/.claude/skills/add-apm-integrations/references/naming-conventions.md b/.claude/skills/add-apm-integrations/references/naming-conventions.md new file mode 100644 index 00000000000..a88272d3895 --- /dev/null +++ b/.claude/skills/add-apm-integrations/references/naming-conventions.md @@ -0,0 +1,46 @@ +# Naming Conventions + +> Referenced from `SKILL.md` Step 4 (module directory name) and Step 4.2 (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 <pkg>.<ClassName>.<member>` across all other files in the module +- Every reference in the form `<ClassName>.<member>` or `<ClassName>.class` + +**Convention in dd-trace-java**: acronym classes use **uppercase**: + +- CORRECT: `JMSDecorator`, `gRPCInstrumentation`, `JDBCConnection` +- WRONG: `JmsDecorator`, `GrpcInstrumentation`, `JdbcConnection` + +This matches the existing module conventions. When in doubt, match a reference instrumentation's casing exactly (see Step 3). + +**After generating each module, sanity-check before declaring done:** + +```bash +# Every public class declaration must match its filename +cd dd-java-agent/instrumentation/$framework/$framework-$version/src/main/java +for f in $(find . -name '*.java'); do + CLS=$(grep -oE 'public (final )?(abstract )?class [A-Za-z0-9_]+' "$f" | awk '{print $NF}') + EXPECTED=$(basename "$f" .java) + [ "$CLS" = "$EXPECTED" ] || echo "MISMATCH: $f declares '$CLS'" +done +``` + +If any MISMATCH lines print, fix them before moving on. From 0d02c6f01f9ce7ed08aa2d48c983c172525c00d4 Mon Sep 17 00:00:00 2001 From: Jordan Wong <jordan.wong@datadoghq.com> Date: Tue, 7 Jul 2026 20:29:09 -0400 Subject: [PATCH 05/22] refactor(skill): extract InstrumenterModule guidance to references/ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move Step 5's 103-line body from SKILL.md into references/instrumenter-module.md, covering: - @AutoService + narrow-interface preferences (ForSingleType > ForKnownTypes > ForTypeHierarchy) with the interface-only API JAR exception (JMS, JPA, JDBC, etc.) - 'Must NOT do' — no static constants for one-shot methods - instrumentationNames() version-qualified alias rule - No helper class for single-target CallDepthThreadLocalMap - Preserve master's integration name on regeneration - Advanced: grouping multiple instrumentations under one module SKILL.md keeps Step 5 as a 6-line summary + link. Preserves all content verbatim; no wording changes. Part 3 of the SKILL.md slim. Signed-off-by: Jordan Wong <jordan.wong@datadoghq.com> --- .claude/skills/add-apm-integrations/SKILL.md | 101 +---------------- .../references/instrumenter-module.md | 104 ++++++++++++++++++ 2 files changed, 106 insertions(+), 99 deletions(-) create mode 100644 .claude/skills/add-apm-integrations/references/instrumenter-module.md diff --git a/.claude/skills/add-apm-integrations/SKILL.md b/.claude/skills/add-apm-integrations/SKILL.md index 3ab0c533094..925167eb333 100644 --- a/.claude/skills/add-apm-integrations/SKILL.md +++ b/.claude/skills/add-apm-integrations/SKILL.md @@ -87,106 +87,9 @@ Filename and `public class` name MUST match character-for-character (including a ## 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` - - **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 -- 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"}; }` - -#### instrumentationNames() must include a version-qualified alias +**Read [InstrumenterModule Guidance](references/instrumenter-module.md).** -```java -@Override -public String[] instrumentationNames() { - // WRONG — only generic name - return new String[]{"jedis"}; - - // CORRECT — generic + version alias - return new String[]{"jedis", "jedis-3.0"}; -} -``` - -The version alias (e.g. `"jedis-3.0"`) lets users enable/disable this specific version independently. - -#### 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 CallDepthThreadLocalMap directly with the Advice or Decorator class as key -if (CallDepthThreadLocalMap.incrementCallDepth(GsonInstrumentation.class) > 0) return; -// ... in exit: -CallDepthThreadLocalMap.reset(GsonInstrumentation.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. - -#### When regenerating an existing module, preserve master's integration name convention - -If you are modifying or regenerating instrumentation for a library that **already exists** in `dd-java-agent/instrumentation/` on master (e.g. `commons-httpclient-2.0/`), READ the existing module's `super(...)` and `instrumentationNames()` declarations and reuse them. - -```java -// Master's existing source uses dashed name: -// super("commons-http-client"); -// CORRECT (matches master, master's DD_TRACE_COMMONS_HTTP_CLIENT_* entries continue to work) -super("commons-http-client"); - -// WRONG (new name, requires adding 6 new supported-configurations.json entries) -super("commons-httpclient", "commons-httpclient-2.0"); -``` - -**Why**: dd-trace-java's `dd-gitlab/config-inversion-linter` requires registered names. Master already has registered entries for its existing convention; inventing a new convention forces a metadata change. Preserving the convention keeps the diff minimal and reviewer attention focused on the code, not boilerplate. - -### 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 an array of instrumentations -- 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. +In short: annotate with `@AutoService`, extend the right `InstrumenterModule.*` subclass, implement the narrowest `Instrumenter` interface possible (`ForSingleType` > `ForKnownTypes` > `ForTypeHierarchy` — with a critical exception for interface-only API JARs like JMS/JPA/JDBC). Include a version-qualified alias in `instrumentationNames()` (`{"jedis", "jedis-3.0"}`). Declare all helper classes. When regenerating an existing module, preserve master's integration name to avoid churn in `supported-configurations.json`. Do NOT extract one-shot method return values into static constants. ## Step 6 – Write the Decorator diff --git a/.claude/skills/add-apm-integrations/references/instrumenter-module.md b/.claude/skills/add-apm-integrations/references/instrumenter-module.md new file mode 100644 index 00000000000..3537572b6ae --- /dev/null +++ b/.claude/skills/add-apm-integrations/references/instrumenter-module.md @@ -0,0 +1,104 @@ +# 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 +- 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"}; }` + +### instrumentationNames() must include a version-qualified alias + +```java +@Override +public String[] instrumentationNames() { + // WRONG — only generic name + return new String[]{"jedis"}; + + // CORRECT — generic + version alias + return new String[]{"jedis", "jedis-3.0"}; +} +``` + +The version alias (e.g. `"jedis-3.0"`) lets users enable/disable this specific version independently. + +### 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 CallDepthThreadLocalMap directly with the Advice or Decorator class as key +if (CallDepthThreadLocalMap.incrementCallDepth(GsonInstrumentation.class) > 0) return; +// ... in exit: +CallDepthThreadLocalMap.reset(GsonInstrumentation.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. + +### When regenerating an existing module, preserve master's integration name convention + +If you are modifying or regenerating instrumentation for a library that **already exists** in `dd-java-agent/instrumentation/` on master (e.g. `commons-httpclient-2.0/`), READ the existing module's `super(...)` and `instrumentationNames()` declarations and reuse them. + +```java +// Master's existing source uses dashed name: +// super("commons-http-client"); +// CORRECT (matches master, master's DD_TRACE_COMMONS_HTTP_CLIENT_* entries continue to work) +super("commons-http-client"); + +// WRONG (new name, requires adding 6 new supported-configurations.json entries) +super("commons-httpclient", "commons-httpclient-2.0"); +``` + +**Why**: dd-trace-java's `dd-gitlab/config-inversion-linter` requires registered names. Master already has registered entries for its existing convention; inventing a new convention forces a metadata change. Preserving the convention keeps the diff minimal and reviewer attention focused on the code, not boilerplate. + +## 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 an array of instrumentations +- 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. From 04cc4c87a0680a3dd8b751b28fcc53a28ce30d0b Mon Sep 17 00:00:00 2001 From: Jordan Wong <jordan.wong@datadoghq.com> Date: Tue, 7 Jul 2026 20:30:10 -0400 Subject: [PATCH 06/22] refactor(skill): extract Advice class guidance to references/ Move Step 7's 149-line body from SKILL.md into references/advice-class.md. This was the largest single-step body in the skill and the highest-risk area to get wrong. Reference file covers: - Advice method annotations + parameter kinds - Span lifecycle (enter/exit order) - onExit resilience to onEnter throwing - Explicit charset for byte[] to String - No NullPointerException catches (SpotBugs enforces) - Single-delegate-method instrumentation (not all overloads) - @AppliesOn + multiple advice classes - 'Must NOT do' list (no loggers, no lambdas, no inline=false, etc.) SKILL.md keeps Step 7 as a summary + link. Preserves all content verbatim. Part 4 of the SKILL.md slim. Signed-off-by: Jordan Wong <jordan.wong@datadoghq.com> --- .claude/skills/add-apm-integrations/SKILL.md | 148 +---------------- .../references/advice-class.md | 150 ++++++++++++++++++ 2 files changed, 152 insertions(+), 146 deletions(-) create mode 100644 .claude/skills/add-apm-integrations/references/advice-class.md diff --git a/.claude/skills/add-apm-integrations/SKILL.md b/.claude/skills/add-apm-integrations/SKILL.md index 925167eb333..5d7081ca319 100644 --- a/.claude/skills/add-apm-integrations/SKILL.md +++ b/.claude/skills/add-apm-integrations/SKILL.md @@ -102,153 +102,9 @@ In short: annotate with `@AutoService`, extend the right `InstrumenterModule.*` ## 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 -- **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. `span.finish()` -7. `scope.close()` - -#### onExit must be resilient to onEnter throwing - -If `onEnter` throws before the scope is set, `onExit` must still decrement the call depth. -A null-check that skips the reset leaks the ThreadLocal: - -```java -// RISKY — if onEnter threw, scope is null and reset is skipped -@Advice.OnMethodExit(suppress = Throwable.class) -public static void exit(@Advice.Enter final AgentScope scope) { - if (scope != null) scope.close(); -} - -// SAFER — onThrowable = Throwable.class ensures exit fires even on onEnter exception -@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) -public static void exit(@Advice.Enter final AgentScope scope) { - if (scope != null) { - scope.close(); - } -} -``` - -When using `CallDepthThreadLocalMap`, always decrement unconditionally in exit. - -#### 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 - -dd-trace-java enforces SpotBugs rule `DCN_NULLPOINTER_EXCEPTION` (no NPE catch). Defensive `try { ... } catch (NullPointerException e) { ... }` patterns will 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. - -### Step 7.1 — 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()` instead of `applyAdvice()`: - -```java -@Override -public void adviceTransformations(AdviceTransformation transformation) { - transformation.applyAdvices( - named("someMethod") - .and(takesArgument(0, named("com.example.Request"))) - .and(takesArgument(1, named("com.example.Response"))), - getClass().getName() + "$ContextTrackingAdvice", // Applied first - getClass().getName() + "$ServiceAdvice" // Applied second - ); -} -``` - -Use the `@AppliesOn` annotation to control which target systems each advice applies to: - -```java -import datadog.trace.agent.tooling.InstrumenterModule.TargetSystem; -import datadog.trace.agent.tooling.annotation.AppliesOn; - -@AppliesOn(TargetSystem.CONTEXT_TRACKING) -public static class ContextTrackingAdvice { - @Advice.OnMethodEnter(suppress = Throwable.class) - public static void enter(@Advice.Argument(0) Request request) { - // This advice only runs when CONTEXT_TRACKING is enabled - } -} - -public static class TracingAdvice { - // Without @AppliesOn, this advice runs for the module's target system - @Advice.OnMethodEnter(suppress = Throwable.class) - public static void enter(@Advice.Argument(0) Request request) { - // Tracing-specific logic - } -} -``` - -**When to use `@AppliesOn`:** - -- Separate context-propagation logic from tracing logic -- Different target systems need different instrumentation behaviours -- Multiple advices apply to the same method with different system requirements - -See `docs/how_instrumentations_work.md` section "@AppliesOn Annotation" for complete details. - -### 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 +**Read [Writing the Advice Class](references/advice-class.md).** +The reference file covers: `static` advice methods; enter/exit annotations (with the constructor exception); parameter annotations (`@Advice.This`, `@Advice.Argument`, `@Advice.Return`, `@Advice.Thrown`, `@Advice.Enter`, `@Advice.Local`); `CallDepthThreadLocalMap` reentrancy guarding; single-delegate-method instrumentation (not all overloads); span lifecycle order; `onExit` resilience to `onEnter` throwing; explicit charset for `byte[]` → `String`; no `NullPointerException` catches (SpotBugs blocks); `@AppliesOn` for multiple advice classes; and the "Must NOT do" list (no logger fields, no lambdas in advice, no `inline=false` in production, no `java.util.logging.*` / `java.nio.*` / `javax.management.*` in bootstrap). ## Step 8 – Add SETTER/GETTER adapters (if applicable) For context propagation to and from upstream services, like HTTP headers, diff --git a/.claude/skills/add-apm-integrations/references/advice-class.md b/.claude/skills/add-apm-integrations/references/advice-class.md new file mode 100644 index 00000000000..d449bd57ef4 --- /dev/null +++ b/.claude/skills/add-apm-integrations/references/advice-class.md @@ -0,0 +1,150 @@ +# 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 `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 +- **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. `span.finish()` +7. `scope.close()` + +### onExit must be resilient to onEnter throwing + +If `onEnter` throws before the scope is set, `onExit` must still decrement the call depth. +A null-check that skips the reset leaks the ThreadLocal: + +```java +// RISKY — if onEnter threw, scope is null and reset is skipped +@Advice.OnMethodExit(suppress = Throwable.class) +public static void exit(@Advice.Enter final AgentScope scope) { + if (scope != null) scope.close(); +} + +// SAFER — onThrowable = Throwable.class ensures exit fires even on onEnter exception +@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) +public static void exit(@Advice.Enter final AgentScope scope) { + if (scope != null) { + scope.close(); + } +} +``` + +When using `CallDepthThreadLocalMap`, always decrement unconditionally in exit. + +### 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 + +dd-trace-java enforces SpotBugs rule `DCN_NULLPOINTER_EXCEPTION` (no NPE catch). Defensive `try { ... } catch (NullPointerException e) { ... }` patterns will 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()` instead of `applyAdvice()`: + +```java +@Override +public void adviceTransformations(AdviceTransformation transformation) { + transformation.applyAdvices( + named("someMethod") + .and(takesArgument(0, named("com.example.Request"))) + .and(takesArgument(1, named("com.example.Response"))), + getClass().getName() + "$ContextTrackingAdvice", // Applied first + getClass().getName() + "$ServiceAdvice" // Applied second + ); +} +``` + +Use the `@AppliesOn` annotation to control which target systems each advice applies to: + +```java +import datadog.trace.agent.tooling.InstrumenterModule.TargetSystem; +import datadog.trace.agent.tooling.annotation.AppliesOn; + +@AppliesOn(TargetSystem.CONTEXT_TRACKING) +public static class ContextTrackingAdvice { + @Advice.OnMethodEnter(suppress = Throwable.class) + public static void enter(@Advice.Argument(0) Request request) { + // This advice only runs when CONTEXT_TRACKING is enabled + } +} + +public static class TracingAdvice { + // Without @AppliesOn, this advice runs for the module's target system + @Advice.OnMethodEnter(suppress = Throwable.class) + public static void enter(@Advice.Argument(0) Request request) { + // Tracing-specific logic + } +} +``` + +**When to use `@AppliesOn`:** + +- Separate context-propagation logic from tracing logic +- Different target systems need different instrumentation behaviours +- Multiple advices apply to the same method with different system requirements + +See `docs/how_instrumentations_work.md` section "@AppliesOn Annotation" for complete details. + +## 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 From 9cecf05ed2dad331e3afe5c4c9c95a143e831732 Mon Sep 17 00:00:00 2001 From: Jordan Wong <jordan.wong@datadoghq.com> Date: Tue, 7 Jul 2026 20:31:13 -0400 Subject: [PATCH 07/22] refactor(skill): extract tests guidance to references/ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move Step 9's 'Instrumentation test' section + all its sub-rules (no .groovy files, supported-configurations.json registration, compileOnly/testImplementation version-split rationale, prior-version-module inclusion) from SKILL.md into references/tests.md. Muzzle content stays in place for now — it's a separate concern and gets its own reference file next. SKILL.md keeps Step 9.1 as a summary + link. Preserves all content verbatim. Part 5 of the SKILL.md slim. Signed-off-by: Jordan Wong <jordan.wong@datadoghq.com> --- .claude/skills/add-apm-integrations/SKILL.md | 135 +---------------- .../add-apm-integrations/references/tests.md | 139 ++++++++++++++++++ 2 files changed, 140 insertions(+), 134 deletions(-) create mode 100644 .claude/skills/add-apm-integrations/references/tests.md diff --git a/.claude/skills/add-apm-integrations/SKILL.md b/.claude/skills/add-apm-integrations/SKILL.md index 5d7081ca319..5ea232539fe 100644 --- a/.claude/skills/add-apm-integrations/SKILL.md +++ b/.claude/skills/add-apm-integrations/SKILL.md @@ -117,140 +117,7 @@ Cover all mandatory test types: ### 1. Instrumentation test (mandatory) -**Write Java tests (JUnit 5), NOT Groovy/Spock.** dd-trace-java policy -forbids new `.groovy` files — the PR bot (`checkNewGroovyFiles`) rejects any PR containing them. -All new tests must go in `src/test/java/`. - -- JUnit 5 test class in `src/test/java/datadog/trace/instrumentation/<framework>/` -- Verify: spans created, tags set, errors propagated, resource names correct -- Use `TEST_WRITER.waitForTraces(N)` for assertions -- Use `runUnderTrace("root", () -> { ... })` for synchronous code (Java lambda, not 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: - -```java -// Example error test (Java) -@Test -void testExceptionSetsErrorTags() throws Exception { - assertThrows(SomeException.class, () -> { - // trigger operation that throws - client.execute(badRequest); - }); - List<List<SpanData>> traces = TEST_WRITER.waitForTraces(1); - SpanData span = traces.get(0).get(0); - assertThat(span.getAttributes().get(ERROR_TYPE)).isNotNull(); - assertThat(span.getAttributes().get(ERROR_MESSAGE)).isNotNull(); -} -``` - -For tests that need a separate JVM, suffix the test class with `ForkedTest` and run via the `forkedTest` task. - -#### No new .groovy files (Java tests only) - -dd-trace-java forbids new `.groovy` files (bot-enforced on every PR). Write Java: - -```java -// WRONG — generates a .groovy file which the bot rejects automatically -// src/test/groovy/JedisClientTest.groovy - -// CORRECT — Java in src/test/java/ -// src/test/java/datadog/trace/instrumentation/jedis3/Jedis3ClientTest.java -@ExtendWith(AgentJUnit5Extension.class) -public class Jedis3ClientTest extends AgentInstrumentationTest { - @Test - void testCommandCreatesSpan() { - // test body - } -} -``` - -The PR bot check (`checkNewGroovyFiles`) will fail the PR if any `.groovy` file is added. - -#### Register new integration names in `metadata/supported-configurations.json` - -Every new integration name in your module (whether from `super("foo-X.Y")` in `InstrumenterModule`, or from `instrumentationNames()` in the decorator) MUST have a corresponding entry in `metadata/supported-configurations.json` at the repo root. The `dd-gitlab/config-inversion-linter` CI job fails otherwise. - -For each new integration name `<NAME>` (uppercase, dashes/dots replaced with underscores), add: - -```json -"DD_TRACE_<NAME>_ENABLED": [ - { - "version": "A", - "type": "boolean", - "default": "false", - "aliases": ["DD_TRACE_INTEGRATION_<NAME>_ENABLED", "DD_INTEGRATION_<NAME>_ENABLED"] - } -], -``` - -If the decorator's `instrumentationNames()` returns a shared name (e.g. `"sparkjava"` covering all sparkjava versions), also add the analytics keys for the shared name: - -```json -"DD_TRACE_<SHARED>_ANALYTICS_ENABLED": [ - { - "version": "A", - "type": "boolean", - "default": "false", - "aliases": ["DD_<SHARED>_ANALYTICS_ENABLED"] - } -], -"DD_TRACE_<SHARED>_ANALYTICS_SAMPLE_RATE": [ - { - "version": "A", - "type": "decimal", - "default": "1.0", - "aliases": ["DD_<SHARED>_ANALYTICS_SAMPLE_RATE"] - } -], -``` - -**Place entries alphabetically** in the JSON file. **Verify the JSON parses** before committing (`python3 -c "import json; json.load(open('metadata/supported-configurations.json'))"`). - -**Type names — match existing conventions**: use `"boolean"`, `"string"`, `"integer"`, `"decimal"` (for floating-point — NOT `"double"`). The `dd-gitlab/validate_supported_configurations_v2_local_file` CI job will fail with non-canonical type names like `"double"`. Cross-check by grepping existing entries for similar fields. - -**How to discover whether entries are missing**: after writing the instrumentation, search `metadata/supported-configurations.json` for each name used in `super(...)` and `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. - -#### 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 `compileOnly` version and run -the sample app. 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 prior version module in testImplementation for mutual exclusion - -When two modules instrument the same library at different versions, add the prior -version's module as a `testImplementation` dependency to confirm they don't double-instrument: - -```groovy -// jedis-3.0/build.gradle -dependencies { - testImplementation project(':dd-java-agent:instrumentation:jedis:jedis-1.4') -} -``` - -This ensures `:test` validates that only the correct module fires for jedis-3.x requests. - +**Read [Writing Tests](references/tests.md).** Java tests only (JUnit 5) in `src/test/java/` — no new `.groovy` files (bot-enforced). Must cover error/exception scenarios. When adding new integration names, register them in `metadata/supported-configurations.json` (config-inversion-linter enforces). When `compileOnly` and `testImplementation` use different versions, comment the specific class that requires the higher version. Include the prior-version module as a `testImplementation` dependency for mutual-exclusion tests. ### 2. Muzzle directives (mandatory) In `build.gradle`, add `muzzle` blocks. **There are two valid patterns** — choose based on whether your version range is open-ended or bounded. diff --git a/.claude/skills/add-apm-integrations/references/tests.md b/.claude/skills/add-apm-integrations/references/tests.md new file mode 100644 index 00000000000..aa38f9c5c6b --- /dev/null +++ b/.claude/skills/add-apm-integrations/references/tests.md @@ -0,0 +1,139 @@ +# 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 Java tests (JUnit 5), NOT Groovy/Spock.** dd-trace-java policy +forbids new `.groovy` files — the PR bot (`checkNewGroovyFiles`) rejects any PR containing them. +All new tests must go in `src/test/java/`. + +- JUnit 5 test class in `src/test/java/datadog/trace/instrumentation/<framework>/` +- Verify: spans created, tags set, errors propagated, resource names correct +- Use `TEST_WRITER.waitForTraces(N)` for assertions +- Use `runUnderTrace("root", () -> { ... })` for synchronous code (Java lambda, not 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: + +```java +// Example error test (Java) +@Test +void testExceptionSetsErrorTags() throws Exception { + assertThrows(SomeException.class, () -> { + // trigger operation that throws + client.execute(badRequest); + }); + List<List<SpanData>> traces = TEST_WRITER.waitForTraces(1); + SpanData span = traces.get(0).get(0); + assertThat(span.getAttributes().get(ERROR_TYPE)).isNotNull(); + assertThat(span.getAttributes().get(ERROR_MESSAGE)).isNotNull(); +} +``` + +For tests that need a separate JVM, suffix the test class with `ForkedTest` and run via the `forkedTest` task. + +### No new .groovy files (Java tests only) + +dd-trace-java forbids new `.groovy` files (bot-enforced on every PR). Write Java: + +```java +// WRONG — generates a .groovy file which the bot rejects automatically +// src/test/groovy/JedisClientTest.groovy + +// CORRECT — Java in src/test/java/ +// src/test/java/datadog/trace/instrumentation/jedis3/Jedis3ClientTest.java +@ExtendWith(AgentJUnit5Extension.class) +public class Jedis3ClientTest extends AgentInstrumentationTest { + @Test + void testCommandCreatesSpan() { + // test body + } +} +``` + +The PR bot check (`checkNewGroovyFiles`) will fail the PR if any `.groovy` file is added. + +### Register new integration names in `metadata/supported-configurations.json` + +Every new integration name in your module (whether from `super("foo-X.Y")` in `InstrumenterModule`, or from `instrumentationNames()` in the decorator) MUST have a corresponding entry in `metadata/supported-configurations.json` at the repo root. The `dd-gitlab/config-inversion-linter` CI job fails otherwise. + +For each new integration name `<NAME>` (uppercase, dashes/dots replaced with underscores), add: + +```json +"DD_TRACE_<NAME>_ENABLED": [ + { + "version": "A", + "type": "boolean", + "default": "false", + "aliases": ["DD_TRACE_INTEGRATION_<NAME>_ENABLED", "DD_INTEGRATION_<NAME>_ENABLED"] + } +], +``` + +If the decorator's `instrumentationNames()` returns a shared name (e.g. `"sparkjava"` covering all sparkjava versions), also add the analytics keys for the shared name: + +```json +"DD_TRACE_<SHARED>_ANALYTICS_ENABLED": [ + { + "version": "A", + "type": "boolean", + "default": "false", + "aliases": ["DD_<SHARED>_ANALYTICS_ENABLED"] + } +], +"DD_TRACE_<SHARED>_ANALYTICS_SAMPLE_RATE": [ + { + "version": "A", + "type": "decimal", + "default": "1.0", + "aliases": ["DD_<SHARED>_ANALYTICS_SAMPLE_RATE"] + } +], +``` + +**Place entries alphabetically** in the JSON file. **Verify the JSON parses** before committing (`python3 -c "import json; json.load(open('metadata/supported-configurations.json'))"`). + +**Type names — match existing conventions**: use `"boolean"`, `"string"`, `"integer"`, `"decimal"` (for floating-point — NOT `"double"`). The `dd-gitlab/validate_supported_configurations_v2_local_file` CI job will fail with non-canonical type names like `"double"`. Cross-check by grepping existing entries for similar fields. + +**How to discover whether entries are missing**: after writing the instrumentation, search `metadata/supported-configurations.json` for each name used in `super(...)` and `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. + +### 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 `compileOnly` version and run +the sample app. 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 prior version module in testImplementation for mutual exclusion + +When two modules instrument the same library at different versions, add the prior +version's module as a `testImplementation` dependency to confirm they don't double-instrument: + +```groovy +// jedis-3.0/build.gradle +dependencies { + testImplementation project(':dd-java-agent:instrumentation:jedis:jedis-1.4') +} +``` + +This ensures `:test` validates that only the correct module fires for jedis-3.x requests. From bcb7d8b9e1be9a158a12f535e33a8dc39a33b16f Mon Sep 17 00:00:00 2001 From: Jordan Wong <jordan.wong@datadoghq.com> Date: Tue, 7 Jul 2026 20:32:06 -0400 Subject: [PATCH 08/22] refactor(skill): extract muzzle guidance to references/ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move Step 9's 'Muzzle directives' section + all its sub-rules (assertInverse gotchas, incompatible-major-version exclusion, skipVersions for malformed release versions) from SKILL.md into references/muzzle.md. SKILL.md keeps Step 9.2 as a summary + link. Preserves all content verbatim; no wording changes. Part 6 of the SKILL.md slim. Final state: SKILL.md 794 → ~215 lines, split into 6 topic-oriented reference files under references/. Signed-off-by: Jordan Wong <jordan.wong@datadoghq.com> --- .claude/skills/add-apm-integrations/SKILL.md | 121 +---------------- .../add-apm-integrations/references/muzzle.md | 125 ++++++++++++++++++ 2 files changed, 127 insertions(+), 119 deletions(-) create mode 100644 .claude/skills/add-apm-integrations/references/muzzle.md diff --git a/.claude/skills/add-apm-integrations/SKILL.md b/.claude/skills/add-apm-integrations/SKILL.md index 5ea232539fe..8a13c84eea8 100644 --- a/.claude/skills/add-apm-integrations/SKILL.md +++ b/.claude/skills/add-apm-integrations/SKILL.md @@ -120,126 +120,9 @@ Cover all mandatory test types: **Read [Writing Tests](references/tests.md).** Java tests only (JUnit 5) in `src/test/java/` — no new `.groovy` files (bot-enforced). Must cover error/exception scenarios. When adding new integration names, register them in `metadata/supported-configurations.json` (config-inversion-linter enforces). When `compileOnly` and `testImplementation` use different versions, comment the specific class that requires the higher version. Include the prior-version module as a `testImplementation` dependency for mutual-exclusion tests. ### 2. Muzzle directives (mandatory) -In `build.gradle`, add `muzzle` blocks. **There are two valid patterns** — choose based on whether your version range is open-ended or bounded. - -**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** (your instrumentation supports `[minVersion, maxVersion)` and an existing sibling module covers `[maxVersion, ∞)`). **Do NOT use `assertInverse = true`** — it can pick boundary versions inside your declared range as inverse-test targets, causing `muzzle-AssertFail-...` failures: - -``` -> Task :muzzle-AssertFail-redis.clients-jedis-jedis-3.6.2 FAILED -MUZZLE PASSED JedisInstrumentation BUT FAILURE WAS EXPECTED -``` - -Instead, declare BOTH the pass range AND the explicit fail ranges that bound it: - -```groovy -muzzle { - pass { - group = "com.example" - module = "framework" - versions = "[$minVersion,$maxVersion)" - } - fail { - group = "com.example" - module = "framework" - versions = "[,$minVersion)" - } -} -``` - -#### 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. - -**Background**: a typical greenfield generation produces a sync instrumentation class (works on older versions) and an async instrumentation class (requires a newer API). The agent picks the higher version as the declared min for muzzle, which is conservative for compileOnly. But `assertInverse = true` then auto-tests a lower version with ONLY the sync class hooks active, and that passes — creating the failure. - -#### 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 -(e.g. `commons-httpclient:commons-httpclient` 2.x vs 4.x), your muzzle `pass` range must -have an explicit upper bound to exclude the incompatible major: - -```groovy -// WRONG — open range accidentally covers commons-httpclient 4.x (different artifact family) -muzzle { - pass { - group = "commons-httpclient" - module = "commons-httpclient" - versions = "[2.0,)" // 4.x would also match but has completely different API - assertInverse = true - } -} - -// CORRECT — bounded range excludes 4.x -muzzle { - pass { - group = "commons-httpclient" - module = "commons-httpclient" - 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 (3.x → 4.x breaks, 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. +**Read [Muzzle Directives](references/muzzle.md).** +Two valid patterns: open-ended range (`[$min,)` with `assertInverse = true` only when the true min is verified) or bounded range (`[$min,$max)` with explicit `fail { versions = "[,$min)" }` — no `assertInverse`). Muzzle range must exclude incompatible major versions when the same `group:module` republishes with a rewritten API. Library-specific quirks (malformed release versions like `jedis-3.6.2`) require `skipVersions` — search adjacent modules for these before declaring new ranges. ### 3. Latest dependency test (mandatory) Use the `latestDepTestLibrary` helper in `build.gradle` to pin the latest available version. Run with: diff --git a/.claude/skills/add-apm-integrations/references/muzzle.md b/.claude/skills/add-apm-integrations/references/muzzle.md new file mode 100644 index 00000000000..5decabbd7b4 --- /dev/null +++ b/.claude/skills/add-apm-integrations/references/muzzle.md @@ -0,0 +1,125 @@ +# 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 two valid patterns** — choose based on whether your version range is open-ended or bounded. + +**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** (your instrumentation supports `[minVersion, maxVersion)` and an existing sibling module covers `[maxVersion, ∞)`). **Do NOT use `assertInverse = true`** — it can pick boundary versions inside your declared range as inverse-test targets, causing `muzzle-AssertFail-...` failures: + +``` +> Task :muzzle-AssertFail-redis.clients-jedis-jedis-3.6.2 FAILED +MUZZLE PASSED JedisInstrumentation BUT FAILURE WAS EXPECTED +``` + +Instead, declare BOTH the pass range AND the explicit fail ranges that bound it: + +```groovy +muzzle { + pass { + group = "com.example" + module = "framework" + versions = "[$minVersion,$maxVersion)" + } + fail { + group = "com.example" + module = "framework" + versions = "[,$minVersion)" + } +} +``` + +## 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. + +**Background**: a typical greenfield generation produces a sync instrumentation class (works on older versions) and an async instrumentation class (requires a newer API). The agent picks the higher version as the declared min for muzzle, which is conservative for compileOnly. But `assertInverse = true` then auto-tests a lower version with ONLY the sync class hooks active, and that passes — creating the failure. + +## 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 +(e.g. `commons-httpclient:commons-httpclient` 2.x vs 4.x), your muzzle `pass` range must +have an explicit upper bound to exclude the incompatible major: + +```groovy +// WRONG — open range accidentally covers commons-httpclient 4.x (different artifact family) +muzzle { + pass { + group = "commons-httpclient" + module = "commons-httpclient" + versions = "[2.0,)" // 4.x would also match but has completely different API + assertInverse = true + } +} + +// CORRECT — bounded range excludes 4.x +muzzle { + pass { + group = "commons-httpclient" + module = "commons-httpclient" + 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 (3.x → 4.x breaks, 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. From 86c3dcb13993e683fa9e4432822a3832c2670970 Mon Sep 17 00:00:00 2001 From: Jordan Wong <jordan.wong@datadoghq.com> Date: Tue, 7 Jul 2026 20:32:39 -0400 Subject: [PATCH 09/22] style(skill): add missing blank lines before Step/subsection headings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cosmetic fixup after the section extractions. Three headings lost their preceding blank line during the awk-based edits — restoring them so the rendered Markdown reads cleanly. No content changes. Signed-off-by: Jordan Wong <jordan.wong@datadoghq.com> --- .claude/skills/add-apm-integrations/SKILL.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.claude/skills/add-apm-integrations/SKILL.md b/.claude/skills/add-apm-integrations/SKILL.md index 8a13c84eea8..93fc776d4d8 100644 --- a/.claude/skills/add-apm-integrations/SKILL.md +++ b/.claude/skills/add-apm-integrations/SKILL.md @@ -105,6 +105,7 @@ In short: annotate with `@AutoService`, extend the right `InstrumenterModule.*` **Read [Writing the Advice Class](references/advice-class.md).** The reference file covers: `static` advice methods; enter/exit annotations (with the constructor exception); parameter annotations (`@Advice.This`, `@Advice.Argument`, `@Advice.Return`, `@Advice.Thrown`, `@Advice.Enter`, `@Advice.Local`); `CallDepthThreadLocalMap` reentrancy guarding; single-delegate-method instrumentation (not all overloads); span lifecycle order; `onExit` resilience to `onEnter` throwing; explicit charset for `byte[]` → `String`; no `NullPointerException` catches (SpotBugs blocks); `@AppliesOn` for multiple advice classes; and the "Must NOT do" list (no logger fields, no lambdas in advice, no `inline=false` in production, no `java.util.logging.*` / `java.nio.*` / `javax.management.*` in bootstrap). + ## Step 8 – Add SETTER/GETTER adapters (if applicable) For context propagation to and from upstream services, like HTTP headers, @@ -118,11 +119,13 @@ Cover all mandatory test types: ### 1. Instrumentation test (mandatory) **Read [Writing Tests](references/tests.md).** Java tests only (JUnit 5) in `src/test/java/` — no new `.groovy` files (bot-enforced). Must cover error/exception scenarios. When adding new integration names, register them in `metadata/supported-configurations.json` (config-inversion-linter enforces). When `compileOnly` and `testImplementation` use different versions, comment the specific class that requires the higher version. Include the prior-version module as a `testImplementation` dependency for mutual-exclusion tests. + ### 2. Muzzle directives (mandatory) **Read [Muzzle Directives](references/muzzle.md).** Two valid patterns: open-ended range (`[$min,)` with `assertInverse = true` only when the true min is verified) or bounded range (`[$min,$max)` with explicit `fail { versions = "[,$min)" }` — no `assertInverse`). Muzzle range must exclude incompatible major versions when the same `group:module` republishes with a rewritten API. Library-specific quirks (malformed release versions like `jedis-3.6.2`) require `skipVersions` — search adjacent modules for these before declaring new ranges. + ### 3. Latest dependency test (mandatory) Use the `latestDepTestLibrary` helper in `build.gradle` to pin the latest available version. Run with: From a835d47f0d329ab528b4ea6b533bf8fb2f5f826c Mon Sep 17 00:00:00 2001 From: Jordan Wong <jordan.wong@datadoghq.com> Date: Wed, 8 Jul 2026 00:24:02 -0400 Subject: [PATCH 10/22] refactor(skill): drop toolkit-internal 'Category A/B' language MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 'Category A' / 'Category B' labels came from toolkit-side research where they were shorthand for the 'target_kind' Pydantic enum values. They have no meaning in dd-trace-java on their own — a contributor reading the skill has no context for what 'Category B' refers to. Replace with the descriptive terms that already exist in dd-trace-java: - 'span-creating instrumentation' — extends InstrumenterModule.Tracing - 'context-tracking instrumentation' — extends InstrumenterModule.ContextTracking (matches the class name + TargetSystem.CONTEXT_TRACKING enum) Changes: - Rename references/category-b-context-propagation.md → references/context-tracking.md - Rewrite Step 4.1 stub in SKILL.md to drop Category A/B and 'target_kind' - Rewrite context-tracking.md body from 'Category B target shape' Pydantic- field enumeration to 'What a context-tracking instrumentation captures', described in Java terms (boundary type, capture/restore points, wrapper class, wrapper methods) instead of toolkit Pydantic field names - Fix advice-class.md's stray 'context-propagation logic' → 'context-tracking logic' to match dd-trace-java's TargetSystem.CONTEXT_TRACKING naming No substantive guidance changed. Reference still points at rxjava-2.0 as the canonical example. Signed-off-by: Jordan Wong <jordan.wong@datadoghq.com> --- .claude/skills/add-apm-integrations/SKILL.md | 6 +- .../references/advice-class.md | 2 +- .../category-b-context-propagation.md | 63 ------------------- .../references/context-tracking.md | 62 ++++++++++++++++++ 4 files changed, 66 insertions(+), 67 deletions(-) delete mode 100644 .claude/skills/add-apm-integrations/references/category-b-context-propagation.md create mode 100644 .claude/skills/add-apm-integrations/references/context-tracking.md diff --git a/.claude/skills/add-apm-integrations/SKILL.md b/.claude/skills/add-apm-integrations/SKILL.md index 93fc776d4d8..3d8ff365756 100644 --- a/.claude/skills/add-apm-integrations/SKILL.md +++ b/.claude/skills/add-apm-integrations/SKILL.md @@ -73,11 +73,11 @@ pattern before writing new code. Use it as a template. **See [Naming Conventions](references/naming-conventions.md) — module directory name must end with a version or an allowed suffix (`-common`, `-stubs`, `-iast`).** -## Step 4.1 – Library category: span-creating vs context-propagation +## Step 4.1 – Span-creating vs context-tracking instrumentation -**Read [Category B — Context Propagation](references/category-b-context-propagation.md) before picking instrumentation targets.** +**Read [Context-Tracking Instrumentation](references/context-tracking.md) before picking instrumentation targets.** -Classify the library along the `target_kind` axis: Category A (span-creating — HTTP/DB/messaging/RPC) or Category B (context-propagation — reactive/async/executor/fiber). Category B does not create spans; it bridges trace context across async boundaries. Get this wrong and you generate the wrong shape of instrumentation. +Decide which kind of `InstrumenterModule` the library needs: `InstrumenterModule.Tracing` (creates spans around I/O — HTTP/DB/messaging/RPC — the common case) or `InstrumenterModule.ContextTracking` (bridges trace context across async boundaries — reactive/async/executor/coroutine libraries; creates no spans of its own). Get this wrong and you write the wrong shape of instrumentation. ## Step 4.2 – Java naming consistency (CRITICAL — non-negotiable) diff --git a/.claude/skills/add-apm-integrations/references/advice-class.md b/.claude/skills/add-apm-integrations/references/advice-class.md index d449bd57ef4..5c7421431d3 100644 --- a/.claude/skills/add-apm-integrations/references/advice-class.md +++ b/.claude/skills/add-apm-integrations/references/advice-class.md @@ -133,7 +133,7 @@ public static class TracingAdvice { **When to use `@AppliesOn`:** -- Separate context-propagation logic from tracing logic +- Separate context-tracking logic from tracing logic - Different target systems need different instrumentation behaviours - Multiple advices apply to the same method with different system requirements diff --git a/.claude/skills/add-apm-integrations/references/category-b-context-propagation.md b/.claude/skills/add-apm-integrations/references/category-b-context-propagation.md deleted file mode 100644 index 329f9003c41..00000000000 --- a/.claude/skills/add-apm-integrations/references/category-b-context-propagation.md +++ /dev/null @@ -1,63 +0,0 @@ -# Category B — Context Propagation - -> Referenced from `SKILL.md` Step 4.1. Read this BEFORE picking instrumentation targets. - -**Before picking instrumentation targets**, classify the library along the `target_kind` axis: - -**Category A — span-creating** (most libraries): performs I/O, makes calls, runs queries. The instrumentation creates spans around those operations. - -- HTTP clients/servers, DB clients, messaging clients, RPC frameworks -- Targets: methods that perform the I/O (e.g., `Connection.sendCommand`, `Client.execute`) -- Tests: assert spans exist with correct tags - -**Category B — context-propagation** (reactive, async, threading, executor, fiber libraries): does NOT perform I/O directly. It coordinates work that other code performs. Instrumentation captures the active trace context at boundary creation and restores it at boundary crossing — **no spans are created by the module**, it only bridges trace context for spans created by other instrumentations or by user code. - -- RxJava, Reactor, CompletableFuture, ListenableFuture, executors, pekko/akka, lettuce async-command queue, ZIO, virtual threads -- Targets: one boundary-crossing type (e.g. `Observable`, `Flowable`, `Single`, `Maybe`, `Completable` for RxJava-shaped libs; `Runnable`/`Callable` for executor-shaped libs) -- Tests: assert that a span created in operation X is still ACTIVE when a callback scheduled by Y runs (parent-child bridging of *user-created* spans, NOT span tags on a target span) - -**Reference implementation for Category B:** - -`dd-java-agent/instrumentation/rxjava/rxjava-2.0/` — uses `InstrumenterModule.ContextTracking`. 5 type-instrumenters (Observable, Flowable, Single, Maybe, Completable), 5 wrappers (Tracing{Observer,Subscriber,SingleObserver,MaybeObserver,CompletableObserver}), 1 `RxJavaModule.java`, 1 `RxJavaAsyncResultExtension.java`. ~600 LOC total. - -## Category B target shape - -For each boundary-crossing type, capture: - -- `library_class` — FQN of the boundary-crossing type (e.g. `io.reactivex.rxjava3.core.Observable`). -- `capture_method` — capture point (usually the constructor — `<init>` or `isConstructor()`). -- `restore_method` — restore point (usually `subscribe(Observer)` / `subscribe(Subscriber)`). -- `wrapped_argument_type` — FQN of the user callback argument that must be wrapped (e.g. `io.reactivex.rxjava3.core.Observer`). -- `context_key_class` — FQN to key the `contextStore` on (almost always equals `library_class`). -- `wrapper_class_name` — class name of the wrapper to generate (e.g. `TracingObserver`). -- `wrapper_methods` — methods on the wrapped type that must reattach context before delegating (e.g. `["onNext", "onError", "onComplete"]`). - -## Tests for Category B - -Context-propagation 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**: - -```java -runUnderTrace("parent", () -> { - constructBoundary() // capture happens here - .subscribe(item -> userCodeStartsAChildSpan()); // restore happens around the lambda -}); -// 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 (Category B) - -When a reactive 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` hooks `subscribe(Observer)` (the implementation). The RxJava 3 reference instrumentation hooks `subscribe(FlowableSubscriber)`, not `subscribe(Subscriber)`. - -## When NOT to use Category B - -If the library DOES perform I/O — sends HTTP requests, runs DB queries, makes RPC calls, talks to a broker, reads/writes a cache — it is **span-creating**, not context-propagation. - -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-propagation instrumentation for the coordination path. `lettuce-5.0` is an example: there is a span-creating instrumentation for Redis commands and a separate context-propagation instrumentation for the async command queue. diff --git a/.claude/skills/add-apm-integrations/references/context-tracking.md b/.claude/skills/add-apm-integrations/references/context-tracking.md new file mode 100644 index 00000000000..ebe79d33d6f --- /dev/null +++ b/.claude/skills/add-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`. ~600 LOC total. 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 (`<init>` 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**: + +```java +runUnderTrace("parent", () -> { + constructBoundary() // capture happens here + .subscribe(item -> userCodeStartsAChildSpan()); // restore happens around the lambda +}); +// 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` hooks `subscribe(Observer)` (the implementation). + +## 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. From 12ddc4d8aebe205130a0217bc732425bca04f5be Mon Sep 17 00:00:00 2001 From: Jordan Wong <jordan.wong@datadoghq.com> Date: Wed, 8 Jul 2026 00:28:23 -0400 Subject: [PATCH 11/22] refactor(skill): remove toolkit-workflow language from reference files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two remaining spots reframed from LLM-agent-workflow perspective to dd-trace-java human-contributor perspective: - muzzle.md 'Background' paragraph: 'a typical greenfield generation produces...' + 'the agent picks the higher version...' → 'this failure mode is common when a module has both a sync and async instrumentation class' + 'declaring the higher version as the muzzle min...'. Same technical content, no LLM-agent workflow assumption. - tests.md 'How to discover' step: 'run the sample app' → 'run your instrumentation test'. 'Sample app' was ambiguous ('the toolkit's sample-app workflow step' vs 'your own test app'); the concrete dd-trace-java term is 'instrumentation test'. No substantive guidance changed. Preserves all rules verbatim. Signed-off-by: Jordan Wong <jordan.wong@datadoghq.com> --- .claude/skills/add-apm-integrations/references/muzzle.md | 2 +- .claude/skills/add-apm-integrations/references/tests.md | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.claude/skills/add-apm-integrations/references/muzzle.md b/.claude/skills/add-apm-integrations/references/muzzle.md index 5decabbd7b4..c8ddea5be51 100644 --- a/.claude/skills/add-apm-integrations/references/muzzle.md +++ b/.claude/skills/add-apm-integrations/references/muzzle.md @@ -68,7 +68,7 @@ muzzle { Add `assertInverse = true` only when you've empirically verified the min via local muzzle sweep. Otherwise, leaving it off is correct. -**Background**: a typical greenfield generation produces a sync instrumentation class (works on older versions) and an async instrumentation class (requires a newer API). The agent picks the higher version as the declared min for muzzle, which is conservative for compileOnly. But `assertInverse = true` then auto-tests a lower version with ONLY the sync class hooks active, and that passes — creating the failure. +**Background**: this failure mode is common when a module has both a "sync" instrumentation class (works on older versions) and an "async" instrumentation class (requires a newer API). Declaring the higher version as the muzzle min is conservative for `compileOnly`, but `assertInverse = true` then auto-tests a lower version with ONLY the sync class hooks active — that passes, creating the "MUZZLE PASSED BUT FAILURE WAS EXPECTED" failure. ## Muzzle range must exclude incompatible major versions diff --git a/.claude/skills/add-apm-integrations/references/tests.md b/.claude/skills/add-apm-integrations/references/tests.md index aa38f9c5c6b..3a2806a73c0 100644 --- a/.claude/skills/add-apm-integrations/references/tests.md +++ b/.claude/skills/add-apm-integrations/references/tests.md @@ -119,9 +119,9 @@ 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 `compileOnly` version and run -the sample app. 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`. +**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 prior version module in testImplementation for mutual exclusion From 016e0597c701db56a63f59d4bef16eb625b40141 Mon Sep 17 00:00:00 2001 From: Jordan Wong <jordan.wong@datadoghq.com> Date: Wed, 8 Jul 2026 10:20:41 -0400 Subject: [PATCH 12/22] fix(skill): address review comments on #11760 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nine fixes from Copilot bot + @mcculls review comments: SKILL.md - Step 4 source layout: 'src/test/groovy/ — Spock tests' → 'src/test/java/ — JUnit 5 tests'. Contradicted Step 9.1's Java-only policy. (Copilot) references/tests.md - Rewrite the error-test example: 'List<List<SpanData>> traces = ...' used OpenTelemetry's SpanData type (won't compile against dd-trace-java's TEST_WRITER, which returns List<List<DDSpan>>). Now uses AgentSpan and span.getTag() per mcculls's guidance that AgentSpan is enough for tests. - Replace 'checkNewGroovyFiles' (unverifiable bot name) with the real workflow: 'Enforce Groovy Migration' (.github/workflows/enforce-groovy-migration.yaml). Both places. - Default value in supported-configurations.json: change 'false' to 'true' per mcculls — ~83% of typical integrations default to true; 'false' is reserved for modules that override defaultEnabled() (OpenTelemetry, Hazelcast, sparkjava). Add a note calling out the branching. references/naming-conventions.md - Remove gRPCInstrumentation as an example — it doesn't exist in the codebase; the gRPC integration uses Grpc* (GrpcClientDecorator etc). Reframe the section to acknowledge acronym casing is not uniform across dd-trace-java and to defer to a reference instrumentation. (Copilot) - Drop the sanity-check bash script entirely. mcculls flagged that its regex only matched 'class', missing enum/interface/@interface, and would produce false MISMATCH lines for any such file (LogHandler.java, ParameterCollector.java, etc.). references/advice-class.md - Rewrite the 'onExit resilient to onEnter throwing' section — the claim that 'onThrowable = Throwable.class ensures exit fires even on onEnter exception' was factually wrong. Per docs/how_instrumentations_work.md:532-552, 'if the OnMethodEnter method throws an exception, the OnMethodExit method is not invoked' — unconditionally; onThrowable cannot override it. onThrowable controls exit-on-target-method-throw, not exit-on-enter-throw. (mcculls) - Add inline note that java.nio.charset.StandardCharsets is a java.nio.* type and forbidden in bootstrap instrumentations (per the same file's Must NOT list). In bootstrap advice, use the string charset name ('UTF-8') instead. (Copilot) references/context-tracking.md - Soften the 'rxjava-2.0 hooks subscribe(Observer)' statement. The module's actual matcher is named('subscribe').and(takesArguments(1)), matching any single-arg subscribe overload with the argument typed as the base callback interface. Direct the reader at the module source instead of copying overload names. (Copilot) Signed-off-by: Jordan Wong <jordan.wong@datadoghq.com> --- .claude/skills/add-apm-integrations/SKILL.md | 2 +- .../references/advice-class.md | 32 ++++++++++--------- .../references/context-tracking.md | 2 +- .../references/naming-conventions.md | 21 +----------- .../add-apm-integrations/references/tests.md | 19 ++++++----- 5 files changed, 31 insertions(+), 45 deletions(-) diff --git a/.claude/skills/add-apm-integrations/SKILL.md b/.claude/skills/add-apm-integrations/SKILL.md index 3d8ff365756..bb7a455f70d 100644 --- a/.claude/skills/add-apm-integrations/SKILL.md +++ b/.claude/skills/add-apm-integrations/SKILL.md @@ -57,7 +57,7 @@ pattern before writing new code. Use it as a template. 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 + - `src/test/java/` — JUnit 5 tests (Groovy/Spock is not accepted for new code — see Step 9.1) 3. Create `build.gradle` with: - `compileOnly` dependencies for the target framework - `testImplementation` dependencies for tests diff --git a/.claude/skills/add-apm-integrations/references/advice-class.md b/.claude/skills/add-apm-integrations/references/advice-class.md index 5c7421431d3..ba0b746bdd4 100644 --- a/.claude/skills/add-apm-integrations/references/advice-class.md +++ b/.claude/skills/add-apm-integrations/references/advice-class.md @@ -31,28 +31,28 @@ Exit method: 6. `span.finish()` 7. `scope.close()` -### onExit must be resilient to onEnter throwing +### onExit handling when the target method throws -If `onEnter` throws before the scope is set, `onExit` must still decrement the call depth. -A null-check that skips the reset leaks the ThreadLocal: +The `onThrowable = Throwable.class` attribute on `@Advice.OnMethodExit` controls whether the exit advice fires when the **instrumented target method** throws. Set it to `Throwable.class` (or omit it — this is the default) if you need to close the scope / finish the span regardless of whether the target method returned normally or threw. ```java -// RISKY — if onEnter threw, scope is null and reset is skipped -@Advice.OnMethodExit(suppress = Throwable.class) -public static void exit(@Advice.Enter final AgentScope scope) { - if (scope != null) scope.close(); -} - -// SAFER — onThrowable = Throwable.class ensures exit fires even on onEnter exception +// 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) { - if (scope != null) { - scope.close(); - } +public static void exit( + @Advice.Enter final AgentScope scope, + @Advice.Thrown final Throwable thrown) { + if (scope != null) { + DECORATE.onError(scope.span(), thrown); + DECORATE.beforeFinish(scope.span()); + scope.span().finish(); + scope.close(); + } } ``` -When using `CallDepthThreadLocalMap`, always decrement unconditionally in exit. +**`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 (except constructor advice — see note below). + +When using `CallDepthThreadLocalMap`, keep the enter and exit calls symmetric — every `incrementCallDepth` must have a matching `reset` in exit, guarded by the same condition on enter. ### Specify charset explicitly when converting byte[] to String @@ -65,6 +65,8 @@ import java.nio.charset.StandardCharsets; String cmd = new String(commandBytes, StandardCharsets.UTF_8); ``` +**Note for bootstrap instrumentations:** `java.nio.charset.StandardCharsets` is a `java.nio.*` type, and the "Must NOT do" list below forbids `java.nio.*` in bootstrap advice code. In a bootstrap instrumentation, use the string form of the charset name instead: `new String(commandBytes, "UTF-8")` (which throws `UnsupportedEncodingException` — catch it or wrap). + ### Do NOT catch `NullPointerException`; use null-check guards instead dd-trace-java enforces SpotBugs rule `DCN_NULLPOINTER_EXCEPTION` (no NPE catch). Defensive `try { ... } catch (NullPointerException e) { ... }` patterns will fail `:spotbugsMain` and block the PR. diff --git a/.claude/skills/add-apm-integrations/references/context-tracking.md b/.claude/skills/add-apm-integrations/references/context-tracking.md index ebe79d33d6f..189b35a5034 100644 --- a/.claude/skills/add-apm-integrations/references/context-tracking.md +++ b/.claude/skills/add-apm-integrations/references/context-tracking.md @@ -53,7 +53,7 @@ When a boundary type exposes multiple overloads of the subscribe / invoke method **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` hooks `subscribe(Observer)` (the implementation). +**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 diff --git a/.claude/skills/add-apm-integrations/references/naming-conventions.md b/.claude/skills/add-apm-integrations/references/naming-conventions.md index a88272d3895..1ef37df2987 100644 --- a/.claude/skills/add-apm-integrations/references/naming-conventions.md +++ b/.claude/skills/add-apm-integrations/references/naming-conventions.md @@ -24,23 +24,4 @@ The filename and the declared `public class` name MUST match exactly, character- - Every `import static <pkg>.<ClassName>.<member>` across all other files in the module - Every reference in the form `<ClassName>.<member>` or `<ClassName>.class` -**Convention in dd-trace-java**: acronym classes use **uppercase**: - -- CORRECT: `JMSDecorator`, `gRPCInstrumentation`, `JDBCConnection` -- WRONG: `JmsDecorator`, `GrpcInstrumentation`, `JdbcConnection` - -This matches the existing module conventions. When in doubt, match a reference instrumentation's casing exactly (see Step 3). - -**After generating each module, sanity-check before declaring done:** - -```bash -# Every public class declaration must match its filename -cd dd-java-agent/instrumentation/$framework/$framework-$version/src/main/java -for f in $(find . -name '*.java'); do - CLS=$(grep -oE 'public (final )?(abstract )?class [A-Za-z0-9_]+' "$f" | awk '{print $NF}') - EXPECTED=$(basename "$f" .java) - [ "$CLS" = "$EXPECTED" ] || echo "MISMATCH: $f declares '$CLS'" -done -``` - -If any MISMATCH lines print, fix them before moving on. +**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/.claude/skills/add-apm-integrations/references/tests.md b/.claude/skills/add-apm-integrations/references/tests.md index 3a2806a73c0..70027f96ab6 100644 --- a/.claude/skills/add-apm-integrations/references/tests.md +++ b/.claude/skills/add-apm-integrations/references/tests.md @@ -5,7 +5,8 @@ ## 1. Instrumentation test (mandatory) **Write Java tests (JUnit 5), NOT Groovy/Spock.** dd-trace-java policy -forbids new `.groovy` files — the PR bot (`checkNewGroovyFiles`) rejects any PR containing them. +forbids new `.groovy` files — the `Enforce Groovy Migration` workflow +(`.github/workflows/enforce-groovy-migration.yaml`) rejects any PR containing them. All new tests must go in `src/test/java/`. - JUnit 5 test class in `src/test/java/datadog/trace/instrumentation/<framework>/` @@ -23,10 +24,10 @@ void testExceptionSetsErrorTags() throws Exception { // trigger operation that throws client.execute(badRequest); }); - List<List<SpanData>> traces = TEST_WRITER.waitForTraces(1); - SpanData span = traces.get(0).get(0); - assertThat(span.getAttributes().get(ERROR_TYPE)).isNotNull(); - assertThat(span.getAttributes().get(ERROR_MESSAGE)).isNotNull(); + List<List<AgentSpan>> traces = TEST_WRITER.waitForTraces(1); + AgentSpan span = traces.get(0).get(0); + assertNotNull(span.getTag("error.type")); + assertNotNull(span.getTag("error.message")); } ``` @@ -37,7 +38,7 @@ For tests that need a separate JVM, suffix the test class with `ForkedTest` and dd-trace-java forbids new `.groovy` files (bot-enforced on every PR). Write Java: ```java -// WRONG — generates a .groovy file which the bot rejects automatically +// WRONG — generates a .groovy file which CI rejects automatically // src/test/groovy/JedisClientTest.groovy // CORRECT — Java in src/test/java/ @@ -51,7 +52,7 @@ public class Jedis3ClientTest extends AgentInstrumentationTest { } ``` -The PR bot check (`checkNewGroovyFiles`) will fail the PR if any `.groovy` file is added. +The `Enforce Groovy Migration` workflow will fail the PR if any `.groovy` file is added. ### Register new integration names in `metadata/supported-configurations.json` @@ -64,12 +65,14 @@ For each new integration name `<NAME>` (uppercase, dashes/dots replaced with und { "version": "A", "type": "boolean", - "default": "false", + "default": "true", "aliases": ["DD_TRACE_INTEGRATION_<NAME>_ENABLED", "DD_INTEGRATION_<NAME>_ENABLED"] } ], ``` +**Default value:** `"true"` for typical integrations (jedis, okhttp, kafka, spring-web, etc. — ~83% of existing entries). Set `"default": "false"` only if the module overrides `defaultEnabled()` to return `false` (e.g. OpenTelemetry, Hazelcast, sparkjava). Cross-check with `grep -A2 "defaultEnabled" dd-java-agent/instrumentation/<framework>*` before choosing. + If the decorator's `instrumentationNames()` returns a shared name (e.g. `"sparkjava"` covering all sparkjava versions), also add the analytics keys for the shared name: ```json From 7674bd984b95c79e9b72751259d60c29397f8869 Mon Sep 17 00:00:00 2001 From: Jordan Wong <jordan.wong@datadoghq.com> Date: Wed, 8 Jul 2026 10:53:45 -0400 Subject: [PATCH 13/22] fix(skill): trim bootstrap note on charset to a single line Copilot's suggestion was 'add an explicit note here'; the initial fix was a full paragraph. Trimming to a single-sentence pointer since the Must NOT list already carries the details. Signed-off-by: Jordan Wong <jordan.wong@datadoghq.com> --- .claude/skills/add-apm-integrations/references/advice-class.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.claude/skills/add-apm-integrations/references/advice-class.md b/.claude/skills/add-apm-integrations/references/advice-class.md index ba0b746bdd4..283be8251f4 100644 --- a/.claude/skills/add-apm-integrations/references/advice-class.md +++ b/.claude/skills/add-apm-integrations/references/advice-class.md @@ -65,7 +65,7 @@ import java.nio.charset.StandardCharsets; String cmd = new String(commandBytes, StandardCharsets.UTF_8); ``` -**Note for bootstrap instrumentations:** `java.nio.charset.StandardCharsets` is a `java.nio.*` type, and the "Must NOT do" list below forbids `java.nio.*` in bootstrap advice code. In a bootstrap instrumentation, use the string form of the charset name instead: `new String(commandBytes, "UTF-8")` (which throws `UnsupportedEncodingException` — catch it or wrap). +**Note for bootstrap instrumentations:** `StandardCharsets` is `java.nio.*`, which the "Must NOT do" list forbids in bootstrap advice. ### Do NOT catch `NullPointerException`; use null-check guards instead From c8cdffbbae07a88f6340b5029c07a88ddb7235ce Mon Sep 17 00:00:00 2001 From: Stuart McCulloch <stuart.mcculloch@datadoghq.com> Date: Thu, 9 Jul 2026 11:58:32 +0100 Subject: [PATCH 14/22] review feedback --- .claude/skills/add-apm-integrations/SKILL.md | 52 +++++---- .../references/advice-class.md | 23 ++-- .../references/context-tracking.md | 10 +- .../references/instrumenter-module.md | 28 +++-- .../add-apm-integrations/references/muzzle.md | 19 +++- .../references/supported-configurations.md | 59 ++++++++++ .../add-apm-integrations/references/tests.md | 103 +++++------------- 7 files changed, 165 insertions(+), 129 deletions(-) create mode 100644 .claude/skills/add-apm-integrations/references/supported-configurations.md diff --git a/.claude/skills/add-apm-integrations/SKILL.md b/.claude/skills/add-apm-integrations/SKILL.md index bb7a455f70d..4ea4e926cb7 100644 --- a/.claude/skills/add-apm-integrations/SKILL.md +++ b/.claude/skills/add-apm-integrations/SKILL.md @@ -30,8 +30,8 @@ Compare the content of the three docs against the rules encoded in Steps 2–11 - 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. +For every discrepancy found, edit this file (`.claude/skills/add-apm-integrations/SKILL.md`) or its referenced files +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 @@ -57,19 +57,13 @@ pattern before writing new code. Use it as a template. 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/java/` — JUnit 5 tests (Groovy/Spock is not accepted for new code — see Step 9.1) + - `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) + - `muzzle { pass { } }` directives (see Step 9.2) 4. Register the new module in `settings.gradle.kts` in **alphabetical order** -5. Register the integration name in `metadata/supported-configurations.json` (see "Register new integration names" in Step 9), or - `checkInstrumenterModuleConfigurations` fails. The name in `super(...)` maps to env var - `DD_TRACE_<NAME>_ENABLED` (`.` and `-` become `_`, uppercased — `couchbase-3` → - `DD_TRACE_COUCHBASE_3_ENABLED`). Add a `"type": "boolean"` entry, in alphabetical order, with - aliases `DD_TRACE_INTEGRATION_<NAME>_ENABLED` and `DD_INTEGRATION_<NAME>_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. +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`).** @@ -83,13 +77,13 @@ Decide which kind of `InstrumenterModule` the library needs: `InstrumenterModule **Read [Naming Conventions](references/naming-conventions.md) § "Java naming consistency".** -Filename and `public class` name MUST match character-for-character (including acronym casing). Use one canonical string everywhere: filename, class decl, `import static`, `ClassName.member` references. The reference file has a sanity-check script — run it before declaring done. +Filename and `public class` name MUST match character-for-character (including acronym casing). Use one canonical string everywhere: filename, class decl, `import static`, `ClassName.member` references. ## Step 5 – Write the InstrumenterModule **Read [InstrumenterModule Guidance](references/instrumenter-module.md).** -In short: annotate with `@AutoService`, extend the right `InstrumenterModule.*` subclass, implement the narrowest `Instrumenter` interface possible (`ForSingleType` > `ForKnownTypes` > `ForTypeHierarchy` — with a critical exception for interface-only API JARs like JMS/JPA/JDBC). Include a version-qualified alias in `instrumentationNames()` (`{"jedis", "jedis-3.0"}`). Declare all helper classes. When regenerating an existing module, preserve master's integration name to avoid churn in `supported-configurations.json`. Do NOT extract one-shot method return values into static constants. +In short: annotate with `@AutoService`, extend the right `InstrumenterModule.*` subclass, implement the narrowest `Instrumenter` interface possible (`ForSingleType` > `ForKnownTypes` > `ForTypeHierarchy` — with a critical exception for interface-only API JARs like JMS/JPA/JDBC). Include a version-qualified alias in `super("jedis", "jedis-3.0")`. Declare all helper classes. When regenerating an existing module, preserve master's integration name to avoid churn in `supported-configurations.json`. Do NOT extract one-shot method return values into static constants. ## Step 6 – Write the Decorator @@ -99,6 +93,8 @@ In short: annotate with `@AutoService`, extend the right `InstrumenterModule.*` - 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_<NAME>_ANALYTICS_ENABLED`, `DD_TRACE_<NAME>_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) @@ -118,17 +114,22 @@ Cover all mandatory test types: ### 1. Instrumentation test (mandatory) -**Read [Writing Tests](references/tests.md).** Java tests only (JUnit 5) in `src/test/java/` — no new `.groovy` files (bot-enforced). Must cover error/exception scenarios. When adding new integration names, register them in `metadata/supported-configurations.json` (config-inversion-linter enforces). When `compileOnly` and `testImplementation` use different versions, comment the specific class that requires the higher version. Include the prior-version module as a `testImplementation` dependency for mutual-exclusion tests. +**Read [Writing Tests](references/tests.md).** Instrumentation tests are Groovy/Spock (`src/test/groovy/`) — add `tag: override groovy enforcement` to the PR to bypass the groovy migration bot. 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 the prior-version module as a `testImplementation` dependency for mutual-exclusion tests. ### 2. Muzzle directives (mandatory) **Read [Muzzle Directives](references/muzzle.md).** -Two valid patterns: open-ended range (`[$min,)` with `assertInverse = true` only when the true min is verified) or bounded range (`[$min,$max)` with explicit `fail { versions = "[,$min)" }` — no `assertInverse`). Muzzle range must exclude incompatible major versions when the same `group:module` republishes with a rewritten API. Library-specific quirks (malformed release versions like `jedis-3.6.2`) require `skipVersions` — search adjacent modules for these before declaring new ranges. +Three valid patterns — see [Muzzle Directives](references/muzzle.md) for full examples: +- **Open-ended** `[$min,)`: use `assertInverse = true` only when the declared min is the verified true minimum. +- **Bounded, sibling module takes over at `$max`** `[$min,$max)`: use explicit `fail { versions = "[,$min)" }` with no `assertInverse` — the plugin can otherwise pick sibling-covered versions as inverse targets and fail unexpectedly. +- **Bounded, incompatible major version above `$max`** `[$min,$max)`: `assertInverse = true` is fine because versions above `$max` genuinely fail muzzle. + +Library-specific quirks (malformed release versions like `jedis-3.6.2`) require `skipVersions` — search adjacent modules for these before declaring new ranges. ### 3. Latest dependency test (mandatory) -Use the `latestDepTestLibrary` helper in `build.gradle` to pin the latest available version. Run with: +Use `latestDepTestImplementation` in `build.gradle` to pin the latest available version. Run with: ```bash ./gradlew :dd-java-agent:instrumentation:$framework-$version:latestDepTest ``` @@ -161,12 +162,15 @@ Run these commands in order and fix any failures before proceeding: ./gradlew spotlessCheck ``` -**If muzzle fails:** check for missing helper class names in `helperClassNames()`. +**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 Step 4, item 5. +(or mismatched) in `metadata/supported-configurations.json` — see [Supported Configurations](references/supported-configurations.md). -**If tests fail:** verify span lifecycle order (start → activate → error → finish → close), helper registration, +**If tests fail:** verify span lifecycle order (start → activate → error → close → finish), helper registration, and `contextStore()` map entries match actual usage. **If spotlessCheck fails:** run `./gradlew spotlessApply` to auto-format, then re-check. @@ -177,7 +181,10 @@ 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_<NAME>_ENABLED` entry (+ the two aliases) for every name passed to `super(...)` -- [ ] `build.gradle` has `compileOnly` deps and `muzzle` directives with `assertInverse = true` +- [ ] `metadata/supported-configurations.json` has `DD_TRACE_<NAME>_ANALYTICS_ENABLED` and `DD_TRACE_<NAME>_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: Pattern A (`assertInverse = true`) for open-ended ranges where the min is verified; Pattern B (explicit `fail` blocks, no `assertInverse`) when a sibling module takes over at the upper bound; Pattern C (`assertInverse = true`) when the upper bound excludes an incompatible major version — 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 @@ -186,7 +193,7 @@ Output this checklist and confirm each item is satisfied: - [ ] 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) +- [ ] Span lifecycle order is correct: startSpan → afterStart → activateSpan (enter); onError → beforeFinish → close → finish (exit) - [ ] Muzzle passes - [ ] Instrumentation tests pass - [ ] `latestDepTest` passes @@ -205,7 +212,8 @@ After the instrumentation is complete (or abandoned), review the full session an 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: +**For each lesson identified**, edit this file (`.claude/skills/add-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 diff --git a/.claude/skills/add-apm-integrations/references/advice-class.md b/.claude/skills/add-apm-integrations/references/advice-class.md index 283be8251f4..fa5ce4c5522 100644 --- a/.claude/skills/add-apm-integrations/references/advice-class.md +++ b/.claude/skills/add-apm-integrations/references/advice-class.md @@ -28,12 +28,12 @@ Enter method: Exit method: 4. `DECORATE.onError(span, throwable)` — only if throwable is non-null 5. `DECORATE.beforeFinish(span)` -6. `span.finish()` -7. `scope.close()` +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. Set it to `Throwable.class` (or omit it — this is the default) if you need to close the scope / finish the span regardless of whether the target method returned normally or threw. +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 @@ -42,10 +42,11 @@ public static void exit( @Advice.Enter final AgentScope scope, @Advice.Thrown final Throwable thrown) { if (scope != null) { - DECORATE.onError(scope.span(), thrown); - DECORATE.beforeFinish(scope.span()); - scope.span().finish(); + AgentSpan span = scope.span(); + DECORATE.onError(span, thrown); + DECORATE.beforeFinish(span); scope.close(); + span.finish(); } } ``` @@ -65,7 +66,7 @@ import java.nio.charset.StandardCharsets; String cmd = new String(commandBytes, StandardCharsets.UTF_8); ``` -**Note for bootstrap instrumentations:** `StandardCharsets` is `java.nio.*`, which the "Must NOT do" list forbids in bootstrap advice. +**Note for bootstrap instrumentations:** `StandardCharsets` (`java.nio.charset`) is safe in bootstrap — it is a pure constants class and is used freely in bootstrap code. The `java.nio.*` bootstrap ban applies specifically to NIO filesystem operations (`FileSystems.getDefault()`, `Path.of()`, etc.) that trigger native library initialization during premain. ### Do NOT catch `NullPointerException`; use null-check guards instead @@ -95,12 +96,12 @@ protected int status(final HttpMethod httpMethod) { ## 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()` instead of `applyAdvice()`: +If your instrumentation needs to apply multiple advices to the same method (e.g. separate context-tracking from tracing logic), use `applyAdvices()` inside `methodAdvice()`: ```java @Override -public void adviceTransformations(AdviceTransformation transformation) { - transformation.applyAdvices( +public void methodAdvice(MethodTransformer transformer) { + transformer.applyAdvices( named("someMethod") .and(takesArgument(0, named("com.example.Request"))) .and(takesArgument(1, named("com.example.Response"))), @@ -149,4 +150,4 @@ See `docs/how_instrumentations_work.md` section "@AppliesOn Annotation" for comp - **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 +- **No `java.util.logging.*`, `java.nio.file.*`, or `javax.management.*`** in bootstrap instrumentations diff --git a/.claude/skills/add-apm-integrations/references/context-tracking.md b/.claude/skills/add-apm-integrations/references/context-tracking.md index 189b35a5034..18c372f0ed9 100644 --- a/.claude/skills/add-apm-integrations/references/context-tracking.md +++ b/.claude/skills/add-apm-integrations/references/context-tracking.md @@ -35,11 +35,11 @@ For each boundary-crossing type in the library, the instrumentation needs to ide 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**: -```java -runUnderTrace("parent", () -> { - constructBoundary() // capture happens here - .subscribe(item -> userCodeStartsAChildSpan()); // restore happens around the lambda -}); +```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. ``` diff --git a/.claude/skills/add-apm-integrations/references/instrumenter-module.md b/.claude/skills/add-apm-integrations/references/instrumenter-module.md index 3537572b6ae..f89c6ee2ed7 100644 --- a/.claude/skills/add-apm-integrations/references/instrumenter-module.md +++ b/.claude/skills/add-apm-integrations/references/instrumenter-module.md @@ -38,20 +38,23 @@ ✅ `public String[] triggerClasses() { return new String[]{"com.example.Foo"}; }` -### instrumentationNames() must include a version-qualified alias +### Module constructor must include a version-qualified alias + +Pass a version-qualified alias to the `InstrumenterModule` constructor — this is what controls `DD_TRACE_<NAME>_ENABLED`: ```java -@Override -public String[] instrumentationNames() { - // WRONG — only generic name - return new String[]{"jedis"}; +// WRONG — only generic name; no per-version enable/disable +public JedisInstrumentation() { + super("jedis"); +} - // CORRECT — generic + version alias - return new String[]{"jedis", "jedis-3.0"}; +// CORRECT — generic + version alias +public JedisInstrumentation() { + super("jedis", "jedis-3.0"); } ``` -The version alias (e.g. `"jedis-3.0"`) lets users enable/disable this specific version independently. +The version alias (e.g. `"jedis-3.0"`) lets users enable/disable this specific version independently via `DD_TRACE_JEDIS_3_0_ENABLED`. Do NOT add version aliases to the decorator's `instrumentationNames()` — that method is for analytics keys only. ### Do not create a helper class just for CallDepthThreadLocalMap when only one type is instrumented @@ -68,10 +71,11 @@ public class GsonHelper { } } -// CORRECT — use CallDepthThreadLocalMap directly with the Advice or Decorator class as key -if (CallDepthThreadLocalMap.incrementCallDepth(GsonInstrumentation.class) > 0) return; +// 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(GsonInstrumentation.class); +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. @@ -97,7 +101,7 @@ super("commons-httpclient", "commons-httpclient-2.0"); 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 an array of instrumentations +- Must implement `typeInstrumentations()` returning a `List<Instrumenter>` - Must **not** implement an `Instrumenter` interface - Member instrumentations must **not** carry `@AutoService` and must **not** extend `TargetSystem` subclasses diff --git a/.claude/skills/add-apm-integrations/references/muzzle.md b/.claude/skills/add-apm-integrations/references/muzzle.md index c8ddea5be51..369b2ca89ed 100644 --- a/.claude/skills/add-apm-integrations/references/muzzle.md +++ b/.claude/skills/add-apm-integrations/references/muzzle.md @@ -4,7 +4,7 @@ ## Muzzle directives (mandatory) -In `build.gradle`, add `muzzle` blocks. **There are two valid patterns** — choose based on whether your version range is open-ended or bounded. +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: @@ -19,14 +19,14 @@ muzzle { } ``` -**Pattern B — Bounded range** (your instrumentation supports `[minVersion, maxVersion)` and an existing sibling module covers `[maxVersion, ∞)`). **Do NOT use `assertInverse = true`** — it can pick boundary versions inside your declared range as inverse-test targets, causing `muzzle-AssertFail-...` failures: +**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 ``` -Instead, declare BOTH the pass range AND the explicit fail ranges that bound it: +To avoid this, declare the pass range AND an explicit fail range below `$minVersion` (the sibling module covers versions above `$maxVersion`): ```groovy muzzle { @@ -43,6 +43,19 @@ muzzle { } ``` +**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: diff --git a/.claude/skills/add-apm-integrations/references/supported-configurations.md b/.claude/skills/add-apm-integrations/references/supported-configurations.md new file mode 100644 index 00000000000..50e5bb39a60 --- /dev/null +++ b/.claude/skills/add-apm-integrations/references/supported-configurations.md @@ -0,0 +1,59 @@ +# 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_<NAME>_ENABLED` entries. Every name passed to the constructor must have a `_ENABLED` entry. +- **`instrumentationNames()` in the decorator** → validated by `checkDecoratorAnalyticsConfigurations` as `DD_TRACE_<NAME>_ANALYTICS_ENABLED` / `DD_TRACE_<NAME>_ANALYTICS_SAMPLE_RATE` entries. + +## `_ENABLED` entry (for each `super(...)` arg) + +For each new integration name `<NAME>` (uppercase, dashes/dots replaced with underscores — `couchbase-3` → `DD_TRACE_COUCHBASE_3_ENABLED`), add: + +```json +"DD_TRACE_<NAME>_ENABLED": [ + { + "version": "A", + "type": "boolean", + "default": "true", + "aliases": ["DD_TRACE_INTEGRATION_<NAME>_ENABLED", "DD_INTEGRATION_<NAME>_ENABLED"] + } +], +``` + +**Default value:** `"true"` for typical integrations (~83% of existing entries). Set `"default": "false"` only if the module overrides `defaultEnabled()` to return `false` (e.g. OpenTelemetry, Hazelcast, sparkjava). Cross-check with `grep -A2 "defaultEnabled" dd-java-agent/instrumentation/<framework>*` before choosing. + +## Analytics entries (for each decorator `instrumentationNames()` return value) + +```json +"DD_TRACE_<NAME>_ANALYTICS_ENABLED": [ + { + "version": "A", + "type": "boolean", + "default": "false", + "aliases": ["DD_<NAME>_ANALYTICS_ENABLED"] + } +], +"DD_TRACE_<NAME>_ANALYTICS_SAMPLE_RATE": [ + { + "version": "A", + "type": "decimal", + "default": "1.0", + "aliases": ["DD_<NAME>_ANALYTICS_SAMPLE_RATE"] + } +], +``` + +## Rules + +**Place entries alphabetically** in the JSON file. + +**Type names — match existing conventions**: use `"boolean"`, `"string"`, `"integer"`, `"decimal"` (for floating-point — NOT `"double"`). The `dd-gitlab/validate_supported_configurations_v2_local_file` CI job will fail with non-canonical type names. + +**Verify the JSON parses** before committing: +```bash +python3 -c "import json; json.load(open('metadata/supported-configurations.json'))" +``` + +**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/.claude/skills/add-apm-integrations/references/tests.md b/.claude/skills/add-apm-integrations/references/tests.md index 70027f96ab6..6cc7b25667e 100644 --- a/.claude/skills/add-apm-integrations/references/tests.md +++ b/.claude/skills/add-apm-integrations/references/tests.md @@ -4,101 +4,52 @@ ## 1. Instrumentation test (mandatory) -**Write Java tests (JUnit 5), NOT Groovy/Spock.** dd-trace-java policy -forbids new `.groovy` files — the `Enforce Groovy Migration` workflow -(`.github/workflows/enforce-groovy-migration.yaml`) rejects any PR containing them. -All new tests must go in `src/test/java/`. +**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. -- JUnit 5 test class in `src/test/java/datadog/trace/instrumentation/<framework>/` +- Groovy/Spock test class in `src/test/groovy/datadog/trace/instrumentation/<framework>/` - Verify: spans created, tags set, errors propagated, resource names correct -- Use `TEST_WRITER.waitForTraces(N)` for assertions -- Use `runUnderTrace("root", () -> { ... })` for synchronous code (Java lambda, not Groovy closure) +- 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: -```java -// Example error test (Java) -@Test -void testExceptionSetsErrorTags() throws Exception { - assertThrows(SomeException.class, () -> { - // trigger operation that throws - client.execute(badRequest); - }); - List<List<AgentSpan>> traces = TEST_WRITER.waitForTraces(1); - AgentSpan span = traces.get(0).get(0); - assertNotNull(span.getTag("error.type")); - assertNotNull(span.getTag("error.message")); +```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 + } + } + } } ``` For tests that need a separate JVM, suffix the test class with `ForkedTest` and run via the `forkedTest` task. -### No new .groovy files (Java tests only) +### Adding new .groovy test files -dd-trace-java forbids new `.groovy` files (bot-enforced on every PR). Write Java: +The `Enforce Groovy Migration` workflow blocks new `.groovy` files by default. For new instrumentation tests (which should be Groovy/Spock), add the `tag: override groovy enforcement` label to the PR to bypass the check. -```java -// WRONG — generates a .groovy file which CI rejects automatically -// src/test/groovy/JedisClientTest.groovy - -// CORRECT — Java in src/test/java/ -// src/test/java/datadog/trace/instrumentation/jedis3/Jedis3ClientTest.java -@ExtendWith(AgentJUnit5Extension.class) -public class Jedis3ClientTest extends AgentInstrumentationTest { - @Test - void testCommandCreatesSpan() { +```groovy +// src/test/groovy/datadog/trace/instrumentation/jedis3/Jedis3ClientTest.groovy +class Jedis3ClientTest extends InstrumentationSpecification { + def "command creates span"() { // test body } } ``` -The `Enforce Groovy Migration` workflow will fail the PR if any `.groovy` file is added. - ### Register new integration names in `metadata/supported-configurations.json` -Every new integration name in your module (whether from `super("foo-X.Y")` in `InstrumenterModule`, or from `instrumentationNames()` in the decorator) MUST have a corresponding entry in `metadata/supported-configurations.json` at the repo root. The `dd-gitlab/config-inversion-linter` CI job fails otherwise. - -For each new integration name `<NAME>` (uppercase, dashes/dots replaced with underscores), add: - -```json -"DD_TRACE_<NAME>_ENABLED": [ - { - "version": "A", - "type": "boolean", - "default": "true", - "aliases": ["DD_TRACE_INTEGRATION_<NAME>_ENABLED", "DD_INTEGRATION_<NAME>_ENABLED"] - } -], -``` - -**Default value:** `"true"` for typical integrations (jedis, okhttp, kafka, spring-web, etc. — ~83% of existing entries). Set `"default": "false"` only if the module overrides `defaultEnabled()` to return `false` (e.g. OpenTelemetry, Hazelcast, sparkjava). Cross-check with `grep -A2 "defaultEnabled" dd-java-agent/instrumentation/<framework>*` before choosing. - -If the decorator's `instrumentationNames()` returns a shared name (e.g. `"sparkjava"` covering all sparkjava versions), also add the analytics keys for the shared name: - -```json -"DD_TRACE_<SHARED>_ANALYTICS_ENABLED": [ - { - "version": "A", - "type": "boolean", - "default": "false", - "aliases": ["DD_<SHARED>_ANALYTICS_ENABLED"] - } -], -"DD_TRACE_<SHARED>_ANALYTICS_SAMPLE_RATE": [ - { - "version": "A", - "type": "decimal", - "default": "1.0", - "aliases": ["DD_<SHARED>_ANALYTICS_SAMPLE_RATE"] - } -], -``` - -**Place entries alphabetically** in the JSON file. **Verify the JSON parses** before committing (`python3 -c "import json; json.load(open('metadata/supported-configurations.json'))"`). - -**Type names — match existing conventions**: use `"boolean"`, `"string"`, `"integer"`, `"decimal"` (for floating-point — NOT `"double"`). The `dd-gitlab/validate_supported_configurations_v2_local_file` CI job will fail with non-canonical type names like `"double"`. Cross-check by grepping existing entries for similar fields. - -**How to discover whether entries are missing**: after writing the instrumentation, search `metadata/supported-configurations.json` for each name used in `super(...)` and `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. +See [Supported Configurations](supported-configurations.md) for the key shapes, CI checks, and JSON format. ### compileOnly and testImplementation may use different versions — explain why From be9be934a735f8d60ff9496c6ce78bece96db79d Mon Sep 17 00:00:00 2001 From: Stuart McCulloch <stuart.mcculloch@datadoghq.com> Date: Thu, 9 Jul 2026 15:01:10 +0100 Subject: [PATCH 15/22] Fix docs inconsistencies --- .claude/skills/add-apm-integrations/SKILL.md | 8 +++++-- docs/add_new_instrumentation.md | 18 ++++++++-------- docs/how_instrumentations_work.md | 22 ++++++++++---------- 3 files changed, 26 insertions(+), 22 deletions(-) diff --git a/.claude/skills/add-apm-integrations/SKILL.md b/.claude/skills/add-apm-integrations/SKILL.md index 4ea4e926cb7..8c940cfe658 100644 --- a/.claude/skills/add-apm-integrations/SKILL.md +++ b/.claude/skills/add-apm-integrations/SKILL.md @@ -100,7 +100,7 @@ In short: annotate with `@AutoService`, extend the right `InstrumenterModule.*` **Read [Writing the Advice Class](references/advice-class.md).** -The reference file covers: `static` advice methods; enter/exit annotations (with the constructor exception); parameter annotations (`@Advice.This`, `@Advice.Argument`, `@Advice.Return`, `@Advice.Thrown`, `@Advice.Enter`, `@Advice.Local`); `CallDepthThreadLocalMap` reentrancy guarding; single-delegate-method instrumentation (not all overloads); span lifecycle order; `onExit` resilience to `onEnter` throwing; explicit charset for `byte[]` → `String`; no `NullPointerException` catches (SpotBugs blocks); `@AppliesOn` for multiple advice classes; and the "Must NOT do" list (no logger fields, no lambdas in advice, no `inline=false` in production, no `java.util.logging.*` / `java.nio.*` / `javax.management.*` in bootstrap). +The reference file covers: `static` advice methods; enter/exit annotations (with the constructor exception); parameter annotations (`@Advice.This`, `@Advice.Argument`, `@Advice.Return`, `@Advice.Thrown`, `@Advice.Enter`, `@Advice.Local`); `CallDepthThreadLocalMap` reentrancy guarding; single-delegate-method instrumentation (not all overloads); span lifecycle order; `onExit` resilience to `onEnter` throwing; explicit charset for `byte[]` → `String`; no `NullPointerException` catches (SpotBugs blocks); `@AppliesOn` for multiple advice classes; and the "Must NOT do" list (no logger fields, no lambdas in advice, no `inline=false` in production, no `java.util.logging.*` / `java.nio.file.*` / `javax.management.*` in bootstrap). ## Step 8 – Add SETTER/GETTER adapters (if applicable) @@ -159,9 +159,12 @@ Run these commands in order and fix any failures before proceeding: ./gradlew :dd-java-agent:instrumentation:$framework-$version:test ./gradlew :dd-java-agent:instrumentation:$framework-$version:latestDepTest ./gradlew checkInstrumenterModuleConfigurations +./gradlew :dd-java-agent:updateAgentJarIntegrationsGoldenFile ./gradlew spotlessCheck ``` +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. @@ -192,7 +195,8 @@ Output this checklist and confirm each item is satisfied: - [ ] 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 +- [ ] 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) - [ ] Muzzle passes - [ ] Instrumentation tests pass diff --git a/docs/add_new_instrumentation.md b/docs/add_new_instrumentation.md index 27a851dd198..31fc9eca3bf 100644 --- a/docs/add_new_instrumentation.md +++ b/docs/add_new_instrumentation.md @@ -17,7 +17,7 @@ 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 @@ -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/how_instrumentations_work.md b/docs/how_instrumentations_work.md index 3c4aa7a9053..eefcdef045e 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"))), @@ -393,8 +393,8 @@ Decorator class names should end in _Decorator._ 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_. -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"))), From 2b44002fb386b616903433236840a7fa8cc47312 Mon Sep 17 00:00:00 2001 From: Stuart McCulloch <stuart.mcculloch@datadoghq.com> Date: Thu, 9 Jul 2026 15:08:42 +0100 Subject: [PATCH 16/22] Rename skill to apm-integrations --- .../{add-apm-integrations => apm-integrations}/SKILL.md | 6 +++--- .../references/advice-class.md | 0 .../references/context-tracking.md | 0 .../references/instrumenter-module.md | 0 .../references/muzzle.md | 0 .../references/naming-conventions.md | 0 .../references/supported-configurations.md | 0 .../references/tests.md | 0 8 files changed, 3 insertions(+), 3 deletions(-) rename .claude/skills/{add-apm-integrations => apm-integrations}/SKILL.md (98%) rename .claude/skills/{add-apm-integrations => apm-integrations}/references/advice-class.md (100%) rename .claude/skills/{add-apm-integrations => apm-integrations}/references/context-tracking.md (100%) rename .claude/skills/{add-apm-integrations => apm-integrations}/references/instrumenter-module.md (100%) rename .claude/skills/{add-apm-integrations => apm-integrations}/references/muzzle.md (100%) rename .claude/skills/{add-apm-integrations => apm-integrations}/references/naming-conventions.md (100%) rename .claude/skills/{add-apm-integrations => apm-integrations}/references/supported-configurations.md (100%) rename .claude/skills/{add-apm-integrations => apm-integrations}/references/tests.md (100%) diff --git a/.claude/skills/add-apm-integrations/SKILL.md b/.claude/skills/apm-integrations/SKILL.md similarity index 98% rename from .claude/skills/add-apm-integrations/SKILL.md rename to .claude/skills/apm-integrations/SKILL.md index 8c940cfe658..a6b178730ab 100644 --- a/.claude/skills/add-apm-integrations/SKILL.md +++ b/.claude/skills/apm-integrations/SKILL.md @@ -1,5 +1,5 @@ --- -name: add-apm-integrations +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: @@ -30,7 +30,7 @@ Compare the content of the three docs against the rules encoded in Steps 2–11 - 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/add-apm-integrations/SKILL.md`) or its referenced files +For every discrepancy found, edit this file (`.claude/skills/apm-integrations/SKILL.md`) or its referenced files 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. @@ -216,7 +216,7 @@ After the instrumentation is complete (or abandoned), review the full session an 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/add-apm-integrations/SKILL.md`) or its referenced files +**For each lesson identified**, edit this file (`.claude/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 diff --git a/.claude/skills/add-apm-integrations/references/advice-class.md b/.claude/skills/apm-integrations/references/advice-class.md similarity index 100% rename from .claude/skills/add-apm-integrations/references/advice-class.md rename to .claude/skills/apm-integrations/references/advice-class.md diff --git a/.claude/skills/add-apm-integrations/references/context-tracking.md b/.claude/skills/apm-integrations/references/context-tracking.md similarity index 100% rename from .claude/skills/add-apm-integrations/references/context-tracking.md rename to .claude/skills/apm-integrations/references/context-tracking.md diff --git a/.claude/skills/add-apm-integrations/references/instrumenter-module.md b/.claude/skills/apm-integrations/references/instrumenter-module.md similarity index 100% rename from .claude/skills/add-apm-integrations/references/instrumenter-module.md rename to .claude/skills/apm-integrations/references/instrumenter-module.md diff --git a/.claude/skills/add-apm-integrations/references/muzzle.md b/.claude/skills/apm-integrations/references/muzzle.md similarity index 100% rename from .claude/skills/add-apm-integrations/references/muzzle.md rename to .claude/skills/apm-integrations/references/muzzle.md diff --git a/.claude/skills/add-apm-integrations/references/naming-conventions.md b/.claude/skills/apm-integrations/references/naming-conventions.md similarity index 100% rename from .claude/skills/add-apm-integrations/references/naming-conventions.md rename to .claude/skills/apm-integrations/references/naming-conventions.md diff --git a/.claude/skills/add-apm-integrations/references/supported-configurations.md b/.claude/skills/apm-integrations/references/supported-configurations.md similarity index 100% rename from .claude/skills/add-apm-integrations/references/supported-configurations.md rename to .claude/skills/apm-integrations/references/supported-configurations.md diff --git a/.claude/skills/add-apm-integrations/references/tests.md b/.claude/skills/apm-integrations/references/tests.md similarity index 100% rename from .claude/skills/add-apm-integrations/references/tests.md rename to .claude/skills/apm-integrations/references/tests.md From 594637c6dc66f132fc83e9c24eab1aa1db6dd56b Mon Sep 17 00:00:00 2001 From: Stuart McCulloch <stuart.mcculloch@datadoghq.com> Date: Thu, 9 Jul 2026 15:45:19 +0100 Subject: [PATCH 17/22] remove redundant note --- .claude/skills/apm-integrations/SKILL.md | 11 +++++++---- .../references/advice-class.md | 4 +--- .../apm-integrations/references/muzzle.md | 19 +++++++++---------- .../references/supported-configurations.md | 2 +- 4 files changed, 18 insertions(+), 18 deletions(-) diff --git a/.claude/skills/apm-integrations/SKILL.md b/.claude/skills/apm-integrations/SKILL.md index a6b178730ab..ce70d41d9cc 100644 --- a/.claude/skills/apm-integrations/SKILL.md +++ b/.claude/skills/apm-integrations/SKILL.md @@ -131,7 +131,7 @@ Library-specific quirks (malformed release versions like `jedis-3.6.2`) require Use `latestDepTestImplementation` in `build.gradle` to pin the latest available version. Run with: ```bash -./gradlew :dd-java-agent:instrumentation:$framework-$version:latestDepTest +./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+`: @@ -155,10 +155,11 @@ Add a smoke test in `dd-smoke-tests/` only if the framework warrants a full end- 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 :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 :dd-java-agent:updateAgentJarIntegrationsGoldenFile ./gradlew spotlessCheck ``` @@ -173,6 +174,8 @@ After `updateAgentJarIntegrationsGoldenFile` runs, commit the updated `metadata/ **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_<NAME>_ANALYTICS_ENABLED` / `DD_TRACE_<NAME>_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. diff --git a/.claude/skills/apm-integrations/references/advice-class.md b/.claude/skills/apm-integrations/references/advice-class.md index fa5ce4c5522..5869bed73f5 100644 --- a/.claude/skills/apm-integrations/references/advice-class.md +++ b/.claude/skills/apm-integrations/references/advice-class.md @@ -53,7 +53,7 @@ public static void exit( **`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 (except constructor advice — see note below). -When using `CallDepthThreadLocalMap`, keep the enter and exit calls symmetric — every `incrementCallDepth` must have a matching `reset` in exit, guarded by the same condition on enter. +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 @@ -66,8 +66,6 @@ import java.nio.charset.StandardCharsets; String cmd = new String(commandBytes, StandardCharsets.UTF_8); ``` -**Note for bootstrap instrumentations:** `StandardCharsets` (`java.nio.charset`) is safe in bootstrap — it is a pure constants class and is used freely in bootstrap code. The `java.nio.*` bootstrap ban applies specifically to NIO filesystem operations (`FileSystems.getDefault()`, `Path.of()`, etc.) that trigger native library initialization during premain. - ### Do NOT catch `NullPointerException`; use null-check guards instead dd-trace-java enforces SpotBugs rule `DCN_NULLPOINTER_EXCEPTION` (no NPE catch). Defensive `try { ... } catch (NullPointerException e) { ... }` patterns will fail `:spotbugsMain` and block the PR. diff --git a/.claude/skills/apm-integrations/references/muzzle.md b/.claude/skills/apm-integrations/references/muzzle.md index 369b2ca89ed..0dcde401d1e 100644 --- a/.claude/skills/apm-integrations/references/muzzle.md +++ b/.claude/skills/apm-integrations/references/muzzle.md @@ -86,16 +86,15 @@ Add `assertInverse = true` only when you've empirically verified the min via loc ## 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 -(e.g. `commons-httpclient:commons-httpclient` 2.x vs 4.x), your muzzle `pass` range must -have an explicit upper bound to exclude the incompatible major: +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 commons-httpclient 4.x (different artifact family) +// WRONG — open range accidentally covers 4.x (different API, same coordinates) muzzle { pass { - group = "commons-httpclient" - module = "commons-httpclient" + group = "com.example" + module = "framework" versions = "[2.0,)" // 4.x would also match but has completely different API assertInverse = true } @@ -104,8 +103,8 @@ muzzle { // CORRECT — bounded range excludes 4.x muzzle { pass { - group = "commons-httpclient" - module = "commons-httpclient" + group = "com.example" + module = "framework" versions = "[2.0,4.0)" assertInverse = true } @@ -113,8 +112,8 @@ muzzle { ``` **How to check**: search Maven Central for the `group:module` coordinates and look for versions -that are clearly a different generation (3.x → 4.x breaks, major API rewrites). Look at the -existing dd-trace-java module for the same library to see how it bounds its range. +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 diff --git a/.claude/skills/apm-integrations/references/supported-configurations.md b/.claude/skills/apm-integrations/references/supported-configurations.md index 50e5bb39a60..6d2e865f181 100644 --- a/.claude/skills/apm-integrations/references/supported-configurations.md +++ b/.claude/skills/apm-integrations/references/supported-configurations.md @@ -49,7 +49,7 @@ For each new integration name `<NAME>` (uppercase, dashes/dots replaced with und **Place entries alphabetically** in the JSON file. -**Type names — match existing conventions**: use `"boolean"`, `"string"`, `"integer"`, `"decimal"` (for floating-point — NOT `"double"`). The `dd-gitlab/validate_supported_configurations_v2_local_file` CI job will fail with non-canonical type names. +**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. **Verify the JSON parses** before committing: ```bash From 49b1b8d41a5f3ec5f5f4490f299a460c86d30400 Mon Sep 17 00:00:00 2001 From: Stuart McCulloch <stuart.mcculloch@datadoghq.com> Date: Thu, 9 Jul 2026 15:54:37 +0100 Subject: [PATCH 18/22] Renamed .claude/skills to .agents/skills and added symlink-style redirects for the old location --- {.claude => .agents}/skills/apm-integrations/SKILL.md | 6 +++--- .../apm-integrations/references/advice-class.md | 0 .../apm-integrations/references/context-tracking.md | 0 .../references/instrumenter-module.md | 3 ++- .../skills/apm-integrations/references/muzzle.md | 0 .../apm-integrations/references/naming-conventions.md | 0 .../references/supported-configurations.md | 0 .../skills/apm-integrations/references/tests.md | 0 .../skills/migrate-groovy-to-java/QUALITY_RULES.md | 0 .../skills/migrate-groovy-to-java/SKILL.md | 0 .../skills/migrate-junit-source-to-tabletest/SKILL.md | 0 .../skills/review-groovy-migration/SKILL.md | 0 {.claude => .agents}/skills/techdebt/SKILL.md | 0 .claude/skills/apm-integrations | 1 + .claude/skills/migrate-groovy-to-java | 1 + .claude/skills/migrate-junit-source-to-tabletest | 1 + .claude/skills/review-groovy-migration | 1 + .claude/skills/techdebt | 1 + docs/add_new_instrumentation.md | 11 ++++++----- 19 files changed, 16 insertions(+), 9 deletions(-) rename {.claude => .agents}/skills/apm-integrations/SKILL.md (97%) rename {.claude => .agents}/skills/apm-integrations/references/advice-class.md (100%) rename {.claude => .agents}/skills/apm-integrations/references/context-tracking.md (100%) rename {.claude => .agents}/skills/apm-integrations/references/instrumenter-module.md (93%) rename {.claude => .agents}/skills/apm-integrations/references/muzzle.md (100%) rename {.claude => .agents}/skills/apm-integrations/references/naming-conventions.md (100%) rename {.claude => .agents}/skills/apm-integrations/references/supported-configurations.md (100%) rename {.claude => .agents}/skills/apm-integrations/references/tests.md (100%) rename {.claude => .agents}/skills/migrate-groovy-to-java/QUALITY_RULES.md (100%) rename {.claude => .agents}/skills/migrate-groovy-to-java/SKILL.md (100%) rename {.claude => .agents}/skills/migrate-junit-source-to-tabletest/SKILL.md (100%) rename {.claude => .agents}/skills/review-groovy-migration/SKILL.md (100%) rename {.claude => .agents}/skills/techdebt/SKILL.md (100%) create mode 120000 .claude/skills/apm-integrations create mode 120000 .claude/skills/migrate-groovy-to-java create mode 120000 .claude/skills/migrate-junit-source-to-tabletest create mode 120000 .claude/skills/review-groovy-migration create mode 120000 .claude/skills/techdebt diff --git a/.claude/skills/apm-integrations/SKILL.md b/.agents/skills/apm-integrations/SKILL.md similarity index 97% rename from .claude/skills/apm-integrations/SKILL.md rename to .agents/skills/apm-integrations/SKILL.md index ce70d41d9cc..7b4ea9c1854 100644 --- a/.claude/skills/apm-integrations/SKILL.md +++ b/.agents/skills/apm-integrations/SKILL.md @@ -30,7 +30,7 @@ Compare the content of the three docs against the rules encoded in Steps 2–11 - 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`) or its referenced files +For every discrepancy found, edit this file (`.agents/skills/apm-integrations/SKILL.md`) or its referenced files 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. @@ -194,7 +194,7 @@ Output this checklist and confirm each item is satisfied: - [ ] `@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) +- [ ] `@Advice.OnMethodEnter(suppress = Throwable.class)` on enter (omit `suppress` when hooking a constructor); `@Advice.OnMethodExit(suppress = Throwable.class)` on exit (suppress is always safe on exit, including constructors) - [ ] 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 @@ -219,7 +219,7 @@ After the instrumentation is complete (or abandoned), review the full session an 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`) or its referenced files +**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 diff --git a/.claude/skills/apm-integrations/references/advice-class.md b/.agents/skills/apm-integrations/references/advice-class.md similarity index 100% rename from .claude/skills/apm-integrations/references/advice-class.md rename to .agents/skills/apm-integrations/references/advice-class.md diff --git a/.claude/skills/apm-integrations/references/context-tracking.md b/.agents/skills/apm-integrations/references/context-tracking.md similarity index 100% rename from .claude/skills/apm-integrations/references/context-tracking.md rename to .agents/skills/apm-integrations/references/context-tracking.md diff --git a/.claude/skills/apm-integrations/references/instrumenter-module.md b/.agents/skills/apm-integrations/references/instrumenter-module.md similarity index 93% rename from .claude/skills/apm-integrations/references/instrumenter-module.md rename to .agents/skills/apm-integrations/references/instrumenter-module.md index f89c6ee2ed7..46339db741c 100644 --- a/.claude/skills/apm-integrations/references/instrumenter-module.md +++ b/.agents/skills/apm-integrations/references/instrumenter-module.md @@ -21,8 +21,9 @@ - **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 + - 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 diff --git a/.claude/skills/apm-integrations/references/muzzle.md b/.agents/skills/apm-integrations/references/muzzle.md similarity index 100% rename from .claude/skills/apm-integrations/references/muzzle.md rename to .agents/skills/apm-integrations/references/muzzle.md diff --git a/.claude/skills/apm-integrations/references/naming-conventions.md b/.agents/skills/apm-integrations/references/naming-conventions.md similarity index 100% rename from .claude/skills/apm-integrations/references/naming-conventions.md rename to .agents/skills/apm-integrations/references/naming-conventions.md diff --git a/.claude/skills/apm-integrations/references/supported-configurations.md b/.agents/skills/apm-integrations/references/supported-configurations.md similarity index 100% rename from .claude/skills/apm-integrations/references/supported-configurations.md rename to .agents/skills/apm-integrations/references/supported-configurations.md diff --git a/.claude/skills/apm-integrations/references/tests.md b/.agents/skills/apm-integrations/references/tests.md similarity index 100% rename from .claude/skills/apm-integrations/references/tests.md rename to .agents/skills/apm-integrations/references/tests.md 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/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/docs/add_new_instrumentation.md b/docs/add_new_instrumentation.md index 31fc9eca3bf..8be6d2dfc42 100644 --- a/docs/add_new_instrumentation.md +++ b/docs/add_new_instrumentation.md @@ -20,15 +20,15 @@ named `google-http-client`. (see [Naming](./how_instrumentations_work.md#naming) 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): -```groovy -include ':dd-java-agent:instrumentation:google-http-client' +```kotlin +include(":dd-java-agent:instrumentation:google-http-client") ``` ## Create the Instrumentation class @@ -59,7 +59,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"); } From 45952d1845ae3363daec29add1506257bb3863e8 Mon Sep 17 00:00:00 2001 From: Stuart McCulloch <stuart.mcculloch@datadoghq.com> Date: Thu, 9 Jul 2026 16:46:56 +0100 Subject: [PATCH 19/22] Align NIO docs advice with skill --- docs/bootstrap_design_guidelines.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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 From 30fc5d3ec6e629e547de12cc482b347b4c82c18e Mon Sep 17 00:00:00 2001 From: Stuart McCulloch <stuart.mcculloch@datadoghq.com> Date: Thu, 9 Jul 2026 16:58:54 +0100 Subject: [PATCH 20/22] Simplify SKILL --- .agents/skills/apm-integrations/SKILL.md | 51 +++---------------- .../references/advice-class.md | 48 ++--------------- .../references/context-tracking.md | 2 +- .../references/instrumenter-module.md | 29 ++--------- .../apm-integrations/references/muzzle.md | 2 +- .../references/naming-conventions.md | 2 +- .../references/supported-configurations.md | 7 +-- .../apm-integrations/references/tests.md | 25 +++------ AGENTS.md | 2 +- docs/add_new_instrumentation.md | 5 +- docs/how_instrumentations_work.md | 2 +- 11 files changed, 33 insertions(+), 142 deletions(-) diff --git a/.agents/skills/apm-integrations/SKILL.md b/.agents/skills/apm-integrations/SKILL.md index 7b4ea9c1854..e04f946965f 100644 --- a/.agents/skills/apm-integrations/SKILL.md +++ b/.agents/skills/apm-integrations/SKILL.md @@ -23,17 +23,6 @@ Before writing any code, read all three files in full: 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 (`.agents/skills/apm-integrations/SKILL.md`) or its referenced files -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: @@ -65,26 +54,16 @@ pattern before writing new code. Use it as a template. 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`).** +**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) before picking instrumentation targets.** - -Decide which kind of `InstrumenterModule` the library needs: `InstrumenterModule.Tracing` (creates spans around I/O — HTTP/DB/messaging/RPC — the common case) or `InstrumenterModule.ContextTracking` (bridges trace context across async boundaries — reactive/async/executor/coroutine libraries; creates no spans of its own). Get this wrong and you write the wrong shape of instrumentation. - -## Step 4.2 – Java naming consistency (CRITICAL — non-negotiable) - -**Read [Naming Conventions](references/naming-conventions.md) § "Java naming consistency".** - -Filename and `public class` name MUST match character-for-character (including acronym casing). Use one canonical string everywhere: filename, class decl, `import static`, `ClassName.member` references. +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).** -In short: annotate with `@AutoService`, extend the right `InstrumenterModule.*` subclass, implement the narrowest `Instrumenter` interface possible (`ForSingleType` > `ForKnownTypes` > `ForTypeHierarchy` — with a critical exception for interface-only API JARs like JMS/JPA/JDBC). Include a version-qualified alias in `super("jedis", "jedis-3.0")`. Declare all helper classes. When regenerating an existing module, preserve master's integration name to avoid churn in `supported-configurations.json`. Do NOT extract one-shot method return values into static constants. - ## Step 6 – Write the Decorator - Extend the most specific available base decorator: @@ -98,9 +77,7 @@ In short: annotate with `@AutoService`, extend the right `InstrumenterModule.*` ## Step 7 – Write the Advice class (highest-risk step) -**Read [Writing the Advice Class](references/advice-class.md).** - -The reference file covers: `static` advice methods; enter/exit annotations (with the constructor exception); parameter annotations (`@Advice.This`, `@Advice.Argument`, `@Advice.Return`, `@Advice.Thrown`, `@Advice.Enter`, `@Advice.Local`); `CallDepthThreadLocalMap` reentrancy guarding; single-delegate-method instrumentation (not all overloads); span lifecycle order; `onExit` resilience to `onEnter` throwing; explicit charset for `byte[]` → `String`; no `NullPointerException` catches (SpotBugs blocks); `@AppliesOn` for multiple advice classes; and the "Must NOT do" list (no logger fields, no lambdas in advice, no `inline=false` in production, no `java.util.logging.*` / `java.nio.file.*` / `javax.management.*` in bootstrap). +**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) @@ -114,18 +91,11 @@ Cover all mandatory test types: ### 1. Instrumentation test (mandatory) -**Read [Writing Tests](references/tests.md).** Instrumentation tests are Groovy/Spock (`src/test/groovy/`) — add `tag: override groovy enforcement` to the PR to bypass the groovy migration bot. 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 the prior-version module as a `testImplementation` dependency for mutual-exclusion tests. +**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).** - -Three valid patterns — see [Muzzle Directives](references/muzzle.md) for full examples: -- **Open-ended** `[$min,)`: use `assertInverse = true` only when the declared min is the verified true minimum. -- **Bounded, sibling module takes over at `$max`** `[$min,$max)`: use explicit `fail { versions = "[,$min)" }` with no `assertInverse` — the plugin can otherwise pick sibling-covered versions as inverse targets and fail unexpectedly. -- **Bounded, incompatible major version above `$max`** `[$min,$max)`: `assertInverse = true` is fine because versions above `$max` genuinely fail muzzle. - -Library-specific quirks (malformed release versions like `jedis-3.6.2`) require `skipVersions` — search adjacent modules for these before declaring new ranges. +**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) @@ -160,8 +130,8 @@ Run these commands in order and fix any failures before proceeding: ./gradlew :dd-java-agent:instrumentation:$framework:$framework-$version:latestDepTest ./gradlew checkInstrumenterModuleConfigurations ./gradlew checkDecoratorAnalyticsConfigurations +./gradlew spotlessApply ./gradlew :dd-java-agent:updateAgentJarIntegrationsGoldenFile -./gradlew spotlessCheck ``` 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. @@ -179,8 +149,6 @@ After `updateAgentJarIntegrationsGoldenFile` runs, commit the updated `metadata/ **If tests fail:** verify span lifecycle order (start → activate → error → close → finish), 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: @@ -189,7 +157,7 @@ Output this checklist and confirm each item is satisfied: - [ ] `metadata/supported-configurations.json` has a `DD_TRACE_<NAME>_ENABLED` entry (+ the two aliases) for every name passed to `super(...)` - [ ] `metadata/supported-configurations.json` has `DD_TRACE_<NAME>_ANALYTICS_ENABLED` and `DD_TRACE_<NAME>_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: Pattern A (`assertInverse = true`) for open-ended ranges where the min is verified; Pattern B (explicit `fail` blocks, no `assertInverse`) when a sibling module takes over at the upper bound; Pattern C (`assertInverse = true`) when the upper bound excludes an incompatible major version — see [Muzzle Directives](references/muzzle.md) +- [ ] 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) @@ -201,10 +169,7 @@ Output this checklist and confirm each item is satisfied: - [ ] 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) -- [ ] Muzzle passes -- [ ] Instrumentation tests pass -- [ ] `latestDepTest` passes -- [ ] `spotlessCheck` passes +- [ ] All Step 10 verification commands passed with no failures ## Step 12 – Retrospective: update this skill with what was learned diff --git a/.agents/skills/apm-integrations/references/advice-class.md b/.agents/skills/apm-integrations/references/advice-class.md index 5869bed73f5..f7f4c866f23 100644 --- a/.agents/skills/apm-integrations/references/advice-class.md +++ b/.agents/skills/apm-integrations/references/advice-class.md @@ -68,7 +68,7 @@ String cmd = new String(commandBytes, StandardCharsets.UTF_8); ### Do NOT catch `NullPointerException`; use null-check guards instead -dd-trace-java enforces SpotBugs rule `DCN_NULLPOINTER_EXCEPTION` (no NPE catch). Defensive `try { ... } catch (NullPointerException e) { ... }` patterns will fail `:spotbugsMain` and block the PR. +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 @@ -94,51 +94,9 @@ protected int status(final HttpMethod httpMethod) { ## 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()`: +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. -```java -@Override -public void methodAdvice(MethodTransformer transformer) { - transformer.applyAdvices( - named("someMethod") - .and(takesArgument(0, named("com.example.Request"))) - .and(takesArgument(1, named("com.example.Response"))), - getClass().getName() + "$ContextTrackingAdvice", // Applied first - getClass().getName() + "$ServiceAdvice" // Applied second - ); -} -``` - -Use the `@AppliesOn` annotation to control which target systems each advice applies to: - -```java -import datadog.trace.agent.tooling.InstrumenterModule.TargetSystem; -import datadog.trace.agent.tooling.annotation.AppliesOn; - -@AppliesOn(TargetSystem.CONTEXT_TRACKING) -public static class ContextTrackingAdvice { - @Advice.OnMethodEnter(suppress = Throwable.class) - public static void enter(@Advice.Argument(0) Request request) { - // This advice only runs when CONTEXT_TRACKING is enabled - } -} - -public static class TracingAdvice { - // Without @AppliesOn, this advice runs for the module's target system - @Advice.OnMethodEnter(suppress = Throwable.class) - public static void enter(@Advice.Argument(0) Request request) { - // Tracing-specific logic - } -} -``` - -**When to use `@AppliesOn`:** - -- Separate context-tracking logic from tracing logic -- Different target systems need different instrumentation behaviours -- Multiple advices apply to the same method with different system requirements - -See `docs/how_instrumentations_work.md` section "@AppliesOn Annotation" for complete details. +See the `@AppliesOn Annotation` section of `docs/how_instrumentations_work.md` for the full API and examples. ## Must NOT do diff --git a/.agents/skills/apm-integrations/references/context-tracking.md b/.agents/skills/apm-integrations/references/context-tracking.md index 18c372f0ed9..9004ee0811f 100644 --- a/.agents/skills/apm-integrations/references/context-tracking.md +++ b/.agents/skills/apm-integrations/references/context-tracking.md @@ -17,7 +17,7 @@ Before picking instrumentation targets, decide which kind of instrumentation the ## 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`. ~600 LOC total. Read this before writing a new context-tracking module. +**`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 diff --git a/.agents/skills/apm-integrations/references/instrumenter-module.md b/.agents/skills/apm-integrations/references/instrumenter-module.md index 46339db741c..cb35c7d7cf0 100644 --- a/.agents/skills/apm-integrations/references/instrumenter-module.md +++ b/.agents/skills/apm-integrations/references/instrumenter-module.md @@ -39,23 +39,20 @@ ✅ `public String[] triggerClasses() { return new String[]{"com.example.Foo"}; }` -### Module constructor must include a version-qualified alias +### Module constructor: new modules add a version alias; existing modules preserve existing names -Pass a version-qualified alias to the `InstrumenterModule` constructor — this is what controls `DD_TRACE_<NAME>_ENABLED`: +**New module**: pass a generic name AND a version-qualified alias so users can enable/disable this version independently: ```java -// WRONG — only generic name; no per-version enable/disable -public JedisInstrumentation() { - super("jedis"); -} - // CORRECT — generic + version alias public JedisInstrumentation() { super("jedis", "jedis-3.0"); } ``` -The version alias (e.g. `"jedis-3.0"`) lets users enable/disable this specific version independently via `DD_TRACE_JEDIS_3_0_ENABLED`. Do NOT add version aliases to the decorator's `instrumentationNames()` — that method is for analytics keys only. +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 @@ -81,22 +78,6 @@ 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. -### When regenerating an existing module, preserve master's integration name convention - -If you are modifying or regenerating instrumentation for a library that **already exists** in `dd-java-agent/instrumentation/` on master (e.g. `commons-httpclient-2.0/`), READ the existing module's `super(...)` and `instrumentationNames()` declarations and reuse them. - -```java -// Master's existing source uses dashed name: -// super("commons-http-client"); -// CORRECT (matches master, master's DD_TRACE_COMMONS_HTTP_CLIENT_* entries continue to work) -super("commons-http-client"); - -// WRONG (new name, requires adding 6 new supported-configurations.json entries) -super("commons-httpclient", "commons-httpclient-2.0"); -``` - -**Why**: dd-trace-java's `dd-gitlab/config-inversion-linter` requires registered names. Master already has registered entries for its existing convention; inventing a new convention forces a metadata change. Preserving the convention keeps the diff minimal and reviewer attention focused on the code, not boilerplate. - ## Advanced: Grouping multiple instrumentations under one module For complex frameworks with multiple version-specific or feature-specific instrumentations, you can group them under a single `InstrumenterModule` (file ending in `Module.java`). The module class: diff --git a/.agents/skills/apm-integrations/references/muzzle.md b/.agents/skills/apm-integrations/references/muzzle.md index 0dcde401d1e..1ca23275f03 100644 --- a/.agents/skills/apm-integrations/references/muzzle.md +++ b/.agents/skills/apm-integrations/references/muzzle.md @@ -81,7 +81,7 @@ muzzle { Add `assertInverse = true` only when you've empirically verified the min via local muzzle sweep. Otherwise, leaving it off is correct. -**Background**: this failure mode is common when a module has both a "sync" instrumentation class (works on older versions) and an "async" instrumentation class (requires a newer API). Declaring the higher version as the muzzle min is conservative for `compileOnly`, but `assertInverse = true` then auto-tests a lower version with ONLY the sync class hooks active — that passes, creating the "MUZZLE PASSED BUT FAILURE WAS EXPECTED" failure. +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 diff --git a/.agents/skills/apm-integrations/references/naming-conventions.md b/.agents/skills/apm-integrations/references/naming-conventions.md index 1ef37df2987..ef089b9c147 100644 --- a/.agents/skills/apm-integrations/references/naming-conventions.md +++ b/.agents/skills/apm-integrations/references/naming-conventions.md @@ -1,6 +1,6 @@ # Naming Conventions -> Referenced from `SKILL.md` Step 4 (module directory name) and Step 4.2 (Java class/file naming). +> 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 diff --git a/.agents/skills/apm-integrations/references/supported-configurations.md b/.agents/skills/apm-integrations/references/supported-configurations.md index 6d2e865f181..815a672520c 100644 --- a/.agents/skills/apm-integrations/references/supported-configurations.md +++ b/.agents/skills/apm-integrations/references/supported-configurations.md @@ -22,7 +22,7 @@ For each new integration name `<NAME>` (uppercase, dashes/dots replaced with und ], ``` -**Default value:** `"true"` for typical integrations (~83% of existing entries). Set `"default": "false"` only if the module overrides `defaultEnabled()` to return `false` (e.g. OpenTelemetry, Hazelcast, sparkjava). Cross-check with `grep -A2 "defaultEnabled" dd-java-agent/instrumentation/<framework>*` before choosing. +**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/<framework>*`. ## Analytics entries (for each decorator `instrumentationNames()` return value) @@ -51,9 +51,6 @@ For each new integration name `<NAME>` (uppercase, dashes/dots replaced with und **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. -**Verify the JSON parses** before committing: -```bash -python3 -c "import json; json.load(open('metadata/supported-configurations.json'))" -``` +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 index 6cc7b25667e..cf25214c266 100644 --- a/.agents/skills/apm-integrations/references/tests.md +++ b/.agents/skills/apm-integrations/references/tests.md @@ -34,19 +34,6 @@ def "exception sets error tags"() { For tests that need a separate JVM, suffix the test class with `ForkedTest` and run via the `forkedTest` task. -### Adding new .groovy test files - -The `Enforce Groovy Migration` workflow blocks new `.groovy` files by default. For new instrumentation tests (which should be Groovy/Spock), add the `tag: override groovy enforcement` label to the PR to bypass the check. - -```groovy -// src/test/groovy/datadog/trace/instrumentation/jedis3/Jedis3ClientTest.groovy -class Jedis3ClientTest extends InstrumentationSpecification { - def "command creates span"() { - // test body - } -} -``` - ### Register new integration names in `metadata/supported-configurations.json` See [Supported Configurations](supported-configurations.md) for the key shapes, CI checks, and JSON format. @@ -78,16 +65,20 @@ your instrumentation test. If a specific class raises `ClassNotFoundException` o class is the reason — check when it became public and use that version for `testImplementation`. Name the class in the comment. -### Include prior version module in testImplementation for mutual exclusion +### Include sibling version modules in testImplementation for mutual exclusion -When two modules instrument the same library at different versions, add the prior -version's module as a `testImplementation` dependency to confirm they don't double-instrument: +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` validates that only the correct module fires for jedis-3.x requests. +This ensures `:test` in each module validates that only the correct module fires for its version range. 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 8be6d2dfc42..e845661039a 100644 --- a/docs/add_new_instrumentation.md +++ b/docs/add_new_instrumentation.md @@ -24,11 +24,10 @@ in alpha order with the other instrumentations in this format: 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: ```kotlin -include(":dd-java-agent:instrumentation:google-http-client") +include(":dd-java-agent:instrumentation:jedis:jedis-3.0") ``` ## Create the Instrumentation class diff --git a/docs/how_instrumentations_work.md b/docs/how_instrumentations_work.md index eefcdef045e..e51bc2dbfe4 100644 --- a/docs/how_instrumentations_work.md +++ b/docs/how_instrumentations_work.md @@ -391,7 +391,7 @@ 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 `MethodTransformer.applyAdvice(ElementMatcher, String)` and methods are matched by the instrumentation's `methodAdvice()` method. From 04208a8f8dbe30cefef6e1c81e7de77fb453fbde Mon Sep 17 00:00:00 2001 From: Stuart McCulloch <stuart.mcculloch@datadoghq.com> Date: Thu, 9 Jul 2026 18:17:46 +0100 Subject: [PATCH 21/22] Fix constructor advice best-practice --- .agents/skills/apm-integrations/SKILL.md | 2 +- .agents/skills/apm-integrations/references/advice-class.md | 4 ++-- docs/how_instrumentations_work.md | 5 ++--- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/.agents/skills/apm-integrations/SKILL.md b/.agents/skills/apm-integrations/SKILL.md index e04f946965f..b614eb5eeb2 100644 --- a/.agents/skills/apm-integrations/SKILL.md +++ b/.agents/skills/apm-integrations/SKILL.md @@ -162,7 +162,7 @@ Output this checklist and confirm each item is satisfied: - [ ] `@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 (omit `suppress` when hooking a constructor); `@Advice.OnMethodExit(suppress = Throwable.class)` on exit (suppress is always safe on exit, including constructors) +- [ ] `@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 diff --git a/.agents/skills/apm-integrations/references/advice-class.md b/.agents/skills/apm-integrations/references/advice-class.md index f7f4c866f23..f0db92887d2 100644 --- a/.agents/skills/apm-integrations/references/advice-class.md +++ b/.agents/skills/apm-integrations/references/advice-class.md @@ -7,7 +7,7 @@ - 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 + - **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 @@ -51,7 +51,7 @@ public static void exit( } ``` -**`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 (except constructor advice — see note below). +**`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. diff --git a/docs/how_instrumentations_work.md b/docs/how_instrumentations_work.md index e51bc2dbfe4..8a52dbf7f09 100644 --- a/docs/how_instrumentations_work.md +++ b/docs/how_instrumentations_work.md @@ -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) From 8394775c4b94e3edcf34fbd7c188cbe5a0b6ff27 Mon Sep 17 00:00:00 2001 From: Stuart McCulloch <stuart.mcculloch@datadoghq.com> Date: Thu, 9 Jul 2026 18:41:22 +0100 Subject: [PATCH 22/22] Address codex comments --- .agents/skills/apm-integrations/SKILL.md | 8 ++++---- .agents/skills/apm-integrations/references/tests.md | 1 + 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/.agents/skills/apm-integrations/SKILL.md b/.agents/skills/apm-integrations/SKILL.md index b614eb5eeb2..0975cdd38c5 100644 --- a/.agents/skills/apm-integrations/SKILL.md +++ b/.agents/skills/apm-integrations/SKILL.md @@ -104,17 +104,17 @@ Use `latestDepTestImplementation` in `build.gradle` to pin the latest available ./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+`: +**`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+' +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+' +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. +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) diff --git a/.agents/skills/apm-integrations/references/tests.md b/.agents/skills/apm-integrations/references/tests.md index cf25214c266..b83a6410c4f 100644 --- a/.agents/skills/apm-integrations/references/tests.md +++ b/.agents/skills/apm-integrations/references/tests.md @@ -26,6 +26,7 @@ def "exception sets error tags"() { trace(1) { span { errored true + errorTags(SomeException, "expected error message") } } }