[reference] eval: blind regeneration of PostgreSQL JDBC driver (toolkit output) - #11997
Draft
jordan-wong wants to merge 3 commits into
Draft
[reference] eval: blind regeneration of PostgreSQL JDBC driver (toolkit output)#11997jordan-wong wants to merge 3 commits into
jordan-wong wants to merge 3 commits into
Conversation
|
🎯 Code Coverage (details) 🔗 Commit SHA: f73161e | Docs | Datadog PR Page | Give us feedback! |
verifyAgentJarIntegrations flagged the new postgresql-42.0 module as
missing from the golden file. Added alphabetically between play-ws and
protobuf, matching PostgreSQLModule's super("postgresql") name.
Not verified locally (Gradle run restricted on this checkout) -- CI
will confirm.
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. |
The advice hooks org.postgresql.jdbc.PgStatement/PgPreparedStatement,
which are structurally unchanged back through at least driver 9.2
(2013) -- much older than the declared floor of 42.0.0. assertInverse
was asserting that all versions outside [42.0.0,) must fail muzzle,
which is false for old-but-compatible releases and caused
muzzle-AssertFail-org.postgresql-postgresql-{9.2,9.3,9.4}* to fail
CI ("unexpectedly passed Muzzle validation").
Root cause is unaddressed here on purpose: the module hooks concrete
driver classes instead of java.sql.* SPI, which is the actual
research finding for this reference PR (R-DB-2, see
docs/eval-research/hypotheses/postgresql.md on the toolkit repo).
This fix only corrects the muzzle version declaration to match
reality -- it does not change what gets hooked.
Verified locally: `./gradlew :dd-java-agent:instrumentation:postgresql:postgresql-42.0:muzzle`
BUILD SUCCESSFUL, all 42.x AssertPass tasks green.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ygree
requested changes
Aug 5, 2026
| @Advice.This final Statement statement, @Advice.Argument(1) final Object query) { | ||
| // In PostgreSQL JDBC 42.0+, argument[1] is CachedQuery, use toString() to get SQL | ||
| if (query != null) { | ||
| PREPARED_SQL.put(statement, query.toString()); |
Contributor
There was a problem hiding this comment.
Should use ContextStore instead of a static shared map.
| if (dbInfo == null) { | ||
| dbInfo = extractDbInfo(statement); | ||
| if (dbInfo != null) { | ||
| InstrumentationContext.get(Statement.class, DBInfo.class).put(statement, dbInfo); |
Contributor
There was a problem hiding this comment.
To avoid the same consecutive call, the ContextStore could be kept in the local variable.
| @Advice.OnMethodEnter(suppress = Throwable.class) | ||
| public static void onEnter( | ||
| @Advice.This final Statement statement, @Advice.Argument(0) final String sql) { | ||
| BATCH_SQL.put(statement, sql); |
Contributor
There was a problem hiding this comment.
Use ContextStore instead of a global map.
gh-worker-dd-mergequeue-cf854d Bot
pushed a commit
that referenced
this pull request
Aug 25, 2026
skill(apm-integrations): sharpen SPI + muzzle rules for database category, add eager-connect idiom From the database eval cycle (reference PRs #11996/#11997/#12032). Most database findings turned out to be adherence gaps against rules that already exist, not missing rules — so this sharpens the existing rules with the concrete failure modes, plus adds one genuinely-new idiom. instrumenter-module.md: - SPI rule: the ForTypeHierarchy exception now explicitly covers being handed a CONCRETE driver that implements a JDK SPI (e.g. org.postgresql.jdbc.PgStatement implements java.sql.Statement), not just interface-only spec jars. The old wording only triggered on "interface-only jar", so an agent given a single concrete driver didn't apply it — the PostgreSQL regen (R-DB-2) fell into exactly this trap and shipped a concrete-class module that also collides at runtime with the existing jdbc/ SPI module. - New: database clients must populate connection metadata eagerly at connect/factory time (JDBC DriverInstrumentation; R2DBC ConnectionFactoryOptions), not lazily per query (R-DB-3). muzzle.md: - assertInverse rule reinforced with the concrete-driver failure mode (R-DB-4): a pinned dependency version is not an API-shape boundary. Draft — will be refined as feedback comes in from the database reference PR reviews. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> skill(apm-integrations): couple module placement to a taken super() name (R-DB-1) Database category gap sweep (2026-08-05) confirmed R-DB-1 is category-wide, not a Cassandra quirk: any library with a version-sibling family directory whose name differs from the integration slug will collide. The existing "modify in place, don't create a parallel module" rule (line 47) doesn't cover the case the Cassandra regen actually hit: - eval slug `cassandra` != family dir `datastax-cassandra/` - surviving siblings (datastax-cassandra-3.0/-3.8) already declare super("cassandra") - under the blind protocol the same-version (4.0) module was DELETED, so "modify it in place" had no target — but the name was still taken The agent created a new top-level instrumentation/cassandra/ module with a duplicate super("cassandra") registration -> silent tracing outage (advice never applied, zero spans, tests timed out, no build error). Fix: grep the tree for the intended super() name BEFORE creating a module; if any module (including untouched version-siblings) holds it, join that family directory rather than minting a new top-level slug. Placement and name are one decision: a taken name dictates the directory. If there is no collision-free home, STOP and surface it. Verified against #12114's existing commit (37e661c): the SPI-collision case (R-DB-2) and eager-connect (R-DB-3) are already covered there; this is the distinct version-sibling-family placement case they don't address. Other sweep findings routed elsewhere (N-DBM-2 silent-downscope -> toolkit prompt; reviewer-check candidates -> toolkit repo), not this skill PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> skill(apm-integrations): cross-ref DB-client design rules to category guide Points instrumenter-module.md at the toolkit categories/database.md decision block (force-read at target-selection) for db.instance sourcing, wrapper pattern, DBM gating, and eager connect metadata (R-DB-3). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> skill(apm-integrations): fix dangling toolkit cross-ref + retain lazy DBInfo fallback (review #12114) Addresses @dougqh's review: the DB-client design-rules bullet pointed at the toolkit file `categories/database.md` "force-read at target-selection" — but that file is not in this repo and "target-selection" is not a named SKILL.md step, so an agent reading the skill hit a dangling pointer. Reworded to: - name the real steps where the rules apply (Step 3 target selection / Step 5 module write); - state plainly that these are the human-readable rules, additionally enforced as force-read toolkit prompt blocks (apm-instrumentation-toolkit#580) when driven by the toolkit — and that the toolkit file is not part of this repo; - fold in the Codex/master-accurate note to retain a lazy parseDBInfo fallback for connect paths the eager Driver.connect hook doesn't cover (DataSource/proxy), not eager-only. No rule content removed; wording only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> skill(apm-integrations): fold 3 bot-review refinements — family-name evidence, concrete-hook scope, R2DBC create() (review #12114) Addresses valid Codex/Datadog-Autotest findings on #12114 (all confirmed against repo facts): - R-DB-1: "taken super() name dictates the directory" was too absolute. Reworded to treat name matches as EVIDENCE, then confirm by coordinates/packages/muzzle. Shared config names (jax-rs across rs/jersey/resteasy; ci-visibility across nine) do NOT identify one family → place by target library; if ambiguous, STOP. Also corrected the mechanism note: equal super() names do not themselves cause a registration outage (InstrumenterIndex indexes by module class); the real harm is a duplicate same-version module (the Cassandra case, unchanged). - R-DB-2: scoped the concrete-driver prohibition to BEHAVIORALLY-REDUNDANT advice. Vendor-only lifecycle/compat hooks not declared on the SPI legitimately need a concrete module (DBMCompatibleConnectionInstrumentation, DB2 JDBC, Tomcat Request.recycle()). Reject a concrete hook only when its method is already advised via the SPI. - R2DBC: corrected the factual claim that ConnectionFactoryOptions is available at ConnectionFactory.create(). create() is zero-arg returning a Publisher; options must be captured earlier at ConnectionFactories.get(options) into a ContextStore<ConnectionFactory,Options> and threaded onto the async-emitted Connection. Wording/accuracy only; no gating rule reversed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> skill(apm-integrations): R-DB-5 must keep the connection/session default, override only when richer (review #12114) Addresses Datadog Autotest :23 (valid): "derive db.instance from the operation not the connection default" read as "drop the default", which would lose db.instance/keyspace for operations with no result metadata (e.g. a Cassandra write). Reworded to the correct two-phase framing — keep the connection/session value as the DEFAULT, override per-operation only when the operation supplies a more specific value (fully-qualified other_ks.table, or keyspace from response ColumnDefinitions). This matches the toolkit R-DB-5 rule (session default + onResponse override), which was already correct; only the skill one-liner was imprecise. The other three Autotest :12/:59/:121 findings this run are re-scans already fixed by the prior two commits (concrete-hook scoping, name-as-evidence, R2DBC create() zero-arg). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> skill(apm-integrations): broaden super() name search + fix Cassandra outage causal claim (review #12114) Two valid Datadog Autotest catches (15:07 run): - :52 — the name-search grep matched only the FIRST super(...) arg, missing later args (super("vertx","vertx-sql-client")) and names held in constants. Broadened to grep all super(...) args and to also scan module classes / name constants when a literal grep misses. - :59 — the Cassandra "Concrete failure" paragraph attributed the tracing outage to two same-name @autoservice registrations, but modules are indexed by class name (equal names are legal). Reframed: the mechanism is a DUPLICATE module advising the same types (mutual suppression via the shared call-depth guard); the shared super("cassandra") name is a symptom, not the cause. Now consistent with the note added earlier. The third finding this run (:23 db.instance keep-the-default) is a stale re-scan already fixed in 04bfe52. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> skill(apm-integrations): make DB rules standalone — remove toolkit references (review #12114) Per architectural rule: the skill must be usable standalone; dependency direction is one-way (toolkit references the skill, never the reverse). The prior wording pointed the reader at the apm-instrumentation-toolkit (categories/database.md, PR #580, "when driven by the toolkit … force-read prompt blocks"). Removed all of it — the database-client design rules now stand on their own with no toolkit dependency. Also dropped the (R-DB-3) eval-bookkeeping tag, which is meaningless to a standalone skill reader. Rule content unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Merge branch 'master' into skill/database-category-rules-20260730 skill(apm-integrations): muzzle result is not proof a matcher target exists (review #12114) Addresses Codex P2 on muzzle.md: the prior text implied a muzzle pass verified the concrete matcher target (PgStatement) exists on old versions. Muzzle derives references from advice bytecode + explicit additional references, NOT from instrumentedType()/named(...) matcher strings, so a matcher-only concrete class is a blind spot. Reworded to: do not infer matcher-target presence/absence from a muzzle result; back version-dependent matcher rules with an explicit muzzle reference or a runtime/latest-dep test. Also folded in the PostgreSQL jdbc2/3/4 vs PgPreparedStatement nuance and dropped the imprecise "unchanged back through 9.2" claim. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> skill(apm-integrations): make the family-placement example repo-generic (standalone) The skill must be readable by any agent working on dd-trace-java integrations, independent of the eval toolkit. Reworded the Cassandra family-placement example to drop eval-specific framing ("regen", "R-DB-1", "the eval was given the slug", "blind protocol") and state it as a plain worked example. Same lesson: a name taken by an existing family means join that family, not create a parallel module; the harm is duplicate advice on the same types. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> 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
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.
🤖 Generated with APM Instrumentation Toolkit
What this is (⚠️ reference PR — NOT intended to be merged as-is)
This is a research reference PR from the 2026-07-19 database category eval cycle. The APM Instrumentation Toolkit was asked to generate a PostgreSQL instrumentation from scratch, blind, against a checkout of
masterwheredd-java-agent/instrumentation/jdbc/had been deleted at theblind_setupstep. This PR is the toolkit's output — specifically what it produces when given ONE JDBC driver instead of the SPI.Please do NOT merge. The toolkit produced a
postgresql/postgresql-42.0/module that hooks concrete PostgreSQL driver classes (org.postgresql.jdbc.PgStatement) rather than thejava.sql.*SPI. That means it would trace PostgreSQL only, missing every other JDBC driver (MySQL, Oracle, SQL Server, DB2, Snowflake, H2, ...). Master's existingjdbc/SPI module remains in place onmasterand this PR does NOT touch it — the two would coexist, with the postgresql module double-instrumenting PostgreSQL. This is the finding, not a fix.The purpose is to give reviewers a concrete artifact to react to: what does the toolkit currently produce when given only one JDBC driver to instrument?
Summary
dd-java-agent/instrumentation/postgresql/postgresql-42.0/targetingorg.postgresql:postgresql:42.0.0.build.gradle+pg_hba.conftest resource; 12 Testcontainers-backed PostgreSQL tests, all passing under the toolkit's internal test loop.PgStatementInstrumentation(targetsorg.postgresql.jdbc.PgStatement) andPgPreparedStatementInstrumentation(targetsorg.postgresql.jdbc.PgPreparedStatement).PostgreSQLModule.contextStore()mapsjava.sql.Statement→DBInfo— the SPI type is used as a cache key, but the actual bytecode advice hooks concrete driver classes.DBInfopopulated lazily on first Statement execute viastatement.getConnection().getMetaData().getURL()— noDriverInstrumentation(contrast with master'sjdbc/DriverInstrumentation.javawhich populatesDBInfoeagerly onDriver.connect).PostgreSQLSQLCommenter.java) generated as a per-driver reimplementation of master'sjdbc/SQLCommenter.java.Known issues to look at first
Concrete driver classes vs
java.sql.*SPI (predicted C-JDBC-1 miss) — the toolkit hookedorg.postgresql.jdbc.PgStatement/PgPreparedStatementinstead ofjava.sql.Statement/PreparedStatement. This ships one integration per driver rather than covering all JDBC drivers with one SPI module. The plan's central hypothesis predicted exactly this outcome. See the cycle report's follow-up section on R-DB-2 (candidate skill rule: SPI-first instrumentation guidance).No
DriverInstrumentationpattern — master'sjdbc/DriverInstrumentation.javahooksDriver.connectto populateDBInfoeagerly. The toolkit chose lazy-on-first-Statement instead. Functionally works, but is an idiomatic mismatch with the dd-trace-java codebase. See R-DB-3 in the cycle report.Reimplements existing SQL commenter feature — master's
jdbc/SQLCommenter.javais generic across all drivers; this PR ships a per-driver PostgreSQL-onlyPostgreSQLSQLCommenter.java. Same DBM feature, per-driver reimplementation.What was measured (C-JDBC-1 through C-JDBC-5)
java.sql.*) vs concrete (org.postgresql.*)DatabaseMetaData.getURL())DriverInstrumentationcapturingDBInfoonDriver.connectDBInfoinInstrumentationContextStatement.classkey rather thanConnection.classagent-bootstrap/Reviewer verdict (toolkit's internal reviewer):
approved=True verdict='approved' todos_fixed=2 todos_remaining=0after 1 review-cycle iteration.Diff-scope note
This PR contains only additions and 3 modifications — the toolkit-generated
postgresql/postgresql-42.0/module plussettings.gradle.kts,metadata/supported-configurations.json, and one test-utility file. Master'sjdbc/module is untouched by this PR.For the research-provenance-complete variant of this experiment (where the blind protocol's 63 deletions of master's
jdbc/are ALSO preserved to reproduce the exact experiment state), the local tageval/postgresql-blind-full-terminal-20260719on the primary checkout retains that full state. It has not been pushed as its own PR.Research provenance
docs/eval-research/cycles/2026-07-19-database-cycle-report.md(toolkit repo)docs/eval-research/hypotheses/postgresql.mddocs/superpowers/plans/2026-06-28-database-category-eval.mdorigin/master@846103dfeb, includes PR skill(apm-integrations): additional rules from recent HTTP-category PR reviews #11927 (084b01b643 skill(apm-integrations): additional rules from recent HTTP-category PR reviews).blind_setupcommit deletedjdbc/(0 files at that tree; parent had 64 files). See cycle report §Blind protocol.Companion PR
Test plan (for reviewers assessing the toolkit output)
The toolkit's internal test loop passed 12 tests against a Testcontainers-backed PostgreSQL. Full CI has not been run yet on this branch — this PR is opened as draft to trigger CI so we can capture:
:check :muzzle :instrumentationLatestDepTestoutcomepostgresql-42.0module conflicts at runtime with master'sjdbc/(both would hook the same PostgreSQL statement execution)The maintainer question this PR is asking is not "should we merge this?" but "should the toolkit's skill teach the agent to prefer the JDK SPI over concrete driver classes?" See R-DB-2 in the cycle report.
Try it out (toolkit)
TOOLKIT_BRANCH=eval/java bash <(gh api 'repos/DataDog/apm-instrumentation-toolkit/contents/bootstrap.sh?ref=eval/java' --jq '.content | @base64d')🤖 Generated with APM Instrumentation Toolkit
CI Triage Status
Last updated: 2026-07-23
Confirmed research findings (do not fix — core instrumentation-logic gap):
test_sql_tracesfailures). The module hooks concreteorg.postgresql.jdbc.PgStatement/PgPreparedStatementviaForSingleTypeinstead ofjava.sql.Statement/PreparedStatementviaForTypeHierarchy. Because master'sjdbc/StatementInstrumentationmatchesimplementsInterface(named("java.sql.Statement"))— which also matches those concrete Pg classes — both modules instrument the same statement execution, and both guard span creation with the same keyCallDepthThreadLocalMap.incrementCallDepth(Statement.class)(jdbcStatementInstrumentation.java:82; this module'sPgStatementAdvice.java:43). Whichever advice's@OnMethodEnterruns first bumps the shared counter; the second seescallDepth > 0and self-suppresses its span. Which one wins depends on instrumentation-application order → intermittent loss of the stored-procedureCALLspan →Test_{Postgres,MySql,MsSql}::test_sql_traces(operation=procedure) fails "Span is not found" on some runs. This reproduces on all 3 CI runs of this PR and never on master (which has no second claimant ofjava.sql.Statement). Correction: an earlier version of this triage called these failures "environmental / infrastructure" — that was WRONG; root cause is now confirmed at the source level as this double-instrumentation collision. Full evidence + investigation trail:docs/eval-research/2026-07-23-system-tests-db-procedure-flake-report.md. Fix = fix R-DB-2 (hook the SPI, don't add a parallel concrete-class module); that removes the collision.dd-gitlab/validate_supported_configurations_v2_local_file— 3 mismatches:DD_TRACE_POSTGRESQL_ENABLED,DD_TRACE_POSTGRESQL_ANALYTICS_ENABLED,DD_TRACE_POSTGRESQL_ANALYTICS_SAMPLE_RATE. Investigated 2026-07-22, not just assumed external: the validator's own error message says "found in the configuration registry but the data found locally does not match" (options A/B/C: match code to registry, add missing registry data, or version a new registry entry). Manually checked https://feature-parity.us1.prod.dog/#/configurations for these 3 keys — none are visible in the registry UI, which is inconsistent with the "found" wording and suggests these are genuinely new config names with no existing registry entry (option C: needs a new registry version created). Our local entries follow the exact convention of master's realDD_TRACE_JDBC_ENABLED(type: boolean,default: true,DD_TRACE_INTEGRATION_*/DD_INTEGRATION_*aliases) — so the local data is not the problem. Neither of us has registry write access to create the new entry ourselves. Confirmed process note, not add-trace-javacode fix — remediation requires someone with feature-parity registry write access.Fixed (scaffolding, root cause is a documented finding):
dd-gitlab/muzzle: [5/8]— 6muzzle-AssertFail-org.postgresql-postgresql-{9.2-1002-jdbc4, 9.2-1004-jdbc41, 9.3-1100-jdbc3, 9.3-1104-jdbc41, 9.4-1200-jdbc4, 9.4.1212.jre7}tasks FAILED: "MUZZLE PASSED PostgreSQLModule BUT FAILURE WAS EXPECTED". The generatedbuild.gradledeclaredversions = "[42.0.0,)"withassertInverse = true, asserting that pre-42.0 driver releases must fail muzzle — butPgStatement/PgPreparedStatementare structurally unchanged back through at least 9.2 (2013), so they pass anyway. Fixed in commitf73161eac7: droppedassertInverse = true. This does NOT touch the hook point (stillorg.postgresql.jdbc.PgStatement/PgPreparedStatement, still the R-DB-2 finding above) — it only corrects a version-range declaration that was factually wrong given what's actually hooked. Verified locally:./gradlew :dd-java-agent:instrumentation:postgresql:postgresql-42.0:muzzle→ BUILD SUCCESSFUL, all 42.x AssertPass tasks green, no AssertFail tasks remain (since assertInverse is what generated them). Underlying finding (concrete-class-vs-SPI) unchanged — see R-DB-2 inhypotheses/postgresql.md.Fixed (config/mechanical):
dd-gitlab/build—verifyAgentJarIntegrationsfailed: "Integration list differs frommetadata/agent-jar-checks.properties... + postgresql". Fixed in commitd6de490ad9: addedpostgresql,\to the golden file (alphabetically betweenplay-wsandprotobuf), matchingPostgreSQLModule'ssuper("postgresql")name. Hand-edited rather than running the Gradle golden-file-update task (Gradle runs restricted on this checkout) — CI confirmed GREEN.Root-caused to R-DB-2 (see "Confirmed research findings" above) — the 7× spring-boot
test_sql_traces(operation=procedure) failures:d6de490ad9andf73161eac7) and why master (no secondStatementclaimant) is always green.UnknownHostException: postgres_dbin the weblog log. That was investigated and ruled out as a red herring — the same singleUnknownHostException(a benign one-time startup retry that succeeds immediately after) appears byte-identically in both passing and failing runs, so it is not the pass/fail differentiator. The differentiator is whether the correctly-taggedpostgresql.query/op=callspan survived the call-depth race: present spans in failing runs are perfectly tagged; only the doubly-instrumentedCALLspan goes missing. Full evidence:docs/eval-research/2026-07-23-system-tests-db-procedure-flake-report.md.Still unclassified: none.