R2DBC net-new instrumentation (io.r2dbc:r2dbc-spi 1.0.0)#12032
R2DBC net-new instrumentation (io.r2dbc:r2dbc-spi 1.0.0)#12032jordan-wong wants to merge 3 commits into
Conversation
Reference PR from toolkit net-new generation (no prior dd-trace-java module exists for R2DBC, so this is not a blind regen). Generated against io.r2dbc:r2dbc-spi:1.0.0.RELEASE. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
jordan-wong
added
tag: ai generated
The advice hooks io.r2dbc.spi.Statement/Connection/Batch interfaces,
which are structurally unchanged across the 0.8.x/0.9.x/1.0.x
releases -- assertInverse was asserting that all versions outside
[1.0.0.RELEASE,) must fail muzzle, which is false for the older-but-
compatible SPI releases and caused
muzzle-AssertFail-io.r2dbc-r2dbc-spi-{0.8.6,0.9.1}.RELEASE to fail
CI ("unexpectedly passed Muzzle validation").
This does not touch the hook point -- R2DBC's core finding (missing
db.name/peer.hostname/db.user/network.destination.port from hooking
ConnectionMetadata instead of ConnectionFactoryOptions, see
docs/eval-research/hypotheses/r2dbc.md on the toolkit repo) is
unrelated and unaddressed here on purpose.
Verified locally: `./gradlew :dd-java-agent:instrumentation:r2dbc:r2dbc-1.0:muzzle`
BUILD SUCCESSFUL.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Integration list differs from metadata/agent-jar-checks.properties --
super("r2dbc") registered in R2dbcInstrumenterModule.java but the
golden file was never updated. Inserted alphabetically between
quartz and ratpack.
Hand-edited rather than running the Gradle golden-file-update task
directly, but verified via `./gradlew :dd-java-agent:verifyAgentJarIntegrations`
BUILD SUCCESSFUL.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
🎯 Code Coverage (details) 🔗 Commit SHA: 368d209 | Docs | Datadog PR Page | Give us feedback! |
🟢 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. |
jordan-wong
changed the title
|
@codex review |
![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: 368d2094ce
ℹ️ 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 void onSubscribe(Subscription s) { | ||
| delegate.onSubscribe(s); |
There was a problem hiding this comment.
When a returned Publisher is cancelled before onComplete/onError (for example via take(1), a timeout, or a disconnected request), this wrapper passes the driver's original Subscription straight through, so TracingSubscriber never observes the cancellation and the span that was already created in execute() can remain open indefinitely. Wrap the Subscription and finish the span on cancel() (with a guard against later terminal signals) so cancelled R2DBC queries do not leak spans.
AGENTS.md reference: AGENTS.md:L79-L81
Useful? React with 👍 / 👎.
| // Connection may be closed or metadata unavailable | ||
| } | ||
| if (INJECT_COMMENT) { | ||
| sql = R2dbcSQLCommenter.inject(sql, null, dbType, null, null, null); |
There was a problem hiding this comment.
When DD_DBM_PROPAGATION_MODE=full, this is the only point where the SQL text is modified, but the traceParent argument is always null because the R2DBC span is not created until execute(); the later execute advice only sets _dd.dbm_trace_injected and cannot update the SQL. The resulting DBM comment has no traceparent, so full-mode DBM cannot correlate the database query with the APM span despite the span being tagged as injected; delay comment injection until a span exists or avoid setting the injected tag unless a traceparent was included.
Useful? React with 👍 / 👎.
| public void onSubscribe(Subscription s) { | ||
| delegate.onSubscribe(s); |
There was a problem hiding this comment.
When a returned Publisher is cancelled before onComplete/onError (for example via take(1), a timeout, or a disconnected request), this wrapper passes the driver's original Subscription straight through, so TracingSubscriber never observes the cancellation and the span that was already created in execute() can remain open indefinitely. Wrap the Subscription and finish the span on cancel() (with a guard against later terminal signals) so cancelled R2DBC queries do not leak spans.
Useful? React with 👍 / 👎.
| public void onComplete() { | ||
| try { | ||
| DECORATE.beforeFinish(span); | ||
| span.finish(); |
There was a problem hiding this comment.
For R2DBC callers that consume the returned Result later (for example Mono.from(statement.execute()).flatMap(result -> result.map(...)) or getRowsUpdated()), the outer Publisher<Result> can complete before the row/update-count Publisher is consumed, but this line finishes the database span at that outer completion. Errors and latency that occur while consuming rows or update counts are then missed; wrap the emitted Result consumption Publishers or otherwise defer finishing until the result stream terminates.
Useful? React with 👍 / 👎.
|
|
||
| String sql = info != null ? info.getSql() : null; | ||
|
|
||
| AgentSpan span = startSpan("r2dbc", R2DBC_QUERY); |
There was a problem hiding this comment.
When clients assemble a reactive pipeline without immediately subscribing (common with Spring/Reactor, or if a branch never subscribes), this starts a span during Statement.execute() even though no database work has begun. That span is attached to the assembly-time context and can stay open forever if the publisher is never subscribed, or record inflated latency if subscribed later; create/decorate the span in the returned publisher's subscribe path instead.
Useful? React with 👍 / 👎.
2 participants
What
Reference PR from the apm-instrumentation-toolkit's net-new generation workflow, targeting
io.r2dbc:r2dbc-spi:1.0.0.RELEASE. No dd-trace-java module exists for R2DBC today, so this was not a blind regen — there was nothing to hide/restore. This measures from-scratch generation quality against an SPI-only Maven coordinate (no concrete driver dependency).Generated module:
dd-java-agent/instrumentation/r2dbc/r2dbc-1.0/Toolkit reviewer verdict:
needs_workThe toolkit's own review_cycle ran 5 iterations and did not reach
approved(todos_remaining=15after iter5, the apparent iteration cap). Total cost: $65.81, duration: ~2h17m.What went right (structural checks all clean):
io.r2dbc.spi.Statement/Connection/Batch), no concrete-driver couplingsuper("r2dbc"), no spurious version aliasWhat's broken — real semantic-completeness gaps (dominant finding):
db.name(REQUIRED tag) is never set.ConnectionInstrumentationonly extracts fromConnectionMetadata, which doesn't expose database/host/port/user. Root cause (per reviewer): should hookConnectionFactory.create()or theConnectionconstructor instead, which exposesConnectionFactoryOptionswith that data.peer.hostname,db.user,network.destination.port(RECOMMENDED tags) — all missing for the same reason.R2dbcConnectionInfohas fields (dbInstance,dbUser,dbHostname) defined but never populated.BatchInstrumentationspans have zero connection metadata tags.db.typevalue between batch ("r2dbc") and statement ("testdb") tests.Minor: 1 inline narrative comment to remove; missing explicit muzzle fail blocks for 0.8.x/0.9.x version families. Originally noted as "not urgent —
assertInverse=truealready isolates," but see CI Triage Status below: thatassertInverse=truewas actually asserting a false compatibility boundary and has been removed as part of a CI fix, so there is now no automated check that pre-1.0 releases are excluded — this remains a real (if low-priority) gap.This PR is left as-is (not iterated further) so the diff reflects exactly what the toolkit produced — do not fix findings on this branch; it's a research artifact, not a merge candidate.
Comparison with OpenTelemetry's R2DBC instrumentation (reference for the correct hook point)
OTel Java solved exactly the gap above by hooking a different SPI entry point. R2DBC exposes connection identity (host/port/db/user) only in
io.r2dbc.spi.ConnectionFactoryOptionsat connection-factory creation — not onConnection/ConnectionMetadata(which exposes onlygetDatabaseProductName()/getDatabaseVersion()). Where you hook decides whether you can populate these tags at all.Connection.createStatement()+connection.getMetadata()(ConnectionInstrumentation.java:30,38,53)ConnectionFactories.find(ConnectionFactoryOptions)(instrumentation/r2dbc-1.0/javaagent/.../R2dbcInstrumentation.java)R2dbcConnectionInfoDbExecutionholding capturedConnectionFactoryOptions(.../library/.../internal/DbExecution.java)db.name/db.namespaceConnectionMetadatalacks itConnectionFactoryOptions.DATABASEpeer.hostname/server.addressConnectionFactoryOptions.HOSTConnectionFactoryOptions.PORTdb.userConnectionFactoryOptions.USERtraceparent(full mode)traceParent=null(ConnectionInstrumentation.java:64)OTel history check: no PR/issue in
opentelemetry-java-instrumentation'sr2dbc-1.0shows these attributes were ever missing-then-added — the factory-level hook is original design, chosen precisely because it's the one place the full connection params are structured data. So this isn't a shared gap; it's a design divergence.Takeaway: the toolkit correctly recognized R2DBC as SPI-shaped and hooked
io.r2dbc.spi.*interfaces — but hooked the wrong SPI entry point for connection identity. The remediation is to captureConnectionFactoryOptionsat factory-create time and thread it forward (as OTel'sDbExecutiondoes), instead of readingConnectionMetadataper statement. Full write-up: toolkit repodocs/eval-research/r2dbc-otel-comparison.md.CI Triage Status
Last updated: 2026-07-22
Confirmed research findings (do not fix — core instrumentation-logic gap):
db.name(REQUIRED tag),peer.hostname/db.user/network.destination.port(RECOMMENDED tags) never populated.ConnectionInstrumentationonly extracts fromConnectionMetadata(product name/version only) instead of hookingConnectionFactory.create()/theConnectionconstructor, which exposesConnectionFactoryOptionswith the actual host/port/db/user data. No CI check catches this — there's no assertion anywhere that these tags must be present — so this finding would otherwise only live in prose. See "What's broken" above for full detail; candidate skill fix is N-DBM-2 (silent-narrowing process gap) in the toolkit'sdocs/eval-research/BACKLOG.md.BatchInstrumentationspans carry zero connection-metadata tags — same root cause as above, compounded (batch path never wires through what statement path partially has).R2dbcConnectionInfofields (dbInstance,dbUser,dbHostname) defined but never populated — same root cause.Fixed (scaffolding, root cause is a documented finding):
dd-gitlab/muzzle: [4/8]—muzzle-AssertFail-io.r2dbc-r2dbc-spi-0.8.6.RELEASEand-0.9.1.RELEASEFAILED: "Instrumentation unexpectedly passed Muzzle validation". The generatedbuild.gradledeclaredversions = "[1.0.0.RELEASE,)"withassertInverse = true, asserting pre-1.0 releases must fail — butio.r2dbc.spi.Statement/Connection/Batchare structurally unchanged across 0.8.x/0.9.x/1.0.x, so they pass anyway. Fixed in commit521b0d52a1: droppedassertInverse = true. Verified locally AND confirmed green in CI (allmuzzle: [*/8]shards SUCCESS post-push). Does not touch the hook point, tags, or integration name — the dominant finding above (missingdb.name/peer.hostname/db.user/etc.) is unrelated and unaddressed here on purpose.Fixed (config/mechanical):
dd-gitlab/build—verifyAgentJarIntegrationsfailed: "Integration list differs frommetadata/agent-jar-checks.properties" —super("r2dbc")registered but the golden file was never updated. Fixed in commit368d2094ce: addedr2dbc,\alphabetically betweenquartzandratpack. Verified locally AND confirmed green in CI post-push.Classified as flake / unrelated:
Still unclassified: none.
Process note (not a code finding):
dd-gitlab/validate_supported_configurations_v2_local_file— Same pattern as PR [reference] eval: blind regeneration of PostgreSQL JDBC driver (toolkit output) #11997's PostgreSQL finding, investigated the same way: manually checked https://feature-parity.us1.prod.dog/#/configurations forDD_TRACE_R2DBC_ENABLED/DD_TRACE_R2DBC_ANALYTICS_ENABLED/DD_TRACE_R2DBC_ANALYTICS_SAMPLE_RATE— none visible in the registry UI, consistent with these being genuinely new config names with no existing registry entry. Our local entries follow the same convention as master's real integrations. Confirmed process note — remediation requires feature-parity registry write access neither of us has, not add-trace-javacommit.🤖 Generated by apm-instrumentation-toolkit. Not a merge candidate — reference/research artifact only.