Skip to content

Commit 9e8d28b

Browse files
committed
skill(apm-integrations): address Codex review on #11990 — 5 P2 comments
- context-tracking.md: narrow ContextStore key rule — Reactor uses Publisher/Subscriber; JAX-RS uses ContainerRequestContext; Vert.x uses its own Context; coroutines use Continuation/CoroutineContext. Do not force Reactor's key type onto libraries that don't expose Publisher / Subscriber. Also broaden lifecycle-boundary examples per library shape. - tests.md: distinguish latest-only APIs (belong in src/latestDepTest/) from removed-in-latest APIs (belong in src/test/, or use replacement API in latestDepTest). The Reactor Schedulers.elastic() removal is the removed-in-latest case, not the latest-only case. - muzzle.md #1 (fail-block scope): explicitly show same-coordinate cases (jedis/okhttp/jetty-server) alongside different-coordinate cases (rxjava, jms api). The bounded 'versions' range in the fail block is what asserts non-overlap for same-coordinate siblings. - muzzle.md #2 (test-dep preservation): extend the preservation list to cover testRuntimeOnly / latestDepTestRuntimeOnly / forkedTestRuntimeOnly. Runtime-only test deps do NOT trigger compile failures if dropped, so losing them silently removes cross-instrumentation coexistence coverage (e.g. rxjava-3.0's testRuntimeOnly on rxjava-2.0). - instrumenter-module.md: broaden the pre-regen source-file enumeration from `src/main/java` to every production source set: src/main/java17, src/main/java11, src/main/groovy, src/main/scala, src/main/kotlin. Kafka-clients-3.8 and jetty-server-12.0 keep classes under java17; a `find` limited to `src/main/java` misses them. All five findings are P2 severity per Codex classification; each corrects a case where the iter-2 rule was too narrow and could mislead a regen.
1 parent 0338411 commit 9e8d28b

4 files changed

Lines changed: 36 additions & 7 deletions

File tree

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -70,8 +70,8 @@ When a library has a first-class context-map concept, the observer/subscriber wr
7070

7171
1. Reads a well-known key (Datadog convention: `"dd.span"`) from the library-native context.
7272
2. Adapts the retrieved object (which implements `WithAgentSpan` or is an `AgentSpan`) into a `datadog.context.Context`.
73-
3. Stores that context in the toolkit-native `ContextStore` keyed by the library's reactive-streams primitives (`Publisher`, `Subscriber`).
74-
4. Activates the stored context at the right lifecycle boundaries: on subscribe, on signal delivery, on blocking, and on internal subscriber handoff (e.g. Reactor's fused-operator path).
73+
3. Stores that context in the toolkit-native `ContextStore` keyed by the appropriate library-specific lifecycle type. **For Reactor / Reactive-Streams libraries**, the key is `Publisher` or `Subscriber`. **For non-Reactive-Streams libraries**, key by the library's own lifecycle primitive: JAX-RS → `ContainerRequestContext`; Vert.x → `io.vertx.core.Context`; Kotlin coroutines → the `Continuation` or `CoroutineContext` element. Do not force a Reactor-style `Publisher`/`Subscriber` key onto a library that doesn't expose those types.
74+
4. Activates the stored context at the right lifecycle boundaries. For Reactive-Streams: on subscribe, on signal delivery, on blocking, and on internal subscriber handoff (e.g. Reactor's fused-operator path). For other shapes, activate at whatever boundaries the library exposes (request-start / request-end for JAX-RS; coroutine resume/suspend for Kotlin; verticle handler entry for Vert.x).
7575

7676
**Reference:** `dd-java-agent/instrumentation/reactor-core-3.1/` — master has `ReactorContextBridge.java` plus four supporting instrumentations that hook the specific lifecycle points: `BlockingPublisherInstrumentation` (for `.block()` / `.blockFirst()` / `.blockLast()` on the calling thread), `ContextWritingSubscriberInstrumentation` (for `.contextWrite(...)` subscribers at subscribe time), `CorePublisherInstrumentation` (for the base publisher interface handing off to downstream subscribers), and `OptimizableOperatorInstrumentation` (for Reactor's internal fused-operator optimization path). Regenerating this module without preserving these classes silently breaks downstream libraries — Spring WebFlux, Spring Kafka reactive `@KafkaListener suspend fun`, resilience4j-reactor, reactor-netty — that rely on the `dd.span` propagation path.
7777

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

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,22 @@ OptimizableOperatorInstrumentation.java
136136

137137
The eval output kept only 2 (`ReactorCoreModule`, `ReactorAsyncResultExtension`) and added 3 new ones (`FluxInstrumentation`, `MonoInstrumentation`, `TracingCoreSubscriber`). Net effect: 5 master classes silently dropped, including `ReactorContextBridge` — which is what breaks Spring WebFlux, Spring Kafka reactive, and other downstream Reactor-based libraries. No CI check on the target module catches it; the regression only surfaces when sibling-module tests fail.
138138

139-
**How to apply this rule:** before generating, enumerate every `.java` file in the existing module — `find dd-java-agent/instrumentation/<module>/src/main/java -name "*.java"` — and record each filename. After generating, diff that list against the classes in your output. Any master class not present in the output must be explicitly justified in the PR description.
139+
**How to apply this rule:** before generating, enumerate every production source file in the existing module — **not just `src/main/java`**. Several modules keep production classes in version-specific or JVM-specific source sets:
140+
141+
- `src/main/java17` — Java 17-only APIs (`kafka-clients-3.8`, `jetty-server-12.0`)
142+
- `src/main/java11` — Java 11-only APIs (some HTTP-client modules)
143+
- `src/main/groovy` — Groovy production classes (rare, but present in some modules)
144+
- `src/main/scala` — Scala production classes (Play framework, some Akka modules)
145+
146+
A regen that walks only `src/main/java` will silently miss version-specific instrumentation classes and lose them from the output. Use this command instead:
147+
148+
```
149+
find dd-java-agent/instrumentation/<module>/src/main \
150+
\( -name "*.java" -o -name "*.groovy" -o -name "*.scala" -o -name "*.kt" \) \
151+
-type f
152+
```
153+
154+
Record each filename. After generating, diff that list against the classes in your output. Any master class not present in the output must be explicitly justified in the PR description.
140155

141156
### Preserve declarative-array ordering (`helperClassNames`, `contextStore` keys)
142157

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

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -202,9 +202,14 @@ muzzle {
202202

203203
An eval regenerated this WITHOUT the `fail` block. Muzzle would still likely fail naturally on rxjava2 (the FQNs don't exist in that artifact), but the explicit assertion is what catches the failure at CI time with a specific error message rather than a generic muzzle mismatch.
204204

205-
**Rule:** for any module whose brand has a prior major version published under different Maven coordinates in the same repo, check master for a `muzzle { fail { name = "..." } }` block. If present, preserve verbatim on regen. When creating a new module for a library that has a prior-major sibling module in the repo, add such a fail block to assert non-overlap.
205+
**Rule:** for any module whose brand has a prior major version — **whether published under different Maven coordinates or under the same coordinates at an incompatible major** — check master for a `muzzle { fail { name = "..." } }` block. If present, preserve verbatim on regen. When creating a new module for a library that has a prior-major sibling module in the repo, add such a fail block to assert non-overlap.
206206

207-
Common cases where this applies: `rxjava-2.0` ↔ `rxjava-3.0`, `okhttp-2.0` ↔ `okhttp-3.0`, `jedis-1.4` ↔ `jedis-3.0` ↔ `jedis-4.0`, `jetty-server-7.0` ↔ `jetty-server-9.0.4` ↔ `jetty-server-11.0` (etc.).
207+
Common cases where this applies:
208+
209+
- **Different Maven coordinates:** `rxjava-2.0` (`io.reactivex.rxjava2:rxjava`) ↔ `rxjava-3.0` (`io.reactivex.rxjava3:rxjava`); `javax-jms-*` ↔ `jakarta-jms-*`; `spring-webflux-5.0` (`org.springframework:spring-webflux`) ↔ `spring-webflux-6.0` (`org.springframework:spring-webflux`, but Jakarta EE 9+ package rename).
210+
- **Same Maven coordinates, incompatible majors:** `okhttp-2.0` ↔ `okhttp-3.0` (both `com.squareup.okhttp*`, package renamed at v3); `jedis-1.4` ↔ `jedis-3.0` ↔ `jedis-4.0` (all `redis.clients:jedis`, API changed across majors); `jetty-server-7.0` ↔ `jetty-server-9.0.4` ↔ `jetty-server-11.0` (all `org.eclipse.jetty:jetty-server`, `javax.servlet` → `jakarta.servlet` at 11+).
211+
212+
For same-coordinate cases, the `fail` block still applies: it asserts the older major's version range does NOT match the newer module's advice, even though the coordinates are identical. Look for `versions = "[,3.0.0)"` bounded ranges in the master's `fail` block, not a coordinate difference.
208213

209214
## Preserve `compileOnly` dependency versions on regen
210215

@@ -232,7 +237,14 @@ dependencies {
232237

233238
## Preserve test-scope build.gradle dependencies on regen
234239

235-
When regenerating an existing module, preserve every `testImplementation`, `latestDepTestImplementation`, and `forkedTestImplementation` dependency verbatim unless the corresponding test file is also being removed. Do not drop cross-module test dependencies — they back annotation-driven and cross-tracer interop tests that silently fail to compile or run without them.
240+
When regenerating an existing module, preserve every test-scope dependency verbatim — **including runtime-only scopes** — unless the corresponding test file is also being removed. The full set to preserve is:
241+
242+
- `testImplementation`, `latestDepTestImplementation`, `forkedTestImplementation` (compile-scope test deps)
243+
- `testRuntimeOnly`, `latestDepTestRuntimeOnly`, `forkedTestRuntimeOnly` (runtime-only test deps — **these do NOT show up as compile failures if dropped**)
244+
245+
Do not drop cross-module test dependencies — they back annotation-driven, cross-tracer interop, and cross-instrumentation-coexistence tests that silently fail to compile or run without them.
246+
247+
**Why runtime-only scopes matter:** modules like `rxjava-3.0` declare `testRuntimeOnly project(':dd-java-agent:instrumentation:rxjava:rxjava-2.0')` to load the rxjava2 instrumenter into the test JVM. This is what verifies rxjava3 tests are NOT accidentally being served by rxjava2's advice (the mutual-exclusion coverage that pairs with the `muzzle { fail { ... } }` block above). Dropping the `testRuntimeOnly` dep still compiles cleanly — but the test JVM no longer has rxjava2's instrumenter loaded, so the mutual-exclusion coverage silently disappears with no CI signal.
236248

237249
**Concrete failure pattern (from dd-trace-java PR #11940, reactor-core-3.1 regen):** the eval dropped these test dependencies:
238250

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,9 @@ Master's solution: put version-sensitive tests in `src/latestDepTest/` where the
127127
**Rule:**
128128
- Before generating tests, `ls src/latestDepTest/` in master's module. If it exists, the regen must include the equivalent source set.
129129
- If master's `build.gradle` has `addTestSuiteForDir('latestDepTest', ...)` or `addTestSuite('latestDepTest')`, preserve that declaration verbatim.
130-
- When generating tests for a library that has deprecated or removed APIs across recent minor versions, use `latestDepTest/` for tests that exercise those APIs and `test/` for tests that exercise stable APIs.
130+
- **Route each test to the source set whose classpath actually satisfies its imports.** Two distinct scenarios drive the split:
131+
- **API is only in the latest version** (added after the pinned min) → the test uses that API, must go in `src/latestDepTest/`. `src/test/` compiles against `testImplementation` (pinned min), where the API is absent — the test would fail to compile there.
132+
- **API is only in the pinned min** (removed in a later version, e.g. Reactor's `Schedulers.elastic()` removed in 3.4+) → the test uses the removed API, must go in `src/test/` and NOT in `latestDepTest/`. Placing it in `src/latestDepTest/` (where the classpath resolves to the latest release) would produce a compile failure like the concrete failure pattern above. Prefer testing the equivalent replacement API (`Schedulers.boundedElastic()`) in `latestDepTest/` instead.
131133
- Common libraries where this split matters: Reactor (`Schedulers.elastic()` removed in 3.4+), Netty (channel handler API changes across 4.x), gRPC (generated-code shape evolves), Kafka clients (consumer API changed 3.0), Cassandra driver (3.x vs 4.x are largely incompatible).
132134

133135
Source: master's `dd-java-agent/instrumentation/reactor-core-3.1/src/latestDepTest/groovy/ReactorCoreTest.groovy`.

0 commit comments

Comments
 (0)