-
Notifications
You must be signed in to change notification settings - Fork 311
Prevent crash in SQL Server's JDBC when tracing execute methods with generated keys #9321
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
…nerated keys When tracing the JDBC statement execute methods, we prepend a comment to the SQL query used for DBM trace propagation. However, there is a bug in the SQL Server JDBC driver that prevents the generated keys from being returned when the SQL comment is prepended to the SQL query. I decided to only append the comment in this specific case to avoid the comment from being truncated. @see microsoft/mssql-jdbc#2729
0a8f95f
to
eebbd7e
Compare
Code coverage: total 68.77%, base diff 11.50%, patch 100.00% (view details) This comment will be updated automatically if new data arrives.🔗 Commit SHA: eebbd7e | Docs | Was this helpful? Give us feedback! |
BenchmarksStartupParameters
See matching parameters
SummaryFound 1 performance improvements and 0 performance regressions! Performance is the same for 45 metrics, 13 unstable metrics.
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.53.0-SNAPSHOT~eebbd7e676, baseline=1.53.0-SNAPSHOT~d6b43cba5c
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.043 s) : 0, 1042693
Total [baseline] (8.572 s) : 0, 8571799
Agent [candidate] (1.051 s) : 0, 1050860
Total [candidate] (8.604 s) : 0, 8603881
section iast
Agent [baseline] (1.186 s) : 0, 1185715
Total [baseline] (9.397 s) : 0, 9396610
Agent [candidate] (1.175 s) : 0, 1175437
Total [candidate] (9.374 s) : 0, 9373603
gantt
title insecure-bank - break down per module: candidate=1.53.0-SNAPSHOT~eebbd7e676, baseline=1.53.0-SNAPSHOT~d6b43cba5c
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.433 ms) : 0, 1433
crashtracking [candidate] (1.43 ms) : 0, 1430
BytebuddyAgent [baseline] (730.379 ms) : 0, 730379
BytebuddyAgent [candidate] (733.47 ms) : 0, 733470
GlobalTracer [baseline] (241.603 ms) : 0, 241603
GlobalTracer [candidate] (243.999 ms) : 0, 243999
AppSec [baseline] (29.839 ms) : 0, 29839
AppSec [candidate] (30.413 ms) : 0, 30413
Debugger [baseline] (6.02 ms) : 0, 6020
Debugger [candidate] (6.072 ms) : 0, 6072
Remote Config [baseline] (647.536 µs) : 0, 648
Remote Config [candidate] (656.515 µs) : 0, 657
Telemetry [baseline] (11.858 ms) : 0, 11858
Telemetry [candidate] (13.801 ms) : 0, 13801
section iast
crashtracking [baseline] (1.429 ms) : 0, 1429
crashtracking [candidate] (1.429 ms) : 0, 1429
BytebuddyAgent [baseline] (855.0 ms) : 0, 855000
BytebuddyAgent [candidate] (848.214 ms) : 0, 848214
GlobalTracer [baseline] (234.667 ms) : 0, 234667
GlobalTracer [candidate] (231.893 ms) : 0, 231893
AppSec [baseline] (26.027 ms) : 0, 26027
AppSec [candidate] (29.529 ms) : 0, 29529
Debugger [baseline] (9.313 ms) : 0, 9313
Debugger [candidate] (6.76 ms) : 0, 6760
Remote Config [baseline] (600.402 µs) : 0, 600
Remote Config [candidate] (618.376 µs) : 0, 618
Telemetry [baseline] (8.303 ms) : 0, 8303
Telemetry [candidate] (8.494 ms) : 0, 8494
IAST [baseline] (29.365 ms) : 0, 29365
IAST [candidate] (27.471 ms) : 0, 27471
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.53.0-SNAPSHOT~eebbd7e676, baseline=1.53.0-SNAPSHOT~d6b43cba5c
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.054 s) : 0, 1054454
Total [baseline] (10.75 s) : 0, 10750130
Agent [candidate] (1.043 s) : 0, 1042964
Total [candidate] (10.803 s) : 0, 10802637
section appsec
Agent [baseline] (1.222 s) : 0, 1222487
Total [baseline] (10.806 s) : 0, 10806150
Agent [candidate] (1.226 s) : 0, 1226439
Total [candidate] (10.744 s) : 0, 10744034
section iast
Agent [baseline] (1.183 s) : 0, 1183255
Total [baseline] (10.924 s) : 0, 10923943
Agent [candidate] (1.174 s) : 0, 1174143
Total [candidate] (10.875 s) : 0, 10874729
section profiling
Agent [baseline] (1.197 s) : 0, 1196534
Total [baseline] (10.925 s) : 0, 10924760
Agent [candidate] (1.195 s) : 0, 1195083
Total [candidate] (10.921 s) : 0, 10921029
gantt
title petclinic - break down per module: candidate=1.53.0-SNAPSHOT~eebbd7e676, baseline=1.53.0-SNAPSHOT~d6b43cba5c
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.448 ms) : 0, 1448
crashtracking [candidate] (1.429 ms) : 0, 1429
BytebuddyAgent [baseline] (737.278 ms) : 0, 737278
BytebuddyAgent [candidate] (730.252 ms) : 0, 730252
GlobalTracer [baseline] (244.03 ms) : 0, 244030
GlobalTracer [candidate] (242.116 ms) : 0, 242116
AppSec [baseline] (30.292 ms) : 0, 30292
AppSec [candidate] (29.941 ms) : 0, 29941
Debugger [baseline] (6.054 ms) : 0, 6054
Debugger [candidate] (6.03 ms) : 0, 6030
Remote Config [baseline] (650.037 µs) : 0, 650
Remote Config [candidate] (651.209 µs) : 0, 651
Telemetry [baseline] (13.578 ms) : 0, 13578
Telemetry [candidate] (11.53 ms) : 0, 11530
section appsec
crashtracking [baseline] (1.434 ms) : 0, 1434
crashtracking [candidate] (1.43 ms) : 0, 1430
BytebuddyAgent [baseline] (754.183 ms) : 0, 754183
BytebuddyAgent [candidate] (756.951 ms) : 0, 756951
GlobalTracer [baseline] (236.022 ms) : 0, 236022
GlobalTracer [candidate] (236.39 ms) : 0, 236390
AppSec [baseline] (169.233 ms) : 0, 169233
AppSec [candidate] (168.159 ms) : 0, 168159
Debugger [baseline] (7.248 ms) : 0, 7248
Debugger [candidate] (8.75 ms) : 0, 8750
Remote Config [baseline] (609.425 µs) : 0, 609
Remote Config [candidate] (617.772 µs) : 0, 618
Telemetry [baseline] (9.145 ms) : 0, 9145
Telemetry [candidate] (9.177 ms) : 0, 9177
IAST [baseline] (23.535 ms) : 0, 23535
IAST [candidate] (23.855 ms) : 0, 23855
section iast
crashtracking [baseline] (1.449 ms) : 0, 1449
crashtracking [candidate] (1.441 ms) : 0, 1441
BytebuddyAgent [baseline] (854.869 ms) : 0, 854869
BytebuddyAgent [candidate] (847.327 ms) : 0, 847327
GlobalTracer [baseline] (233.93 ms) : 0, 233930
GlobalTracer [candidate] (231.684 ms) : 0, 231684
AppSec [baseline] (27.864 ms) : 0, 27864
AppSec [candidate] (26.339 ms) : 0, 26339
Debugger [baseline] (6.69 ms) : 0, 6690
Debugger [candidate] (8.482 ms) : 0, 8482
Remote Config [baseline] (608.334 µs) : 0, 608
Remote Config [candidate] (576.47 µs) : 0, 576
Telemetry [baseline] (8.276 ms) : 0, 8276
Telemetry [candidate] (8.174 ms) : 0, 8174
IAST [baseline] (28.461 ms) : 0, 28461
IAST [candidate] (29.072 ms) : 0, 29072
section profiling
crashtracking [baseline] (1.418 ms) : 0, 1418
crashtracking [candidate] (1.414 ms) : 0, 1414
BytebuddyAgent [baseline] (760.62 ms) : 0, 760620
BytebuddyAgent [candidate] (761.745 ms) : 0, 761745
GlobalTracer [baseline] (223.179 ms) : 0, 223179
GlobalTracer [candidate] (221.485 ms) : 0, 221485
AppSec [baseline] (30.222 ms) : 0, 30222
AppSec [candidate] (29.758 ms) : 0, 29758
Debugger [baseline] (6.3 ms) : 0, 6300
Debugger [candidate] (6.287 ms) : 0, 6287
Remote Config [baseline] (702.927 µs) : 0, 703
Remote Config [candidate] (672.221 µs) : 0, 672
Telemetry [baseline] (16.303 ms) : 0, 16303
Telemetry [candidate] (16.015 ms) : 0, 16015
ProfilingAgent [baseline] (108.342 ms) : 0, 108342
ProfilingAgent [candidate] (108.027 ms) : 0, 108027
Profiling [baseline] (108.984 ms) : 0, 108984
Profiling [candidate] (108.684 ms) : 0, 108684
LoadParameters
See matching parameters
SummaryFound 3 performance improvements and 0 performance regressions! Performance is the same for 9 metrics, 12 unstable metrics.
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.53.0-SNAPSHOT~eebbd7e676, baseline=1.53.0-SNAPSHOT~d6b43cba5c
dateFormat X
axisFormat %s
section baseline
no_agent (4.353 ms) : 4305, 4402
. : milestone, 4353,
iast (9.411 ms) : 9260, 9563
. : milestone, 9411,
iast_FULL (14.146 ms) : 13861, 14431
. : milestone, 14146,
iast_GLOBAL (10.228 ms) : 10049, 10407
. : milestone, 10228,
profiling (8.685 ms) : 8545, 8825
. : milestone, 8685,
tracing (7.624 ms) : 7508, 7740
. : milestone, 7624,
section candidate
no_agent (4.419 ms) : 4361, 4476
. : milestone, 4419,
iast (9.488 ms) : 9335, 9641
. : milestone, 9488,
iast_FULL (14.275 ms) : 13991, 14560
. : milestone, 14275,
iast_GLOBAL (10.204 ms) : 10026, 10383
. : milestone, 10204,
profiling (8.324 ms) : 8198, 8451
. : milestone, 8324,
tracing (7.453 ms) : 7340, 7566
. : milestone, 7453,
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.53.0-SNAPSHOT~eebbd7e676, baseline=1.53.0-SNAPSHOT~d6b43cba5c
dateFormat X
axisFormat %s
section baseline
no_agent (36.851 ms) : 36552, 37151
. : milestone, 36851,
appsec (48.22 ms) : 47785, 48654
. : milestone, 48220,
code_origins (44.336 ms) : 43946, 44726
. : milestone, 44336,
iast (45.982 ms) : 45589, 46374
. : milestone, 45982,
profiling (49.961 ms) : 49502, 50420
. : milestone, 49961,
tracing (45.675 ms) : 45283, 46068
. : milestone, 45675,
section candidate
no_agent (37.791 ms) : 37483, 38098
. : milestone, 37791,
appsec (49.117 ms) : 48680, 49554
. : milestone, 49117,
code_origins (45.492 ms) : 45109, 45875
. : milestone, 45492,
iast (42.719 ms) : 42361, 43076
. : milestone, 42719,
profiling (47.955 ms) : 47491, 48420
. : milestone, 47955,
tracing (44.929 ms) : 44547, 45310
. : milestone, 44929,
DacapoParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 10 metrics, 2 unstable metrics. Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.53.0-SNAPSHOT~eebbd7e676, baseline=1.53.0-SNAPSHOT~d6b43cba5c
dateFormat X
axisFormat %s
section baseline
no_agent (15.421 s) : 15421000, 15421000
. : milestone, 15421000,
appsec (14.966 s) : 14966000, 14966000
. : milestone, 14966000,
iast (18.388 s) : 18388000, 18388000
. : milestone, 18388000,
iast_GLOBAL (18.326 s) : 18326000, 18326000
. : milestone, 18326000,
profiling (15.366 s) : 15366000, 15366000
. : milestone, 15366000,
tracing (14.931 s) : 14931000, 14931000
. : milestone, 14931000,
section candidate
no_agent (15.528 s) : 15528000, 15528000
. : milestone, 15528000,
appsec (14.719 s) : 14719000, 14719000
. : milestone, 14719000,
iast (18.199 s) : 18199000, 18199000
. : milestone, 18199000,
iast_GLOBAL (17.972 s) : 17972000, 17972000
. : milestone, 17972000,
profiling (15.229 s) : 15229000, 15229000
. : milestone, 15229000,
tracing (14.985 s) : 14985000, 14985000
. : milestone, 14985000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.53.0-SNAPSHOT~eebbd7e676, baseline=1.53.0-SNAPSHOT~d6b43cba5c
dateFormat X
axisFormat %s
section baseline
no_agent (1.473 ms) : 1461, 1484
. : milestone, 1473,
appsec (3.675 ms) : 3455, 3896
. : milestone, 3675,
iast (2.202 ms) : 2139, 2265
. : milestone, 2202,
iast_GLOBAL (2.24 ms) : 2177, 2303
. : milestone, 2240,
profiling (2.445 ms) : 2292, 2599
. : milestone, 2445,
tracing (2.013 ms) : 1964, 2061
. : milestone, 2013,
section candidate
no_agent (1.474 ms) : 1463, 1486
. : milestone, 1474,
appsec (3.654 ms) : 3438, 3869
. : milestone, 3654,
iast (2.201 ms) : 2139, 2263
. : milestone, 2201,
iast_GLOBAL (2.236 ms) : 2174, 2299
. : milestone, 2236,
profiling (2.491 ms) : 2324, 2658
. : milestone, 2491,
tracing (2.016 ms) : 1967, 2064
. : milestone, 2016,
|
@@ -75,6 +76,7 @@ public static class StatementAdvice { | |||
@Advice.OnMethodEnter(suppress = Throwable.class) | |||
public static AgentScope onEnter( | |||
@Advice.Argument(value = 0, readOnly = false) String sql, | |||
@Advice.AllArguments(typing = DYNAMIC) Object[] args, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've decided to use AllArguments here, because some of the execute
methods we are hooking accepts an int for the generatedKeys
, while others accept an array of ints for columnIndexes
or and array of strings for columnNames
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getting byte-buddy to pack all arguments into an array could affect performance - have you done any benchmarking of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative approach that would avoid this would be to write different advices for the different method signatures - you can still share code between the advices by moving that code to the decorator (or similar helper)
While this would take longer to write, it would be much more performant... also you'd only write the code once while this advice will be invoked many millions of time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, I did some benchmarking with an existing setup and the overhead is minor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@na-ji btw, is the typing = DYNAMIC
needed?
I found a couple of existing instrumentations that use @Advice.AllArguments
and they don't need typing = DYNAMIC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wary about the change to add
@Advice.AllArguments(typing = DYNAMIC) Object[] args
and would prefer to see this covered by more focused advice, because that would avoid the additional byte-buddy allocation of an array to hold all the arguments and copying them in individually - this is a hot method and additional allocations could easily affect latency.
But if a benchmark of a DB application being traced before this change and after doesn't show any difference then I'm ok with this change (I'd just like to get confirmation of that)
Also :dd-java-agent:instrumentation:jdbc:forkedTest
is failing on CI for this branch - that needs fixing before this could be merged and backported
I did some benchmarking with an existing setup and the overhead is minor.
So it's just the :dd-java-agent:instrumentation:jdbc:forkedTest
that needs fixing
What Does This Do
Modify the way we append/prepend the DBM tracing comment into queries.
Motivation
When tracing the JDBC statement execute methods, we prepend a comment to the SQL query used for DBM trace propagation.
However, there is a bug in the SQL Server JDBC driver that prevents the generated keys from being returned when the SQL comment is prepended to the SQL query.
I decided to only append the comment in this specific case to avoid the comment from being truncated.
See microsoft/mssql-jdbc#2729
Additional Notes
This is the code that used to crash on the injected app when DBM trace propagation was enabled
Contributor Checklist
type:
and (comp:
orinst:
) labels in addition to any usefull labelsclose
,fix
or any linking keywords when referencing an issue.Use
solves
instead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]