Skip to content

Commit 1243a8d

Browse files
committed
skill(apm-integrations): address Copilot review on #11990 — latestDepTest nuance, find vs ls glob, muzzle fail scope, ContextStore implementation accuracy
1 parent de407b5 commit 1243a8d

4 files changed

Lines changed: 5 additions & 5 deletions

File tree

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ Context-tracking instrumentations allocate two kinds of long-lived state that ad
8989
Place both at the narrowest scope that preserves correctness:
9090

9191
- **Wrap only at chain boundaries** — subscribe, `.contextWrite(...)`, `.block()` — not at every internal operator. Operator-level wrapping causes N-fold allocations for an N-operator chain and does not add propagation coverage that boundary wrapping doesn't already provide.
92-
- **Use Subscriber-lifecycle context stores** — the store should drop entries when the subscription completes or cancels. `ContextStore` implementations in dd-trace-java are weakly-keyed; make sure the key is the subscriber (which has a bounded lifetime) rather than the publisher (which may be shared and long-lived).
92+
- **Use Subscriber-lifecycle context stores** — the store should drop entries when the subscription completes or cancels. `ContextStore` implementations in dd-trace-java use field injection on the key instance when possible (the common fast path), or a weak-map fallback when field injection is unavailable. In either case, choose a key with a bounded lifetime: use the subscriber (scoped to one subscription) rather than the publisher (which may be shared and long-lived across many subscriptions).
9393
- **Avoid double-wrapping** — when a downstream operator already carries the context via the library-native context map (e.g., Reactor's `Context` flows through the whole operator chain by construction), do not add a per-operator wrap on top.
9494

9595
**Why this matters:** a 10-operator Reactor chain with per-operator wrapping allocates 10× the wrappers of a boundary-only implementation, and every allocation must be reclaimed when the subscription completes. In steady-state reactive services, that's the difference between context-tracking being invisible in profiling and being a measurable overhead.

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ OptimizableOperatorInstrumentation.java
103103

104104
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.
105105

106-
**How to apply this rule:** before generating, run `ls dd-java-agent/instrumentation/<module>/src/main/java/**/` and record every filename. After generating, diff the list of classes in your output against that record. Any master class not present in the output must be explicitly justified in the PR description.
106+
**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.
107107

108108
### Preserve declarative-array ordering (`helperClassNames`, `contextStore` keys)
109109

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@ When in doubt, **search adjacent module build.gradle files for `skipVersions`**
138138

139139
## Namespace-isolation `fail` blocks for major-version siblings
140140

141-
When a library has multiple major versions published under **different** `group:module` coordinates but under the same brand (e.g., `io.reactivex.rxjava2:rxjava` and `io.reactivex.rxjava3:rxjava`), and the two versions cannot share advice (the instrumentation must never resolve against the wrong major), master modules explicitly assert namespace isolation with a `muzzle { fail { ... } }` block.
141+
When a library has multiple major versions that must never be instrumented by the same advice — whether published under different `group:module` coordinates (e.g., `io.reactivex.rxjava2:rxjava` vs `io.reactivex.rxjava3:rxjava`) or under the same coordinates at incompatible major versions (e.g., `org.springframework:spring-webflux` at major 5 vs 6) — master modules explicitly assert namespace isolation with a `muzzle { fail { ... } }` block.
142142

143143
The block is defense-in-depth: it catches accidental cross-version advice matching that would otherwise pass silently. When regenerating a module that has such a block, preserve it verbatim.
144144

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ This ensures `:test` in each module validates that only the correct module fires
8888

8989
When regenerating an existing module, check if master has a `src/latestDepTest/` source set (declared via `addTestSuite('latestDepTest')` or `addTestSuiteForDir('latestDepTest', ...)` in `build.gradle`). If so, preserve that split — do NOT collapse latestDep-specific tests into the base `src/test/` directory.
9090

91-
**Why this matters:** for libraries whose API surface changes across minor versions (Reactor deprecates and removes APIs; Netty changes signatures; gRPC evolves generated code), the base test directory compiles against `testImplementation` (pinned to the module's declared min version) AND against `latestDepTestImplementation` (which resolves to the latest published version). If a test uses an API that was removed after the declared min, putting it in the base directory causes a compile failure in `latestDepTest` even though the test itself is intended to run against the older version.
91+
**Why this matters:** for libraries whose API surface changes across minor versions (Reactor deprecates and removes APIs; Netty changes signatures; gRPC evolves generated code), tests in `src/test/` compile against both `testImplementation` (pinned to min) and `latestDepTestImplementation` (latest) when the module uses `addTestSuiteForDir('latestDepTest', 'test')` — the common pattern that reuses `src/test/` sources for both suites. For modules that use `addTestSuite('latestDepTest')` instead, the `latestDepTest` suite has its own sources at `src/latestDepTest/` and `src/test/` is not compiled against `latestDepTestImplementation`. In both cases: if a test uses an API removed in a later version, it will cause a `latestDepTest` compile failure.
9292

9393
Master's solution: put version-sensitive tests in `src/latestDepTest/` where they only compile against `latestDepTestImplementation` and can freely use the current API. When the latest version removes an API, only the `latestDepTest` copy needs updating.
9494

@@ -100,7 +100,7 @@ Master's solution: put version-sensitive tests in `src/latestDepTest/` where the
100100
- 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.
101101
- 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).
102102

103-
Source: master's `dd-java-agent/instrumentation/reactor-core-3.1/src/latestDepTest/groovy/ReactorCoreTest.groovy`; failure pattern documented in `docs/eval-research/cycles/2026-07-14-async-cycle-report.md` RI-7.
103+
Source: master's `dd-java-agent/instrumentation/reactor-core-3.1/src/latestDepTest/groovy/ReactorCoreTest.groovy`.
104104

105105
## No banner/separator comments in test files
106106

0 commit comments

Comments
 (0)