skill(apm-integrations): async cycle iteration-2 — 13 rules from #11939/#11940 review feedback#11990
Draft
jordan-wong wants to merge 5 commits into
Draft
skill(apm-integrations): async cycle iteration-2 — 13 rules from #11939/#11940 review feedback#11990jordan-wong wants to merge 5 commits into
jordan-wong wants to merge 5 commits into
Conversation
…acement 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/
…age, 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
… 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.
…s 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.
…hods 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.
Contributor
|
🎯 Code Coverage (details) 🔗 Commit SHA: de407b5 | Docs | Datadog PR Page | Give us feedback! |
Contributor
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds 13 rules to the apm-integrations skill, driven by two sources:
Iteration-1 async cycle findings — 7 skill prescriptions from the
CI-triage of the async blind regens for rxjava-3.0 ( rxjava-3.0 async blind regeneration for #11927 #11939) and
reactor-core-3.1 ([reference] reactor-core-3.1 async blind regeneration for #11927 #11940). Full inventory:
docs/eval-research/cycles/2026-07-14-async-cycle-report.mdinapm-instrumentation-toolkit.
Reviewer feedback on those two PRs — 6 additional rules from
@ygree, @mcculls, and Codex reviews. Full extraction:
docs/eval-research/http-findings-2026-07-16/FEEDBACK-CONSOLIDATED-2026-07-16.md.Rules added
context-tracking.md(2 new subsections):*ContextBridge-stylehelper preservation for libraries with a first-class Context concept
(Reactor, Kotlin coroutines, JAX-RS, Vert.x). Directly addresses the
primary iteration-1 regression: reactor-core-3.1 dropped
ReactorContextBridge+ 4 supporting classes, silently breaking SpringWebFlux + Spring Kafka reactive.
Subscriber-lifecycle stores, avoid double-wrapping.
instrumenter-module.md(5 new/strengthened subsections):super(...)alias strengthening — concrete rxjava-3.0 example showingDD_TRACE_RXJAVA_3_ENABLEDbreakage when the version alias is dropped.showing graal-native-image cross-module reference breakage.
*Instrumentation.javaclasses on regen —reactor-core-3.1 kept only 2 of 7 master classes.
helperClassNames,contextStore).Class.getName()calls incontextStore().muzzle.md(3 new subsections):failblocks for major-version siblings — rxjava-3.0master has
fail { name = "rxjava2-must-not-match" }; eval dropped it.compileOnlydependency versions on regen — extends existingtestImplementation parity rule.
eval dropped 8 test deps flagged by @mcculls.
tests.md(2 new subsections):latestDepTestsource set.advice-class.md(1 new subsection):Javadoc already covers the intent.
Validation
Iteration-2 blind regens ran against this skill branch. Results:
reactor-core-3.1 iteration-2 — all 6 iteration-1 gaps CLOSED. Regen
produced a byte-identical
ReactorContextBridge.javaplus the 4 supportingoperator instrumentations that iteration-1 dropped. Local
:check :muzzle :latestDepTestgreen. See #11974 ([reference]) for theconcrete output. Cost: $7.67 (down from iteration-1's $13).
rxjava-3.0 iteration-2 — 2 of 7 gaps CLOSED (
super(...)alias fix,helperClassNames ordering). 5 gaps STILL-OPEN as rule-adherence failures
(rule exists in the skill, model did not apply it): muzzle
failblock,reactive-streamsversion parity,contextStorehoist, no inline Advicecomments, no banner test comments. Local
:check :muzzle :latestDepTestgreen. Seedocs/eval-research/runs/rxjava3/attempt2/outcome.mdinapm-instrumentation-toolkit.
The reactor result shows the rules generalize correctly for Reactor-shape
context-propagation. The rxjava3 result surfaces a rule-adherence gap that
should be addressed in a follow-up (candidates:
reviewer_checks/greprules, or moving these rules to more prominent SKILL.md positions).
Cross-references
[reference] reactor-core-3.1 iteration-2 regen against skill-async-iteration-2 #11974 (reactor-core-3.1 iter-2)
docs/eval-research/cycles/2026-07-14-async-cycle-report.mdin apm-instrumentation-toolkit
Not requesting reviewers on open — waiting to sequence after #11927 discussion settles.
🤖 Generated with APM Instrumentation Toolkit