From 792d8b7dfb70916b98831a92c610812bdbfb3e4e Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Mon, 25 May 2026 13:07:07 +0200 Subject: [PATCH] docs(instrumentation): add cross-cutting advice patterns reference doc Adds docs/instrumentation/advice-patterns.md covering helperClassNames nested class requirement, muzzleDirective non-null constraint, FieldValue typing, suppress-Throwable loop limitation, Config.get() muzzle failure, reflection cache for version-variant return types, and business logic extraction rule. Adds dd-java-agent/instrumentation/AGENTS.md as a lean index. --- AGENTS.md | 1 + dd-java-agent/instrumentation/AGENTS.md | 40 ++++++ docs/instrumentation/advice-patterns.md | 174 ++++++++++++++++++++++++ 3 files changed, 215 insertions(+) create mode 100644 dd-java-agent/instrumentation/AGENTS.md create mode 100644 docs/instrumentation/advice-patterns.md diff --git a/AGENTS.md b/AGENTS.md index e09dff0d473..99f3a3bd45c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -39,6 +39,7 @@ docs/ Developer documentation (see below) | Working with Gradle | [docs/how_to_work_with_gradle.md](docs/how_to_work_with_gradle.md) | | Bootstrap/premain constraints | [docs/bootstrap_design_guidelines.md](docs/bootstrap_design_guidelines.md) | | CI/CD workflows | [.github/workflows/README.md](.github/workflows/README.md) | +| Instrumentation patterns: helpers, muzzle, FieldValue, reflection cache | [docs/instrumentation/](docs/instrumentation/) | **When working on a topic above, read the linked file first** — they are the source of truth maintained by humans. diff --git a/dd-java-agent/instrumentation/AGENTS.md b/dd-java-agent/instrumentation/AGENTS.md new file mode 100644 index 00000000000..db03e94cf80 --- /dev/null +++ b/dd-java-agent/instrumentation/AGENTS.md @@ -0,0 +1,40 @@ +# Instrumentation — Cross-Cutting Patterns + +Extended reference: [docs/instrumentation/advice-patterns.md](../../docs/instrumentation/advice-patterns.md) + +## `helperClassNames()` — list ALL nested inner classes + +ByteBuddy injects exactly the listed classes. It does NOT auto-discover `$Inner` classes. +Any nested class used at runtime must be listed explicitly. Silent failure: `NoClassDefFoundError` +swallowed by `suppress = Throwable.class` -- data silently discarded. + +## `muzzleDirective()` must never return null in modules with `assertInverse = true` + +A null return causes the new instrumentation to be included in every muzzle test in the module, +including inverse-mode tests where it is expected to fail. CI fails with +"MUZZLE PASSED BUT FAILURE WAS EXPECTED". + +Check the module's `build.gradle` for `assertInverse = true` before adding a new instrumentation. +If present, override `muzzleDirective()` with a non-null value matching the new muzzle block. + +## `@Advice.FieldValue` -- use the most specific stable-descriptor type + +Never use `Object`. Type to the field's declared type or to an interface with stable descriptors +across all muzzle-tested versions. Correct typing gives muzzle validation for free. + +## Business logic belongs in helper classes, not in advice methods + +Advice methods should contain only null/context guards and a single call to a helper or module. +Any branching, data transformation, or object construction must move to a `*Helper` class and +be covered by unit tests directly on the helper. + +## `@RequiresRequestContext` + `Config.get()` = muzzle failure + +Do not initialize `static final` fields from `Config.get()` in any `@RequiresRequestContext` +advice inner class. Move such constants to the helper class in `helperClassNames()`. +See [docs/instrumentation/advice-patterns.md](../../docs/instrumentation/advice-patterns.md#constants-from-configget-must-not-be-in-requiresrequestcontext-advice-inner-classes). + +## `suppress = Throwable.class` does not protect loops inside helpers + +An uncaught exception inside a `for` loop in a helper short-circuits remaining iterations. +Each per-item operation that can fail independently must be wrapped in its own `try/catch`. diff --git a/docs/instrumentation/advice-patterns.md b/docs/instrumentation/advice-patterns.md new file mode 100644 index 00000000000..ec6a272e858 --- /dev/null +++ b/docs/instrumentation/advice-patterns.md @@ -0,0 +1,174 @@ +# Bytecode Advice Patterns — dd-trace-java + +Non-obvious rules for writing `@Advice` instrumentation in dd-trace-java. Based on review +feedback from smola and manuel-alvarez-alvarez across 210+ PRs. + +## `helperClassNames()` must list all nested inner classes + +ByteBuddy injects exactly the classes listed. It does NOT auto-discover nested inner classes of +listed parent classes. Every `$Inner` class used at runtime must be listed explicitly: + +```java +@Override +public String[] helperClassNames() { + return new String[] { + "com.example.MyHelper", + "com.example.MyHelper$InnerClass", // must be explicit + "com.example.MyHelper$Inner$Nested", // and nested-inner too + }; +} +``` + +If a class is omitted, `NoClassDefFoundError` is silently swallowed by +`suppress = Throwable.class` in the advice -- data is lost with no visible error. + +## `muzzleDirective()` must not return null in modules with `assertInverse = true` + +`MuzzleVersionScanPlugin` treats `null` from `muzzleDirective()` as "include in every test": + +```java +// MuzzleVersionScanPlugin.java +if (null == directiveToTest || directiveToTest.equals(muzzleDirective)) { + toBeTested.add(module); +} +``` + +If a module has any muzzle block with `assertInverse = true`, every new instrumentation in that +module MUST override `muzzleDirective()` with a non-null value. Without it, the new +instrumentation is included in the inverse-mode tests where failure is expected -- muzzle passes +instead, CI fails with "MUZZLE PASSED BUT FAILURE WAS EXPECTED". + +## `@Advice.FieldValue` -- type at the most specific stable descriptor + +Type the parameter to the exact declared type of the field, or to an interface/supertype whose +method descriptors are stable across all muzzle-tested versions. Never use `Object`: + +```java +// Wrong -- requires reflection to call methods, no muzzle validation +@Advice.FieldValue("request") Object requestField + +// Wrong -- concrete type with version-varying descriptors causes muzzle failure +@Advice.FieldValue("request") org.apache.catalina.connector.Request requestField + +// Correct -- interface with stable descriptor, muzzle validates the relationship +@Advice.FieldValue("request") org.apache.catalina.Request catRequest +org.apache.catalina.Response cr = catRequest.getResponse(); +``` + +Using an interface type gives muzzle validation for free: if the field stops implementing that +interface in any version, muzzle detects it at build time. + +## `suppress = Throwable.class` does NOT protect loops inside helpers + +`suppress = Throwable.class` on `@Advice.OnMethodExit` protects the advice boundary. An +exception thrown inside a `for` loop in a helper method short-circuits the remaining iterations +silently -- the WAF receives partial data with no visible error. + +Wrap each per-item operation in its own `try/catch`: + +```java +for (Part part : parts) { + try { + processPartSafely(part); + } catch (Exception ignored) { + // malformed part -- continue with remaining parts + } +} +``` + +## Business logic must be extracted to testable helper classes + +For traditional `@Advice` instrumentations (not callsite), advice methods should contain only: +- Null/context guards (`if (span == null) return`) +- A single call to a module or helper + +Any logic beyond this -- data transformations, conditional branches, object construction, +parsing -- must move to a `static final` method in a `*Helper` class and be covered by unit +tests directly on that helper. Integration tests through the instrumentation are not sufficient. + +**Exception**: AppSec blocking logic (`tryCommitBlockingResponse`, `RequestBlockingAction` flows) +is exempt from this rule. It has a dedicated testing pattern and historically causes problems +when extracted. + +## Reflection cache for version-variant return types + +When a method's return type differs between library versions (e.g., `javax.*` vs `jakarta.*`), +a direct bytecode reference fails muzzle for the other version. + +Canonical solution -- cache the `Method` as a `static final` field resolved once in a `static {}` +block. This runs once when the helper class loads in the app classloader, where the library is +present. Zero per-request cost: + +```java +private static final Method GET_HEADERS; + +static { + Method m = null; + try { + m = InputPart.class.getMethod("getHeaders"); + } catch (NoSuchMethodException ignored) { + } + GET_HEADERS = m; +} +``` + +Always add a null guard before invoking: `if (GET_HEADERS == null) return ...`. + +**Why `static final` and not `volatile`:** class initialization is atomic per JVM spec -- the +`static {}` block runs exactly once. `volatile` would be needed only for lazy initialization of +instance or non-static fields. + +## Constants from `Config.get()` must NOT be in `@RequiresRequestContext` advice inner classes + +Muzzle validates `@RequiresRequestContext`-annotated classes against the instrumented library's +classpath -- where `Config` does not exist. This causes `MuzzleValidationException` in CI even +though the code compiles correctly. + +Move such constants to the helper class declared in `helperClassNames()`: + +```java +// Wrong -- in @RequiresRequestContext advice inner class +@RequiresRequestContext(RequestContextSlot.APPSEC) +static class ParseBodyAdvice { + private static final int MAX_FILES = Config.get().getAppSecMaxFileContentCount(); // CI failure +} + +// Correct -- in the helper class +public class NettyMultipartHelper { + static final int MAX_FILES_TO_INSPECT = Config.get().getAppSecMaxFileContentCount(); +} +``` + +## One-shot methods must not be extracted to constants + +`triggerClasses()`, `contextStore()`, `classLoaderMatcher()`, and `methodAdvice()` are called +once by the ByteBuddy framework. Extracting their return values to `static final` constants adds +constant-pool bloat with no benefit. + +## Thread safety in shared fields + +Any field accessed from multiple threads must use a thread-safe type. Common mistakes: + +- `ArrayList` is not thread-safe. Use `CopyOnWriteArrayList` or `Collections.synchronizedList()`. +- Read-then-write on `AtomicXxx` is not atomic. Use `updateAndGet` or `accumulateAndGet` with a + lambda instead of `get()` + `set()`. +- For concurrent maximums: `accumulateAndGet(value, Math.max)`. + +## Logging level + +Internal agent messages that are not actionable by customers must use `DEBUG`, not `WARN` or +`INFO`. Do not log inside loops -- if a limit is exceeded, log once at the boundary. + +## `catch (Exception/Throwable ignored)` requires an explanatory comment + +Every silent catch block must answer two questions inline: +1. What scenario causes this exception? +2. What is the fallback behavior? + +```java +} catch (Exception ignored) { + // malformed Content-Disposition header -- skip part, continue loop +} +``` + +Reviewers (smola, manuel) reject silent catches without explanation.