-
Notifications
You must be signed in to change notification settings - Fork 312
Container hash tags propagation #9282
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
… propagation feature flag is enabled
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 44 metrics, 15 unstable metrics. Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.52.0-SNAPSHOT~ac2eb86cf1, baseline=1.53.0-SNAPSHOT~d43410737f
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.051 s) : 0, 1050903
Total [baseline] (8.654 s) : 0, 8654200
Agent [candidate] (1.055 s) : 0, 1054865
Total [candidate] (8.669 s) : 0, 8668902
section iast
Agent [baseline] (1.189 s) : 0, 1188898
Total [baseline] (9.339 s) : 0, 9339238
Agent [candidate] (1.184 s) : 0, 1184239
Total [candidate] (9.376 s) : 0, 9375710
gantt
title insecure-bank - break down per module: candidate=1.52.0-SNAPSHOT~ac2eb86cf1, baseline=1.53.0-SNAPSHOT~d43410737f
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.46 ms) : 0, 1460
crashtracking [candidate] (1.462 ms) : 0, 1462
BytebuddyAgent [baseline] (734.961 ms) : 0, 734961
BytebuddyAgent [candidate] (738.184 ms) : 0, 738184
GlobalTracer [baseline] (243.987 ms) : 0, 243987
GlobalTracer [candidate] (243.825 ms) : 0, 243825
AppSec [baseline] (30.338 ms) : 0, 30338
AppSec [candidate] (30.456 ms) : 0, 30456
Debugger [baseline] (6.117 ms) : 0, 6117
Debugger [candidate] (6.104 ms) : 0, 6104
Remote Config [baseline] (686.554 µs) : 0, 687
Remote Config [candidate] (685.542 µs) : 0, 686
Telemetry [baseline] (12.389 ms) : 0, 12389
Telemetry [candidate] (13.049 ms) : 0, 13049
section iast
crashtracking [baseline] (1.465 ms) : 0, 1465
crashtracking [candidate] (1.468 ms) : 0, 1468
BytebuddyAgent [baseline] (857.676 ms) : 0, 857676
BytebuddyAgent [candidate] (855.093 ms) : 0, 855093
GlobalTracer [baseline] (234.851 ms) : 0, 234851
GlobalTracer [candidate] (234.324 ms) : 0, 234324
IAST [baseline] (27.495 ms) : 0, 27495
IAST [candidate] (28.614 ms) : 0, 28614
AppSec [baseline] (29.745 ms) : 0, 29745
AppSec [candidate] (28.762 ms) : 0, 28762
Debugger [baseline] (7.479 ms) : 0, 7479
Debugger [candidate] (5.813 ms) : 0, 5813
Remote Config [baseline] (607.547 µs) : 0, 608
Remote Config [candidate] (623.866 µs) : 0, 624
Telemetry [baseline] (8.358 ms) : 0, 8358
Telemetry [candidate] (8.422 ms) : 0, 8422
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.52.0-SNAPSHOT~ac2eb86cf1, baseline=1.53.0-SNAPSHOT~d43410737f
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.047 s) : 0, 1047139
Total [baseline] (10.727 s) : 0, 10727341
Agent [candidate] (1.063 s) : 0, 1063221
Total [candidate] (10.849 s) : 0, 10848969
section appsec
Agent [baseline] (1.225 s) : 0, 1224698
Total [baseline] (10.81 s) : 0, 10810046
Agent [candidate] (1.228 s) : 0, 1228007
Total [candidate] (10.852 s) : 0, 10852119
section iast
Agent [baseline] (1.178 s) : 0, 1177523
Total [baseline] (7.679 s) : 0, 7679305
Agent [candidate] (1.198 s) : 0, 1197896
Total [candidate] (11.031 s) : 0, 11031457
section profiling
Agent [baseline] (1.197 s) : 0, 1197259
Total [baseline] (10.916 s) : 0, 10915832
Agent [candidate] (1.211 s) : 0, 1211206
Total [candidate] (11.052 s) : 0, 11051685
gantt
title petclinic - break down per module: candidate=1.52.0-SNAPSHOT~ac2eb86cf1, baseline=1.53.0-SNAPSHOT~d43410737f
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.451 ms) : 0, 1451
crashtracking [candidate] (1.486 ms) : 0, 1486
BytebuddyAgent [baseline] (733.337 ms) : 0, 733337
BytebuddyAgent [candidate] (743.088 ms) : 0, 743088
GlobalTracer [baseline] (242.872 ms) : 0, 242872
GlobalTracer [candidate] (246.117 ms) : 0, 246117
AppSec [baseline] (30.067 ms) : 0, 30067
AppSec [candidate] (30.677 ms) : 0, 30677
Debugger [baseline] (6.08 ms) : 0, 6080
Debugger [candidate] (6.166 ms) : 0, 6166
Remote Config [baseline] (665.806 µs) : 0, 666
Remote Config [candidate] (690.489 µs) : 0, 690
Telemetry [baseline] (11.538 ms) : 0, 11538
Telemetry [candidate] (13.658 ms) : 0, 13658
section appsec
crashtracking [baseline] (1.464 ms) : 0, 1464
crashtracking [candidate] (1.461 ms) : 0, 1461
BytebuddyAgent [baseline] (755.329 ms) : 0, 755329
BytebuddyAgent [candidate] (758.062 ms) : 0, 758062
GlobalTracer [baseline] (235.745 ms) : 0, 235745
GlobalTracer [candidate] (236.057 ms) : 0, 236057
AppSec [baseline] (170.65 ms) : 0, 170650
AppSec [candidate] (169.953 ms) : 0, 169953
Debugger [baseline] (7.279 ms) : 0, 7279
Debugger [candidate] (5.926 ms) : 0, 5926
Remote Config [baseline] (658.022 µs) : 0, 658
Remote Config [candidate] (622.533 µs) : 0, 623
Telemetry [baseline] (8.57 ms) : 0, 8570
Telemetry [candidate] (10.992 ms) : 0, 10992
IAST [baseline] (23.792 ms) : 0, 23792
IAST [candidate] (23.727 ms) : 0, 23727
section iast
crashtracking [baseline] (1.448 ms) : 0, 1448
crashtracking [candidate] (1.479 ms) : 0, 1479
BytebuddyAgent [baseline] (849.682 ms) : 0, 849682
BytebuddyAgent [candidate] (866.264 ms) : 0, 866264
GlobalTracer [baseline] (232.724 ms) : 0, 232724
GlobalTracer [candidate] (235.292 ms) : 0, 235292
AppSec [baseline] (28.135 ms) : 0, 28135
AppSec [candidate] (25.64 ms) : 0, 25640
Debugger [baseline] (5.762 ms) : 0, 5762
Debugger [candidate] (10.946 ms) : 0, 10946
Remote Config [baseline] (578.542 µs) : 0, 579
Remote Config [candidate] (605.899 µs) : 0, 606
Telemetry [baseline] (8.13 ms) : 0, 8130
Telemetry [candidate] (8.277 ms) : 0, 8277
IAST [baseline] (29.952 ms) : 0, 29952
IAST [candidate] (27.948 ms) : 0, 27948
section profiling
ProfilingAgent [baseline] (107.669 ms) : 0, 107669
ProfilingAgent [candidate] (109.694 ms) : 0, 109694
crashtracking [baseline] (1.435 ms) : 0, 1435
crashtracking [candidate] (1.455 ms) : 0, 1455
BytebuddyAgent [baseline] (762.597 ms) : 0, 762597
BytebuddyAgent [candidate] (770.681 ms) : 0, 770681
GlobalTracer [baseline] (222.005 ms) : 0, 222005
GlobalTracer [candidate] (223.777 ms) : 0, 223777
AppSec [baseline] (29.987 ms) : 0, 29987
AppSec [candidate] (31.047 ms) : 0, 31047
Debugger [baseline] (7.016 ms) : 0, 7016
Debugger [candidate] (6.375 ms) : 0, 6375
Remote Config [baseline] (703.43 µs) : 0, 703
Remote Config [candidate] (713.306 µs) : 0, 713
Telemetry [baseline] (15.503 ms) : 0, 15503
Telemetry [candidate] (16.469 ms) : 0, 16469
Profiling [baseline] (108.318 ms) : 0, 108318
Profiling [candidate] (110.366 ms) : 0, 110366
LoadParameters
See matching parameters
SummaryFound 2 performance improvements and 2 performance regressions! Performance is the same for 8 metrics, 12 unstable metrics.
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.52.0-SNAPSHOT~ac2eb86cf1, baseline=1.53.0-SNAPSHOT~d43410737f
dateFormat X
axisFormat %s
section baseline
no_agent (37.504 ms) : 37200, 37808
. : milestone, 37504,
appsec (48.942 ms) : 48509, 49375
. : milestone, 48942,
code_origins (44.599 ms) : 44196, 45002
. : milestone, 44599,
iast (43.637 ms) : 43258, 44017
. : milestone, 43637,
profiling (47.586 ms) : 47140, 48032
. : milestone, 47586,
tracing (44.263 ms) : 43874, 44652
. : milestone, 44263,
section candidate
no_agent (37.417 ms) : 37113, 37721
. : milestone, 37417,
appsec (46.677 ms) : 46293, 47060
. : milestone, 46677,
code_origins (44.309 ms) : 43932, 44686
. : milestone, 44309,
iast (45.277 ms) : 44882, 45673
. : milestone, 45277,
profiling (48.829 ms) : 48377, 49281
. : milestone, 48829,
tracing (44.755 ms) : 44372, 45137
. : milestone, 44755,
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.52.0-SNAPSHOT~ac2eb86cf1, baseline=1.53.0-SNAPSHOT~d43410737f
dateFormat X
axisFormat %s
section baseline
no_agent (4.431 ms) : 4381, 4482
. : milestone, 4431,
iast (9.326 ms) : 9175, 9477
. : milestone, 9326,
iast_FULL (14.324 ms) : 14033, 14616
. : milestone, 14324,
iast_GLOBAL (9.767 ms) : 9600, 9934
. : milestone, 9767,
profiling (8.945 ms) : 8807, 9083
. : milestone, 8945,
tracing (7.49 ms) : 7368, 7611
. : milestone, 7490,
section candidate
no_agent (4.445 ms) : 4395, 4496
. : milestone, 4445,
iast (9.31 ms) : 9156, 9463
. : milestone, 9310,
iast_FULL (14.194 ms) : 13911, 14476
. : milestone, 14194,
iast_GLOBAL (10.306 ms) : 10124, 10488
. : milestone, 10306,
profiling (8.61 ms) : 8467, 8752
. : milestone, 8610,
tracing (7.463 ms) : 7352, 7574
. : milestone, 7463,
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.52.0-SNAPSHOT~ac2eb86cf1, baseline=1.53.0-SNAPSHOT~d43410737f
dateFormat X
axisFormat %s
section baseline
no_agent (1.478 ms) : 1467, 1490
. : milestone, 1478,
appsec (3.605 ms) : 3393, 3817
. : milestone, 3605,
iast (2.189 ms) : 2126, 2252
. : milestone, 2189,
iast_GLOBAL (2.24 ms) : 2177, 2303
. : milestone, 2240,
profiling (2.048 ms) : 1997, 2099
. : milestone, 2048,
tracing (2.018 ms) : 1968, 2067
. : milestone, 2018,
section candidate
no_agent (1.477 ms) : 1466, 1489
. : milestone, 1477,
appsec (3.669 ms) : 3452, 3886
. : milestone, 3669,
iast (2.207 ms) : 2144, 2271
. : milestone, 2207,
iast_GLOBAL (2.239 ms) : 2176, 2303
. : milestone, 2239,
profiling (2.064 ms) : 2012, 2117
. : milestone, 2064,
tracing (2.022 ms) : 1973, 2071
. : milestone, 2022,
Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.52.0-SNAPSHOT~ac2eb86cf1, baseline=1.53.0-SNAPSHOT~d43410737f
dateFormat X
axisFormat %s
section baseline
no_agent (15.064 s) : 15064000, 15064000
. : milestone, 15064000,
appsec (14.772 s) : 14772000, 14772000
. : milestone, 14772000,
iast (18.519 s) : 18519000, 18519000
. : milestone, 18519000,
iast_GLOBAL (17.932 s) : 17932000, 17932000
. : milestone, 17932000,
profiling (15.399 s) : 15399000, 15399000
. : milestone, 15399000,
tracing (15.045 s) : 15045000, 15045000
. : milestone, 15045000,
section candidate
no_agent (15.266 s) : 15266000, 15266000
. : milestone, 15266000,
appsec (14.987 s) : 14987000, 14987000
. : milestone, 14987000,
iast (18.565 s) : 18565000, 18565000
. : milestone, 18565000,
iast_GLOBAL (17.966 s) : 17966000, 17966000
. : milestone, 17966000,
profiling (15.515 s) : 15515000, 15515000
. : milestone, 15515000,
tracing (14.992 s) : 14992000, 14992000
. : milestone, 14992000,
|
Extract getBaseHash to be available for SQLCommenter TBD: fix SQLCommenterTest TBD: add DB_DBM_INJECT_SERVICE_HASH_ENABLED
🎯 Code Coverage 🔗 Commit SHA: ac2eb86 | Docs | Was this helpful? Give us feedback! |
Clean up hasDDComment.
if (sql == null || sql.isEmpty()) { | ||
return sql; | ||
} | ||
if (hasDDComment(sql, appendComment)) { |
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.
Move this check to after the appendComment decision is made. Otherwise, it won't work when preferAppend differs from appendComment. This can result in the comment being added twice. Tests were added to ensure this behavior is confirmed.
if (log.isDebugEnabled()) { | ||
log.debug("exception thrown while encoding sql comment %s", e); | ||
} | ||
String encodedValue; |
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.
Do the encoding first to handle a possible encoding exception properly; otherwise, in case of an exception, it will inject a key with an unclosed quote.
if (firstWord.equalsIgnoreCase("call")) { | ||
appendComment = true; | ||
} | ||
|
||
// Append the comment in the case of a pg_hint_plan extension | ||
if (dbType.startsWith("postgres") && containsPgHint(sql)) { | ||
if (dbType.startsWith("postgres") && sql.contains("/*+")) { |
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.
Fix the bug to ensure the hint is detected at the beginning. The original check was sql.indexOf("/*+") > 0
. Remove containsPgHint for simplicity. Tests were added to ensure this behavior is confirmed.
if ((!(sql.endsWith(CLOSE_COMMENT)) && appendComment) | ||
|| ((!(sql.startsWith(OPEN_COMMENT))) && !appendComment)) { | ||
return false; | ||
} | ||
// else check to see if it's a DBM trace sql comment | ||
int startIdx = 2; |
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.
Replace the magic constant 2
.
} | ||
int startComment = appendComment ? startIdx : sql.length(); | ||
boolean found = false; | ||
if (startComment > 2) { |
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.
This condition is useless because it's never true.
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
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.
nice, lgtm
if (DataStreamsTags.baseHash != 0) { | ||
this.hash = DataStreamsTags.baseHash; | ||
} | ||
this.hash = BaseHash.getBaseHash(); |
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.
@piochelepiotr I expected baseHash (novice DSM mental model)to be sent twice: pathHash computation and DSM pipeline payload, why is it used only here and this.hash is overwritten? Doesn't it end up with loss of informaiton for DSM backend
communication/src/main/java/datadog/communication/ddagent/DDAgentFeaturesDiscovery.java
Outdated
Show resolved
Hide resolved
if (primaryTag != null) { | ||
builder.append(primaryTag); | ||
} | ||
if (processTags != null) { |
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.
When processTags
is not null but containerTagsHash
is null, we still append to builder, is this intentional? Maybe we could split it up and do a proper null check:
if (processTags != null) {
builder.append(processTags);
}
if (containerTagsHash != null && !containerTagsHash.isEmpty()) {
builder.append(containerTagsHash);
}
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.
Good point!
@raphaelgavache @piochelepiotr Should we include containerTagsHash in the baseHash calculation if processTags are missing?
String newContainerTagsHash = response.header(DATADOG_CONTAINER_TAGS_HASH); | ||
if (newContainerTagsHash != null) { | ||
ContainerInfo containerInfo = ContainerInfo.get(); | ||
synchronized (containerInfo) { |
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 think that the info endpoint cannot be called concurrently so perhaps this might be avoided
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.
Looks good. I think it needs an eye from DSM to double check. Also the JDBC tests looks failing since dbm related code has been changed
What Does This Do
containerTagsHash
(received from the Agent) inbaseHash
calculationbaseHash
into SQLbaseHash
inDataStreamsTags
Motivation
Backpropagation of container tags. The agent calculates the hash, which is then injected into DBM queries and the DSM pathway context.
Additional Notes
The agent sends the containerTagsHash in the response header. The Java Tracer stores it in the ContainerInfo.
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: OLDAIDM-700