Add support for DBM comment injection with MongoDB #9589
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🎯 Code Coverage 🔗 Commit SHA: 704d509 | Docs | Was this helpful? Give us feedback! |
Yoone
changed the title
BenchmarksStartupParameters
See matching parameters
SummaryFound 7 performance improvements and 3 performance regressions! Performance is the same for 46 metrics, 3 unstable metrics.
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.54.0-SNAPSHOT~704d509e92, baseline=1.55.0-SNAPSHOT~37454135e4
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.018 s) : 0, 1018123
Total [baseline] (10.679 s) : 0, 10678852
Agent [candidate] (1.008 s) : 0, 1007855
Total [candidate] (10.665 s) : 0, 10665453
section appsec
Agent [baseline] (1.193 s) : 0, 1192827
Total [baseline] (10.955 s) : 0, 10954518
Agent [candidate] (1.191 s) : 0, 1191088
Total [candidate] (10.957 s) : 0, 10957231
section iast
Agent [baseline] (1.151 s) : 0, 1150615
Total [baseline] (10.876 s) : 0, 10876245
Agent [candidate] (1.144 s) : 0, 1144153
Total [candidate] (11.011 s) : 0, 11010621
section profiling
Agent [baseline] (1.177 s) : 0, 1176890
Total [baseline] (11.083 s) : 0, 11082969
Agent [candidate] (1.151 s) : 0, 1150717
Total [candidate] (11.016 s) : 0, 11016227
gantt
title petclinic - break down per module: candidate=1.54.0-SNAPSHOT~704d509e92, baseline=1.55.0-SNAPSHOT~37454135e4
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.464 ms) : 0, 1464
crashtracking [candidate] (1.448 ms) : 0, 1448
BytebuddyAgent [baseline] (694.961 ms) : 0, 694961
BytebuddyAgent [candidate] (690.591 ms) : 0, 690591
GlobalTracer [baseline] (241.466 ms) : 0, 241466
GlobalTracer [candidate] (247.994 ms) : 0, 247994
AppSec [baseline] (32.633 ms) : 0, 32633
AppSec [candidate] (30.855 ms) : 0, 30855
Debugger [baseline] (6.394 ms) : 0, 6394
Debugger [candidate] (6.315 ms) : 0, 6315
Remote Config [baseline] (690.261 µs) : 0, 690
Remote Config [candidate] (673.874 µs) : 0, 674
Telemetry [baseline] (9.197 ms) : 0, 9197
Telemetry [candidate] (8.839 ms) : 0, 8839
Flare Poller [baseline] (10.276 ms) : 0, 10276
section appsec
crashtracking [baseline] (1.456 ms) : 0, 1456
crashtracking [candidate] (1.464 ms) : 0, 1464
BytebuddyAgent [baseline] (716.757 ms) : 0, 716757
BytebuddyAgent [candidate] (714.169 ms) : 0, 714169
GlobalTracer [baseline] (234.198 ms) : 0, 234198
GlobalTracer [candidate] (240.991 ms) : 0, 240991
AppSec [baseline] (175.335 ms) : 0, 175335
AppSec [candidate] (171.168 ms) : 0, 171168
Debugger [baseline] (6.134 ms) : 0, 6134
Debugger [candidate] (6.032 ms) : 0, 6032
Remote Config [baseline] (646.06 µs) : 0, 646
Remote Config [candidate] (642.767 µs) : 0, 643
Telemetry [baseline] (8.374 ms) : 0, 8374
Telemetry [candidate] (10.756 ms) : 0, 10756
Flare Poller [baseline] (3.922 ms) : 0, 3922
IAST [baseline] (24.961 ms) : 0, 24961
IAST [candidate] (24.727 ms) : 0, 24727
section iast
crashtracking [baseline] (1.456 ms) : 0, 1456
crashtracking [candidate] (1.456 ms) : 0, 1456
BytebuddyAgent [baseline] (814.781 ms) : 0, 814781
BytebuddyAgent [candidate] (809.257 ms) : 0, 809257
GlobalTracer [baseline] (231.727 ms) : 0, 231727
GlobalTracer [candidate] (237.183 ms) : 0, 237183
AppSec [baseline] (35.524 ms) : 0, 35524
AppSec [candidate] (33.887 ms) : 0, 33887
Debugger [baseline] (6.068 ms) : 0, 6068
Debugger [candidate] (6.067 ms) : 0, 6067
Remote Config [baseline] (606.148 µs) : 0, 606
Remote Config [candidate] (593.97 µs) : 0, 594
Telemetry [baseline] (8.515 ms) : 0, 8515
Telemetry [candidate] (8.259 ms) : 0, 8259
Flare Poller [baseline] (4.164 ms) : 0, 4164
IAST [baseline] (26.572 ms) : 0, 26572
IAST [candidate] (26.149 ms) : 0, 26149
section profiling
crashtracking [baseline] (1.445 ms) : 0, 1445
crashtracking [candidate] (1.441 ms) : 0, 1441
BytebuddyAgent [baseline] (732.017 ms) : 0, 732017
BytebuddyAgent [candidate] (718.187 ms) : 0, 718187
GlobalTracer [baseline] (219.215 ms) : 0, 219215
GlobalTracer [candidate] (223.372 ms) : 0, 223372
AppSec [baseline] (33.786 ms) : 0, 33786
AppSec [candidate] (31.313 ms) : 0, 31313
Debugger [baseline] (6.587 ms) : 0, 6587
Debugger [candidate] (6.512 ms) : 0, 6512
Remote Config [baseline] (698.89 µs) : 0, 699
Remote Config [candidate] (705.643 µs) : 0, 706
Telemetry [baseline] (15.725 ms) : 0, 15725
Telemetry [candidate] (16.029 ms) : 0, 16029
Flare Poller [baseline] (5.016 ms) : 0, 5016
ProfilingAgent [baseline] (108.662 ms) : 0, 108662
ProfilingAgent [candidate] (100.915 ms) : 0, 100915
Profiling [baseline] (109.804 ms) : 0, 109804
Profiling [candidate] (101.565 ms) : 0, 101565
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.54.0-SNAPSHOT~704d509e92, baseline=1.55.0-SNAPSHOT~37454135e4
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.019 s) : 0, 1018842
Total [baseline] (8.673 s) : 0, 8672537
Agent [candidate] (1.005 s) : 0, 1005369
Total [candidate] (8.652 s) : 0, 8652225
section iast
Agent [baseline] (1.165 s) : 0, 1164665
Total [baseline] (9.275 s) : 0, 9275431
Agent [candidate] (1.152 s) : 0, 1151886
Total [candidate] (9.294 s) : 0, 9294299
gantt
title insecure-bank - break down per module: candidate=1.54.0-SNAPSHOT~704d509e92, baseline=1.55.0-SNAPSHOT~37454135e4
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.463 ms) : 0, 1463
crashtracking [candidate] (1.451 ms) : 0, 1451
BytebuddyAgent [baseline] (694.879 ms) : 0, 694879
BytebuddyAgent [candidate] (688.174 ms) : 0, 688174
GlobalTracer [baseline] (241.899 ms) : 0, 241899
GlobalTracer [candidate] (247.782 ms) : 0, 247782
AppSec [baseline] (33.048 ms) : 0, 33048
AppSec [candidate] (30.931 ms) : 0, 30931
Debugger [baseline] (6.403 ms) : 0, 6403
Debugger [candidate] (6.374 ms) : 0, 6374
Remote Config [baseline] (682.651 µs) : 0, 683
Remote Config [candidate] (669.638 µs) : 0, 670
Telemetry [baseline] (9.246 ms) : 0, 9246
Telemetry [candidate] (8.939 ms) : 0, 8939
Flare Poller [baseline] (10.198 ms) : 0, 10198
section iast
crashtracking [baseline] (1.472 ms) : 0, 1472
crashtracking [candidate] (1.471 ms) : 0, 1471
BytebuddyAgent [baseline] (823.948 ms) : 0, 823948
BytebuddyAgent [candidate] (815.202 ms) : 0, 815202
GlobalTracer [baseline] (235.19 ms) : 0, 235190
GlobalTracer [candidate] (238.541 ms) : 0, 238541
AppSec [baseline] (35.661 ms) : 0, 35661
AppSec [candidate] (34.188 ms) : 0, 34188
Debugger [baseline] (6.157 ms) : 0, 6157
Debugger [candidate] (6.038 ms) : 0, 6038
Remote Config [baseline] (615.962 µs) : 0, 616
Remote Config [candidate] (598.632 µs) : 0, 599
Telemetry [baseline] (8.788 ms) : 0, 8788
Telemetry [candidate] (8.367 ms) : 0, 8367
Flare Poller [baseline] (4.2 ms) : 0, 4200
IAST [baseline] (27.379 ms) : 0, 27379
IAST [candidate] (26.32 ms) : 0, 26320
LoadParameters
See matching parameters
SummaryFound 2 performance improvements and 1 performance regressions! Performance is the same for 9 metrics, 12 unstable metrics.
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.54.0-SNAPSHOT~704d509e92, baseline=1.55.0-SNAPSHOT~37454135e4
dateFormat X
axisFormat %s
section baseline
no_agent (37.512 ms) : 37205, 37819
. : milestone, 37512,
appsec (47.723 ms) : 47301, 48144
. : milestone, 47723,
code_origins (44.82 ms) : 44442, 45197
. : milestone, 44820,
iast (45.405 ms) : 45009, 45802
. : milestone, 45405,
profiling (48.392 ms) : 47960, 48824
. : milestone, 48392,
tracing (46.31 ms) : 45915, 46705
. : milestone, 46310,
section candidate
no_agent (36.881 ms) : 36590, 37173
. : milestone, 36881,
appsec (48.337 ms) : 47906, 48769
. : milestone, 48337,
code_origins (44.43 ms) : 44051, 44809
. : milestone, 44430,
iast (44.834 ms) : 44453, 45215
. : milestone, 44834,
profiling (48.374 ms) : 47926, 48822
. : milestone, 48374,
tracing (42.981 ms) : 42603, 43359
. : milestone, 42981,
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.54.0-SNAPSHOT~704d509e92, baseline=1.55.0-SNAPSHOT~37454135e4
dateFormat X
axisFormat %s
section baseline
no_agent (4.236 ms) : 4181, 4291
. : milestone, 4236,
iast (10.317 ms) : 10141, 10493
. : milestone, 10317,
iast_FULL (14.846 ms) : 14546, 15147
. : milestone, 14846,
iast_GLOBAL (10.404 ms) : 10219, 10589
. : milestone, 10404,
profiling (9.258 ms) : 9114, 9401
. : milestone, 9258,
tracing (7.568 ms) : 7455, 7682
. : milestone, 7568,
section candidate
no_agent (4.379 ms) : 4330, 4428
. : milestone, 4379,
iast (9.459 ms) : 9301, 9617
. : milestone, 9459,
iast_FULL (14.235 ms) : 13949, 14521
. : milestone, 14235,
iast_GLOBAL (10.544 ms) : 10355, 10732
. : milestone, 10544,
profiling (9.129 ms) : 8973, 9285
. : milestone, 9129,
tracing (7.657 ms) : 7547, 7767
. : milestone, 7657,
DacapoParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 11 metrics, 1 unstable metrics. Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.54.0-SNAPSHOT~704d509e92, baseline=1.55.0-SNAPSHOT~37454135e4
dateFormat X
axisFormat %s
section baseline
no_agent (1.474 ms) : 1462, 1485
. : milestone, 1474,
appsec (3.744 ms) : 3525, 3964
. : milestone, 3744,
iast (2.196 ms) : 2133, 2259
. : milestone, 2196,
iast_GLOBAL (2.255 ms) : 2191, 2319
. : milestone, 2255,
profiling (2.078 ms) : 2025, 2132
. : milestone, 2078,
tracing (2.026 ms) : 1977, 2076
. : milestone, 2026,
section candidate
no_agent (1.476 ms) : 1465, 1488
. : milestone, 1476,
appsec (3.642 ms) : 3427, 3857
. : milestone, 3642,
iast (2.188 ms) : 2125, 2250
. : milestone, 2188,
iast_GLOBAL (2.257 ms) : 2194, 2321
. : milestone, 2257,
profiling (2.049 ms) : 1998, 2100
. : milestone, 2049,
tracing (2.014 ms) : 1965, 2063
. : milestone, 2014,
Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.54.0-SNAPSHOT~704d509e92, baseline=1.55.0-SNAPSHOT~37454135e4
dateFormat X
axisFormat %s
section baseline
no_agent (15.004 s) : 15004000, 15004000
. : milestone, 15004000,
appsec (15.1 s) : 15100000, 15100000
. : milestone, 15100000,
iast (18.247 s) : 18247000, 18247000
. : milestone, 18247000,
iast_GLOBAL (18.028 s) : 18028000, 18028000
. : milestone, 18028000,
profiling (14.831 s) : 14831000, 14831000
. : milestone, 14831000,
tracing (15.212 s) : 15212000, 15212000
. : milestone, 15212000,
section candidate
no_agent (15.468 s) : 15468000, 15468000
. : milestone, 15468000,
appsec (15.045 s) : 15045000, 15045000
. : milestone, 15045000,
iast (18.18 s) : 18180000, 18180000
. : milestone, 18180000,
iast_GLOBAL (17.891 s) : 17891000, 17891000
. : milestone, 17891000,
profiling (15.394 s) : 15394000, 15394000
. : milestone, 15394000,
tracing (15.263 s) : 15263000, 15263000
. : milestone, 15263000,
|
Yoone
added
type: enhancement
Comment on lines
+16
to
+19
| private static final int SPACE_CHARS = 2; // Leading and trailing spaces | ||
| private static final int COMMENT_DELIMITERS = 4; // "/*" + "*/" | ||
| private static final int BUFFER_EXTRA = 4; | ||
| private static final int SQL_COMMENT_OVERHEAD = SPACE_CHARS + COMMENT_DELIMITERS + BUFFER_EXTRA; |
There was a problem hiding this comment.
Just thinking if we could use real .length() instead? to make sure that we are in sync with actual string consts?
There was a problem hiding this comment.
I don't feel strongly either way, but it is worth mentioning that dynamic constants are totally fine in Java.
For all intents and purposes, you can usually treat these two as equivalent.
static final int COMMENT_DELIMITERS = 4;
static final int COMMENT_DELIMITERS = "/**/".length();
While the simple constant is a little better because it gets constant propagated by javac, the dynamic constant is fine because it will still get constant propagated by the JIT.
There was a problem hiding this comment.
Yep, I think we can keep consts as we have not many of them and all in same place.
Comment on lines
+77
to
+86
| String commentContent = | ||
| SharedDBCommenter.buildComment(dbService, dbType, hostname, dbName, traceParent); | ||
|
|
||
| StringBuilder sb = new StringBuilder(sql.length() + INJECTED_COMMENT_ESTIMATED_SIZE); | ||
| if (commentContent == null) { | ||
| return sql; | ||
| } | ||
|
|
||
| // SQL-specific wrapping with /* */ | ||
| StringBuilder sb = | ||
| new StringBuilder(sql.length() + commentContent.length() + SQL_COMMENT_OVERHEAD); |
There was a problem hiding this comment.
This builder logic looks very over-complicated to me...
How about just to add sql and if needed append comment?
StringBuilder sb =
new StringBuilder(sql.length() + (appendComment ? (commentContent.length() + SQL_COMMENT_OVERHEAD) : 0));
sb.append(sql);
if (appendComment) {
sb.append(SPACE);
sb.append(OPEN_COMMENT);
sb.append(commentContent);
sb.append(CLOSE_COMMENT);
}
There was a problem hiding this comment.
Yeah, it would be okay to err on the side of a too big buffer. The GC is going to over allocate space for the array anyway, so the simpler calculation will probably be a net win both in performance and readability.
Comment on lines
+46
to
+50
| log.warn( | ||
| "Linking Database Monitoring profiles to spans is not supported for the following query type: {}. " | ||
| + "To disable this feature please set the following environment variable: DD_DBM_PROPAGATION_MODE=disabled", | ||
| event.getCommand().getClass().getSimpleName(), | ||
| e); |
There was a problem hiding this comment.
Maybe make sense to split into 2 messages to be more human friendly?
Also, do we really need to print stack-trace here (you passed e) to log.warn() I think it will print stack-trace.
There was a problem hiding this comment.
Agreed on stacktrace, removing e. For the length of the message, I took inspiration from existing logs in JDBCDecorator.
Comment on lines
+116
to
+122
| private static String buildTraceParent(AgentSpan span) { | ||
| // W3C traceparent format: version-traceId-spanId-flags | ||
| String traceIdHex = span.getTraceId().toHexStringPadded(32); | ||
| String spanIdHex = String.format("%016x", span.getSpanId()); | ||
| String flags = "00"; // '01' if sampled, '00' if not | ||
| return String.format("00-%s-%s-%s", traceIdHex, spanIdHex, flags); | ||
| } |
There was a problem hiding this comment.
I have a feeling this code can be simplifier to one String.format.
like:
spanIdHexcan be used directly in resulting format?flags = 00can be used directly in resulting format?
WDYT?
P.S. minor note: probably String.format can be heavy from performance standpoint, is this on hot path? Maybe make sense to refactor to StringBuilder (not sure here, need to benchmark?)
Yoone
force-pushed
the
yoann.bentz/mongodb-dbm-support
branch
from
22aab0a to
631958f
Compare
...e-tests/appsec/src/main/groovy/datadog/smoketest/appsec/AbstractAppSecServerSmokeTest.groovy
Outdated
Show resolved
Hide resolved
...src/main/java/datadog/trace/instrumentation/jdbc/DBMCompatibleConnectionInstrumentation.java
Outdated
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What Does This Do
Addind support for injecting trace info in
mongospans, which is used by DBM. This PR mirrors the logic that was added in dd-trace-py.Motivation
Additional Notes
SQLCommenterlogic was mostly moved toSharedDBCommenterto share it with the newMongoCommentInjector, and now focuses on adding/*+*/, which Mongo does not needTo-do
SharedDBCommenterfrom JDBC instrumentation to dd-trace-core (to fix pipeline, but also because Mongo logic should not depend on JDBC)Contributor Checklist
type:and (comp:orinst:) labels in addition to any usefull labelsclose,fixor any linking keywords when referencing an issue.Use
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]