Commit 5a8dcc6
skill(apm-integrations): async instrumentation - add rules from review feedback (#11990)
skill(apm-integrations): library-native context-map pattern + wrap-placement guidance
Adds two subsections to context-tracking.md driven by reactor-core-3.1 blind
regen findings (dd-trace-java PR #11940):
1. **Library-native context maps — the *ContextBridge pattern** (RI-6).
Prescribes preservation/regeneration of the Reactor-style context-bridge
helper for libraries with a first-class Context concept (Reactor,
Kotlin coroutines, JAX-RS, Vert.x). The subscriber-wrapping pattern
alone is insufficient — regenerating a reactive module without the
bridge silently breaks Spring WebFlux, Spring Kafka reactive, and
related downstream libraries.
2. **Wrap placement and context-store lifecycle** (from @mcculls' review
of PR #11940). Boundary-only wrapping, Subscriber-lifecycle stores,
avoid double-wrapping. Prevents the "increased memory use" pattern
the eval output demonstrated.
Motivating incident: dd-trace-java PR #11940 eval dropped 5 of 7 master
reactor-core-3.1 instrumentation classes including ReactorContextBridge,
which caused spring-messaging-4.0's KafkaBatchListenerCoroutineTest to
time out.
Sources:
- @mcculls on PR #11940 FluxInstrumentation.java:20
- docs/eval-research/cycles/2026-07-14-async-cycle-report.md RI-5, RI-6
- Master reference: dd-java-agent/instrumentation/reactor-core-3.1/
skill(apm-integrations): strengthen Rule #2 for regen — super(), package, classes, ordering
Adds five prescriptions to instrumenter-module.md driven by async cycle
iteration-1 findings (docs/eval-research/cycles/2026-07-14-async-cycle-report.md)
plus @ygree review feedback on rxjava-3.0 PR #11939.
1. **super() alias strengthening (RI-1)** — concrete rxjava-3.0 example
showing DD_TRACE_RXJAVA_3_ENABLED breakage when the version alias is
dropped. Existing "preserve super() verbatim" rule was too abstract.
2. **Package layout preservation on regen (RI-4)** — concrete reactor-core-3.1
example showing graal-native-image cross-module reference breakage when
the eval renamed reactor.core -> reactorcore.
3. **Enumerate all master *Instrumentation.java classes on regen (RI-5)** —
the reactor-core-3.1 regen kept 2 of 7 master classes, silently dropping
ReactorContextBridge and 4 others. Rule prescribes an explicit
pre-generation enumeration + post-generation diff check.
4. **Preserve declarative-array ordering (NR-1)** — @ygree flagged
"meaningless reshuffling" in helperClassNames(). Rule: preserve
master's ordering unless a semantic change requires reordering.
5. **Hoist repeated Class.getName() in contextStore() (NR-2)** — @ygree
flagged five inlined copies of Context.class.getName() as a regression
from master's hoist pattern.
Sources:
- docs/eval-research/cycles/2026-07-14-async-cycle-report.md RI-1, RI-4, RI-5
- @ygree PR #11939 comments 3591691401, 3591695153
skill(apm-integrations): namespace-isolation fail block + dep version parity
Adds three prescriptions to muzzle.md from async cycle iteration-1 findings
and @mcculls review feedback.
1. **Namespace-isolation fail block for major-version siblings (RI-2).**
rxjava-3.0 master has `muzzle { fail { name = "rxjava2-must-not-match" } }`
to assert rxjava3 advice never matches rxjava2 coordinates. Eval dropped
the block. Rule prescribes preservation for any module with a prior-major
sibling module in the repo.
2. **compileOnly dep version parity on regen (RI-3).** rxjava-3.0 master
uses reactive-streams 1.0.3; eval regenerated as 1.0.0, silently
narrowing the tested API surface. Rule extends the existing
testImplementation parity rule to compileOnly.
3. **Test-scope build.gradle dep preservation (NR-5, @mcculls PR #11940).**
reactor-core-3.1 eval dropped 8 testImplementation and latestDepTest
deps that back annotation-driven tests and cross-module interop tests.
Rule prescribes superset-of-master semantics for test deps.
skill(apm-integrations): latestDepTest source set + no banner comments in tests
Adds two prescriptions to tests.md from async cycle iteration-1 findings
and @ygree review feedback.
1. **latestDepTest source-set preservation (RI-7).** reactor-core-3.1
master has src/latestDepTest/groovy/ for version-sensitive tests.
The eval collapsed all tests into src/test/java/, which caused
Schedulers.elastic() (removed in Reactor 3.4+) to break :latestDepTest
compilation across all JVM shards. Rule prescribes preserving the
split when master has it, and using it for libraries with breaking
API changes across recent minor versions.
2. **No banner comments in test files (NR-4).** @ygree flagged
`// --------- Successful completion ---------` style banners in
RxJava3ResultExtensionTest.java as "distracting and don't offer much
value because their scope is unclear." Rule: omit or extract into
separate test classes with focused Javadoc.
skill(apm-integrations): no inline explanatory comments in Advice methods
Adds NR-3 from @ygree review of PR #11939. Advice bodies are typically
short; the wrapper/helper class is where reviewers look to understand
semantics. Duplicating the explanation as a `//`-comment at the advice
call site inflates the diff and drifts out of sync with the wrapper's
Javadoc.
Source: @ygree, PR #11939 comment on SingleInstrumentation.java:55.
skill(apm-integrations): address Copilot review on #11990 — latestDepTest nuance, find vs ls glob, muzzle fail scope, ContextStore implementation accuracy
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.
skill(apm-integrations): condense regen-preservation content, wire new rules into SKILL.md
Feedback from @mcculls on this PR and separately from a senior engineer:
async is a hard area where AI output should be a starting point for
human+AI iteration, not held to a "perfect on first try" bar. Most of
this PR's new content was regen-specific "keep things stable when
rewriting" directives, disproportionate for a skill whose SKILL.md is
otherwise written entirely for the common case (writing a new
integration). 8 of the 12 new sections fell into this category.
Collapsed all 8 into a single "editing an existing module" note per
file (3 files), each a few sentences: read the current file, preserve
what's there unless you have a reason to change it, applies to
super()/package/class-set/ordering/dependencies/test-source-sets alike.
Kept exactly one concrete example (ReactorContextBridge — the
highest-stakes case, since dropping it silently breaks sibling modules
that reference the class by FQN) instead of one narrated failure story
per rule. Net: ~365 lines added by the PR, ~200 removed here.
The 4 sections that are genuine new async-instrumentation content
(library-native context maps / *ContextBridge pattern, wrap-placement
memory considerations, no-inline-advice-comments, no-banner-comments)
are unchanged — those aren't regen-specific, they apply to any reactive
library instrumentation, new or edited.
Also addresses the Datadog Autotest P1 finding (2026-07-21): the new
rules existed in reference files but SKILL.md's per-step pointers were
generic ("read this file") rather than pointing at the specific new
subsections. Added explicit pointers at Steps 4.1, 5, 7, and 9.2/9.3 so
a code-gen LLM sees "read the ContextBridge section" / "add the
namespace-isolation fail block" inline in its step-by-step flow instead
of needing to discover them by skimming a 200+ line reference file.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
heading fix
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
skill(apm-integrations): fix 4 factual errors from Codex review — verified against dd-trace-java source
Codex flagged (2026-07-23) that the previous fix broadening the
ContextBridge rule beyond Reactor invented plausible-sounding but
factually wrong specifics for 3 libraries. Verified each claim against
actual dd-trace-java modules before fixing:
- JAX-RS: SKILL.md listed it as needing a ContextTracking bridge, but
AbstractRequestContextInstrumentation extends InstrumenterModule.Tracing
— it's span-owning code, not context-tracking. Removed the JAX-RS
mention from SKILL.md and context-tracking.md entirely.
- Kotlin coroutines: told agents to key a ContextStore by
Continuation/CoroutineContext, but KotlinCoroutinesModule has no
contextStore() at all — it uses ThreadContextElement
(DatadogThreadContextElement implements ThreadContextElement<Context>).
Corrected to point at the actual pattern.
- Vert.x: told agents to key by io.vertx.core.Context, but that type is
shared across many handler executions on the same Vert.x context —
existing vertx-web instrumentation keys per-request state on
RoutingContext instead. Keying by core Context would cause
cross-request parentage. Corrected to point at RoutingContext.
- Reactor wrap-placement: the "prefer subscriber over publisher key"
guidance implied these were alternatives, but master's
ReactorCoreModule.contextStore() keys BOTH Publisher (via
HandoffContext, read before a subscriber exists) AND Subscriber (via
Context, for the wrapping pattern) — they're complementary, not a
choice. Reworded so the publisher-keyed store is called out as exempt
from the "prefer bounded lifetime" optimization.
Also fixed a latestDepTest gap Codex found: the prior wording said to
put removed-in-latest-version tests in src/test/, but that's only safe
when the module uses addTestSuite('latestDepTest') with separate
sources. Modules using addTestSuiteForDir('latestDepTest', 'test')
reuse src/test/ for both suites, so a removed-API test placed there
still gets compiled against latestDepTestImplementation and fails the
same way. Split the guidance by which build.gradle wiring the module
uses.
Root cause: the earlier fix (responding to a prior Codex comment about
Reactor-only bias) generalized to other libraries from category-level
reasoning ("JAX-RS surely uses a context map like Reactor does") rather
than reading those libraries' actual dd-trace-java modules first. Fixed
per the java-eval-research skill's canonical-first rule: don't publish
a claim about library-specific behavior without checking the source.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Merge branch 'master' into feat/skill-async-iteration-2
Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>1 parent 77f9ba5 commit 5a8dcc6
6 files changed
Lines changed: 170 additions & 8 deletions
File tree
- .agents/skills/apm-integrations
- references
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
60 | 60 | | |
61 | 61 | | |
62 | 62 | | |
| 63 | + | |
| 64 | + | |
63 | 65 | | |
64 | 66 | | |
65 | | - | |
| 67 | + | |
66 | 68 | | |
67 | 69 | | |
68 | 70 | | |
| |||
77 | 79 | | |
78 | 80 | | |
79 | 81 | | |
80 | | - | |
| 82 | + | |
81 | 83 | | |
82 | 84 | | |
83 | 85 | | |
| |||
95 | 97 | | |
96 | 98 | | |
97 | 99 | | |
98 | | - | |
| 100 | + | |
99 | 101 | | |
100 | 102 | | |
101 | 103 | | |
| 104 | + | |
| 105 | + | |
102 | 106 | | |
103 | 107 | | |
104 | 108 | | |
| |||
Lines changed: 33 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
152 | 152 | | |
153 | 153 | | |
154 | 154 | | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
Lines changed: 34 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
55 | 55 | | |
56 | 56 | | |
57 | 57 | | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
58 | 92 | | |
59 | 93 | | |
60 | 94 | | |
| |||
Lines changed: 1 addition & 5 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
79 | 79 | | |
80 | 80 | | |
81 | 81 | | |
82 | | - | |
83 | | - | |
84 | | - | |
| 82 | + | |
85 | 83 | | |
86 | 84 | | |
87 | 85 | | |
88 | | - | |
89 | | - | |
90 | 86 | | |
91 | 87 | | |
92 | 88 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
172 | 172 | | |
173 | 173 | | |
174 | 174 | | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
0 commit comments