-
Notifications
You must be signed in to change notification settings - Fork 288
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
Fix exception handling for SQL Server full mode #7405
Fix exception handling for SQL Server full mode #7405
Conversation
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 48 metrics, 15 unstable metrics. Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.39.0-SNAPSHOT~2675ae1381, baseline=1.39.0-SNAPSHOT~23ce5ef2d1
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.048 s) : 0, 1047944
Total [baseline] (8.488 s) : 0, 8487543
Agent [candidate] (1.055 s) : 0, 1055243
Total [candidate] (8.502 s) : 0, 8501875
section iast
Agent [baseline] (1.178 s) : 0, 1177604
Total [baseline] (8.993 s) : 0, 8992683
Agent [candidate] (1.187 s) : 0, 1186975
Total [candidate] (8.997 s) : 0, 8996730
section iast_HARDCODED_SECRET_DISABLED
Agent [baseline] (1.178 s) : 0, 1178441
Total [baseline] (8.952 s) : 0, 8951693
Agent [candidate] (1.179 s) : 0, 1178594
Total [candidate] (8.954 s) : 0, 8954260
section iast_TELEMETRY_OFF
Agent [baseline] (1.173 s) : 0, 1173207
Total [baseline] (9.006 s) : 0, 9005623
Agent [candidate] (1.172 s) : 0, 1172163
Total [candidate] (8.954 s) : 0, 8953632
gantt
title insecure-bank - break down per module: candidate=1.39.0-SNAPSHOT~2675ae1381, baseline=1.39.0-SNAPSHOT~23ce5ef2d1
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (668.678 ms) : 0, 668678
BytebuddyAgent [candidate] (673.712 ms) : 0, 673712
GlobalTracer [baseline] (306.737 ms) : 0, 306737
GlobalTracer [candidate] (309.101 ms) : 0, 309101
AppSec [baseline] (51.002 ms) : 0, 51002
AppSec [candidate] (50.764 ms) : 0, 50764
Remote Config [baseline] (708.304 µs) : 0, 708
Remote Config [candidate] (698.28 µs) : 0, 698
Telemetry [baseline] (7.373 ms) : 0, 7373
Telemetry [candidate] (7.438 ms) : 0, 7438
section iast
BytebuddyAgent [baseline] (783.274 ms) : 0, 783274
BytebuddyAgent [candidate] (789.921 ms) : 0, 789921
GlobalTracer [baseline] (296.947 ms) : 0, 296947
GlobalTracer [candidate] (298.642 ms) : 0, 298642
AppSec [baseline] (51.284 ms) : 0, 51284
AppSec [candidate] (50.619 ms) : 0, 50619
IAST [baseline] (23.381 ms) : 0, 23381
IAST [candidate] (24.725 ms) : 0, 24725
Remote Config [baseline] (586.806 µs) : 0, 587
Remote Config [candidate] (607.971 µs) : 0, 608
Telemetry [baseline] (8.635 ms) : 0, 8635
Telemetry [candidate] (8.858 ms) : 0, 8858
section iast_HARDCODED_SECRET_DISABLED
BytebuddyAgent [baseline] (783.226 ms) : 0, 783226
BytebuddyAgent [candidate] (784.559 ms) : 0, 784559
GlobalTracer [baseline] (297.532 ms) : 0, 297532
GlobalTracer [candidate] (298.049 ms) : 0, 298049
AppSec [baseline] (49.711 ms) : 0, 49711
AppSec [candidate] (48.785 ms) : 0, 48785
IAST [baseline] (26.043 ms) : 0, 26043
IAST [candidate] (22.667 ms) : 0, 22667
Remote Config [baseline] (593.743 µs) : 0, 594
Remote Config [candidate] (593.448 µs) : 0, 593
Telemetry [baseline] (7.761 ms) : 0, 7761
Telemetry [candidate] (10.377 ms) : 0, 10377
section iast_TELEMETRY_OFF
BytebuddyAgent [baseline] (780.088 ms) : 0, 780088
BytebuddyAgent [candidate] (779.766 ms) : 0, 779766
GlobalTracer [baseline] (296.994 ms) : 0, 296994
GlobalTracer [candidate] (297.104 ms) : 0, 297104
AppSec [baseline] (50.066 ms) : 0, 50066
AppSec [candidate] (46.894 ms) : 0, 46894
IAST [baseline] (24.316 ms) : 0, 24316
IAST [candidate] (27.407 ms) : 0, 27407
Remote Config [baseline] (598.796 µs) : 0, 599
Remote Config [candidate] (605.168 µs) : 0, 605
Telemetry [baseline] (7.623 ms) : 0, 7623
Telemetry [candidate] (6.859 ms) : 0, 6859
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.39.0-SNAPSHOT~2675ae1381, baseline=1.39.0-SNAPSHOT~23ce5ef2d1
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.046 s) : 0, 1046172
Total [baseline] (10.262 s) : 0, 10261825
Agent [candidate] (1.048 s) : 0, 1047814
Total [candidate] (10.343 s) : 0, 10342986
section appsec
Agent [baseline] (1.174 s) : 0, 1173978
Total [baseline] (10.5 s) : 0, 10499586
Agent [candidate] (1.166 s) : 0, 1166179
Total [candidate] (10.495 s) : 0, 10495189
section iast
Agent [baseline] (1.175 s) : 0, 1175020
Total [baseline] (10.755 s) : 0, 10754770
Agent [candidate] (1.175 s) : 0, 1175395
Total [candidate] (10.778 s) : 0, 10777826
section profiling
Agent [baseline] (1.245 s) : 0, 1244954
Total [baseline] (10.578 s) : 0, 10578337
Agent [candidate] (1.245 s) : 0, 1244676
Total [candidate] (10.593 s) : 0, 10592647
gantt
title petclinic - break down per module: candidate=1.39.0-SNAPSHOT~2675ae1381, baseline=1.39.0-SNAPSHOT~23ce5ef2d1
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (667.494 ms) : 0, 667494
BytebuddyAgent [candidate] (668.61 ms) : 0, 668610
GlobalTracer [baseline] (306.528 ms) : 0, 306528
GlobalTracer [candidate] (307.059 ms) : 0, 307059
AppSec [baseline] (50.735 ms) : 0, 50735
AppSec [candidate] (50.559 ms) : 0, 50559
Remote Config [baseline] (684.23 µs) : 0, 684
Remote Config [candidate] (693.058 µs) : 0, 693
Telemetry [baseline] (7.292 ms) : 0, 7292
Telemetry [candidate] (7.413 ms) : 0, 7413
section appsec
BytebuddyAgent [baseline] (682.263 ms) : 0, 682263
BytebuddyAgent [candidate] (678.533 ms) : 0, 678533
GlobalTracer [baseline] (301.93 ms) : 0, 301930
GlobalTracer [candidate] (300.339 ms) : 0, 300339
AppSec [baseline] (156.568 ms) : 0, 156568
AppSec [candidate] (155.128 ms) : 0, 155128
IAST [baseline] (20.967 ms) : 0, 20967
IAST [candidate] (19.646 ms) : 0, 19646
Remote Config [baseline] (616.491 µs) : 0, 616
Remote Config [candidate] (610.019 µs) : 0, 610
Telemetry [baseline] (8.547 ms) : 0, 8547
Telemetry [candidate] (8.217 ms) : 0, 8217
section iast
BytebuddyAgent [baseline] (782.177 ms) : 0, 782177
BytebuddyAgent [candidate] (782.581 ms) : 0, 782581
GlobalTracer [baseline] (296.41 ms) : 0, 296410
GlobalTracer [candidate] (296.464 ms) : 0, 296464
AppSec [baseline] (51.267 ms) : 0, 51267
AppSec [candidate] (52.538 ms) : 0, 52538
IAST [baseline] (21.694 ms) : 0, 21694
IAST [candidate] (22.69 ms) : 0, 22690
Remote Config [baseline] (579.883 µs) : 0, 580
Remote Config [candidate] (578.721 µs) : 0, 579
Telemetry [baseline] (9.369 ms) : 0, 9369
Telemetry [candidate] (7.041 ms) : 0, 7041
section profiling
BytebuddyAgent [baseline] (664.104 ms) : 0, 664104
BytebuddyAgent [candidate] (663.228 ms) : 0, 663228
GlobalTracer [baseline] (389.955 ms) : 0, 389955
GlobalTracer [candidate] (391.207 ms) : 0, 391207
AppSec [baseline] (51.514 ms) : 0, 51514
AppSec [candidate] (50.973 ms) : 0, 50973
Remote Config [baseline] (693.44 µs) : 0, 693
Remote Config [candidate] (684.339 µs) : 0, 684
Telemetry [baseline] (7.217 ms) : 0, 7217
Telemetry [candidate] (7.196 ms) : 0, 7196
ProfilingAgent [baseline] (94.274 ms) : 0, 94274
ProfilingAgent [candidate] (94.221 ms) : 0, 94221
Profiling [baseline] (94.299 ms) : 0, 94299
Profiling [candidate] (94.245 ms) : 0, 94245
LoadParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 11 metrics, 17 unstable metrics. Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.39.0-SNAPSHOT~2675ae1381, baseline=1.39.0-SNAPSHOT~23ce5ef2d1
dateFormat X
axisFormat %s
section baseline
no_agent (1.348 ms) : 1329, 1367
. : milestone, 1348,
appsec (1.701 ms) : 1679, 1723
. : milestone, 1701,
appsec_no_iast (1.722 ms) : 1695, 1749
. : milestone, 1722,
iast (1.488 ms) : 1465, 1510
. : milestone, 1488,
profiling (1.53 ms) : 1504, 1556
. : milestone, 1530,
tracing (1.458 ms) : 1433, 1483
. : milestone, 1458,
section candidate
no_agent (1.343 ms) : 1324, 1362
. : milestone, 1343,
appsec (1.713 ms) : 1688, 1737
. : milestone, 1713,
appsec_no_iast (1.702 ms) : 1677, 1726
. : milestone, 1702,
iast (1.459 ms) : 1437, 1482
. : milestone, 1459,
profiling (1.472 ms) : 1448, 1497
. : milestone, 1472,
tracing (1.466 ms) : 1439, 1494
. : milestone, 1466,
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.39.0-SNAPSHOT~2675ae1381, baseline=1.39.0-SNAPSHOT~23ce5ef2d1
dateFormat X
axisFormat %s
section baseline
no_agent (368.487 µs) : 349, 388
. : milestone, 368,
iast (480.992 µs) : 460, 502
. : milestone, 481,
iast_FULL (550.642 µs) : 530, 572
. : milestone, 551,
iast_GLOBAL (505.865 µs) : 484, 528
. : milestone, 506,
iast_HARDCODED_SECRET_DISABLED (489.928 µs) : 468, 512
. : milestone, 490,
iast_INACTIVE (446.225 µs) : 425, 467
. : milestone, 446,
iast_TELEMETRY_OFF (466.947 µs) : 446, 488
. : milestone, 467,
tracing (439.77 µs) : 419, 460
. : milestone, 440,
section candidate
no_agent (363.266 µs) : 343, 383
. : milestone, 363,
iast (476.419 µs) : 455, 498
. : milestone, 476,
iast_FULL (547.875 µs) : 527, 569
. : milestone, 548,
iast_GLOBAL (501.407 µs) : 480, 523
. : milestone, 501,
iast_HARDCODED_SECRET_DISABLED (481.057 µs) : 460, 502
. : milestone, 481,
iast_INACTIVE (451.071 µs) : 430, 472
. : milestone, 451,
iast_TELEMETRY_OFF (473.502 µs) : 452, 495
. : milestone, 474,
tracing (442.285 µs) : 422, 463
. : milestone, 442,
DacapoParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 12 metrics, 0 unstable metrics. Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.39.0-SNAPSHOT~2675ae1381, baseline=1.39.0-SNAPSHOT~23ce5ef2d1
dateFormat X
axisFormat %s
section baseline
no_agent (1.462 ms) : 1451, 1474
. : milestone, 1462,
appsec (2.222 ms) : 2188, 2257
. : milestone, 2222,
iast (1.961 ms) : 1920, 2003
. : milestone, 1961,
iast_GLOBAL (2.03 ms) : 1986, 2073
. : milestone, 2030,
profiling (1.869 ms) : 1835, 1903
. : milestone, 1869,
tracing (1.859 ms) : 1826, 1892
. : milestone, 1859,
section candidate
no_agent (1.467 ms) : 1455, 1478
. : milestone, 1467,
appsec (2.225 ms) : 2190, 2261
. : milestone, 2225,
iast (1.979 ms) : 1937, 2021
. : milestone, 1979,
iast_GLOBAL (2.029 ms) : 1985, 2072
. : milestone, 2029,
profiling (1.876 ms) : 1841, 1911
. : milestone, 1876,
tracing (1.846 ms) : 1813, 1879
. : milestone, 1846,
Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.39.0-SNAPSHOT~2675ae1381, baseline=1.39.0-SNAPSHOT~23ce5ef2d1
dateFormat X
axisFormat %s
section baseline
no_agent (14.964 s) : 14964000, 14964000
. : milestone, 14964000,
appsec (15.003 s) : 15003000, 15003000
. : milestone, 15003000,
iast (18.847 s) : 18847000, 18847000
. : milestone, 18847000,
iast_GLOBAL (17.787 s) : 17787000, 17787000
. : milestone, 17787000,
profiling (15.144 s) : 15144000, 15144000
. : milestone, 15144000,
tracing (15.28 s) : 15280000, 15280000
. : milestone, 15280000,
section candidate
no_agent (15.521 s) : 15521000, 15521000
. : milestone, 15521000,
appsec (15.07 s) : 15070000, 15070000
. : milestone, 15070000,
iast (18.828 s) : 18828000, 18828000
. : milestone, 18828000,
iast_GLOBAL (17.906 s) : 17906000, 17906000
. : milestone, 17906000,
profiling (15.322 s) : 15322000, 15322000
. : milestone, 15322000,
tracing (14.997 s) : 14997000, 14997000
. : milestone, 14997000,
|
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 don't follow your changes compared to the PR description "Finish span in the finally block to avoid leaks after exceptions.". Can you elaborate a bit more but here is what I understand:
First, if there is an exception from statement closure, it will be caught and the span will still finish.
Then, you reduce the exception catch scope from Exception
to SQLException
so you will be catching fewer kinds of issues. It is expected?
Finally, you are no more closing the span if the instrumentedStatement
is null
, so you will be leaking more spans.
@vandonr As you approved the changes, can you help me with the concerns I raise in the comment above? |
Hmm, yeah, so this PR is stemming from this comment on a previous PR. Actually I read the code a bit too fast without seeing your comment, but you are right, this is too high, I think we need yet another finally block a bit lower :( |
Thanks for catching it. I implemented the nested exception handling now. |
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.
lgtm now
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.
Thanks for the fix and the discussion about Exception
/Throwable
! 👍
What Does This Do
Finish span in the
finally
block to avoid leaks after exceptions.Motivation
Additional Notes
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]