Uh oh!
There was an error while loading. Please reload this page.
skill(apm-integrations): async instrumentation - add rules from review feedback - #11990
Conversation
This comment has been minimized.
This comment has been minimized.
🟢 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. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…Test nuance, find vs ls glob, muzzle fail scope, ContextStore implementation accuracy
…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.
There was a problem hiding this comment.
New rules accurately capture findings from iteration-1 CI failures (reactor-core-3.1 dropped context bridges, rxjava-3.0 dropped version aliases). However, the rules are documented but not wired into SKILL.md steps — code-gen LLMs won't apply them without explicit prompt references. Also, several important existing rules on async HTTP clients and advice-class constraints were removed without explanation, creating a documentation gap.
Future regenerations of reactive modules will likely omit context bridges, drop version aliases, skip muzzle namespace-isolation blocks, and collapse latestDepTest source sets — all regressions documented in this PR.
Assertion details
- Input: Code-gen LLM uses apm-integrations skill to generate a new reactive library instrumentation
- Expected:
LLM should reference and apply the new rules: library-native context maps, namespace-isolation muzzle blocks, latestDepTest source set preservation, super() alias preservation - Actual:
SKILL.md does not reference these new subsections explicitly. LLM reads SKILL.md and references files, but without explicit step-by-step pointers (e.g. 'Step 5.1: Read the new "super() alias" subsection in instrumenter-module.md'), the rules remain optional/advisory. This replicates the iter-2 rxjava-3.0 finding where 5 rule-adherence gaps remained despite rules existing in the skill.
Future developers won't find guidance on async HTTP client pitfalls or understand why certain advice patterns exist in production modules. Code-gen or manual implementations may reinvent wrong patterns (lambdas in advice, reassigning futures, creating duplicate spans).
Assertion details
- Input: Developer reads advice-class.md to understand how to write advice for async HTTP clients or route enrichers
- Expected:
Documentation explains: (1) when NOT to create a second span in async wrappers, (2) when to use executor instrumentation vs reactivate patterns, (3) CompletableFuture cancellation preservation via named callbacks not lambdas, (4) why not to extract advice logic into single-use helpers, (5) route-only enricher patterns for SparkJava-like frameworks - Actual:
All five subsections removed from advice-class.md. Replaced with single 'No inline explanatory comments' rule. No explanation in PR description. Removed sections had concrete examples from feign-11.0, okhttp-3.0, SparkJava.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · Commit 1243a8d · What is Autotest? · Any feedback? Reach out in #autotest
…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.
…Test nuance, find vs ls glob, muzzle fail scope, ContextStore implementation accuracy
1243a8d to
0338411CompareHi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:1243a8d54b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- 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.
mcculls
left a comment
There was a problem hiding this comment.
General comment - a lot of this appears to be helping to minimize changes when rewriting an existing integration. Is all of this text appropriate given most of the time we expect to be writing new integrations?
A lot of it could be replaced by telling the AI to do things like "edit in place"
…w 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>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:22d970969a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…ified 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>| **A context-tracking module for such a library MUST include a `*ContextBridge`-style helper** that: | ||
| 1. Reads a well-known key (Datadog convention: `"dd.span"`) from the library-native context. | ||
| 2. Adapts the retrieved object (which implements `WithAgentSpan` or is an `AgentSpan`) into a `datadog.context.Context`. |
There was a problem hiding this comment.
Note an AgentSpan is already a datadog.context.Context so no cast is necessary.
This is ok for now - I'll rewrite this once all my datadog.context.Context refactorings have landed
| **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. | ||
| **How to detect the pattern in an unfamiliar library:** search the library's public API for a `Context` type with `put`/`get`/`hasKey` methods that user code can write to (as opposed to a Datadog-internal context). If such a type exists, the library has a native context map and needs a `*ContextBridge` helper. If the library instead carries context via a thread-local mechanism (e.g. Kotlin coroutines' `ThreadContextElement`) or a per-request object already used for span ownership elsewhere in the codebase (e.g. Vert.x web's `RoutingContext`), follow that library's existing pattern instead of introducing a `ContextStore`-backed bridge — read the sibling instrumentation module for that library first. |
There was a problem hiding this comment.
Some frameworks work better with the swap approach provided by datadog.context.Context rather than attach/detach or continuations - but again, this is something I'll add in the next week or so
mcculls
left a comment
There was a problem hiding this comment.
LGTM - thanks for the cleanup
I added a small note to remind myself to add a section about datadog.context.Context going forwards, and its use in async frameworks or anything requiring continuations,
PR Shepherd is now watching this PR: fixing basic CI failures, rebasing when it falls behind, and re-queueing after transient merge-queue failures. To disable it, add the If you have any questions, reach the team in #ai-devx-flow. |
/merge |
View all feedbacks in Devflow UI.
The expected merge time in
|
Uh oh!
There was an error while loading. Please reload this page.
5a8dcc6
into
masterUh oh!
There was an error while loading. Please reload this page.
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. See #11991 ([reference]) for the concrete output.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), [reference] rxjava-3.0 iteration-2 regen against skill-async-iteration-2 #11991 (rxjava-3.0 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