[reference] reactor-core-3.1 iteration-2 regen against skill-async-iteration-2#11974
[reference] reactor-core-3.1 iteration-2 regen against skill-async-iteration-2#11974jordan-wong wants to merge 2 commits into
Conversation
Reference output from the toolkit's automated instrumentation-authoring workflow, run against the iteration-2 skill draft (feat/skill-async-iteration-2, 5 skill commits on top of master). Iteration-2 was scoped to test whether the skill's new library-native context-map guidance closes the primary iteration-1 gap: dropped ReactorContextBridge + 4 supporting classes that broke Spring WebFlux + Spring Kafka reactive. Not intended to be merged as-is. Base: master @ 05671ce (iteration-2 skill under test: de407b5) Cost: \$7.67, wall time ~1h 6m, reviewer approved with 0 todos remaining. Diff scope: 3 net-new Java test files under src/test/java/testdog/trace/instrumentation/reactor/core/. All other generated content (production classes, build.gradle, latestDepTest source set) matches master byte-identically — the skill's Rule #2 preservation strengthenings and library-native context-map prescriptions produced a faithful regeneration of the full 7-class master surface, including ReactorContextBridge and the 4 supporting operator instrumentations that iteration-1 dropped. Score against iteration-1 gaps: all 6 CLOSED (RI-4 package layout, RI-5 enumerate master classes, RI-6 library-native context-map pattern, RI-7 latestDepTest source set, NR-5 test-scope build.gradle deps, NR-6 wrap placement). See docs/eval-research/runs/reactor-core/attempt2/outcome.md in apm-instrumentation-toolkit for the full score table.
🟢 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. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds Java-native reference tests for Reactor Core 3.1 async/context propagation behavior to validate the iteration-2 regeneration rules and ensure parity with master’s existing instrumentation behavior.
Changes:
- Add processor/subscription propagation tests (
SubscriptionTest) - Add end-to-end Reactor span parenting, errors, cancellation, scheduler hops, and regression checks (
ReactorCoreTest) - Add tests for the async-result-extension wrapping path under OTel mode (
ReactorAsyncResultExtensionTest)
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 16 comments.
| File | Description |
|---|---|
| dd-java-agent/instrumentation/reactor-core-3.1/src/test/java/testdog/trace/instrumentation/reactor/core/SubscriptionTest.java | New subscription-time propagation tests across processors/Flux/Mono |
| dd-java-agent/instrumentation/reactor-core-3.1/src/test/java/testdog/trace/instrumentation/reactor/core/ReactorCoreTest.java | New comprehensive Reactor span-parenting and regression coverage in Java |
| dd-java-agent/instrumentation/reactor-core-3.1/src/test/java/testdog/trace/instrumentation/reactor/core/ReactorAsyncResultExtensionTest.java | New async-result-extension tests (success/failure/cancellation) under OTel config |
Comments suppressed due to low confidence (1)
dd-java-agent/instrumentation/reactor-core-3.1/src/test/java/testdog/trace/instrumentation/reactor/core/ReactorAsyncResultExtensionTest.java:1
- Both cancellation tests assert traces immediately after subscribe(), but the span/trace emission may complete asynchronously (especially across scheduler boundaries), making these tests flaky. Add a synchronization point before assertTraces (e.g., wait until the writer has at least 1 trace/span, or block on a latch signaled by the subscriber when cancel/complete is observed).
package testdog.trace.instrumentation.reactor.core;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| CountDownLatch published = new CountDownLatch(consumers); | ||
| CountDownLatch subscribed = new CountDownLatch(consumers); |
| published.await(); | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| } |
| subscribed.await(); | ||
| processor.sink().next(new Connection()); | ||
| published.await(); |
| static int query() { | ||
| AgentSpan span = startSpan("test", "Connection.query"); | ||
| span.finish(); | ||
| return new Random().nextInt(); | ||
| } |
| AgentSpan span = startSpan("test", "publisher-parent"); | ||
| publisherParentId = span.getSpanId(); | ||
| AgentScope scope = activateSpan(span); |
|
|
||
| @ParameterizedTest(name = "Fluxes produce the right number of results on ''{0}'' scheduler") | ||
| @MethodSource("schedulerArgs") | ||
| void schedulers(String schedulerName, Object scheduler) { |
| List<String> values = | ||
| Flux.fromIterable(Arrays.asList(1, 2, 3, 4)) | ||
| .parallel() | ||
| .runOn((reactor.core.scheduler.Scheduler) scheduler) |
| void withSpanAnnotatedAsyncCancelledMono() { | ||
| CountDownLatch latch = new CountDownLatch(1); | ||
| Mono<String> mono = ReactorTracedMethods.traceAsyncMono(latch); |
| latch.countDown(); | ||
| mono.subscribe(new ReactorTracedMethods.CancelSubscriber<String>()); | ||
|
|
||
| assertTraces( | ||
| trace( | ||
| otelSpan("ReactorTracedMethods.traceAsyncMono") | ||
| .tags(defaultTags(), otelComponent(), internalSpanKind()))); |
| void withSpanAnnotatedAsyncCancelledFlux() { | ||
| CountDownLatch latch = new CountDownLatch(1); | ||
| Flux<String> flux = ReactorTracedMethods.traceAsyncFlux(latch); |
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db0d41c88c
ℹ️ 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".
| @@ -0,0 +1,167 @@ | |||
| package testdog.trace.instrumentation.reactor.core; | |||
|
|
|||
| import static datadog.trace.agent.test.assertions.Matchers.validates; | |||
There was a problem hiding this comment.
This static import points at datadog.trace.agent.test.assertions.Matchers, but that package has no Matchers class; the validates helper used by the JUnit assertion APIs lives in datadog.trace.test.junit.utils.assertions.Matchers (as used by the sibling RxJava3 JUnit tests). When :dd-java-agent:instrumentation:reactor-core-3.1:compileTestJava compiles these new tests, javac fails before any Reactor coverage can run.
Useful? React with 👍 / 👎.
| @@ -0,0 +1,671 @@ | |||
| package testdog.trace.instrumentation.reactor.core; | |||
|
|
|||
| import static datadog.trace.agent.test.assertions.Matchers.validates; | |||
There was a problem hiding this comment.
This file repeats the same invalid datadog.trace.agent.test.assertions.Matchers.validates import, so fixing only the async-result test still leaves compileTestJava broken. Import validates from datadog.trace.test.junit.utils.assertions.Matchers instead so this ReactorCoreTest class can compile.
Useful? React with 👍 / 👎.
| import datadog.trace.agent.test.assertions.SpanMatcher; | ||
| import datadog.trace.agent.test.assertions.TagsMatcher; | ||
| import datadog.trace.bootstrap.instrumentation.api.Tags; | ||
| import datadog.trace.junit.utils.config.WithConfig; |
There was a problem hiding this comment.
After the matcher import is fixed, this class still does not compile because there is no datadog.trace.junit.utils.config.WithConfig package in this repo; the JUnit config annotation is under datadog.trace.test.junit.utils.config.WithConfig (the package used by the existing JUnit instrumentation tests). This prevents the new async-result Reactor tests from compiling or applying the required OTel config.
Useful? React with 👍 / 👎.
|
|
||
| @Test | ||
| void broadcastingFluxPropagatesParentSpan() throws InterruptedException { | ||
| Flux<Connection> connection = Flux.<Connection>just(new Connection()).publish().autoConnect(); |
There was a problem hiding this comment.
In this broadcast scenario, replacing the ConnectableFlux with autoConnect() means the Flux.just source connects and emits as soon as the background thread subscribes, while the parent scope is still active. That makes the test pass even if Reactor fails to restore the subscribe-time context for signals published later/outside the subscribing thread; the original shape subscribed under the parent and then connected from the test thread after subscribed.await(), which is the behavior this test needs to exercise.
Useful? React with 👍 / 👎.
![datadog-prod-us1-5[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
The generated Java test suite validates chain-span propagation, scheduling, cancellation, error handling, and @WithSpan async results correctly — all assertion arithmetic and array indexing checks out. The single meaningful gap: ReactorCoreTest.java has zero tests that call subscriberContext(Context.of("dd.span", span)), leaving ReactorContextBridge.explicitContextFromSubscriber() completely unreachable from Java tests despite being the primary feature the PR claims the skill now covers.
📊 Validated against 2 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit b4283de · What is Autotest? · Any feedback? Reach out in #autotest
| // --- windowUntil does not throw NPE on advice ---------------------------- | ||
|
|
||
| @Test | ||
| void windowUntilDoesNotThrowNpe() { |
There was a problem hiding this comment.
The skill's claimed fix for the 'library-native context maps' gap (RI-6, now CLOSED) is unverifiable from the Java test output alone; Spring WebFlux + Spring Kafka reactive integrations rely on ReactorContextBridge and their correctness cannot be asserted by the generated Java suite.
Assertion details
- Input: Any Reactor chain using
.subscriberContext(Context.of("dd.span", span))with a DD/OTel/OpenTracing span to propagate context via ReactorContextBridge - Expected:
Java tests mirror the 3 Groovy scenarios: (1) datadog/opentracing/otel span placed in dd.span key propagates as parent to downstream spans; (2) non-span value in dd.span key is tolerated without exception; (3) span in context is activated at subscribe time - Actual:
Zero Java tests call subscriberContext or reference dd.span. ReactorContextBridge.explicitContextFromSubscriber() is completely unreachable from any Java test. A regression anywhere in the 120-line ReactorContextBridge class would silently pass all Java tests.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · Any feedback? Reach out in #autotest
| trace(span().root().operationName("onNext")), | ||
| trace(span().root().operationName("after")), | ||
| trace(span().root().operationName("finally"))); | ||
| } |
There was a problem hiding this comment.
A regression in OptimizableOperatorInstrumentation's handling of @Trace-annotated Mono sources would pass all Java subscription tests undetected.
Assertion details
- Input: A @Trace-annotated method that returns Mono.from(Mono.just(...)) — the MonoSource path that OptimizableOperatorInstrumentation intercepts
- Expected:
A Java counterpart to the Groovy 'Optimized mono should finish attached spans' test: call the @Trace method under a parent trace and assert the annotation span is correctly closed and parented - Actual:
No Java test covers this code path. The Groovy SubscriptionTest has the test (lines 152-178) but it is absent from the Java SubscriptionTest.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · Any feedback? Reach out in #autotest
[reference] reactor-core-3.1 iteration-2 regen against skill-async-iteration-2
Reference output from the toolkit's automated instrumentation-authoring
workflow, run against the iteration-2 skill draft. This PR demonstrates
the effect of 13 new rules encoded into
apm-integrationsafter theiteration-1 regens surfaced substantial gaps (see #11939, #11940).
Not intended to be merged as-is.
Research context
Iteration-2 of the async research cycle in the apm-instrumentation-toolkit.
Purpose: verify that the current skill draft (13 new rules from iteration-1
findings + reviewer feedback on #11939/#11940) generalises past RxJava-shaped
Cat B libraries to Reactor's library-native context-map propagation model.
Primary hypothesis under test: does the skill's new
context-tracking.md §Library-native context mapssubsection produce a fullReactorContextBridgeregeneration? Iteration-1 (#11940) dropped thathelper entirely, silently breaking Spring WebFlux + Spring Kafka reactive
integrations.
Result: yes, decisively. The regen produced a byte-identical
ReactorContextBridge.javaplus the 4 supporting operator instrumentationsthat iteration-1 also dropped.
Skill under test
feat/skill-async-iteration-2(5 commits on top ofmaster)reviewer-derived rules (NR-1..NR-6)
docs/eval-research/http-findings-2026-07-16/FEEDBACK-CONSOLIDATED-2026-07-16.mdin apm-instrumentation-toolkit
Diff scope
3 net-new Java test files under
src/test/java/testdog/trace/instrumentation/reactor/core/:ReactorAsyncResultExtensionTest.java(167 lines)ReactorCoreTest.java(681 lines)SubscriptionTest.java(324 lines)Everything else the toolkit generated (production classes,
build.gradle,src/latestDepTest/source set) matches master byte-identically. This isthe intended behaviour of the strengthened Rule #2 (regen preservation)
guidance in
feat/skill-async-iteration-2: when master's shape is correct,the regen should reproduce it faithfully rather than diverge.
Changes applied on top of eval output
./gradlew spotlessApply) — the generatedReactorCoreTest.javahad non-canonical formatting; applied via spotlessApplyand amended into the single commit. No functional changes.
Score against iteration-1 gaps
reactor.core.*(dotted) vsreactorcore.*(concatenated)datadog/trace/instrumentation/reactor/core— matches master exactly*Instrumentation.javaclasses droppedReactorCoreModule,ReactorContextBridge,ReactorAsyncResultExtension,BlockingPublisherInstrumentation,ContextWritingSubscriberInstrumentation,CorePublisherInstrumentation,OptimizableOperatorInstrumentationdd.spancontext-map propagation not generatedReactorContextBridge.javaregenerated byte-identical to master (7 references todd.span/reactor.util.context.Context)src/latestDepTest/source set not createdsrc/latestDepTest/{groovy,java}/present with all 3 master tests +annotatedsample/CorePublisherInstrumentation+OptimizableOperatorInstrumentation, not per-operator; usesContextStore<Subscriber, Context>(subscriber-lifecycle)All 6 gaps closed at 55% of iteration-1's cost ($7.67 vs $13).
Where to focus review
The new Java test files at
src/test/java/testdog/trace/instrumentation/reactor/core/are the primary review surface. They exercise:
ReactorCoreTest.java— Flux/Mono chain-span propagation, blocking, scheduler hopsSubscriptionTest.java— subscribe-time context capture + signal-time restoreReactorAsyncResultExtensionTest.java— the async-result-extension wrapping pathCoexisting with master's
src/test/groovy/*.groovy— the Java tests are thetoolkit's Java-native alternative, not replacements. Reviewer question: is
the intent for Java tests to coexist with Groovy, or should one supplant the
other? Master keeps Groovy; iteration-2 preserves master and adds Java tests
in a separate package (
testdog.trace.instrumentation.reactor.core).Pre-review checklist
src/test/java/testdog/trace/instrumentation/reactor/core/, no metadata, no siblings, no.analysis/leakage[reference]prefix, "not intended to merge as-is", attributionCross-references
docs/eval-research/runs/reactor-core/attempt2/outcome.mdin apm-instrumentation-toolkitdocs/eval-research/runs/reactor-core/attempt2/skill-provenance.mddocs/eval-research/http-findings-2026-07-16/FEEDBACK-CONSOLIDATED-2026-07-16.md🤖 Generated with APM Instrumentation Toolkit