DataDog · jordan-wong · Jul 10, 2026 · Jul 13, 2026 · Jul 13, 2026 · Jul 13, 2026
@@ -92,6 +92,20 @@ 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 — 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`
 
 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 +121,34 @@ 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.
+
+### 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.)
+
+**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.
+
+**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
+final AgentSpan span = activeSpan();
+if (span != null && routeMatch != null) {
+  HTTP_RESOURCE_DECORATOR.withRoute(span, method.name(), routeMatch.getMatchUri());
+}
+```
+
+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
+
+`*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.
@@ -60,3 +60,63 @@ 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 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`. 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 — 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<Response> future,
+                        @Advice.Enter AgentSpan span) {
+  future = future.whenComplete((result, error) -> finishSpan(span, result, error));
+}
+
+// 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<Response> future,
+                        @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<Response, Throwable>` in a separate helper file listed in `helperClassNames()`:
+
+```java
+public final class ClientCompletionCallback implements BiConsumer<Response, Throwable> {
+  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 + 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.
@@ -39,21 +39,54 @@
 
   ✅ `public String[] triggerClasses() { return new String[]{"com.example.Foo"}; }`
 
-### Module constructor: new modules add a version alias; existing modules preserve existing names
+### Before writing a new module, scan for an existing one
 
-**New module**: pass a generic name AND a version-qualified alias so users can enable/disable this version independently:
+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).
+
+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: choose names based on sibling structure
+
+Each name passed to `super(...)` becomes a distinct `DD_TRACE_<NAME>_ENABLED` flag. Choose the number of names based on whether version-specific siblings exist (or are imminent):
+
+**Single module, no version siblings, no imminent sibling planned** — pass ONE name:
+
+```java
+// 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_<NAME>_<VER>_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:
 
 ```java
-// CORRECT — generic + version alias
-public JedisInstrumentation() {
-    super("jedis", "jedis-3.0");
+// CORRECT — okhttp has real siblings (okhttp-2.0 and okhttp-3.0)
+public OkHttp3Instrumentation() {
+    super("okhttp", "okhttp-3");
 }
 ```
 
-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.
+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 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, 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.
+
+**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
 
 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:
@@ -88,3 +121,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` — 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
+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.
@@ -56,6 +56,43 @@ muzzle {
 }
 ```
 
+## Test dependencies should default to the module's declared minimum version
+
+**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 with no justification
+muzzle {
+  pass { group = "redis.clients"; module = "jedis"; versions = "[3.0,)" }
+}
+dependencies {
+  testImplementation("redis.clients:jedis:4.0.0")   // ← breaks the min-version guarantee
+}
+
+// DEFAULT — testImplementation at the declared min; latestDepTestImplementation for the newest
+dependencies {
+  testImplementation("redis.clients:jedis:3.0.0")
+  latestDepTestImplementation("redis.clients:jedis:+")
+}
+```
+
+**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
 
 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: