Fix Oracle DBM trace correlation#10829
Fix Oracle DBM trace correlation#10829gh-worker-dd-mergequeue-cf854d[bot] merged 5 commits intomasterfrom
Conversation
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 64 metrics, 7 unstable metrics. Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.61.0-SNAPSHOT~7e995627f9, baseline=1.61.0-SNAPSHOT~a953f33c70
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.057 s) : 0, 1057287
Total [baseline] (11.081 s) : 0, 11080577
Agent [candidate] (1.057 s) : 0, 1056590
Total [candidate] (11.06 s) : 0, 11059608
section appsec
Agent [baseline] (1.246 s) : 0, 1246338
Total [baseline] (11.209 s) : 0, 11208855
Agent [candidate] (1.25 s) : 0, 1249967
Total [candidate] (11.207 s) : 0, 11206668
section iast
Agent [baseline] (1.229 s) : 0, 1228703
Total [baseline] (11.418 s) : 0, 11417788
Agent [candidate] (1.229 s) : 0, 1229277
Total [candidate] (11.317 s) : 0, 11316860
section profiling
Agent [baseline] (1.19 s) : 0, 1190442
Total [baseline] (11.078 s) : 0, 11077716
Agent [candidate] (1.187 s) : 0, 1186628
Total [candidate] (10.935 s) : 0, 10935320
gantt
title petclinic - break down per module: candidate=1.61.0-SNAPSHOT~7e995627f9, baseline=1.61.0-SNAPSHOT~a953f33c70
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.189 ms) : 0, 1189
crashtracking [candidate] (1.184 ms) : 0, 1184
BytebuddyAgent [baseline] (627.693 ms) : 0, 627693
BytebuddyAgent [candidate] (628.417 ms) : 0, 628417
AgentMeter [baseline] (29.143 ms) : 0, 29143
AgentMeter [candidate] (29.161 ms) : 0, 29161
GlobalTracer [baseline] (256.928 ms) : 0, 256928
GlobalTracer [candidate] (257.202 ms) : 0, 257202
AppSec [baseline] (31.626 ms) : 0, 31626
AppSec [candidate] (31.692 ms) : 0, 31692
Debugger [baseline] (60.268 ms) : 0, 60268
Debugger [candidate] (60.053 ms) : 0, 60053
Remote Config [baseline] (586.912 µs) : 0, 587
Remote Config [candidate] (584.59 µs) : 0, 585
Telemetry [baseline] (7.966 ms) : 0, 7966
Telemetry [candidate] (8.001 ms) : 0, 8001
Flare Poller [baseline] (5.865 ms) : 0, 5865
Flare Poller [candidate] (4.311 ms) : 0, 4311
section appsec
crashtracking [baseline] (1.181 ms) : 0, 1181
crashtracking [candidate] (1.19 ms) : 0, 1190
BytebuddyAgent [baseline] (658.001 ms) : 0, 658001
BytebuddyAgent [candidate] (660.363 ms) : 0, 660363
AgentMeter [baseline] (12.005 ms) : 0, 12005
AgentMeter [candidate] (12.014 ms) : 0, 12014
GlobalTracer [baseline] (257.913 ms) : 0, 257913
GlobalTracer [candidate] (259.028 ms) : 0, 259028
AppSec [baseline] (177.862 ms) : 0, 177862
AppSec [candidate] (177.842 ms) : 0, 177842
Debugger [baseline] (66.355 ms) : 0, 66355
Debugger [candidate] (66.313 ms) : 0, 66313
Remote Config [baseline] (620.712 µs) : 0, 621
Remote Config [candidate] (614.775 µs) : 0, 615
Telemetry [baseline] (8.341 ms) : 0, 8341
Telemetry [candidate] (8.292 ms) : 0, 8292
Flare Poller [baseline] (3.606 ms) : 0, 3606
Flare Poller [candidate] (3.536 ms) : 0, 3536
IAST [baseline] (24.201 ms) : 0, 24201
IAST [candidate] (24.371 ms) : 0, 24371
section iast
crashtracking [baseline] (1.188 ms) : 0, 1188
crashtracking [candidate] (1.195 ms) : 0, 1195
BytebuddyAgent [baseline] (797.062 ms) : 0, 797062
BytebuddyAgent [candidate] (797.175 ms) : 0, 797175
AgentMeter [baseline] (11.32 ms) : 0, 11320
AgentMeter [candidate] (11.357 ms) : 0, 11357
GlobalTracer [baseline] (247.134 ms) : 0, 247134
GlobalTracer [candidate] (247.671 ms) : 0, 247671
AppSec [baseline] (26.604 ms) : 0, 26604
AppSec [candidate] (27.469 ms) : 0, 27469
Debugger [baseline] (70.734 ms) : 0, 70734
Debugger [candidate] (69.925 ms) : 0, 69925
Remote Config [baseline] (538.43 µs) : 0, 538
Remote Config [candidate] (535.202 µs) : 0, 535
Telemetry [baseline] (9.304 ms) : 0, 9304
Telemetry [candidate] (9.236 ms) : 0, 9236
Flare Poller [baseline] (3.381 ms) : 0, 3381
Flare Poller [candidate] (3.348 ms) : 0, 3348
IAST [baseline] (25.346 ms) : 0, 25346
IAST [candidate] (25.261 ms) : 0, 25261
section profiling
crashtracking [baseline] (1.18 ms) : 0, 1180
crashtracking [candidate] (1.174 ms) : 0, 1174
BytebuddyAgent [baseline] (688.301 ms) : 0, 688301
BytebuddyAgent [candidate] (687.06 ms) : 0, 687060
AgentMeter [baseline] (8.68 ms) : 0, 8680
AgentMeter [candidate] (8.645 ms) : 0, 8645
GlobalTracer [baseline] (216.644 ms) : 0, 216644
GlobalTracer [candidate] (215.493 ms) : 0, 215493
AppSec [baseline] (32.394 ms) : 0, 32394
AppSec [candidate] (32.135 ms) : 0, 32135
Debugger [baseline] (64.751 ms) : 0, 64751
Debugger [candidate] (64.087 ms) : 0, 64087
Remote Config [baseline] (572.683 µs) : 0, 573
Remote Config [candidate] (561.648 µs) : 0, 562
Telemetry [baseline] (8.538 ms) : 0, 8538
Telemetry [candidate] (8.395 ms) : 0, 8395
Flare Poller [baseline] (4.279 ms) : 0, 4279
Flare Poller [candidate] (4.224 ms) : 0, 4224
ProfilingAgent [baseline] (93.972 ms) : 0, 93972
ProfilingAgent [candidate] (93.679 ms) : 0, 93679
Profiling [baseline] (94.536 ms) : 0, 94536
Profiling [candidate] (94.233 ms) : 0, 94233
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.61.0-SNAPSHOT~7e995627f9, baseline=1.61.0-SNAPSHOT~a953f33c70
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.057 s) : 0, 1057345
Total [baseline] (8.876 s) : 0, 8876145
Agent [candidate] (1.054 s) : 0, 1054488
Total [candidate] (8.85 s) : 0, 8849963
section iast
Agent [baseline] (1.229 s) : 0, 1228823
Total [baseline] (9.591 s) : 0, 9591036
Agent [candidate] (1.228 s) : 0, 1228259
Total [candidate] (9.576 s) : 0, 9576227
gantt
title insecure-bank - break down per module: candidate=1.61.0-SNAPSHOT~7e995627f9, baseline=1.61.0-SNAPSHOT~a953f33c70
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.207 ms) : 0, 1207
crashtracking [candidate] (1.201 ms) : 0, 1201
BytebuddyAgent [baseline] (629.414 ms) : 0, 629414
BytebuddyAgent [candidate] (627.404 ms) : 0, 627404
AgentMeter [baseline] (29.264 ms) : 0, 29264
AgentMeter [candidate] (29.168 ms) : 0, 29168
GlobalTracer [baseline] (257.164 ms) : 0, 257164
GlobalTracer [candidate] (256.85 ms) : 0, 256850
AppSec [baseline] (31.727 ms) : 0, 31727
AppSec [candidate] (31.704 ms) : 0, 31704
Debugger [baseline] (59.638 ms) : 0, 59638
Debugger [candidate] (59.33 ms) : 0, 59330
Remote Config [baseline] (576.855 µs) : 0, 577
Remote Config [candidate] (581.253 µs) : 0, 581
Telemetry [baseline] (7.987 ms) : 0, 7987
Telemetry [candidate] (7.971 ms) : 0, 7971
Flare Poller [baseline] (4.247 ms) : 0, 4247
Flare Poller [candidate] (4.241 ms) : 0, 4241
section iast
crashtracking [baseline] (1.197 ms) : 0, 1197
crashtracking [candidate] (1.196 ms) : 0, 1196
BytebuddyAgent [baseline] (797.838 ms) : 0, 797838
BytebuddyAgent [candidate] (796.923 ms) : 0, 796923
AgentMeter [baseline] (11.349 ms) : 0, 11349
AgentMeter [candidate] (11.351 ms) : 0, 11351
GlobalTracer [baseline] (247.577 ms) : 0, 247577
GlobalTracer [candidate] (247.656 ms) : 0, 247656
AppSec [baseline] (26.469 ms) : 0, 26469
AppSec [candidate] (26.555 ms) : 0, 26555
Debugger [baseline] (69.243 ms) : 0, 69243
Debugger [candidate] (69.935 ms) : 0, 69935
Remote Config [baseline] (525.68 µs) : 0, 526
Remote Config [candidate] (531.126 µs) : 0, 531
Telemetry [baseline] (9.664 ms) : 0, 9664
Telemetry [candidate] (9.236 ms) : 0, 9236
Flare Poller [baseline] (3.469 ms) : 0, 3469
Flare Poller [candidate] (3.326 ms) : 0, 3326
IAST [baseline] (25.338 ms) : 0, 25338
IAST [candidate] (25.359 ms) : 0, 25359
LoadParameters
See matching parameters
SummaryFound 1 performance improvements and 1 performance regressions! Performance is the same for 18 metrics, 16 unstable metrics.
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.61.0-SNAPSHOT~7e995627f9, baseline=1.61.0-SNAPSHOT~a953f33c70
dateFormat X
axisFormat %s
section baseline
no_agent (19.327 ms) : 19129, 19524
. : milestone, 19327,
appsec (18.449 ms) : 18257, 18641
. : milestone, 18449,
code_origins (17.656 ms) : 17481, 17831
. : milestone, 17656,
iast (18.086 ms) : 17904, 18267
. : milestone, 18086,
profiling (18.622 ms) : 18431, 18813
. : milestone, 18622,
tracing (18.021 ms) : 17840, 18203
. : milestone, 18021,
section candidate
no_agent (18.032 ms) : 17848, 18216
. : milestone, 18032,
appsec (18.448 ms) : 18261, 18635
. : milestone, 18448,
code_origins (17.973 ms) : 17792, 18154
. : milestone, 17973,
iast (17.693 ms) : 17515, 17872
. : milestone, 17693,
profiling (18.537 ms) : 18357, 18718
. : milestone, 18537,
tracing (17.827 ms) : 17651, 18003
. : milestone, 17827,
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.61.0-SNAPSHOT~7e995627f9, baseline=1.61.0-SNAPSHOT~a953f33c70
dateFormat X
axisFormat %s
section baseline
no_agent (1.247 ms) : 1234, 1259
. : milestone, 1247,
iast (3.081 ms) : 3037, 3124
. : milestone, 3081,
iast_FULL (5.84 ms) : 5781, 5899
. : milestone, 5840,
iast_GLOBAL (3.569 ms) : 3515, 3623
. : milestone, 3569,
profiling (2.028 ms) : 2011, 2045
. : milestone, 2028,
tracing (1.766 ms) : 1752, 1780
. : milestone, 1766,
section candidate
no_agent (1.19 ms) : 1179, 1202
. : milestone, 1190,
iast (3.135 ms) : 3094, 3175
. : milestone, 3135,
iast_FULL (6.098 ms) : 6035, 6160
. : milestone, 6098,
iast_GLOBAL (3.545 ms) : 3484, 3606
. : milestone, 3545,
profiling (2.221 ms) : 2199, 2243
. : milestone, 2221,
tracing (1.806 ms) : 1791, 1821
. : milestone, 1806,
DacapoParameters
See matching parameters
SummaryFound 1 performance improvements and 0 performance regressions! Performance is the same for 11 metrics, 0 unstable metrics.
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.61.0-SNAPSHOT~7e995627f9, baseline=1.61.0-SNAPSHOT~a953f33c70
dateFormat X
axisFormat %s
section baseline
no_agent (1.472 ms) : 1461, 1484
. : milestone, 1472,
appsec (3.734 ms) : 3516, 3951
. : milestone, 3734,
iast (2.254 ms) : 2185, 2323
. : milestone, 2254,
iast_GLOBAL (2.294 ms) : 2225, 2364
. : milestone, 2294,
profiling (2.07 ms) : 2015, 2124
. : milestone, 2070,
tracing (2.067 ms) : 2014, 2121
. : milestone, 2067,
section candidate
no_agent (1.472 ms) : 1460, 1483
. : milestone, 1472,
appsec (2.519 ms) : 2464, 2574
. : milestone, 2519,
iast (2.254 ms) : 2185, 2322
. : milestone, 2254,
iast_GLOBAL (2.297 ms) : 2227, 2366
. : milestone, 2297,
profiling (2.121 ms) : 2064, 2178
. : milestone, 2121,
tracing (2.054 ms) : 2000, 2107
. : milestone, 2054,
Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.61.0-SNAPSHOT~7e995627f9, baseline=1.61.0-SNAPSHOT~a953f33c70
dateFormat X
axisFormat %s
section baseline
no_agent (15.462 s) : 15462000, 15462000
. : milestone, 15462000,
appsec (14.618 s) : 14618000, 14618000
. : milestone, 14618000,
iast (18.437 s) : 18437000, 18437000
. : milestone, 18437000,
iast_GLOBAL (17.947 s) : 17947000, 17947000
. : milestone, 17947000,
profiling (14.923 s) : 14923000, 14923000
. : milestone, 14923000,
tracing (15.053 s) : 15053000, 15053000
. : milestone, 15053000,
section candidate
no_agent (14.736 s) : 14736000, 14736000
. : milestone, 14736000,
appsec (14.847 s) : 14847000, 14847000
. : milestone, 14847000,
iast (18.331 s) : 18331000, 18331000
. : milestone, 18331000,
iast_GLOBAL (18.201 s) : 18201000, 18201000
. : milestone, 18201000,
profiling (14.853 s) : 14853000, 14853000
. : milestone, 14853000,
tracing (14.804 s) : 14804000, 14804000
. : milestone, 14804000,
|
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
...ent/instrumentation/jdbc/src/main/java/datadog/trace/instrumentation/jdbc/JDBCDecorator.java
Outdated
Show resolved
Hide resolved
...entation/jdbc/src/main/java/datadog/trace/instrumentation/jdbc/StatementInstrumentation.java
Show resolved
Hide resolved
dd-java-agent/instrumentation/jdbc/src/test/groovy/OracleInjectionForkedTest.groovy
Show resolved
Hide resolved
dd-java-agent/instrumentation/jdbc/src/test/groovy/OracleInjectionForkedTest.groovy
Outdated
Show resolved
Hide resolved
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f04c0f9bf
ℹ️ 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".
| def adminConn = java.sql.DriverManager.getConnection(adminUrl, "system", oracle.getPassword()) | ||
| def rs = adminConn.createStatement().executeQuery( | ||
| "SELECT sql_fulltext FROM v\$sql " + | ||
| "WHERE sql_fulltext LIKE '%1729%' AND sql_fulltext LIKE '%dddbs%' " + |
There was a problem hiding this comment.
Exclude the admin lookup SQL from the v$sql match
This lookup can return the verifier query itself instead of the instrumented markerQuery, because the lookup SQL text contains both literals ('1729' and 'dddbs') used in its own LIKE filters and Oracle records executed statements in v$sql; with ROWNUM = 1, assertions may pass even if the target query was not injected correctly. Add a predicate that uniquely identifies the target statement (or excludes the lookup statement) so the test actually validates the intended SQL text.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
updated with adding like predicate on the comment
| dbService, | ||
| dbInfo.getType(), | ||
| dbInfo.getHost(), | ||
| DECORATE.isOracle(dbInfo) ? DECORATE.getDbInstance(dbInfo) : dbInfo.getDb(), |
There was a problem hiding this comment.
since it seems that all the info to return the correct data is contained in dbInfo, couldn't we put this logic in dbInfo.getDb() rather than having to write it explicitely here ?
There was a problem hiding this comment.
I took a look at dbInfo.getDb(), I'd rather not hide this logic in a pure getter function, as it could cause confusion down the line if there is splitting logic internally. If you're not a fan of this ternary here, I think we can also add a decorator method to JDBCDecorator, so the Oracle split lives there. Something like getDbNameForComment() or getDbForInjection()?
There was a problem hiding this comment.
I was more thinking that some other place in the code could want the DB instance from the dbInfo, and they'd have to repeat the same logic, while if it's in the getter, we know it's always going to work.
Would there be any reason to not want to get the instance upon calling getDb on an oracle dbInfo ?
There was a problem hiding this comment.
I see, that's fair. I'll push up a commit shortly
There was a problem hiding this comment.
Updated getDb so it simply returns instance if db is null
…ame into dddbs and dddb SQL comment tags instead of the generic type string and null
d695249 to
7e99562
Compare
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
The merge request has been interrupted because the build 103327668 took longer than expected. The current limit for the base branch 'master' is 120 minutes. |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
What Does This Do
Fix Oracle DBM trace correlation by injecting the database instance name into dddbs and dddb SQL comment tags instead of the generic type string and null.
Motivation
https://datadoghq.atlassian.net/browse/SDBM-2434
On the APM service page, Oracle-backed services show broken DBM trace correlation:
Checked the oracle integration and backend pipeline are correct. The Java tracer is injecting the wrong values.
Root cause
Two bugs in the SQL comment injection path:
Bug 1 — dddbs='oracle' instead of the instance name:
getDbService() called dbService(type, instance) which, when split-by-instance=false (the default), ignores the
instance name and returns the type-based cache entry ("oracle").
Bug 2 — dddb always absent:
Both injection sites passed dbInfo.getDb() for the dddb tag. For Oracle, the JDBC URL parser sets instance only —
getDb() is always null — so dddb was silently skipped by SQLCommenter.
Fix
type-based naming cache. Uses getInstance() (not the dbInstance() fallback to getDb()) so databases like SQL Server
that carry identity in getDb() are unaffected.
db).
span.getServiceName()); dddb now uses DECORATE.getDbInstance(dbInfo) (was dbInfo.getDb()).
Result
Oracle connections now inject:
/ddps='my-service',dddbs='freepdb1',ddh='localhost',dddb='freepdb1'/
instead of:
/ddps='my-service',dddbs='oracle',ddh='localhost'/
Testing
correct comment format against a mock Oracle connection.
container (Testcontainers), runs a query, then queries v$sql as system to verify the exact SQL text Oracle received
contains dddbs='freepdb1' and dddb='freepdb1' and does not contain dddbs='oracle'.
Additional Notes
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: https://datadoghq.atlassian.net/browse/SDBM-2434
Note: Once your PR is ready to merge, add it to the merge queue by commenting
/merge./merge -ccancels the queue request./merge -f --reason "reason"skips all merge queue checks; please use this judiciously, as some checks do not run at the PR-level. For more information, see this doc.