Propagate Oracle dynamic service hash without SQL comments - #12234
Propagate Oracle dynamic service hash without SQL comments#12234joelmarcotte wants to merge 4 commits into
Conversation
|
🎯 Code Coverage (details) 🔗 Commit SHA: 35692f7 | Docs | View more details | 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. |
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab2505d811
ℹ️ 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".
There was a problem hiding this comment.
The new Oracle ACTION path ignores the process-tag propagation opt-out. It can send a hash without the matching span tag, so DBM-to-APM correlation fails.
🤖 Datadog Autotest · Commit ab2505d · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| private final String warehouse; | ||
| private final String schema; | ||
| private volatile String poolName; | ||
| private volatile String oracleServiceAction; |
There was a problem hiding this comment.
The mix of final & volatiles here worries me from thread-safety / correctness perspective.
If I recall correctly, DbInfo objects are cached, so we really should set everything as construction rather than mutating later on.
Maybe switching to a builder idiom would be a better approach.
Normally, I don't like builders for performance reasons, but if we're already caching, it would be fine.
There was a problem hiding this comment.
[Drafted by Claude on behalf of @dougqh]
Confirmed the caching concern is real, and worse than just field-mix hygiene: JDBCConnectionUrlParser.extractDBInfo caches DBInfo by (url, props) in a fixed 32-entry DDCache (JDBCConnectionUrlParser.java:847-861), shared across every Connection that connects with the same URL. That's why this PR had to add .toBuilder().build() after both extractDBInfo call sites (DriverInstrumentation.java, JDBCDecorator.parseDBInfoFromConnection) — without that defensive copy, markOracleServiceAction's "have I already set this for this connection?" flag would be shared across unrelated connections, and the second connection to a given URL would silently skip setClientInfo, breaking DBM correlation for it.
poolName/oracleServiceAction are per-Connection/session state; DBInfo is per-URL parse-result state that's cache-shared by design. Mixing them means correctness now depends on every call site remembering to copy — nothing in the type enforces it, so a future call site that reads DBInfo from the context map or extractDBInfo directly and forwards it without .toBuilder().build() reintroduces the cross-connection leak.
I'd rather keep DBInfo as the pure immutable parse result and track the mutable per-connection bits (poolName, oracleServiceAction) in their own holder keyed off InstrumentationContext.get(Connection.class, ...), which is already the correct scope this code relies on elsewhere.
There was a problem hiding this comment.
So, I tried to go this way with these new changes but this significantly increases the scope (and reviewers needed).
dougqh
left a comment
There was a problem hiding this comment.
I think the mutable parts of DbInfo need to live elsewhere.
Please see inline comments for more details.
There was a problem hiding this comment.
A BaseHash update can put one hash in Oracle ACTION and a different hash on the JDBC span. This mismatch prevents DBM correlation for that query.
🤖 Datadog Autotest · Commit c8283cb · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
There was a problem hiding this comment.
More details
The opt-in path keeps Oracle statement text unchanged. It updates ACTION when BaseHash changes and keeps mutable ACTION state per connection. No concrete failure mode clears the reporting threshold.
🤖 Datadog Autotest · Commit 35692f7 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
What changed
DD_DBM_PROPAGATION_ORACLE_ACTION_ONLY_ENABLEDconfiguration. It defaults tofalse.DD_DBM_PROPAGATION_MODE=dynamic_service, write_DD_DDSH:<BaseHash>throughsetClientInfo("OCSID.ACTION", ...)for Oracle connections.Why
SDBM-2904 requires Oracle DBM/APM correlation without changing statement text. Oracle SQL Plan Management matches exact SQL text, so injected comments can prevent stored baselines from matching.
The ACTION payload uses the same signed
BaseHashalready emitted asddsh, so the backend can reuse the existing dynamic-service linker.Enablement
Verification
./gradlew :dd-trace-api:spotlessApply :internal-api:spotlessApply :dd-java-agent:agent-bootstrap:spotlessApply :dd-java-agent:instrumentation:jdbc:spotlessApply./gradlew :internal-api:test --tests 'datadog.trace.api.ConfigTest.Oracle DBM action propagation enabled*'./gradlew :dd-java-agent:instrumentation:jdbc:forkedTest --tests 'OracleInjectionForkedTest' --tests 'OracleDynamicServiceActionInjectionForkedTest' --tests 'DBMDynamicServiceInjectionForkedTest'Links