Skip to content

[reference] eval: blind regeneration of datastax-cassandra-4.0 (toolkit output) - #11996

Draft
jordan-wong wants to merge 3 commits into
masterfrom
eval/datastax-cassandra-4.0-db-blind-regen-20260719
Draft

[reference] eval: blind regeneration of datastax-cassandra-4.0 (toolkit output)#11996
jordan-wong wants to merge 3 commits into
masterfrom
eval/datastax-cassandra-4.0-db-blind-regen-20260719

Conversation

@jordan-wong

@jordan-wong jordan-wong commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

🤖 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 Cassandra instrumentation from scratch, blind, against a checkout of master where dd-java-agent/instrumentation/datastax-cassandra/datastax-cassandra-4.0/ had been deleted at the blind_setup step. This PR is the toolkit's output.

Please do NOT merge. This PR creates a new module at dd-java-agent/instrumentation/cassandra/cassandra-4.0/ sitting alongside master's existing datastax-cassandra/datastax-cassandra-4.0/. Both register super(\"cassandra\") — a public-config-API name collision. See the "Known issues" section below.

The purpose is to give reviewers a concrete artifact to react to: what does the toolkit currently produce when asked to instrument Cassandra?

Summary

  • New module dd-java-agent/instrumentation/cassandra/ targeting com.datastax.oss:java-driver-core:4.0.0
  • 6 main source files + 1 test + build.gradle; 8 Testcontainers-backed tests, all passing under the toolkit's internal test loop.
  • Chose a novel hook point: com.datastax.oss.driver.internal.core.session.DefaultSession.execute(Request, GenericType) — the single concrete dispatch method through which every CqlSession.execute* variant flows (sync execute, executeAsync). One method-advice covers all variants (see class comment on CqlSessionExecuteInstrumentation.java).
  • Async covered via SpanFinishingCallback on CompletionStage.whenComplete.
  • Peer/host info from ContactPointsUtil.getContactPoints(session).
  • db.statement handles SimpleStatement + BoundStatement.getPreparedStatement().getQuery().

Known issues to look at first

  1. Module-name collisionsuper(\"cassandra\") in the new CassandraClientModule collides with master's datastax-cassandra/datastax-cassandra-4.0/CassandraClientModule which also uses super(\"cassandra\"). The toolkit had the "read the existing module's super(...) and copy it verbatim" rule loaded at generation time (post-skill(apm-integrations): additional rules from recent HTTP-category PR reviews #11927) and violated it. See the cycle report's follow-up section on R-DB-1. Left as generated — see CI Triage Status below for a note on an earlier attempt to work around this via a rename, which was reverted since renaming a super(...) name is itself the kind of integration-identity decision this rule exists to prevent.

  2. Reactive coverage gapCqlSession.executeReactive returns a Publisher<ReactiveRow>. The DefaultSession.execute(Request, GenericType) hook does not intercept the reactive dispatch. No test exercises it. Real coverage gap in a real driver API.

  3. Internal-class target risk — hooking DefaultSession (a driver-internal class) is more fragile than hooking the CqlSession interface. DataStax could rename it between minor versions.

What was measured (C-CASS-1 through C-CASS-4)

# Criterion Result
C-CASS-1 Wrapper (TracingSession) vs individual execute* overloads NEITHER — novel single-dispatch-point pattern
C-CASS-2 sync + async + reactive coverage PARTIAL (async yes; reactive not covered)
C-CASS-3 peer.service from contact points PASS
C-CASS-4 db.statement on prepared statements PASS

Reviewer verdict (toolkit's internal reviewer): approved=True verdict='approved' todos_fixed=9 todos_remaining=0 after 3 review-cycle iterations.

Research provenance

Test plan (for reviewers assessing the toolkit output)

The toolkit's internal test loop passed 8 tests against a Testcontainers-backed Cassandra. 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 :instrumentationLatestDepTest outcome (multi-JVM matrix in CI)
  • Whether the naming collision with master's datastax-cassandra is caught by any lint/registry check

The maintainer question this PR is asking is not "should we merge this?" but "is this the shape we would want the toolkit to produce for Cassandra?"

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-22

Confirmed research findings (do not fix — core instrumentation-logic gap):

  • R-DB-1, module-name collisionsuper("cassandra") collides with master's real datastax-cassandra/datastax-cassandra-4.0/ module, also super("cassandra"). This is the point of the PR (see "Known issues" above) — left as generated, not fixed. Correction (2026-07-22): an earlier pass on this branch (commit 75b06bbe03) renamed this to super("cassandra-toolkit") as a workaround to also satisfy check-instrumentation-naming in one commit. That was a mistake — integration names are public config API, and this codebase has an explicit rule (from PR skill(apm-integrations): additional rules from recent HTTP-category PR reviews #11927, quoted in docs/eval-research/hypotheses/cassandra.md on the toolkit repo) against silently renaming them; prior research cycles (HTTP, async) handled analogous name matches by documenting the collision, not inventing a workaround name. Reverted in commit 587b271765super("cassandra") restored, collision is back and documented here as unfixed, consistent with how R-DB-1 should be handled.
  • Datadog PR Gates / No new flaky tests — FAILED, flagging test fingerprints tied to first_flaked_branch:"eval/datastax-cassandra-4.0-db-blind-regen-20260719". Almost certainly CassandraClientTest's tests (the only new tests this branch introduces) showing intermittent behavior against a real Testcontainers-backed Cassandra instance. Classified (b) — not scaffolding, would require investigating/fixing actual test timing/isolation behavior, and the flakiness is itself informative about the toolkit's generated-test quality. Left as-is; persisted across multiple full CI re-runs (stable finding, not itself a transient artifact — see below for how this differs from test_inst_latest, which WAS transient).

Fixed (scaffolding, root cause is a documented finding):

  • dd-gitlab/check-instrumentation-naming — naming linter rejected the module: "Module name 'cassandra' must end with a version (e.g., '2.0', '3.1.0') or one of: '-common', '-stubs', '-iast'". Fixed in commit 75b06bbe03: moved dd-java-agent/instrumentation/cassandra/cassandra/cassandra-4.0/, updated settings.gradle.kts. This part of the fix stands — pure directory/build-config move, no integration-identity implications. (The same commit also renamed super(...), which was reverted — see the R-DB-1 entry above.) Confirmed SUCCESS and stable across the post-revert re-run.
  • dd-gitlab/config-inversion-linter — appeared transiently after the super("cassandra-toolkit") rename (missing registry entry for the new name), and disappeared once that rename was reverted. Confirmed SUCCESS and stable post-revert — was purely a side-effect of the reverted rename, not an independent finding.

Classified as flake / unrelated:

  • main / End-to-end #10 / akka-http 10 — original failure was in tests/appsec/test_blocking_addresses.py::Test_Blocking_request_body_filenames::test_blocking, ValueError: No appsec event validate this condition. AppSec/WAF test with no code path through Cassandra instrumentation or any file touched by this PR. Confirmed transient: passed on the subsequent full CI re-run (13/13 akka-http shards SUCCESS). No fix needed.
  • dd-gitlab/test_inst_latest: [21, 4/6]Correction (2026-07-22): originally documented as a "genuine latestDepTest failure" (CassandraClientTest > peerServiceInputTagsSetWithKeyspace()/peerServiceCleanup() failing with DriverTimeoutException/InvalidKeyspaceException against the latest 4.x driver). Re-triage after CI fully settled post-revert: this shard passed cleanly on the re-run with no code changes to the module in between. Reclassifying as (c) flake — the original failure was Testcontainers timing sensitivity, not a deterministic driver-compatibility regression. (Distinct from the persistent Datadog PR Gates flaky-test finding above, which has recurred across multiple runs rather than resolving on retry.)

Downstream aggregates (no independent signal):

  • Check system tests success, dd-gitlab/default-pipeline — both fail only because other checks fail at the time of evaluation; no separate action. Currently passing since all other checks except No new flaky tests are green.

Still unclassified: none.

CI status as of 2026-07-22 (fully settled, 584/585 checks, no pending/in-progress): Only 1 failure remains: Datadog PR Gates / No new flaky tests, a stable (b) finding. All other checks are green, including two that were previously red and have since resolved on retry (test_inst_latest: [21,4/6], akka-http). Per Phase 9e, this PR is now maximally settled as a reference artifact — the remaining red check IS the signal.

@jordan-wong jordan-wong added tag: apm integration toolkit Changes generated by DataDog/apm-instrumentation-toolkit as part of IDM AIT experimentation ai-generated labels Jul 20, 2026
@datadog-prod-us1-3

datadog-prod-us1-3 Bot commented Jul 20, 2026

Copy link
Copy Markdown

🎯 Code Coverage (details)
Patch Coverage: 100.00%
Overall Coverage: 57.27% (-0.03%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 587b271 | Docs | Datadog PR Page | Give us feedback!

@dd-octo-sts

dd-octo-sts Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

🟢 Java Benchmark SLOs — All performance SLOs passed

Suite Status
Startup 🟢 pass

SLO thresholds are defined here based on automatically generated metrics. A warning is raised when results are within 5% of the threshold.

PR vs. master results
Scenario Candidate master Δ (95% CI of mean)
startup:insecure-bank:iast:Agent 13.95 s 13.90 s [-0.3%; +1.1%] (no difference)
startup:insecure-bank:tracing:Agent 12.94 s 13.02 s [-1.6%; +0.4%] (no difference)
startup:petclinic:appsec:Agent 16.95 s 16.81 s [-0.2%; +1.8%] (no difference)
startup:petclinic:iast:Agent 17.00 s 16.54 s [-1.5%; +7.1%] (no difference)
startup:petclinic:profiling:Agent 16.90 s 16.81 s [-0.7%; +1.8%] (no difference)
startup:petclinic:sca:Agent 16.90 s 16.85 s [-0.6%; +1.2%] (no difference)
startup:petclinic:tracing:Agent 16.06 s 15.71 s [-2.1%; +6.6%] (no difference)

Commit: 587b2717 · CI Pipeline · Benchmarking Platform UI


Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion.

@PerfectSlayer PerfectSlayer added tag: ai generated Largely based on code generated by an AI or LLM and removed ai-generated labels Jul 20, 2026
jordan-wong and others added 2 commits July 22, 2026 18:20
Two related problems, fixed together since fixing the naming
violation alone would have shipped a real double-instrumentation bug:

1. dd-gitlab/check-instrumentation-naming FAILED: module directory
   `cassandra/` has no version suffix. Moved to
   `cassandra/cassandra-4.0/` per the naming convention (see
   datastax-cassandra/datastax-cassandra-4.0/ for the established
   pattern), updated settings.gradle.kts accordingly.

2. Not previously caught by any CI check, found via direct inspection
   (R-DB-1): CassandraClientModule registered super("cassandra"),
   identical to master's real datastax-cassandra-4.0 module's
   CassandraClientInstrumentation super("cassandra") -- both would
   register and instrument Cassandra client execution simultaneously.
   Renamed to super("cassandra-toolkit") to remove the collision.
   Added cassandra-toolkit to metadata/agent-jar-checks.properties.

This module remains a research reference artifact, not a real
proposal -- master's datastax-cassandra/ is untouched and remains
the canonical Cassandra integration. See docs/eval-research/hypotheses/cassandra.md
(apm-instrumentation-toolkit repo) for R-DB-1, and the parallel-module
rule-adherence gap it documents (the toolkit did not check for an
existing master module before generating this one).

Verified locally:
- `./gradlew :dd-java-agent:instrumentation:checkInstrumentationNaming` -- BUILD SUCCESSFUL
- `./gradlew :dd-java-agent:verifyAgentJarIntegrations` -- BUILD SUCCESSFUL
- `./gradlew :dd-java-agent:instrumentation:cassandra:cassandra-4.0:compileJava :dd-java-agent:instrumentation:cassandra:cassandra-4.0:compileTestJava` -- BUILD SUCCESSFUL

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…fix only

Reverts part of 75b06bb. Renaming super("cassandra") ->
super("cassandra-toolkit") to unblock check-instrumentation-naming
was wrong: integration names are public config API, and this
codebase already has a documented rule against silently renaming
them (see docs/eval-research/hypotheses/cassandra.md on the toolkit
repo, quoting PR #11927: "read the existing module's super(...) and
copy it verbatim... renaming one silently breaks customer
DD_TRACE_*_ENABLED settings"). Also inconsistent with how prior
research cycles (HTTP, async) handled analogous name matches --
match the real name, document the collision as the finding, don't
invent a workaround name to unblock CI.

The directory move (cassandra/ -> cassandra/cassandra-4.0/) stays --
that's a pure naming-convention fix with no integration-identity
implications. The super("cassandra") collision with master's real
datastax-cassandra/datastax-cassandra-4.0/ module is restored and
remains the R-DB-1 research finding, unfixed, same as before this
branch's CI-triage pass touched it.

Verified locally:
- `./gradlew :dd-java-agent:instrumentation:checkInstrumentationNaming` -- BUILD SUCCESSFUL
- `./gradlew :dd-java-agent:verifyAgentJarIntegrations` -- BUILD SUCCESSFUL

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@ValentinZakharov

Copy link
Copy Markdown
Contributor

The toolkit uses a different architectural pattern from the one used on the master branch.

On master, the session is wrapped:
CompletionStage<Session> -> CompletionStage<TracingSession>
In this PR, the toolkit instruments the execute() method instead

Both approaches are valid, but the wrapper approach is preferable for long-lived objects because:

  • TracingSession provides session life cycle control
  • no recursion problems (in current PR, toolkit use CallDepthThreadLocalMap, to avoid recursion)
  • more safe for internal calls (driver internals, retry logic, heartbeat)

Although instrumenting execute() is simpler, the wrapper approach is better in terms of performance and robustness

@ValentinZakharov ValentinZakharov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Async test doesn't verify context propagation
asyncQuery() only checks that a span is created - it runs without a parent span, so it cannot detect if the cassandra span is incorrectly attached to the wrong trace or becomes a root span instead of a child
We need to add a parent span around the async call and assert that the cassandra span is a child of it

@ValentinZakharov

Copy link
Copy Markdown
Contributor

Amazing!
The toolkit added DBM, which is missing in master, so we can generate integrations with broad DataDog products landscape. 👍

* Utility class for Cassandra Database Monitoring (DBM) comment injection. When DBM propagation is
* enabled, this injects trace context as a CQL comment prepended to the query string so that the
* Datadog database agent can correlate queries back to traces.
*/

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we avoid redundant comments?
The purpose of the class is clear from the code and requires no further explanation

@ValentinZakharov

Copy link
Copy Markdown
Contributor

db.instance tag can be wrong for cross-keyspace queries

The keyspace is taken from session.getKeyspace() - the default set at session creation. If a query uses a fully-qualified table name (SELECT * FROM other_ks.users ...) on a session with a different default keyspace, db.instance will show the wrong value

The existing cassandra instrumentation from master reads the actual keyspace from ColumnDefinitions in the response after execution

@ValentinZakharov

Copy link
Copy Markdown
Contributor

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 587b271765

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread settings.gradle.kts
":dd-java-agent:instrumentation:axway-api-7.5",
":dd-java-agent:instrumentation:azure-functions-1.2.2",
":dd-java-agent:instrumentation:caffeine-1.0",
":dd-java-agent:instrumentation:cassandra:cassandra-4.0",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Avoid registering a second Cassandra module

Including this module ships it alongside the existing :dd-java-agent:instrumentation:datastax-cassandra:datastax-cassandra-4.0 entry already present later in settings.gradle.kts; both modules use super("cassandra") and target DefaultSession, so a default-enabled 4.x CqlSession call is traced by the existing TracingSession wrapper and then again by this new execute advice. That produces duplicate/nested Cassandra spans and gives users no separate config knob to disable only the new path, so this should replace/update the existing module rather than register a second one.

Useful? React with 👍 / 👎.

Comment on lines +105 to +106
scope.close();
((CompletionStage<?>) result).whenComplete(new SpanFinishingCallback(span));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Register async completion before closing scope

In the async branch, the scope is closed before the whenComplete callback is registered. The repository rule for instrumentation scope lifecycles requires async callback registration while the current span is still active; otherwise registration happens without the Cassandra span in scope for context-capturing instrumentation or future helper logic. Register the callback first, then close the scope.

AGENTS.md reference: AGENTS.md:L66-L66

Useful? React with 👍 / 👎.

Comment on lines +25 to +26
if (error != null) {
DECORATE.onError(span, error);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Unwrap async failures before tagging errors

When an async driver operation completes with a CompletionException wrapper, this passes the wrapper directly to onError, so the span reports CompletionException instead of the underlying Cassandra exception such as SyntaxError or coordinator failures. The existing Cassandra 4 wrapper handles this by unwrapping before decorating the span; this callback should do the same before calling DECORATE.onError.

Useful? React with 👍 / 👎.

import org.testcontainers.containers.CassandraContainer;

@TestMethodOrder(MethodOrderer.OrderAnnotation.class)
class CassandraClientTest extends AbstractInstrumentationTest {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Move instrumentation tests to Spock

This adds the Cassandra instrumentation coverage as a Java/JUnit class even though these Testcontainers-backed checks extend the instrumentation test harness rather than being unit tests. The repo convention reserves JUnit 5 for unit tests and uses Groovy/Spock for instrumentation and smoke tests, so keeping this as Java makes it inconsistent with the expected instrumentation-test runner style.

AGENTS.md reference: AGENTS.md:L62-L62

Useful? React with 👍 / 👎.

Comment on lines +105 to +106
scope.close();
((CompletionStage<?>) result).whenComplete(new SpanFinishingCallback(span));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Register async completion before closing scope

In the async branch, the scope is closed before the whenComplete callback is registered. The repository rule for instrumentation scope lifecycles requires async callback registration while the current span is still active; otherwise registration happens without the Cassandra span in scope for context-capturing instrumentation or future helper logic. Register the callback first, then close the scope.

Useful? React with 👍 / 👎.

DECORATE.afterStart(span);
DECORATE.onConnection(span, session);
DECORATE.onStatement(span, query);
final String contactPoints = ContactPointsUtil.getContactPoints(session);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid rebuilding contact points on every query

perf: This hot @Advice.OnMethodEnter path calls ContactPointsUtil.getContactPoints(session) for every statement, and that helper walks all metadata nodes and builds a comma-separated string each time. In high-QPS or multi-node Cassandra clients this creates per-query allocation and repeated metadata traversal for a low-cardinality value; capture/cache it per session like the existing 4.x instrumentation does instead.

Useful? React with 👍 / 👎.

Comment on lines +83 to +85
final String dbName = ContactPointsUtil.getKeyspace(session);
final String hostname = ContactPointsUtil.getFirstHost(session);
final String injectedQuery = CassandraDBMUtil.injectComment(span, query, hostname, dbName);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Gate DBM metadata lookup when disabled

perf: With the default DBM-disabled configuration, every SimpleStatement still resolves the keyspace and first host before CassandraDBMUtil.injectComment immediately returns the original query. That puts metadata traversal and config lookup on all simple-query spans for a result that is usually discarded; check a cached DBM-enabled flag before collecting dbName and hostname.

Useful? React with 👍 / 👎.

Comment on lines +59 to +60
protected String dbInstance(final CqlSession session) {
return session.getKeyspace().map(k -> k.asCql(false)).orElse(null);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Use unquoted keyspace names for db.instance

For sessions built with a keyspace, asCql(false) always renders the identifier in double-quoted CQL form, so a normal keyspace like peer_test is tagged as "peer_test". That changes db.instance and any peer-service or split-by-instance naming derived from it compared with the existing Cassandra instrumentation, so use the internal/unquoted keyspace representation here.

Useful? React with 👍 / 👎.

Comment on lines +76 to +78
public void onResponse(final AgentSpan span, final ResultSet result) {
if (result != null) {
final ExecutionInfo executionInfo = result.getExecutionInfo();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve statement keyspaces in db.instance

When cassandra-keyspace-statement-extraction is enabled, the existing Cassandra 4 instrumentation updates db.instance from result metadata so SELECT * FROM test_keyspace.users is attributed to test_keyspace even if the session has no keyspace or a different one. This response handling only records the coordinator, so those spans keep a null or stale session keyspace and split-by-instance/peer-service naming is wrong for fully-qualified statements.

Useful? React with 👍 / 👎.

Comment on lines +103 to +107
if (result instanceof CompletionStage) {
// Async path: close the scope but let the callback finish the span
scope.close();
((CompletionStage<?>) result).whenComplete(new SpanFinishingCallback(span));
} else {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Handle reactive result sets asynchronously

For driver versions that expose executeReactive, the dispatch returns a reactive Publisher/result set whose query runs when subscribed, not when the publisher object is created. Because only CompletionStage is treated as async here, a reactive result either falls through the sync path and finishes the span immediately before rows/errors/coordinator are known, or bypasses this advice entirely; add reactive-specific subscription instrumentation instead of finishing at method exit.

Useful? React with 👍 / 👎.

@Override
public void accept(final Object result, final Throwable error) {
try {
if (error != null) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If throwable is CoordinatorException, extract the coordinator node and call onPeerConnection() to set peer.hostname / peer.ipv4 / peer.port from the actual node that rejected the request. Otherwise the first node from session metadata will be used and can be wrong.

} else {
// Sync path: finish span immediately
try {
if (throwable != null) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here - error may be a CoordinatorException carrying the node that actually failed

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tag: ai generated Largely based on code generated by an AI or LLM tag: apm integration toolkit Changes generated by DataDog/apm-instrumentation-toolkit as part of IDM AIT experimentation tag: do not merge Do not merge changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants