diff --git a/.agents/skills/apm-integrations/references/advice-class.md b/.agents/skills/apm-integrations/references/advice-class.md index f0db92887d2..b75cde27623 100644 --- a/.agents/skills/apm-integrations/references/advice-class.md +++ b/.agents/skills/apm-integrations/references/advice-class.md @@ -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. diff --git a/.agents/skills/apm-integrations/references/context-tracking.md b/.agents/skills/apm-integrations/references/context-tracking.md index 9004ee0811f..d94272beb61 100644 --- a/.agents/skills/apm-integrations/references/context-tracking.md +++ b/.agents/skills/apm-integrations/references/context-tracking.md @@ -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 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 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` 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 + 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. diff --git a/.agents/skills/apm-integrations/references/instrumenter-module.md b/.agents/skills/apm-integrations/references/instrumenter-module.md index cb35c7d7cf0..c6ecda9fd0c 100644 --- a/.agents/skills/apm-integrations/references/instrumenter-module.md +++ b/.agents/skills/apm-integrations/references/instrumenter-module.md @@ -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__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___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. diff --git a/.agents/skills/apm-integrations/references/muzzle.md b/.agents/skills/apm-integrations/references/muzzle.md index 1ca23275f03..e1f281630cd 100644 --- a/.agents/skills/apm-integrations/references/muzzle.md +++ b/.agents/skills/apm-integrations/references/muzzle.md @@ -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: diff --git a/.agents/skills/apm-integrations/references/supported-configurations.md b/.agents/skills/apm-integrations/references/supported-configurations.md index 815a672520c..e5a856716a1 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`, `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 b83a6410c4f..2aad929bfe8 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 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 + +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.