rxjava-3.0 async blind regeneration for #11927 - #11939
Open
jordan-wong wants to merge 2 commits into
Open
Conversation
jordan-wong
added
type: feature
Contributor
|
🎯 Code Coverage (details) 🔗 Commit SHA: b4f67bd | 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. |
Automated integration-authoring pass on rxjava-3.0, produced against the draft skill in #11927 (which was in-tree on the target checkout at generation time — using the skill-loading precedence fix from the 2026-07-13 HTTP cycle correction). Reference for #11927 reviewers to see concrete output the current skill draft produces. Not intended to be merged as-is. Base: master @ 05671ce (rebased from original bd134f3; the original branch inherited a stash-merge revert of #11852 during eval branch setup that dropped DD_TRACE_ROBOLECTRIC_ENABLED from metadata plus 172 unrelated files. This rebased version cherry-picks ONLY the rxjava-3.0 module output onto current master, preserving all unrelated upstream changes.) Cost: \$43.23, wall time ~3.5h, reviewer approved with 0 todos remaining. Diff scope: dd-java-agent/instrumentation/rxjava/rxjava-3.0/ only. Metadata: no change (all rxjava3 entries already present on master).
jordan-wong
force-pushed
the
eval/rxjava-3.0-async-blind-regen-20260714
branch
from
July 14, 2026 12:03
bd134f3 to
b46346b
Compare
Contributor
Author
|
Rebased 2026-07-14 — branch force-pushed onto current master to fix the shared Root cause (not a toolkit bug): the original eval branch inherited a stash-merge revert of #11852 during branch setup. The merge commit Fix: cherry-picked only the in-scope Result: 21 files changed (all under Original commit: |
jordan-wong
added
tag: do not merge
mcculls
reviewed
Jul 15, 2026
mcculls
left a comment
mcculls
left a comment
Contributor
There was a problem hiding this comment.
This is remarkably close, if we ignore some of the additional local knowledge / testing
Contributor
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
jordan-wong
changed the title
![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: b46346b45b
ℹ️ 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".
| public final class RxJavaModule extends InstrumenterModule.ContextTracking { | ||
| public RxJavaModule() { | ||
| super("rxjava", "rxjava-3"); | ||
| super("rxjava"); |
There was a problem hiding this comment.
With only super("rxjava"), this module's integration-name list no longer includes rxjava-3, and InstrumenterConfig.isIntegrationEnabled(...) only checks the names supplied by the module. In deployments that use the version-specific opt-out, e.g. DD_TRACE_RXJAVA_3_ENABLED=false, RxJava 3 instrumentation will now remain enabled unless all RxJava instrumentation is disabled via the shared rxjava name, which is a customer-facing config regression for apps that need to disable only RxJava 3.
Useful? React with 👍 / 👎.
![datadog-datadog-prod-us1-2[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
The regen dropped super("rxjava", "rxjava-3") down to super("rxjava"), which silently breaks DD_TRACE_RXJAVA_3_ENABLED=false: InstrumenterConfig.isIntegrationEnabled() iterates only the supplied name list and applies anyEnabled &= configEnabled per entry — without "rxjava-3" in the list, the flag is never consulted and the integration stays enabled. Fix applied (alias restored; muzzle fail block also restored).
📊 Validated against 2 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit b46346b · What is Autotest? · Any feedback? Reach out in #autotest
| public final class RxJavaModule extends InstrumenterModule.ContextTracking { | ||
| public RxJavaModule() { | ||
| super("rxjava", "rxjava-3"); | ||
| super("rxjava"); |
Contributor
There was a problem hiding this comment.
Any customer who disabled rxjava3 instrumentation with DD_TRACE_RXJAVA_3_ENABLED=false will have it involuntarily re-enabled after upgrading to an agent built from this diff.
Assertion details
- Input: Customer sets DD_TRACE_RXJAVA_3_ENABLED=false (maps to system property dd.trace.rxjava-3.enabled=false / config key trace.rxjava-3.enabled) to opt out of rxjava3 instrumentation
- Expected:
InstrumenterConfig.isIntegrationEnabled(["rxjava", "rxjava-3"], true) returns false because the AND-loop evaluates trace.rxjava-3.enabled=false → anyEnabled &= false - Actual:
With super("rxjava") the names list is ["rxjava"] only. The loop never checks trace.rxjava-3.enabled. anyEnabled &= true (rxjava not set) → returns true; the integration stays enabled despite the explicit opt-out.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · Any feedback? Reach out in #autotest
Suggested change
| super("rxjava"); | |
| super("rxjava", "rxjava-3"); |
| group = "io.reactivex.rxjava3" | ||
| module = "rxjava" | ||
| versions = "[3.0.0,)" | ||
| } |
Contributor
There was a problem hiding this comment.
Removes the build-time safety net preventing rxjava3 instrumentation from being applied to rxjava2 classes; namespace-separation bugs can silently reach production.
Assertion details
- Input: A future change accidentally broadens type constraints in a rxjava3 Advice class, making it match io.reactivex.rxjava2.* types
- Expected:
CI muzzle task fails with 'rxjava2-must-not-match' assertion, blocking the merge - Actual:
Without the fail block, muzzle only checks the pass case. The namespace-separation constraint is undocumented and unverified; the accidental match would reach production undetected.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · Any feedback? Reach out in #autotest
ygree
reviewed
Jul 16, 2026
| @Override | ||
| public String[] helperClassNames() { | ||
| return new String[] { | ||
| packageName + ".TracingCompletableObserver", |
Contributor
There was a problem hiding this comment.
It appears to be a rather meaningless reshuffling.
ygree
reviewed
Jul 16, 2026
| store.put("io.reactivex.rxjava3.core.Maybe", contextClass); | ||
| store.put("io.reactivex.rxjava3.core.Observable", contextClass); | ||
| store.put("io.reactivex.rxjava3.core.Single", contextClass); | ||
| store.put("io.reactivex.rxjava3.core.Observable", Context.class.getName()); |
ygree
reviewed
Jul 16, 2026
| if (observer != null) { | ||
| Context parentContext = InstrumentationContext.get(Single.class, Context.class).get(single); | ||
| if (parentContext != null) { | ||
| // wrap the observer so spans from its events treat the captured span as their parent |
Contributor
There was a problem hiding this comment.
I'm not sure if this comment is useful, especially since there is a corresponding Javadoc attached to the TracingSingleObserver class. Comments like this create more noise.
ygree
reviewed
Jul 16, 2026
| class RxJava3ResultExtensionTest extends AbstractInstrumentationTest { | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Successful async completion: span finishes when reactive type completes |
Contributor
There was a problem hiding this comment.
These unattached comments, which are like separators, seem distracting and don't offer much value because their scope is unclear. If it's a group of methods, then extract a separate class with proper Javadoc. Otherwise, why keep it close to the test method?
ygree
reviewed
Jul 16, 2026
| @@ -0,0 +1,190 @@ | |||
| package datadog.trace.instrumentation.rxjava3; | |||
Contributor
There was a problem hiding this comment.
A few test files have been changed, and it's difficult to discern exactly what has changed. I'd expect an explanation from the author about these changes, or for the changes to be restructured so that a clear diff is feasible.
ygree
requested changes
Jul 16, 2026
This was referenced Jul 17, 2026
jordan-wong
added a commit
that referenced
this pull request
Jul 21, 2026
…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
jordan-wong
added a commit
that referenced
this pull request
Jul 21, 2026
…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.
gh-worker-dd-mergequeue-cf854d Bot
pushed a commit
that referenced
this pull request
Jul 27, 2026
…w 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>
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
3 participants
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.
[reference] rxjava-3.0 blind regeneration for #11927
Reference output from the toolkit's automated instrumentation-authoring
workflow, run against the draft skill in #11927. Not intended to be merged
as-is.
Research context
This PR is part of the async-category research cycle: verifying whether
the current skill draft in #11927 generalizes to Category B
(context-propagation) libraries after the HTTP-cycle skill updates. rxjava-3.0
was chosen because it has a mature existing implementation on master,
making regen output directly comparable.
Changes applied on top of eval output
Original eval branch inherited a stash-merge revert of #11852 during eval
branch setup, dropping DD_TRACE_ROBOLECTRIC_ENABLED from metadata plus 172
unrelated files. Root cause was a stale local checkout, not a toolkit or
skill issue. Rebased onto current master by cherry-picking only in-scope
paths under
dd-java-agent/instrumentation/rxjava/rxjava-3.0/.Original:
bd134f34bd→ rebased:b46346b45bWhere to focus review
The reference output should be compared against the master implementation
to surface how well the current skill draft preserves regen-critical
details. Known divergences:
super(...)alias dropped. Master hassuper("rxjava", "rxjava-3");eval produced
super("rxjava"). This silently breaks any customerusing
DD_TRACE_RXJAVA_3_ENABLEDas an opt-out toggle. This is thecanonical Rule First PR - Implements the Opentracing API #2 (regen-preservation) violation the skill should
prevent.
muzzle { fail { ... } }block missing. Master has an explicitfailblock asserting rxjava3 advice must not match against therxjava2 artifact (rxjava3 and rxjava2 use different Java namespaces
but the same Maven module name, so this is a real hazard). Eval
dropped the block entirely.
reactive-streamscompileOnly version regressed from1.0.3to1.0.0. Silent narrowing of the tested surface.Local CI-equivalent (
check+muzzle+latestDepTest) is fullygreen. The one remaining red check on CI (
muzzle: [3/8]) does notreproduce locally and appears to be a CI-side sharding flake.
Base: master @
05671ce3b0