Skip to content

Commit c0575ac

Browse files
vjfridgevjfridge
committed
Merge branch 'master' into leo.romanovsky/ffl-2693-java-agentless-configuration-source
2 parents 488b477 + 037a02b commit c0575ac

953 files changed

Lines changed: 9034 additions & 4116 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎.agents/skills/apm-integrations/references/advice-class.md‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,20 @@ protected int status(final HttpMethod httpMethod) {
9292

9393
**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.
9494

95+
### Do not double-span async HTTP clients
96+
97+
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.
98+
99+
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:
100+
101+
**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:
102+
103+
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.
104+
105+
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.
106+
107+
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.
108+
95109
## Multiple advice classes and `@AppliesOn`
96110

97111
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
107121
- **No `InstrumentationContext.get()`** outside of Advice code
108122
- **No `inline=false`** in production code (only for debugging; must be removed before committing)
109123
- **No `java.util.logging.*`, `java.nio.file.*`, or `javax.management.*`** in bootstrap instrumentations
124+
- **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.
125+
126+
### Route-only enrichers: enrich the outer span, do not replace it
127+
128+
**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.
129+
130+
**Applies to (route-only enrichers):**
131+
- SparkJava — see `dd-java-agent/instrumentation/spark/sparkjava-2.3/.../RoutesInstrumentation.java`
132+
133+
**Does NOT apply to (frameworks that own a handler/controller span):**
134+
- 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(), ...)`
135+
- 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
136+
- Any other framework that owns a per-handler span (Spring MVC controllers, Vert.x routes, Micronaut route handlers, etc.)
137+
138+
**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.
139+
140+
**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):
141+
142+
```java
143+
// CORRECT — matches the pattern in dd-java-agent/instrumentation/spark/sparkjava-2.3/.../RoutesInstrumentation.java
144+
final AgentSpan span = activeSpan();
145+
if (span != null && routeMatch != null) {
146+
HTTP_RESOURCE_DECORATOR.withRoute(span, method.name(), routeMatch.getMatchUri());
147+
}
148+
```
149+
150+
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).
151+
152+
### Advice classes must not declare non-constant static fields
153+
154+
`*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.

‎.agents/skills/apm-integrations/references/context-tracking.md‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,3 +60,63 @@ When a boundary type exposes multiple overloads of the subscribe / invoke method
6060
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.
6161

6262
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.
63+
64+
## Preserving cancellation on `CompletableFuture` / `CompletionStage` returns
65+
66+
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.
67+
68+
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.
69+
70+
```java
71+
// WRONG — three issues in one:
72+
// (a) reassigning `future = ...` severs cancellation from the caller
73+
// (b) unnecessary readOnly = false
74+
// (c) lambda body compiles to a synthetic class that isn't helper-injected
75+
@Advice.OnMethodExit(suppress = Throwable.class)
76+
public static void exit(@Advice.Return(readOnly = false) CompletableFuture<Response> future,
77+
@Advice.Enter AgentSpan span) {
78+
future = future.whenComplete((result, error) -> finishSpan(span, result, error));
79+
}
80+
81+
// CORRECT — attach a named callback for its side-effect; keep the return read-only;
82+
// run on both normal and throwable exit so the caller-thrown case still cleans up.
83+
@Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class)
84+
public static void exit(@Advice.Return CompletableFuture<Response> future,
85+
@Advice.Enter AgentSpan span,
86+
@Advice.Thrown Throwable thrown) {
87+
if (thrown != null) {
88+
// The instrumented method threw before returning a future — no future to attach to.
89+
// Finish the span here directly.
90+
ClientCompletionCallback.finishOnThrow(span, thrown);
91+
return;
92+
}
93+
if (future != null) {
94+
future.whenComplete(new ClientCompletionCallback(span));
95+
}
96+
}
97+
```
98+
99+
where `ClientCompletionCallback` is a named `BiConsumer<Response, Throwable>` in a separate helper file listed in `helperClassNames()`:
100+
101+
```java
102+
public final class ClientCompletionCallback implements BiConsumer<Response, Throwable> {
103+
private final AgentSpan span;
104+
105+
public ClientCompletionCallback(AgentSpan span) {
106+
this.span = span;
107+
}
108+
109+
@Override
110+
public void accept(Response result, Throwable error) {
111+
// finish the span with the observed outcome
112+
}
113+
114+
public static void finishOnThrow(AgentSpan span, Throwable thrown) {
115+
// handle the enter-but-no-future case
116+
}
117+
}
118+
```
119+
120+
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.
121+
122+
If the wrapper genuinely needs to return a different `CompletionStage` (rare), forward `cancel(...)` to the original future explicitly.

‎.agents/skills/apm-integrations/references/instrumenter-module.md‎

Lines changed: 54 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -39,21 +39,54 @@
3939

4040
✅ `public String[] triggerClasses() { return new String[]{"com.example.Foo"}; }`
4141

42-
### Module constructor: new modules add a version alias; existing modules preserve existing names
42+
### Before writing a new module, scan for an existing one
4343

44-
**New module**: pass a generic name AND a version-qualified alias so users can enable/disable this version independently:
44+
Before creating `dd-java-agent/instrumentation/$framework/$framework-$version/`, check whether `dd-java-agent/instrumentation/$framework/` already exists and what's in it.
45+
46+
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).
47+
48+
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.
49+
50+
### Module constructor: choose names based on sibling structure
51+
52+
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):
53+
54+
**Single module, no version siblings, no imminent sibling planned** — pass ONE name:
55+
56+
```java
57+
// CORRECT — single-module framework (freemarker lives in freemarker-2.3.9/
58+
// and freemarker-2.3.24/ sibling directories yet still passes ONE name because
59+
// the two directories share the same integration name)
60+
public DollarVariableInstrumentation() {
61+
super("freemarker");
62+
}
63+
```
64+
65+
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.
66+
67+
**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.
68+
69+
**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:
4570

4671
```java
47-
// CORRECT — generic + version alias
48-
public JedisInstrumentation() {
49-
super("jedis", "jedis-3.0");
72+
// CORRECT — okhttp has real siblings (okhttp-2.0 and okhttp-3.0)
73+
public OkHttp3Instrumentation() {
74+
super("okhttp", "okhttp-3");
5075
}
5176
```
5277

53-
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.
78+
Users can then set `DD_TRACE_OKHTTP_ENABLED=false` (group off) OR `DD_TRACE_OKHTTP_3_ENABLED=false` (this version only).
79+
80+
**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.
5481

5582
**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.
5683

84+
Before choosing, list the `dd-java-agent/instrumentation/$framework/` directory — the contents are the ground truth for whether siblings exist.
85+
86+
Do NOT add version aliases to the decorator's `instrumentationNames()` — that method is for analytics keys only.
87+
88+
**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.
89+
5790
### Do not create a helper class just for CallDepthThreadLocalMap when only one type is instrumented
5891

5992
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
88121
- Member instrumentations must **not** carry `@AutoService` and must **not** extend `TargetSystem` subclasses
89122

90123
See `docs/how_instrumentations_work.md` section "Grouping Instrumentations" for details.
124+
125+
## Enrichment helpers must be declared in `helperClassNames()`
126+
127+
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:
128+
129+
```java
130+
@Override
131+
public String[] helperClassNames() {
132+
return new String[] {
133+
packageName + ".SparkJavaRouteEnricher",
134+
};
135+
}
136+
```
137+
138+
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.

‎.agents/skills/apm-integrations/references/muzzle.md‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,43 @@ muzzle {
5656
}
5757
```
5858

59+
## Test dependencies should default to the module's declared minimum version
60+
61+
**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.
62+
63+
```groovy
64+
// WRONG — module is jedis-3.0 (min = 3.0.0) but tests run against 4.0 with no justification
65+
muzzle {
66+
pass { group = "redis.clients"; module = "jedis"; versions = "[3.0,)" }
67+
}
68+
dependencies {
69+
testImplementation("redis.clients:jedis:4.0.0") // ← breaks the min-version guarantee
70+
}
71+
72+
// DEFAULT — testImplementation at the declared min; latestDepTestImplementation for the newest
73+
dependencies {
74+
testImplementation("redis.clients:jedis:3.0.0")
75+
latestDepTestImplementation("redis.clients:jedis:+")
76+
}
77+
```
78+
79+
**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:
80+
81+
- Document the reason with a comment at the top of `build.gradle` (a reviewer must be able to see why immediately)
82+
- The `super(...)` alias should reflect the version the code exercises, not the compile-time minimum
83+
84+
Example — `dd-java-agent/instrumentation/spark/sparkjava-2.3/build.gradle`:
85+
86+
```groovy
87+
// building against 2.3 and testing against 2.4 because JettyHandler is available since 2.4 only
88+
dependencies {
89+
compileOnly group: 'com.sparkjava', name: 'spark-core', version: '2.3'
90+
testImplementation group: 'com.sparkjava', name: 'spark-core', version: '2.4'
91+
}
92+
```
93+
94+
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.
95+
5996
## Do NOT use `assertInverse = true` unless the declared min is the actual minimum compatible version
6097

6198
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:

0 commit comments

Comments
 (0)