From 66f3f707b435abc4cba47c6cf8391089cfe88637 Mon Sep 17 00:00:00 2001 From: Jordan Wong Date: Fri, 10 Jul 2026 10:56:52 -0400 Subject: [PATCH 1/7] skill(apm-integrations): HTTP client follow-up rules from PR reviews MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five rules distilled from HTTP-client and HTTP-server PR reviews (feign #11709, commons-httpclient #11717, sparkjava #11708) that were NOT covered by the R13-R33 additions in #11760. - advice-class.md: async double-span (do not wrap an async client with advice when the sync delegate is already instrumented; produces two spans per request). Source: @ValentinZakharov on PR #11709. - advice-class.md: HelperMethods refactor anti-pattern (do not extract advice logic into a per-instrumentation helper class just to shorten the advice body — inline unless genuinely shared). Source: @ygree on PR #11717. - instrumenter-module.md: regeneration must preserve every override the master version has — not just super(...), but also defaultEnabled(), helperClassNames(), contextStore(), orderPriority(), etc. Source: Codex reviews on PRs #11709 and #11708 (recurring pattern). - instrumenter-module.md: scan dd-java-agent/instrumentation/$framework/ before generating; do not create parallel duplicate modules. Source: @PerfectSlayer's historical review on #10941, still an unencoded gap. - muzzle.md: base testImplementation dep version must match the module's declared minimum, not the latest — extends the existing latestDep parity rule to base tests. Source: @PerfectSlayer on PR #11708. 3 files, +44 lines. No changes to existing rules. Reviewers: @mcculls (skill hygiene, reviewed #11760), @PerfectSlayer (HTTP domain, contributed rules from #10941 + #11708). --- .../references/advice-class.md | 7 ++++++ .../references/instrumenter-module.md | 15 +++++++++++++ .../apm-integrations/references/muzzle.md | 22 +++++++++++++++++++ 3 files changed, 44 insertions(+) diff --git a/.agents/skills/apm-integrations/references/advice-class.md b/.agents/skills/apm-integrations/references/advice-class.md index f0db92887d2..a99750eccb7 100644 --- a/.agents/skills/apm-integrations/references/advice-class.md +++ b/.agents/skills/apm-integrations/references/advice-class.md @@ -92,6 +92,12 @@ protected int status(final HttpMethod httpMethod) { **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. +### Do not double-span async HTTP clients + +If the target method delegates to a sync client that is already instrumented (common in async-wrapper classes like `AsyncFeignClient`, `AsyncHttpClient`, etc.), do NOT open a second span in the async wrapper. The sync client's advice already opens the client span; wrapping again produces two spans per request, with the outer span holding no additional context. + +Before adding advice to an async wrapper, trace the call path to the sync delegate. If the delegate is already instrumented for span emission, the async wrapper only needs context-propagation advice (capture on submission, restore on completion) — not a second span. See `context-tracking.md` for the propagation pattern. + ## Multiple advice classes and `@AppliesOn` If your instrumentation needs to apply multiple advices to the same method (e.g. separate context-tracking from tracing logic), use `applyAdvices()` inside `methodAdvice()`. Use the `@AppliesOn` annotation to control which target systems each advice applies to. @@ -107,3 +113,4 @@ See the `@AppliesOn Annotation` section of `docs/how_instrumentations_work.md` f - **No `InstrumentationContext.get()`** outside of Advice code - **No `inline=false`** in production code (only for debugging; must be removed before committing) - **No `java.util.logging.*`, `java.nio.file.*`, or `javax.management.*`** in bootstrap instrumentations +- **Do not extract advice logic into a helper class just to shorten the advice body.** Advice methods are inlined by ByteBuddy; extracting into `SomethingHelper.doTheThing(...)` adds a static-method hop, an extra file, and misleads reviewers into thinking the helper is shared when it is used by exactly one advice. Keep advice inline unless the same logic is genuinely shared across multiple advice classes. When it IS shared, the helper belongs in `helperClassNames()` and named accordingly (e.g. `TracingUtils`, not `FooBarHelper`). The CallDepth helper-class carveout (see `instrumenter-module.md`) is a separate case for multi-type instrumentations. diff --git a/.agents/skills/apm-integrations/references/instrumenter-module.md b/.agents/skills/apm-integrations/references/instrumenter-module.md index cb35c7d7cf0..619a63af0d4 100644 --- a/.agents/skills/apm-integrations/references/instrumenter-module.md +++ b/.agents/skills/apm-integrations/references/instrumenter-module.md @@ -39,6 +39,19 @@ ✅ `public String[] triggerClasses() { return new String[]{"com.example.Foo"}; }` +### Before writing a new module, scan for an existing one + +Before creating `dd-java-agent/instrumentation/$framework/$framework-$version/`, list the parent directory `dd-java-agent/instrumentation/$framework/` to see what's already there: + +``` +ls dd-java-agent/instrumentation/$framework/ +# e.g. commons-httpclient-2.0/ (already exists) +``` + +If an existing module covers the same framework at a compatible version, **modify it in place** — do NOT create a parallel `$framework-2.0-generated/` or nested `$framework/$framework-2.0/` copy. Duplicate modules cause muzzle to match twice, double the CI cost, and create reviewer confusion (see PR #10941's "the more I read about it, the less I understand what was done" — a duplicate module that the reviewer could not disentangle from the original). + +If the existing module targets a genuinely different version range (e.g. existing `foo-1.0/` and you're adding `foo-3.0/`), a version-sibling is correct — but confirm by reading the existing module's muzzle range first. + ### Module constructor: new modules add a version alias; existing modules preserve existing names **New module**: pass a generic name AND a version-qualified alias so users can enable/disable this version independently: @@ -54,6 +67,8 @@ The version alias (e.g. `"jedis-3.0"`) maps to `DD_TRACE_JEDIS_3_0_ENABLED`. Do **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. +**When regenerating an existing module, preserve every override the master version has.** Not just `super(...)` — also `defaultEnabled()`, `helperClassNames()`, `contextStore()`, `orderPriority()`, `muzzleDirective()`, and any other overridden method. Read the current version of the file (on master) before generating; carry each override forward verbatim unless there's a documented reason to change it. Silent loss of `defaultEnabled() = false` (or similar opt-in flags) ships an integration with a different default than users expected. + ### 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: diff --git a/.agents/skills/apm-integrations/references/muzzle.md b/.agents/skills/apm-integrations/references/muzzle.md index 1ca23275f03..e2e8a75e104 100644 --- a/.agents/skills/apm-integrations/references/muzzle.md +++ b/.agents/skills/apm-integrations/references/muzzle.md @@ -56,6 +56,28 @@ muzzle { } ``` +## Test dependencies must match the module's declared minimum version + +The base `testImplementation` dep version in `build.gradle` must match the module's declared minimum version (from the module directory suffix and the muzzle `versions = "[X.Y,)"` range). Do not pin `testImplementation` to a newer version than the module claims to support — the tests will silently exercise a version the module wasn't declared to work with, and `muzzle` won't catch it because muzzle checks classpath compatibility, not test behavior. + +```groovy +// WRONG — module is jedis-3.0 (min = 3.0.0) but tests run against 4.0 +muzzle { + pass { group = "redis.clients"; module = "jedis"; versions = "[3.0,)" } +} +dependencies { + testImplementation("redis.clients:jedis:4.0.0") // ← breaks the min-version guarantee +} + +// CORRECT — testImplementation at the declared min; latestDepTestImplementation for the newest +dependencies { + testImplementation("redis.clients:jedis:3.0.0") + latestDepTestImplementation("redis.clients:jedis:+") +} +``` + +This is a stricter form of the `latestDep` range rule (see Step 9.3 of the main SKILL.md) — it applies to the *base* test dependency too, not just latestDep. + ## 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: From b9a9ab3dfc9faa18a2de3900d3a35a945f97604a Mon Sep 17 00:00:00 2001 From: Jordan Wong Date: Mon, 13 Jul 2026 08:08:26 -0400 Subject: [PATCH 2/7] skill(apm-integrations): additional rules from HTTP feedback audit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds 9 rules across 5 files based on extended audit of PR #11708 (sparkjava), PR #11709 (feign), and the sparkjava CI-fix commit history: - context-tracking: CompletableFuture cancellation preservation (codex, #11709) - advice-class: framework-inside-framework span identity (codex, #11708) - advice-class: no non-constant static fields in advice (CI-fix commit f6d1263e) - tests: 5 test-hygiene rules (PerfectSlayer, #11708) — no Thread.sleep, static server field, shared test bases, ForkedTest justification, no default jvmArgs - supported-configurations: registry type correctness (CI-fix commits 53836a07, 48a84c05) - instrumenter-module: helperClassNames() for enrichment helpers (CI-fix commit 2c372a3c) --- .../references/advice-class.md | 16 ++++++++++ .../references/context-tracking.md | 14 +++++++++ .../references/instrumenter-module.md | 15 ++++++++++ .../references/supported-configurations.md | 10 +++++++ .../apm-integrations/references/tests.md | 30 +++++++++++++++++++ 5 files changed, 85 insertions(+) diff --git a/.agents/skills/apm-integrations/references/advice-class.md b/.agents/skills/apm-integrations/references/advice-class.md index a99750eccb7..43d7e2fff87 100644 --- a/.agents/skills/apm-integrations/references/advice-class.md +++ b/.agents/skills/apm-integrations/references/advice-class.md @@ -114,3 +114,19 @@ See the `@AppliesOn Annotation` section of `docs/how_instrumentations_work.md` f - **No `inline=false`** in production code (only for debugging; must be removed before committing) - **No `java.util.logging.*`, `java.nio.file.*`, or `javax.management.*`** in bootstrap instrumentations - **Do not extract advice logic into a helper class just to shorten the advice body.** Advice methods are inlined by ByteBuddy; extracting into `SomethingHelper.doTheThing(...)` adds a static-method hop, an extra file, and misleads reviewers into thinking the helper is shared when it is used by exactly one advice. Keep advice inline unless the same logic is genuinely shared across multiple advice classes. When it IS shared, the helper belongs in `helperClassNames()` and named accordingly (e.g. `TracingUtils`, not `FooBarHelper`). The CallDepth helper-class carveout (see `instrumenter-module.md`) is a separate case for multi-type instrumentations. + +### Framework-inside-framework: enrich, do not replace, the outer span + +When an inner framework (Spark, Ratpack routes, JAX-RS handlers) runs *inside* an outer HTTP server (Jetty, Netty, Undertow, servlet containers), the outer server's instrumentation already opened the request span (typically `servlet.request` or `jetty-server`). Do NOT create a new `Decorator`, rename the active span, or overwrite its component tag from inside the inner framework's advice. That silently rewrites `servlet.request` → `spark.request` in production traces — a breaking observability change. + +Instead, enrich the active span with the matched route only: + +```java +HTTP_RESOURCE_DECORATOR.withRoute(activeSpan(), request.method(), route.matchedPath()); +``` + +No `AgentScope`, no `startSpan()`, no `decorator.afterStart()`. This applies to any inner routing/dispatch framework that runs inside another instrumented HTTP server. It does NOT apply to standalone HTTP clients, which own their own span identity. + +### Advice classes must not declare non-constant static fields + +`*Advice.java` classes are inlined at instrumentation sites; non-constant static fields (fields that aren't `static final` primitives or string literals) get pulled into every instrumented callsite and violate muzzle's assumptions. Keep only `static final` constants — no logger references, no cached decorators, no state. If you need shared state, put it on a helper class registered via `helperClassNames()`, not on the advice. diff --git a/.agents/skills/apm-integrations/references/context-tracking.md b/.agents/skills/apm-integrations/references/context-tracking.md index 9004ee0811f..598202129c7 100644 --- a/.agents/skills/apm-integrations/references/context-tracking.md +++ b/.agents/skills/apm-integrations/references/context-tracking.md @@ -60,3 +60,17 @@ When a boundary type exposes multiple overloads of the subscribe / invoke method 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. + +## Preserving cancellation on `CompletableFuture` / `CompletionStage` returns + +When advice wraps a `CompletableFuture` returned from an async client via `@Advice.Return(readOnly = false)`, do NOT reassign the return with `future = future.whenComplete(...)`. `whenComplete` produces a **dependent stage**; cancelling that stage does not cancel the original request. The caller's `future.cancel(true)` then only cancels the dependent stage and leaves the underlying I/O running. + +```java +// WRONG — severs cancellation from the caller +future = future.whenComplete((result, error) -> finishSpan(span, result, error)); + +// CORRECT — attach the callback without reassigning +future.whenComplete((result, error) -> finishSpan(span, result, error)); +``` + +If the wrapper truly needs to return a different `CompletionStage` (rare), forward `cancel(...)` to the original future explicitly. Applies to any async client instrumentation returning a cancellable `CompletionStage`. diff --git a/.agents/skills/apm-integrations/references/instrumenter-module.md b/.agents/skills/apm-integrations/references/instrumenter-module.md index 619a63af0d4..f04a27d7794 100644 --- a/.agents/skills/apm-integrations/references/instrumenter-module.md +++ b/.agents/skills/apm-integrations/references/instrumenter-module.md @@ -103,3 +103,18 @@ For complex frameworks with multiple version-specific or feature-specific instru - Member instrumentations must **not** carry `@AutoService` and must **not** extend `TargetSystem` subclasses See `docs/how_instrumentations_work.md` section "Grouping Instrumentations" for details. + +## Enrichment helpers must be declared in `helperClassNames()` + +If your advice delegates to a helper class (e.g. `SparkJavaRouteEnricher.enrich(...)` from inside `RoutesAdvice`), the helper's fully-qualified class name MUST be listed in `helperClassNames()` on the `InstrumenterModule`: + +```java +@Override +public String[] helperClassNames() { + return new String[] { + packageName + ".SparkJavaRouteEnricher", + }; +} +``` + +Without this, the helper class is not loaded into the target application's classloader at instrumentation time, and the advice will `NoClassDefFoundError` at runtime. This is checked by muzzle; a missing helper reference is a common failure mode when refactoring advice. diff --git a/.agents/skills/apm-integrations/references/supported-configurations.md b/.agents/skills/apm-integrations/references/supported-configurations.md index 815a672520c..2433f7d1678 100644 --- a/.agents/skills/apm-integrations/references/supported-configurations.md +++ b/.agents/skills/apm-integrations/references/supported-configurations.md @@ -54,3 +54,13 @@ For each new integration name `` (uppercase, dashes/dots replaced with und 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. + +## Populating new entries — common mistakes + +When adding a new tracer configuration to `metadata/supported-configurations.json`: + +- Every new `super(...)` name needs a corresponding `_ENABLED` entry — omitting entries causes `checkInstrumenterModuleConfigurations` to fail. +- Use the registry-canonical `type` field: `boolean` for `_ENABLED`, `double` for ratio/rate values (NOT `decimal`), `integer` for counts, `string` for enumerations. Consult `metadata/supported-configurations.json` for existing examples of each type. +- Every new `_ANALYTICS_SAMPLE_RATE` needs both an `_ANALYTICS_ENABLED` (boolean) and an `_ANALYTICS_SAMPLE_RATE` (double) entry. + +Run `./gradlew checkInstrumenterModuleConfigurations` locally before pushing to catch registration mismatches. diff --git a/.agents/skills/apm-integrations/references/tests.md b/.agents/skills/apm-integrations/references/tests.md index b83a6410c4f..9e06ce9bcf8 100644 --- a/.agents/skills/apm-integrations/references/tests.md +++ b/.agents/skills/apm-integrations/references/tests.md @@ -83,3 +83,33 @@ dependencies { ``` This ensures `:test` in each module validates that only the correct module fires for its version range. + +## Test hygiene + +### No `Thread.sleep()` in tests — use deterministic waits + +`Thread.sleep(...)` is a recipe for flake. Use a deterministic mechanism instead: + +- `TEST_WRITER.waitForTraces(N)` — waits until exactly N traces have been recorded, with a bounded timeout +- `CountDownLatch` / `CompletableFuture.get(timeout, TimeUnit)` — for signalling from async callbacks +- Spock's `PollingConditions` — for polling an assertion until it holds + +If you catch yourself writing `Thread.sleep(...)`, name the specific signal you're waiting for and wait on that signal directly. + +### Embedded servers use a static field — do not recreate per test + +For tests that start an embedded server (Jetty, Netty, Undertow, Spark, etc.), initialize the server once as a `@Shared` or `static` field and reuse it across test methods. Do NOT construct a new server in each `setup:` / `@Before` unless you have a concrete reason (e.g. per-test configuration). Recreating the server per test multiplies test wall-time and adds a startup-race surface for no benefit. Follow the pattern of existing server-instrumentation tests in the same framework family. + +### Factor shared test scaffolding into a base class + +If two sibling test classes (e.g. `FooTest` and `FooForkedTest`) need the same setup, request builder, or assertion helpers, extract them into a shared abstract base — do NOT copy-paste between the two files. Duplicated helper code across a handful of test classes is how bespoke JUnit scaffolding metastasizes across the codebase. + +### `ForkedTest` variants must have a concrete isolation reason + +The `ForkedTest` suffix runs a test in its own JVM via the `forkedTest` task. Only add a `ForkedTest` variant when the test genuinely needs JVM isolation — e.g. a system property that must be set before class-loading, an agent-level configuration that cannot be reset between tests, or a class-loader state that leaks. Do NOT mechanically add a `ForkedTest` alongside every `Test` class; each fork adds JVM startup cost to CI. + +State the isolation reason in a comment on the `ForkedTest` class. + +### Do not add default jvmArgs to test tasks + +`dd.trace.enabled=true` is the default; adding `jvmArgs '-Ddd.trace.enabled=true'` to a `Test` task in `build.gradle` is noise. Only add jvmArgs that meaningfully diverge from defaults (e.g. enabling a specific integration that's off by default, or a debug flag). If you're tempted to copy a `jvmArgs` block from a sibling module, check whether each flag is actually needed for this module. From b5c86517482396fd8e25e39f59184fe8527879e8 Mon Sep 17 00:00:00 2001 From: Jordan Wong Date: Mon, 13 Jul 2026 08:15:27 -0400 Subject: [PATCH 3/7] skill(apm-integrations): condition super(...) naming on sibling structure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reconciles two conflicting maintainer positions on the constructor pattern: - Stuart McCulloch (PR #11760 comment 3552616459) — new instrumentations should adopt the version-alias pattern - Valentin Zakharov (PR #11709 comment 3532152120) — single-module frameworks should pass one name; extra names mint DD_TRACE__ENABLED flags with no counterpart to gate against The empirical convention in dd-trace-java confirms Valentin's rule for single-module frameworks (feign, freemarker, liberty, sparkjava all pass one name; freemarker and liberty do so even with real version siblings) and McCulloch's rule for frameworks with real sibling versions (okhttp shipping okhttp-2.0 and okhttp-3.0 with a shared 'okhttp' group flag). Also fixes the previous jedis example which claimed 'jedis-3.0' alias but the shipping jedis-3.0 module actually passes super("jedis", "redis"). --- .../references/instrumenter-module.md | 33 +++++++++++++++---- 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/.agents/skills/apm-integrations/references/instrumenter-module.md b/.agents/skills/apm-integrations/references/instrumenter-module.md index f04a27d7794..1e9d05cd345 100644 --- a/.agents/skills/apm-integrations/references/instrumenter-module.md +++ b/.agents/skills/apm-integrations/references/instrumenter-module.md @@ -52,21 +52,42 @@ If an existing module covers the same framework at a compatible version, **modif If the existing module targets a genuinely different version range (e.g. existing `foo-1.0/` and you're adding `foo-3.0/`), a version-sibling is correct — but confirm by reading the existing module's muzzle range first. -### Module constructor: new modules add a version alias; existing modules preserve existing names +### Module constructor: choose names based on sibling structure -**New module**: pass a generic name AND a version-qualified alias so users can enable/disable this version independently: +Each name passed to `super(...)` becomes a distinct `DD_TRACE__ENABLED` flag. Choose the number of names based on whether version-specific siblings exist (or are imminent): + +**Single module, no version siblings, no imminent sibling planned** — pass ONE name: ```java -// CORRECT — generic + version alias -public JedisInstrumentation() { - super("jedis", "jedis-3.0"); +// CORRECT — single-module framework +public FeignInstrumentation() { + super("feign"); } ``` -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. +Adding a version alias here mints a `DD_TRACE___ENABLED` flag that has no counterpart to gate against; it doubles the config surface for no operator benefit. Empirically, most single-module frameworks in dd-trace-java (`feign`, `freemarker`, `liberty`, `sparkjava`) use one name — even when they live in a versioned directory. + +**Multiple version siblings exist** (`okhttp-2.0/` AND `okhttp-3.0/`, `jedis-1.4/` AND `jedis-3.0/` AND `jedis-4.0/`) — pass a shared group name PLUS a version-qualified alias so each version has an independent toggle sharing one group flag: + +```java +// CORRECT — okhttp has real siblings (okhttp-2.0 and okhttp-3.0) +public OkHttp3Instrumentation() { + super("okhttp", "okhttp-3"); +} +``` + +Users can then set `DD_TRACE_OKHTTP_ENABLED=false` (group off) OR `DD_TRACE_OKHTTP_3_ENABLED=false` (this version only). + +**New module you expect will imminently sibling** — add the alias upfront and document why in the commit message. If no sibling appears, drop the alias in a follow-up. **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. +Before choosing, run `ls dd-java-agent/instrumentation/$framework/` — the directory contents are the ground truth for whether siblings exist. + +Do NOT add version aliases to the decorator's `instrumentationNames()` — that method is for analytics keys only. + +_Rationale traceable to reviewer comments: single-name-for-single-module (Valentin Zakharov on PR #11709, comment `3532152120`); version alias when siblings exist (Stuart McCulloch on PR #11760, comment `3552616459`)._ + **When regenerating an existing module, preserve every override the master version has.** Not just `super(...)` — also `defaultEnabled()`, `helperClassNames()`, `contextStore()`, `orderPriority()`, `muzzleDirective()`, and any other overridden method. Read the current version of the file (on master) before generating; carry each override forward verbatim unless there's a documented reason to change it. Silent loss of `defaultEnabled() = false` (or similar opt-in flags) ships an integration with a different default than users expected. ### Do not create a helper class just for CallDepthThreadLocalMap when only one type is instrumented From 6616d81d359f7f73b58c0c38a5b9bd4a4e799d05 Mon Sep 17 00:00:00 2001 From: Jordan Wong Date: Mon, 13 Jul 2026 14:28:54 -0400 Subject: [PATCH 4/7] skill(apm-integrations): address Copilot review comments on #11927 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 7 findings, all verified against master: - tests.md: waitForTraces(N) waits for >=N (not exactly N) with 20s bounded timeout per ListWriter.java - supported-configurations.md: fix internal contradiction — registry uses "decimal" (rates) and "int" (counts), NOT "double"/"integer" (matches canonical guidance earlier in same doc, line 52) - instrumenter-module.md: replace non-existent super("feign") example with freemarker (real, verifiable); replace misleading single-name list with an accurate one; add sparkjava-2.3 counter-example explaining why super("sparkjava","sparkjava-2.4") uses the -2.4 alias despite living in the -2.3/ directory (compile against 2.3, test against 2.4 for JettyHandler) - muzzle.md: reframe testImplementation=min as default preference, not absolute rule; document justified deviation with sparkjava-2.3 build.gradle as the canonical example - context-tracking.md: reframe CompletableFuture pattern to emphasize that the correct (non-reassigning) pattern does NOT require @Advice.Return(readOnly=false); only add readOnly=false if you have a documented reason to substitute the return value - advice-class.md: S1 framework-in-framework example now includes the null-check on activeSpan(), matching sparkjava's RoutesInstrumentation.java:52-55 (withRoute() does not guard) Copilot's citations were checked against master before applying. --- .../references/advice-class.md | 8 ++++-- .../references/context-tracking.md | 24 +++++++++++++----- .../references/instrumenter-module.md | 12 ++++++--- .../apm-integrations/references/muzzle.md | 25 +++++++++++++++---- .../references/supported-configurations.md | 4 +-- .../apm-integrations/references/tests.md | 2 +- 6 files changed, 55 insertions(+), 20 deletions(-) diff --git a/.agents/skills/apm-integrations/references/advice-class.md b/.agents/skills/apm-integrations/references/advice-class.md index 43d7e2fff87..7a3be7386ef 100644 --- a/.agents/skills/apm-integrations/references/advice-class.md +++ b/.agents/skills/apm-integrations/references/advice-class.md @@ -119,10 +119,14 @@ See the `@AppliesOn Annotation` section of `docs/how_instrumentations_work.md` f When an inner framework (Spark, Ratpack routes, JAX-RS handlers) runs *inside* an outer HTTP server (Jetty, Netty, Undertow, servlet containers), the outer server's instrumentation already opened the request span (typically `servlet.request` or `jetty-server`). Do NOT create a new `Decorator`, rename the active span, or overwrite its component tag from inside the inner framework's advice. That silently rewrites `servlet.request` → `spark.request` in production traces — a breaking observability change. -Instead, enrich the active span with the matched route only: +Instead, enrich the active span with the matched route only. `HTTP_RESOURCE_DECORATOR.withRoute(...)` does NOT guard against a null span, so you MUST null-check before calling it — otherwise the advice NPEs when there is no active span (rare but possible under certain execution paths): ```java -HTTP_RESOURCE_DECORATOR.withRoute(activeSpan(), request.method(), route.matchedPath()); +// CORRECT — matches the pattern in dd-java-agent/instrumentation/spark/sparkjava-2.3/.../RoutesInstrumentation.java +final AgentSpan span = activeSpan(); +if (span != null && routeMatch != null) { + HTTP_RESOURCE_DECORATOR.withRoute(span, method.name(), routeMatch.getMatchUri()); +} ``` No `AgentScope`, no `startSpan()`, no `decorator.afterStart()`. This applies to any inner routing/dispatch framework that runs inside another instrumented HTTP server. It does NOT apply to standalone HTTP clients, which own their own span identity. diff --git a/.agents/skills/apm-integrations/references/context-tracking.md b/.agents/skills/apm-integrations/references/context-tracking.md index 598202129c7..5f594233202 100644 --- a/.agents/skills/apm-integrations/references/context-tracking.md +++ b/.agents/skills/apm-integrations/references/context-tracking.md @@ -63,14 +63,26 @@ Hybrid libraries that BOTH coordinate work AND perform I/O usually get one span- ## Preserving cancellation on `CompletableFuture` / `CompletionStage` returns -When advice wraps a `CompletableFuture` returned from an async client via `@Advice.Return(readOnly = false)`, do NOT reassign the return with `future = future.whenComplete(...)`. `whenComplete` produces a **dependent stage**; cancelling that stage does not cancel the original request. The caller's `future.cancel(true)` then only cancels the dependent stage and leaves the underlying I/O running. +When advice attaches a completion callback to a `CompletableFuture` returned from an async client, do NOT reassign the return with `future = future.whenComplete(...)`. `whenComplete` produces a **dependent stage**; cancelling that stage does not cancel the original request. The caller's `future.cancel(true)` then only cancels the dependent stage and leaves the underlying I/O running. + +The correct pattern attaches the callback for side-effects only, without reassigning the return — so `@Advice.Return` does NOT need `readOnly = false`: ```java -// WRONG — severs cancellation from the caller -future = future.whenComplete((result, error) -> finishSpan(span, result, error)); +// WRONG — severs cancellation from the caller; also unnecessarily requires readOnly = false +@Advice.OnMethodExit(suppress = Throwable.class) +public static void exit(@Advice.Return(readOnly = false) CompletableFuture future, + @Advice.Enter AgentSpan span) { + future = future.whenComplete((result, error) -> finishSpan(span, result, error)); +} -// CORRECT — attach the callback without reassigning -future.whenComplete((result, error) -> finishSpan(span, result, error)); +// CORRECT — attach the callback for its side-effect; keep the return read-only +@Advice.OnMethodExit(suppress = Throwable.class) +public static void exit(@Advice.Return CompletableFuture future, + @Advice.Enter AgentSpan span) { + future.whenComplete((result, error) -> finishSpan(span, result, error)); +} ``` -If the wrapper truly needs to return a different `CompletionStage` (rare), forward `cancel(...)` to the original future explicitly. Applies to any async client instrumentation returning a cancellable `CompletionStage`. +Only add `readOnly = false` if you have a documented reason to substitute the return value. If your goal is just to observe completion, the read-only pattern is both safer (preserves cancellation) and simpler. Applies to any async client instrumentation returning a cancellable `CompletionStage`. + +If the wrapper genuinely needs to return a different `CompletionStage` (rare), forward `cancel(...)` to the original future explicitly. diff --git a/.agents/skills/apm-integrations/references/instrumenter-module.md b/.agents/skills/apm-integrations/references/instrumenter-module.md index 1e9d05cd345..1696a849755 100644 --- a/.agents/skills/apm-integrations/references/instrumenter-module.md +++ b/.agents/skills/apm-integrations/references/instrumenter-module.md @@ -59,13 +59,17 @@ Each name passed to `super(...)` becomes a distinct `DD_TRACE__ENABLED` fl **Single module, no version siblings, no imminent sibling planned** — pass ONE name: ```java -// CORRECT — single-module framework -public FeignInstrumentation() { - super("feign"); +// CORRECT — single-module framework (freemarker lives in freemarker-2.3.9/ +// and freemarker-2.3.24/ sibling directories yet still passes ONE name because +// the two directories share the same integration name) +public DollarVariableInstrumentation() { + super("freemarker"); } ``` -Adding a version alias here mints a `DD_TRACE___ENABLED` flag that has no counterpart to gate against; it doubles the config surface for no operator benefit. Empirically, most single-module frameworks in dd-trace-java (`feign`, `freemarker`, `liberty`, `sparkjava`) use one name — even when they live in a versioned directory. +Adding a version alias here mints a `DD_TRACE___ENABLED` flag that has no counterpart to gate against; it doubles the config surface for no operator benefit. Empirically, single-name-only frameworks in dd-trace-java include `freemarker` (across `freemarker-2.3.9/` and `freemarker-2.3.24/`), `liberty` (across `liberty-20.0/` and `liberty-23.0/`), and most other framework directories with a single integration name. + +**Counter-example — `sparkjava`:** the `sparkjava-2.3/` module uses `super("sparkjava", "sparkjava-2.4")` (note the `-2.4`, not `-2.3`) because the module compiles against Spark 2.3 but tests against 2.4 (Spark's `JettyHandler` is available from 2.4). The versioned alias here reflects the version the code EXERCISES, not the compile-time minimum. This is intentional; do NOT invent a `-2.3` alias just because the directory is named `sparkjava-2.3/`. If in doubt, read the master `super(...)` and copy it verbatim. **Multiple version siblings exist** (`okhttp-2.0/` AND `okhttp-3.0/`, `jedis-1.4/` AND `jedis-3.0/` AND `jedis-4.0/`) — pass a shared group name PLUS a version-qualified alias so each version has an independent toggle sharing one group flag: diff --git a/.agents/skills/apm-integrations/references/muzzle.md b/.agents/skills/apm-integrations/references/muzzle.md index e2e8a75e104..e1f281630cd 100644 --- a/.agents/skills/apm-integrations/references/muzzle.md +++ b/.agents/skills/apm-integrations/references/muzzle.md @@ -56,12 +56,12 @@ muzzle { } ``` -## Test dependencies must match the module's declared minimum version +## Test dependencies should default to the module's declared minimum version -The base `testImplementation` dep version in `build.gradle` must match the module's declared minimum version (from the module directory suffix and the muzzle `versions = "[X.Y,)"` range). Do not pin `testImplementation` to a newer version than the module claims to support — the tests will silently exercise a version the module wasn't declared to work with, and `muzzle` won't catch it because muzzle checks classpath compatibility, not test behavior. +**Default:** the base `testImplementation` dep version in `build.gradle` should match the module's declared minimum version (from the module directory suffix and the muzzle `versions = "[X.Y,)"` range). Pinning `testImplementation` to a newer version than the module claims to support silently exercises a version the module wasn't declared to work with, and `muzzle` won't catch it because muzzle checks classpath compatibility, not test behavior. ```groovy -// WRONG — module is jedis-3.0 (min = 3.0.0) but tests run against 4.0 +// WRONG — module is jedis-3.0 (min = 3.0.0) but tests run against 4.0 with no justification muzzle { pass { group = "redis.clients"; module = "jedis"; versions = "[3.0,)" } } @@ -69,14 +69,29 @@ dependencies { testImplementation("redis.clients:jedis:4.0.0") // ← breaks the min-version guarantee } -// CORRECT — testImplementation at the declared min; latestDepTestImplementation for the newest +// DEFAULT — testImplementation at the declared min; latestDepTestImplementation for the newest dependencies { testImplementation("redis.clients:jedis:3.0.0") latestDepTestImplementation("redis.clients:jedis:+") } ``` -This is a stricter form of the `latestDep` range rule (see Step 9.3 of the main SKILL.md) — it applies to the *base* test dependency too, not just latestDep. +**Justified deviation:** a module may `compileOnly` against the declared minimum and `testImplementation` against a higher version when the test genuinely requires a class/API that only exists in the higher version. In that case: + +- Document the reason with a comment at the top of `build.gradle` (a reviewer must be able to see why immediately) +- The `super(...)` alias should reflect the version the code exercises, not the compile-time minimum + +Example — `dd-java-agent/instrumentation/spark/sparkjava-2.3/build.gradle`: + +```groovy +// building against 2.3 and testing against 2.4 because JettyHandler is available since 2.4 only +dependencies { + compileOnly group: 'com.sparkjava', name: 'spark-core', version: '2.3' + testImplementation group: 'com.sparkjava', name: 'spark-core', version: '2.4' +} +``` + +Without a written justification, prefer the default (test-at-min). This is a stricter form of the `latestDep` range rule (see Step 9.3 of the main SKILL.md) — it applies to the *base* test dependency too, not just latestDep. ## Do NOT use `assertInverse = true` unless the declared min is the actual minimum compatible version diff --git a/.agents/skills/apm-integrations/references/supported-configurations.md b/.agents/skills/apm-integrations/references/supported-configurations.md index 2433f7d1678..e5a856716a1 100644 --- a/.agents/skills/apm-integrations/references/supported-configurations.md +++ b/.agents/skills/apm-integrations/references/supported-configurations.md @@ -60,7 +60,7 @@ The Gradle checks (`checkInstrumenterModuleConfigurations`, `checkDecoratorAnaly When adding a new tracer configuration to `metadata/supported-configurations.json`: - Every new `super(...)` name needs a corresponding `_ENABLED` entry — omitting entries causes `checkInstrumenterModuleConfigurations` to fail. -- Use the registry-canonical `type` field: `boolean` for `_ENABLED`, `double` for ratio/rate values (NOT `decimal`), `integer` for counts, `string` for enumerations. Consult `metadata/supported-configurations.json` for existing examples of each type. -- Every new `_ANALYTICS_SAMPLE_RATE` needs both an `_ANALYTICS_ENABLED` (boolean) and an `_ANALYTICS_SAMPLE_RATE` (double) entry. +- Use the registry-canonical `type` field: `boolean` for `_ENABLED`, `decimal` for ratio/rate values (NOT `double`), `int` for counts (NOT `integer`), `string` for enumerations, `array` and `map` where applicable. The `dd-gitlab/validate_supported_configurations_v2_local_file` CI job will fail on non-canonical type names. Consult `metadata/supported-configurations.json` for existing examples of each. +- Every new `_ANALYTICS_SAMPLE_RATE` needs both an `_ANALYTICS_ENABLED` (`boolean`) and an `_ANALYTICS_SAMPLE_RATE` (`decimal`) entry. Run `./gradlew checkInstrumenterModuleConfigurations` locally before pushing to catch registration mismatches. diff --git a/.agents/skills/apm-integrations/references/tests.md b/.agents/skills/apm-integrations/references/tests.md index 9e06ce9bcf8..2aad929bfe8 100644 --- a/.agents/skills/apm-integrations/references/tests.md +++ b/.agents/skills/apm-integrations/references/tests.md @@ -90,7 +90,7 @@ This ensures `:test` in each module validates that only the correct module fires `Thread.sleep(...)` is a recipe for flake. Use a deterministic mechanism instead: -- `TEST_WRITER.waitForTraces(N)` — waits until exactly N traces have been recorded, with a bounded timeout +- `TEST_WRITER.waitForTraces(N)` — waits until at least N traces have been recorded (`traceCount >= N`), with a bounded timeout of 20s (see `dd-trace-core/src/main/java/datadog/trace/common/writer/ListWriter.java`). Use `TEST_WRITER.size()` afterwards to assert the exact count you expect. - `CountDownLatch` / `CompletableFuture.get(timeout, TimeUnit)` — for signalling from async callbacks - Spock's `PollingConditions` — for polling an assertion until it holds From 04a07191eee75192a55c61772bb7575192706579 Mon Sep 17 00:00:00 2001 From: Jordan Wong Date: Mon, 13 Jul 2026 14:39:00 -0400 Subject: [PATCH 5/7] skill(apm-integrations): scrub audit-trail phrasing from shipping content MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Reword "regenerating an existing module" as "rewriting or refactoring" — the guidance applies whenever an existing module is being changed, not just to automated regeneration flows. - Remove the "Rationale traceable to reviewer comments (PR/comment IDs)" footnote. That attribution belongs in commit messages and PR descriptions, not in the shipping skill; readers of the skill just need the rule. --- .../skills/apm-integrations/references/instrumenter-module.md | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/.agents/skills/apm-integrations/references/instrumenter-module.md b/.agents/skills/apm-integrations/references/instrumenter-module.md index 1696a849755..7430fd0973e 100644 --- a/.agents/skills/apm-integrations/references/instrumenter-module.md +++ b/.agents/skills/apm-integrations/references/instrumenter-module.md @@ -90,9 +90,7 @@ Before choosing, run `ls dd-java-agent/instrumentation/$framework/` — the dire Do NOT add version aliases to the decorator's `instrumentationNames()` — that method is for analytics keys only. -_Rationale traceable to reviewer comments: single-name-for-single-module (Valentin Zakharov on PR #11709, comment `3532152120`); version alias when siblings exist (Stuart McCulloch on PR #11760, comment `3552616459`)._ - -**When regenerating an existing module, preserve every override the master version has.** Not just `super(...)` — also `defaultEnabled()`, `helperClassNames()`, `contextStore()`, `orderPriority()`, `muzzleDirective()`, and any other overridden method. Read the current version of the file (on master) before generating; carry each override forward verbatim unless there's a documented reason to change it. Silent loss of `defaultEnabled() = false` (or similar opt-in flags) ships an integration with a different default than users expected. +**When rewriting or refactoring an existing module, preserve every override the master version has.** Not just `super(...)` — also `defaultEnabled()`, `helperClassNames()`, `contextStore()`, `orderPriority()`, `muzzleDirective()`, and any other overridden method. Read the current file on master before writing; carry each override forward verbatim unless there's a documented reason to change it. Silent loss of `defaultEnabled() = false` (or similar opt-in flags) ships an integration with a different default than users expected. ### Do not create a helper class just for CallDepthThreadLocalMap when only one type is instrumented From 23e504f7bf80442ca60639f8889f5e466ddfc189 Mon Sep 17 00:00:00 2001 From: Jordan Wong Date: Tue, 14 Jul 2026 07:42:05 -0400 Subject: [PATCH 6/7] skill(apm-integrations): address second-round Codex review comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings, all P2, all verified against master: - advice-class.md (framework-in-framework): narrow the rule to route-only enrichers (SparkJava). JAX-RS annotations and Ratpack legitimately create handler/controller spans in addition to the outer server span (jax-rs-annotations-2.0/JaxRsAnnotationsInstrumentation.java:128; ratpack-1.5/TracingHandler.java:41). Original blanket wording would cause future JAX-RS/Ratpack instrumentation to drop expected child spans. - advice-class.md (async wrapper): completion-only propagation is not enough when the sync delegate runs on a worker thread — the sync client's advice creates its span BEFORE the future completes. Document the two supported approaches: (1) rely on executor instrumentation, or (2) reactivate around the delegate submission via a shared wrapper. Reference java-concurrent-1.8's existing patterns. - context-tracking.md (CompletableFuture): the CORRECT example was itself wrong on two counts caught by Codex: (a) missing onThrowable = Throwable.class means the exit advice skips when the instrumented method throws before returning the future — any span started on enter would leak (b) used a lambda body, which the "no lambdas in advice methods" rule in advice-class.md:111 explicitly forbids — lambdas compile to synthetic classes that are not helper-injected Rewrite the CORRECT example with @Advice.Thrown handling, a named BiConsumer helper (ClientCompletionCallback) instead of a lambda, and helperClassNames() implications. --- .../references/advice-class.md | 28 +++++++++-- .../references/context-tracking.md | 48 ++++++++++++++++--- 2 files changed, 64 insertions(+), 12 deletions(-) diff --git a/.agents/skills/apm-integrations/references/advice-class.md b/.agents/skills/apm-integrations/references/advice-class.md index 7a3be7386ef..b75cde27623 100644 --- a/.agents/skills/apm-integrations/references/advice-class.md +++ b/.agents/skills/apm-integrations/references/advice-class.md @@ -96,7 +96,15 @@ protected int status(final HttpMethod httpMethod) { If the target method delegates to a sync client that is already instrumented (common in async-wrapper classes like `AsyncFeignClient`, `AsyncHttpClient`, etc.), do NOT open a second span in the async wrapper. The sync client's advice already opens the client span; wrapping again produces two spans per request, with the outer span holding no additional context. -Before adding advice to an async wrapper, trace the call path to the sync delegate. If the delegate is already instrumented for span emission, the async wrapper only needs context-propagation advice (capture on submission, restore on completion) — not a second span. See `context-tracking.md` for the propagation pattern. +Before adding advice to an async wrapper, trace the call path to the sync delegate. If the delegate is already instrumented for span emission, the async wrapper only needs context-propagation advice — not a second span. But note that "context-propagation only" is more nuanced than a completion-callback: + +**If the sync delegate runs on a worker/executor thread** (the common shape for `AsyncXxxClient`), completion-only propagation is insufficient. The sync client's advice creates its HTTP span while the worker executes — BEFORE the future completes — so a "restore context on completion" callback runs too late; the sync span would emit as a root or under the wrong context. The wrapper's propagation advice needs to either: + +1. **Rely on executor instrumentation** — if the worker is scheduled via a `java.util.concurrent.Executor`/`ExecutorService` and the toolkit's `java-concurrent-1.8` module wraps it, context propagates automatically. No additional wrapper advice needed. Verify by reading the wrapper's submission code and confirming the executor is one the toolkit instruments. + +2. **Reactivate around the delegate submission** — wrap the `Runnable`/`Callable` submitted to the worker so it opens a scope with the captured context before invoking the sync call. This is the pattern used by `java-concurrent-1.8`'s wrappers. Do NOT reinvent this per client — factor into a shared helper. + +The completion callback advice (whenComplete-style) is still useful for span-close cleanup on the caller's future, but it does not by itself guarantee the sync client's advice sees the right parent. See `context-tracking.md` for the propagation patterns and the specific `readOnly`/lambda constraints. ## Multiple advice classes and `@AppliesOn` @@ -115,11 +123,21 @@ See the `@AppliesOn Annotation` section of `docs/how_instrumentations_work.md` f - **No `java.util.logging.*`, `java.nio.file.*`, or `javax.management.*`** in bootstrap instrumentations - **Do not extract advice logic into a helper class just to shorten the advice body.** Advice methods are inlined by ByteBuddy; extracting into `SomethingHelper.doTheThing(...)` adds a static-method hop, an extra file, and misleads reviewers into thinking the helper is shared when it is used by exactly one advice. Keep advice inline unless the same logic is genuinely shared across multiple advice classes. When it IS shared, the helper belongs in `helperClassNames()` and named accordingly (e.g. `TracingUtils`, not `FooBarHelper`). The CallDepth helper-class carveout (see `instrumenter-module.md`) is a separate case for multi-type instrumentations. -### Framework-inside-framework: enrich, do not replace, the outer span +### Route-only enrichers: enrich the outer span, do not replace it + +**Scope:** this rule applies specifically to instrumentations that only observe **route matching / dispatch decisions** inside an outer HTTP server — the SparkJava case. It does NOT apply to frameworks that own a handler/controller span in addition to the outer server span. + +**Applies to (route-only enrichers):** +- SparkJava — see `dd-java-agent/instrumentation/spark/sparkjava-2.3/.../RoutesInstrumentation.java` + +**Does NOT apply to (frameworks that own a handler/controller span):** +- JAX-RS annotations — `dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/.../JaxRsAnnotationsInstrumentation.java:128` legitimately calls `startSpan(JAX_RS_CONTROLLER.toString(), ...)` +- Ratpack — `dd-java-agent/instrumentation/ratpack-1.5/.../TracingHandler.java:41` legitimately calls `startSpan("ratpack", ...)` and relies on executor instrumentation to keep the outer Netty span as its parent +- Any other framework that owns a per-handler span (Spring MVC controllers, Vert.x routes, Micronaut route handlers, etc.) -When an inner framework (Spark, Ratpack routes, JAX-RS handlers) runs *inside* an outer HTTP server (Jetty, Netty, Undertow, servlet containers), the outer server's instrumentation already opened the request span (typically `servlet.request` or `jetty-server`). Do NOT create a new `Decorator`, rename the active span, or overwrite its component tag from inside the inner framework's advice. That silently rewrites `servlet.request` → `spark.request` in production traces — a breaking observability change. +**How to tell:** if the framework's expected trace shape has a per-request/per-handler span in addition to the outer HTTP server span, it OWNS that span — do not apply this rule. If the framework only decorates the outer span with a matched-route tag, it is a route-only enricher — apply this rule. -Instead, enrich the active span with the matched route only. `HTTP_RESOURCE_DECORATOR.withRoute(...)` does NOT guard against a null span, so you MUST null-check before calling it — otherwise the advice NPEs when there is no active span (rare but possible under certain execution paths): +**For route-only enrichers only:** the outer server's instrumentation already opened the request span (typically `servlet.request` or `jetty-server`). Do NOT create a new `Decorator`, rename the active span, or overwrite its component tag from inside the route-matcher's advice. Enrich the active span with the matched route only. `HTTP_RESOURCE_DECORATOR.withRoute(...)` does NOT guard against a null span, so you MUST null-check before calling it — otherwise the advice NPEs when there is no active span (rare but possible under certain execution paths): ```java // CORRECT — matches the pattern in dd-java-agent/instrumentation/spark/sparkjava-2.3/.../RoutesInstrumentation.java @@ -129,7 +147,7 @@ if (span != null && routeMatch != null) { } ``` -No `AgentScope`, no `startSpan()`, no `decorator.afterStart()`. This applies to any inner routing/dispatch framework that runs inside another instrumented HTTP server. It does NOT apply to standalone HTTP clients, which own their own span identity. +For route-only enrichers: no `AgentScope`, no `startSpan()`, no `decorator.afterStart()`. This rule does NOT apply to standalone HTTP clients (which own their own span identity) or to handler-owning frameworks like JAX-RS / Ratpack (which legitimately create controller spans). ### Advice classes must not declare non-constant static fields diff --git a/.agents/skills/apm-integrations/references/context-tracking.md b/.agents/skills/apm-integrations/references/context-tracking.md index 5f594233202..d94272beb61 100644 --- a/.agents/skills/apm-integrations/references/context-tracking.md +++ b/.agents/skills/apm-integrations/references/context-tracking.md @@ -65,24 +65,58 @@ Hybrid libraries that BOTH coordinate work AND perform I/O usually get one span- When advice attaches a completion callback to a `CompletableFuture` returned from an async client, do NOT reassign the return with `future = future.whenComplete(...)`. `whenComplete` produces a **dependent stage**; cancelling that stage does not cancel the original request. The caller's `future.cancel(true)` then only cancels the dependent stage and leaves the underlying I/O running. -The correct pattern attaches the callback for side-effects only, without reassigning the return — so `@Advice.Return` does NOT need `readOnly = false`: +The correct pattern attaches the callback for side-effects only, without reassigning the return — so `@Advice.Return` does NOT need `readOnly = false`. It also declares `onThrowable = Throwable.class` so the exit runs even when the instrumented method throws before returning its future (otherwise ByteBuddy skips exit advice on thrown paths and any span/scope started on enter leaks). And per the "no lambdas in advice methods" rule in `advice-class.md`, the completion callback must be a named helper class, not a lambda — lambdas compile to synthetic classes that muzzle does not helper-inject and ByteBuddy cannot resolve at the instrumentation site. ```java -// WRONG — severs cancellation from the caller; also unnecessarily requires readOnly = false +// WRONG — three issues in one: +// (a) reassigning `future = ...` severs cancellation from the caller +// (b) unnecessary readOnly = false +// (c) lambda body compiles to a synthetic class that isn't helper-injected @Advice.OnMethodExit(suppress = Throwable.class) public static void exit(@Advice.Return(readOnly = false) CompletableFuture future, @Advice.Enter AgentSpan span) { future = future.whenComplete((result, error) -> finishSpan(span, result, error)); } -// CORRECT — attach the callback for its side-effect; keep the return read-only -@Advice.OnMethodExit(suppress = Throwable.class) +// CORRECT — attach a named callback for its side-effect; keep the return read-only; +// run on both normal and throwable exit so the caller-thrown case still cleans up. +@Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class) public static void exit(@Advice.Return CompletableFuture future, - @Advice.Enter AgentSpan span) { - future.whenComplete((result, error) -> finishSpan(span, result, error)); + @Advice.Enter AgentSpan span, + @Advice.Thrown Throwable thrown) { + if (thrown != null) { + // The instrumented method threw before returning a future — no future to attach to. + // Finish the span here directly. + ClientCompletionCallback.finishOnThrow(span, thrown); + return; + } + if (future != null) { + future.whenComplete(new ClientCompletionCallback(span)); + } +} +``` + +where `ClientCompletionCallback` is a named `BiConsumer` in a separate helper file listed in `helperClassNames()`: + +```java +public final class ClientCompletionCallback implements BiConsumer { + private final AgentSpan span; + + public ClientCompletionCallback(AgentSpan span) { + this.span = span; + } + + @Override + public void accept(Response result, Throwable error) { + // finish the span with the observed outcome + } + + public static void finishOnThrow(AgentSpan span, Throwable thrown) { + // handle the enter-but-no-future case + } } ``` -Only add `readOnly = false` if you have a documented reason to substitute the return value. If your goal is just to observe completion, the read-only pattern is both safer (preserves cancellation) and simpler. Applies to any async client instrumentation returning a cancellable `CompletionStage`. +Only add `readOnly = false` if you have a documented reason to substitute the return value. If your goal is just to observe completion, the read-only + named-callback pattern is safer (preserves cancellation), obeys the no-lambdas-in-advice rule, and handles the thrown-before-return case. If the wrapper genuinely needs to return a different `CompletionStage` (rare), forward `cancel(...)` to the original future explicitly. From 3c2214d6f2b4babec1c79037f427086d82156720 Mon Sep 17 00:00:00 2001 From: Jordan Wong Date: Wed, 15 Jul 2026 11:10:26 -0400 Subject: [PATCH 7/7] skill(apm-integrations): address mcculls review on instrumenter-module.md --- .../references/instrumenter-module.md | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/.agents/skills/apm-integrations/references/instrumenter-module.md b/.agents/skills/apm-integrations/references/instrumenter-module.md index 7430fd0973e..c6ecda9fd0c 100644 --- a/.agents/skills/apm-integrations/references/instrumenter-module.md +++ b/.agents/skills/apm-integrations/references/instrumenter-module.md @@ -41,12 +41,7 @@ ### Before writing a new module, scan for an existing one -Before creating `dd-java-agent/instrumentation/$framework/$framework-$version/`, list the parent directory `dd-java-agent/instrumentation/$framework/` to see what's already there: - -``` -ls dd-java-agent/instrumentation/$framework/ -# e.g. commons-httpclient-2.0/ (already exists) -``` +Before creating `dd-java-agent/instrumentation/$framework/$framework-$version/`, check whether `dd-java-agent/instrumentation/$framework/` already exists and what's in it. If an existing module covers the same framework at a compatible version, **modify it in place** — do NOT create a parallel `$framework-2.0-generated/` or nested `$framework/$framework-2.0/` copy. Duplicate modules cause muzzle to match twice, double the CI cost, and create reviewer confusion (see PR #10941's "the more I read about it, the less I understand what was done" — a duplicate module that the reviewer could not disentangle from the original). @@ -82,11 +77,11 @@ public OkHttp3Instrumentation() { Users can then set `DD_TRACE_OKHTTP_ENABLED=false` (group off) OR `DD_TRACE_OKHTTP_3_ENABLED=false` (this version only). -**New module you expect will imminently sibling** — add the alias upfront and document why in the commit message. If no sibling appears, drop the alias in a follow-up. +**New module you expect will soon have a sibling** — add the alias upfront and document why in the commit message. If no sibling appears, drop the alias in a follow-up. **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. -Before choosing, run `ls dd-java-agent/instrumentation/$framework/` — the directory contents are the ground truth for whether siblings exist. +Before choosing, list the `dd-java-agent/instrumentation/$framework/` directory — the contents are the ground truth for whether siblings exist. Do NOT add version aliases to the decorator's `instrumentationNames()` — that method is for analytics keys only. @@ -129,7 +124,7 @@ See `docs/how_instrumentations_work.md` section "Grouping Instrumentations" for ## Enrichment helpers must be declared in `helperClassNames()` -If your advice delegates to a helper class (e.g. `SparkJavaRouteEnricher.enrich(...)` from inside `RoutesAdvice`), the helper's fully-qualified class name MUST be listed in `helperClassNames()` on the `InstrumenterModule`: +If your advice delegates to a helper class (e.g. `SparkJavaRouteEnricher.enrich(...)` from inside `RoutesAdvice`), the helper's fully-qualified class name MUST be listed in `helperClassNames()` on the `InstrumenterModule` — unless the helper is supplied on the boot-class-path (e.g. from `agent-bootstrap`), in which case it is already available without injection: ```java @Override