-
Notifications
You must be signed in to change notification settings - Fork 314
Add option to JDBC instrumentation to always append DBM comment #9798
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
} | ||
|
||
public boolean shouldAppendSqlComment() { | ||
return Config.get().isDbmAlwaysAppendSqlComment(); |
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.
If this Config.get().isDbm... is stored in a static final, then the JIT will be able to optimize this better. In that case, the JVM will be able constant propagate static final vars. And this method will also get inlined, so it will be equivalent to just using a constant inline in the code.
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.
That's what I wanted to do at first. But I haven't found why, it doesn't work on one specific test:
Lines 113 to 114 in 9f6ef81
// Using INJECT_COMMENT fails to update when a test calls injectSysConfig | |
if (!DECORATE.shouldInjectSQLComment()) { |
The other test, it works without issue. The issue happening is that when calling injectSysConfig
during setup, the value isn't set.
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.
Can you clarify what you mean by the value isn't set? A static final is set once and only once during class initialization.
Do you mean that the values aren't available to Config? The values used by Config are external and should be available. Unless you are also wanting to support remote configuration here
...src/main/java/datadog/trace/instrumentation/jdbc/DBMCompatibleConnectionInstrumentation.java
Outdated
Show resolved
Hide resolved
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.
Changes look good to me, just a small comment about documentation.
🎯 Code Coverage 🔗 Commit SHA: e620029 | Docs | Was this helpful? Give us feedback! |
BenchmarksStartupParameters
See matching parameters
SummaryFound 1 performance improvements and 0 performance regressions! Performance is the same for 58 metrics, 6 unstable metrics.
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.55.0-SNAPSHOT~e620029e84, baseline=1.55.0-SNAPSHOT~5d0320341a
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.023 s) : 0, 1022640
Total [baseline] (10.659 s) : 0, 10658551
Agent [candidate] (1.02 s) : 0, 1020024
Total [candidate] (10.672 s) : 0, 10671849
section appsec
Agent [baseline] (1.196 s) : 0, 1196416
Total [baseline] (10.929 s) : 0, 10928699
Agent [candidate] (1.196 s) : 0, 1195828
Total [candidate] (10.774 s) : 0, 10773800
section iast
Agent [baseline] (1.151 s) : 0, 1150530
Total [baseline] (11.064 s) : 0, 11063854
Agent [candidate] (1.158 s) : 0, 1158089
Total [candidate] (11.128 s) : 0, 11128356
section profiling
Agent [baseline] (1.177 s) : 0, 1176597
Total [baseline] (10.989 s) : 0, 10988872
Agent [candidate] (1.164 s) : 0, 1163948
Total [candidate] (10.918 s) : 0, 10918408
gantt
title petclinic - break down per module: candidate=1.55.0-SNAPSHOT~e620029e84, baseline=1.55.0-SNAPSHOT~5d0320341a
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.468 ms) : 0, 1468
crashtracking [candidate] (1.461 ms) : 0, 1461
BytebuddyAgent [baseline] (697.732 ms) : 0, 697732
BytebuddyAgent [candidate] (694.298 ms) : 0, 694298
GlobalTracer [baseline] (244.489 ms) : 0, 244489
GlobalTracer [candidate] (243.025 ms) : 0, 243025
AppSec [baseline] (32.484 ms) : 0, 32484
AppSec [candidate] (32.344 ms) : 0, 32344
Debugger [baseline] (6.374 ms) : 0, 6374
Debugger [candidate] (6.249 ms) : 0, 6249
Remote Config [baseline] (697.84 µs) : 0, 698
Remote Config [candidate] (664.155 µs) : 0, 664
Telemetry [baseline] (9.403 ms) : 0, 9403
Telemetry [candidate] (9.288 ms) : 0, 9288
Flare Poller [baseline] (8.785 ms) : 0, 8785
Flare Poller [candidate] (11.533 ms) : 0, 11533
section appsec
crashtracking [baseline] (1.481 ms) : 0, 1481
crashtracking [candidate] (1.466 ms) : 0, 1466
BytebuddyAgent [baseline] (719.134 ms) : 0, 719134
BytebuddyAgent [candidate] (718.771 ms) : 0, 718771
GlobalTracer [baseline] (234.619 ms) : 0, 234619
GlobalTracer [candidate] (235.299 ms) : 0, 235299
IAST [baseline] (24.969 ms) : 0, 24969
IAST [candidate] (24.996 ms) : 0, 24996
AppSec [baseline] (175.701 ms) : 0, 175701
AppSec [candidate] (174.947 ms) : 0, 174947
Debugger [baseline] (6.056 ms) : 0, 6056
Debugger [candidate] (6.075 ms) : 0, 6075
Remote Config [baseline] (643.339 µs) : 0, 643
Remote Config [candidate] (627.675 µs) : 0, 628
Telemetry [baseline] (8.65 ms) : 0, 8650
Telemetry [candidate] (8.575 ms) : 0, 8575
Flare Poller [baseline] (3.961 ms) : 0, 3961
Flare Poller [candidate] (3.923 ms) : 0, 3923
section iast
crashtracking [baseline] (1.467 ms) : 0, 1467
crashtracking [candidate] (1.482 ms) : 0, 1482
BytebuddyAgent [baseline] (814.092 ms) : 0, 814092
BytebuddyAgent [candidate] (819.935 ms) : 0, 819935
GlobalTracer [baseline] (231.721 ms) : 0, 231721
GlobalTracer [candidate] (232.832 ms) : 0, 232832
IAST [baseline] (26.873 ms) : 0, 26873
IAST [candidate] (27.153 ms) : 0, 27153
AppSec [baseline] (35.2 ms) : 0, 35200
AppSec [candidate] (35.424 ms) : 0, 35424
Debugger [baseline] (6.143 ms) : 0, 6143
Debugger [candidate] (6.218 ms) : 0, 6218
Remote Config [baseline] (611.835 µs) : 0, 612
Remote Config [candidate] (597.436 µs) : 0, 597
Telemetry [baseline] (8.798 ms) : 0, 8798
Telemetry [candidate] (8.748 ms) : 0, 8748
Flare Poller [baseline] (4.243 ms) : 0, 4243
Flare Poller [candidate] (4.26 ms) : 0, 4260
section profiling
crashtracking [baseline] (1.477 ms) : 0, 1477
crashtracking [candidate] (1.453 ms) : 0, 1453
BytebuddyAgent [baseline] (729.395 ms) : 0, 729395
BytebuddyAgent [candidate] (721.342 ms) : 0, 721342
GlobalTracer [baseline] (220.361 ms) : 0, 220361
GlobalTracer [candidate] (218.664 ms) : 0, 218664
AppSec [baseline] (32.535 ms) : 0, 32535
AppSec [candidate] (32.197 ms) : 0, 32197
Debugger [baseline] (8.33 ms) : 0, 8330
Debugger [candidate] (7.446 ms) : 0, 7446
Remote Config [baseline] (740.511 µs) : 0, 741
Remote Config [candidate] (695.016 µs) : 0, 695
Telemetry [baseline] (13.77 ms) : 0, 13770
Telemetry [candidate] (15.173 ms) : 0, 15173
Flare Poller [baseline] (4.961 ms) : 0, 4961
Flare Poller [candidate] (4.133 ms) : 0, 4133
ProfilingAgent [baseline] (110.636 ms) : 0, 110636
ProfilingAgent [candidate] (109.171 ms) : 0, 109171
Profiling [baseline] (111.269 ms) : 0, 111269
Profiling [candidate] (109.791 ms) : 0, 109791
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.55.0-SNAPSHOT~e620029e84, baseline=1.55.0-SNAPSHOT~5d0320341a
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.029 s) : 0, 1029366
Total [baseline] (8.703 s) : 0, 8702875
Agent [candidate] (1.021 s) : 0, 1021097
Total [candidate] (8.667 s) : 0, 8667439
section iast
Agent [baseline] (1.155 s) : 0, 1155179
Total [baseline] (9.297 s) : 0, 9296848
Agent [candidate] (1.15 s) : 0, 1150490
Total [candidate] (9.264 s) : 0, 9263658
gantt
title insecure-bank - break down per module: candidate=1.55.0-SNAPSHOT~e620029e84, baseline=1.55.0-SNAPSHOT~5d0320341a
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.486 ms) : 0, 1486
crashtracking [candidate] (1.476 ms) : 0, 1476
BytebuddyAgent [baseline] (700.726 ms) : 0, 700726
BytebuddyAgent [candidate] (695.41 ms) : 0, 695410
GlobalTracer [baseline] (244.772 ms) : 0, 244772
GlobalTracer [candidate] (243.531 ms) : 0, 243531
AppSec [baseline] (32.77 ms) : 0, 32770
AppSec [candidate] (32.228 ms) : 0, 32228
Debugger [baseline] (6.382 ms) : 0, 6382
Debugger [candidate] (6.333 ms) : 0, 6333
Remote Config [baseline] (683.191 µs) : 0, 683
Remote Config [candidate] (667.713 µs) : 0, 668
Telemetry [baseline] (9.498 ms) : 0, 9498
Telemetry [candidate] (9.436 ms) : 0, 9436
Flare Poller [baseline] (11.801 ms) : 0, 11801
Flare Poller [candidate] (10.864 ms) : 0, 10864
section iast
crashtracking [baseline] (1.493 ms) : 0, 1493
crashtracking [candidate] (1.495 ms) : 0, 1495
BytebuddyAgent [baseline] (817.942 ms) : 0, 817942
BytebuddyAgent [candidate] (814.788 ms) : 0, 814788
GlobalTracer [baseline] (231.952 ms) : 0, 231952
GlobalTracer [candidate] (231.474 ms) : 0, 231474
IAST [baseline] (26.921 ms) : 0, 26921
IAST [candidate] (26.516 ms) : 0, 26516
AppSec [baseline] (35.31 ms) : 0, 35310
AppSec [candidate] (35.05 ms) : 0, 35050
Debugger [baseline] (6.239 ms) : 0, 6239
Debugger [candidate] (6.168 ms) : 0, 6168
Remote Config [baseline] (619.118 µs) : 0, 619
Remote Config [candidate] (599.837 µs) : 0, 600
Telemetry [baseline] (8.846 ms) : 0, 8846
Telemetry [candidate] (8.677 ms) : 0, 8677
Flare Poller [baseline] (4.39 ms) : 0, 4390
Flare Poller [candidate] (4.301 ms) : 0, 4301
LoadParameters
See matching parameters
SummaryFound 2 performance improvements and 4 performance regressions! Performance is the same for 6 metrics, 12 unstable metrics.
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.55.0-SNAPSHOT~e620029e84, baseline=1.55.0-SNAPSHOT~5d0320341a
dateFormat X
axisFormat %s
section baseline
no_agent (37.902 ms) : 37597, 38208
. : milestone, 37902,
appsec (48.582 ms) : 48134, 49030
. : milestone, 48582,
code_origins (42.873 ms) : 42500, 43245
. : milestone, 42873,
iast (44.182 ms) : 43797, 44568
. : milestone, 44182,
profiling (50.705 ms) : 50254, 51155
. : milestone, 50705,
tracing (43.261 ms) : 42889, 43633
. : milestone, 43261,
section candidate
no_agent (37.887 ms) : 37578, 38195
. : milestone, 37887,
appsec (48.654 ms) : 48213, 49095
. : milestone, 48654,
code_origins (44.174 ms) : 43799, 44548
. : milestone, 44174,
iast (45.872 ms) : 45472, 46272
. : milestone, 45872,
profiling (47.893 ms) : 47409, 48376
. : milestone, 47893,
tracing (44.266 ms) : 43885, 44648
. : milestone, 44266,
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.55.0-SNAPSHOT~e620029e84, baseline=1.55.0-SNAPSHOT~5d0320341a
dateFormat X
axisFormat %s
section baseline
no_agent (4.2 ms) : 4146, 4254
. : milestone, 4200,
iast (9.157 ms) : 9008, 9306
. : milestone, 9157,
iast_FULL (14.064 ms) : 13781, 14348
. : milestone, 14064,
iast_GLOBAL (10.61 ms) : 10422, 10799
. : milestone, 10610,
profiling (9.052 ms) : 8894, 9209
. : milestone, 9052,
tracing (7.509 ms) : 7402, 7617
. : milestone, 7509,
section candidate
no_agent (4.328 ms) : 4279, 4377
. : milestone, 4328,
iast (9.174 ms) : 9024, 9324
. : milestone, 9174,
iast_FULL (15.014 ms) : 14714, 15315
. : milestone, 15014,
iast_GLOBAL (11.081 ms) : 10882, 11280
. : milestone, 11081,
profiling (8.606 ms) : 8454, 8758
. : milestone, 8606,
tracing (7.596 ms) : 7488, 7703
. : milestone, 7596,
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 tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.55.0-SNAPSHOT~e620029e84, baseline=1.55.0-SNAPSHOT~5d0320341a
dateFormat X
axisFormat %s
section baseline
no_agent (1.478 ms) : 1466, 1489
. : milestone, 1478,
appsec (3.653 ms) : 3440, 3867
. : milestone, 3653,
iast (2.217 ms) : 2154, 2281
. : milestone, 2217,
iast_GLOBAL (2.251 ms) : 2187, 2315
. : milestone, 2251,
profiling (2.078 ms) : 2026, 2131
. : milestone, 2078,
tracing (2.044 ms) : 1994, 2094
. : milestone, 2044,
section candidate
no_agent (1.479 ms) : 1467, 1490
. : milestone, 1479,
appsec (3.653 ms) : 3440, 3866
. : milestone, 3653,
iast (2.214 ms) : 2150, 2278
. : milestone, 2214,
iast_GLOBAL (2.26 ms) : 2196, 2324
. : milestone, 2260,
profiling (2.483 ms) : 2257, 2709
. : milestone, 2483,
tracing (2.025 ms) : 1976, 2075
. : milestone, 2025,
Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.55.0-SNAPSHOT~e620029e84, baseline=1.55.0-SNAPSHOT~5d0320341a
dateFormat X
axisFormat %s
section baseline
no_agent (15.607 s) : 15607000, 15607000
. : milestone, 15607000,
appsec (15.314 s) : 15314000, 15314000
. : milestone, 15314000,
iast (18.62 s) : 18620000, 18620000
. : milestone, 18620000,
iast_GLOBAL (17.923 s) : 17923000, 17923000
. : milestone, 17923000,
profiling (15.148 s) : 15148000, 15148000
. : milestone, 15148000,
tracing (15.032 s) : 15032000, 15032000
. : milestone, 15032000,
section candidate
no_agent (15.336 s) : 15336000, 15336000
. : milestone, 15336000,
appsec (15.038 s) : 15038000, 15038000
. : milestone, 15038000,
iast (18.715 s) : 18715000, 18715000
. : milestone, 18715000,
iast_GLOBAL (17.962 s) : 17962000, 17962000
. : milestone, 17962000,
profiling (15.427 s) : 15427000, 15427000
. : milestone, 15427000,
tracing (15.148 s) : 15148000, 15148000
. : milestone, 15148000,
|
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.
Overall, I'm fine with the change as is. The optimizations would be nice, but I don't think they are essential.
What Does This Do
This PR adds a new option to the JDBC instrumentation. This option allows to always append the comment injected when DBM trace propagation is enabled.
Motivation
This is useful to add supports to GCP’s Query Insights on Cloud SQL. When query insights is enabled, it is prepending a comment to all the queries executed. If another comment exists, it can't recognize its own comment.
Additional Notes
Contributor Checklist
type:
and (comp:
orinst:
) labels in addition to any useful labelsclose
,fix
or any linking keywords when referencing an issue.Use
solves
instead, and assign the PR milestone to the issueJira ticket: DBMON-5791 APMS-17494