-
Notifications
You must be signed in to change notification settings - Fork 311
Stable Config improvements #9259
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
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 48 metrics, 11 unstable metrics. Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.52.0-SNAPSHOT~2706bcab10, baseline=1.53.0-SNAPSHOT~ee43e5f19c
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.062 s) : 0, 1061656
Total [baseline] (10.868 s) : 0, 10867761
Agent [candidate] (1.045 s) : 0, 1045424
Total [candidate] (10.74 s) : 0, 10740275
section appsec
Agent [baseline] (1.222 s) : 0, 1221823
Total [baseline] (10.784 s) : 0, 10784208
Agent [candidate] (1.219 s) : 0, 1219351
Total [candidate] (10.76 s) : 0, 10760438
section iast
Agent [baseline] (1.177 s) : 0, 1176844
Total [baseline] (10.874 s) : 0, 10874193
Agent [candidate] (1.175 s) : 0, 1174667
Total [candidate] (10.913 s) : 0, 10912720
section profiling
Agent [baseline] (1.202 s) : 0, 1201884
Total [baseline] (10.919 s) : 0, 10918978
Agent [candidate] (1.202 s) : 0, 1202465
Total [candidate] (10.929 s) : 0, 10928627
gantt
title petclinic - break down per module: candidate=1.52.0-SNAPSHOT~2706bcab10, baseline=1.53.0-SNAPSHOT~ee43e5f19c
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.459 ms) : 0, 1459
crashtracking [candidate] (1.434 ms) : 0, 1434
BytebuddyAgent [baseline] (741.797 ms) : 0, 741797
BytebuddyAgent [candidate] (732.198 ms) : 0, 732198
GlobalTracer [baseline] (244.869 ms) : 0, 244869
GlobalTracer [candidate] (242.107 ms) : 0, 242107
AppSec [baseline] (30.499 ms) : 0, 30499
AppSec [candidate] (29.986 ms) : 0, 29986
Debugger [baseline] (6.097 ms) : 0, 6097
Debugger [candidate] (6.045 ms) : 0, 6045
Remote Config [baseline] (663.497 µs) : 0, 663
Remote Config [candidate] (656.208 µs) : 0, 656
Telemetry [baseline] (15.028 ms) : 0, 15028
Telemetry [candidate] (11.922 ms) : 0, 11922
section appsec
crashtracking [baseline] (1.433 ms) : 0, 1433
crashtracking [candidate] (1.436 ms) : 0, 1436
BytebuddyAgent [baseline] (754.518 ms) : 0, 754518
BytebuddyAgent [candidate] (752.711 ms) : 0, 752711
GlobalTracer [baseline] (235.135 ms) : 0, 235135
GlobalTracer [candidate] (234.352 ms) : 0, 234352
AppSec [baseline] (167.635 ms) : 0, 167635
AppSec [candidate] (168.568 ms) : 0, 168568
Debugger [baseline] (7.988 ms) : 0, 7988
Debugger [candidate] (8.773 ms) : 0, 8773
Remote Config [baseline] (623.507 µs) : 0, 624
Remote Config [candidate] (617.526 µs) : 0, 618
Telemetry [baseline] (9.882 ms) : 0, 9882
Telemetry [candidate] (8.282 ms) : 0, 8282
IAST [baseline] (23.498 ms) : 0, 23498
IAST [candidate] (23.534 ms) : 0, 23534
section iast
crashtracking [baseline] (1.428 ms) : 0, 1428
crashtracking [candidate] (1.432 ms) : 0, 1432
BytebuddyAgent [baseline] (849.766 ms) : 0, 849766
BytebuddyAgent [candidate] (848.136 ms) : 0, 848136
GlobalTracer [baseline] (232.653 ms) : 0, 232653
GlobalTracer [candidate] (231.799 ms) : 0, 231799
AppSec [baseline] (27.371 ms) : 0, 27371
AppSec [candidate] (27.865 ms) : 0, 27865
Debugger [baseline] (7.561 ms) : 0, 7561
Debugger [candidate] (7.601 ms) : 0, 7601
Remote Config [baseline] (567.232 µs) : 0, 567
Remote Config [candidate] (577.484 µs) : 0, 577
Telemetry [baseline] (8.234 ms) : 0, 8234
Telemetry [candidate] (8.086 ms) : 0, 8086
IAST [baseline] (28.149 ms) : 0, 28149
IAST [candidate] (28.158 ms) : 0, 28158
section profiling
crashtracking [baseline] (1.415 ms) : 0, 1415
crashtracking [candidate] (1.415 ms) : 0, 1415
BytebuddyAgent [baseline] (766.359 ms) : 0, 766359
BytebuddyAgent [candidate] (766.368 ms) : 0, 766368
GlobalTracer [baseline] (223.15 ms) : 0, 223150
GlobalTracer [candidate] (222.617 ms) : 0, 222617
AppSec [baseline] (30.335 ms) : 0, 30335
AppSec [candidate] (30.252 ms) : 0, 30252
Debugger [baseline] (6.308 ms) : 0, 6308
Debugger [candidate] (6.321 ms) : 0, 6321
Remote Config [baseline] (677.067 µs) : 0, 677
Remote Config [candidate] (692.276 µs) : 0, 692
Telemetry [baseline] (15.381 ms) : 0, 15381
Telemetry [candidate] (15.969 ms) : 0, 15969
ProfilingAgent [baseline] (108.445 ms) : 0, 108445
ProfilingAgent [candidate] (109.004 ms) : 0, 109004
Profiling [baseline] (109.077 ms) : 0, 109077
Profiling [candidate] (109.674 ms) : 0, 109674
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.52.0-SNAPSHOT~2706bcab10, baseline=1.53.0-SNAPSHOT~ee43e5f19c
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.046 s) : 0, 1046047
Total [baseline] (8.653 s) : 0, 8653448
Agent [candidate] (1.05 s) : 0, 1050143
Total [candidate] (8.601 s) : 0, 8601228
section iast
Agent [baseline] (1.172 s) : 0, 1171971
Total [baseline] (9.333 s) : 0, 9332756
Agent [candidate] (1.174 s) : 0, 1174152
Total [candidate] (9.331 s) : 0, 9331000
gantt
title insecure-bank - break down per module: candidate=1.52.0-SNAPSHOT~2706bcab10, baseline=1.53.0-SNAPSHOT~ee43e5f19c
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.433 ms) : 0, 1433
crashtracking [candidate] (1.439 ms) : 0, 1439
BytebuddyAgent [baseline] (732.13 ms) : 0, 732130
BytebuddyAgent [candidate] (736.394 ms) : 0, 736394
GlobalTracer [baseline] (243.067 ms) : 0, 243067
GlobalTracer [candidate] (242.773 ms) : 0, 242773
AppSec [baseline] (30.26 ms) : 0, 30260
AppSec [candidate] (30.325 ms) : 0, 30325
Debugger [baseline] (6.047 ms) : 0, 6047
Debugger [candidate] (6.088 ms) : 0, 6088
Remote Config [baseline] (660.816 µs) : 0, 661
Remote Config [candidate] (656.841 µs) : 0, 657
Telemetry [baseline] (11.448 ms) : 0, 11448
Telemetry [candidate] (11.317 ms) : 0, 11317
section iast
crashtracking [baseline] (1.418 ms) : 0, 1418
crashtracking [candidate] (1.442 ms) : 0, 1442
BytebuddyAgent [baseline] (845.799 ms) : 0, 845799
BytebuddyAgent [candidate] (848.189 ms) : 0, 848189
GlobalTracer [baseline] (231.841 ms) : 0, 231841
GlobalTracer [candidate] (231.462 ms) : 0, 231462
AppSec [baseline] (26.614 ms) : 0, 26614
AppSec [candidate] (26.162 ms) : 0, 26162
Debugger [baseline] (8.291 ms) : 0, 8291
Debugger [candidate] (6.667 ms) : 0, 6667
Remote Config [baseline] (581.118 µs) : 0, 581
Remote Config [candidate] (593.269 µs) : 0, 593
Telemetry [baseline] (8.172 ms) : 0, 8172
Telemetry [candidate] (8.088 ms) : 0, 8088
IAST [baseline] (28.278 ms) : 0, 28278
IAST [candidate] (30.636 ms) : 0, 30636
LoadParameters
See matching parameters
SummaryFound 0 performance improvements and 1 performance regressions! Performance is the same for 11 metrics, 12 unstable metrics.
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.52.0-SNAPSHOT~2706bcab10, baseline=1.53.0-SNAPSHOT~ee43e5f19c
dateFormat X
axisFormat %s
section baseline
no_agent (37.565 ms) : 37252, 37878
. : milestone, 37565,
appsec (49.305 ms) : 48866, 49744
. : milestone, 49305,
code_origins (44.595 ms) : 44213, 44977
. : milestone, 44595,
iast (44.835 ms) : 44432, 45238
. : milestone, 44835,
profiling (46.373 ms) : 45943, 46804
. : milestone, 46373,
tracing (44.259 ms) : 43882, 44636
. : milestone, 44259,
section candidate
no_agent (37.847 ms) : 37547, 38146
. : milestone, 37847,
appsec (48.614 ms) : 48178, 49050
. : milestone, 48614,
code_origins (45.784 ms) : 45380, 46188
. : milestone, 45784,
iast (45.028 ms) : 44637, 45419
. : milestone, 45028,
profiling (46.887 ms) : 46450, 47325
. : milestone, 46887,
tracing (44.067 ms) : 43679, 44456
. : milestone, 44067,
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.52.0-SNAPSHOT~2706bcab10, baseline=1.53.0-SNAPSHOT~ee43e5f19c
dateFormat X
axisFormat %s
section baseline
no_agent (4.44 ms) : 4388, 4492
. : milestone, 4440,
iast (9.09 ms) : 8944, 9237
. : milestone, 9090,
iast_FULL (13.728 ms) : 13453, 14002
. : milestone, 13728,
iast_GLOBAL (10.35 ms) : 10156, 10545
. : milestone, 10350,
profiling (8.592 ms) : 8460, 8724
. : milestone, 8592,
tracing (7.662 ms) : 7553, 7770
. : milestone, 7662,
section candidate
no_agent (4.451 ms) : 4397, 4505
. : milestone, 4451,
iast (9.314 ms) : 9161, 9467
. : milestone, 9314,
iast_FULL (13.712 ms) : 13440, 13984
. : milestone, 13712,
iast_GLOBAL (10.105 ms) : 9928, 10282
. : milestone, 10105,
profiling (9.35 ms) : 9182, 9517
. : milestone, 9350,
tracing (7.686 ms) : 7578, 7795
. : milestone, 7686,
DacapoParameters
See matching parameters
SummaryFound 1 performance improvements and 0 performance regressions! Performance is the same for 11 metrics, 0 unstable metrics.
Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.52.0-SNAPSHOT~2706bcab10, baseline=1.53.0-SNAPSHOT~ee43e5f19c
dateFormat X
axisFormat %s
section baseline
no_agent (15.009 s) : 15009000, 15009000
. : milestone, 15009000,
appsec (14.765 s) : 14765000, 14765000
. : milestone, 14765000,
iast (18.223 s) : 18223000, 18223000
. : milestone, 18223000,
iast_GLOBAL (17.903 s) : 17903000, 17903000
. : milestone, 17903000,
profiling (15.231 s) : 15231000, 15231000
. : milestone, 15231000,
tracing (14.855 s) : 14855000, 14855000
. : milestone, 14855000,
section candidate
no_agent (14.851 s) : 14851000, 14851000
. : milestone, 14851000,
appsec (14.737 s) : 14737000, 14737000
. : milestone, 14737000,
iast (18.734 s) : 18734000, 18734000
. : milestone, 18734000,
iast_GLOBAL (18.134 s) : 18134000, 18134000
. : milestone, 18134000,
profiling (15.348 s) : 15348000, 15348000
. : milestone, 15348000,
tracing (14.809 s) : 14809000, 14809000
. : milestone, 14809000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.52.0-SNAPSHOT~2706bcab10, baseline=1.53.0-SNAPSHOT~ee43e5f19c
dateFormat X
axisFormat %s
section baseline
no_agent (1.469 ms) : 1458, 1481
. : milestone, 1469,
appsec (3.638 ms) : 3423, 3853
. : milestone, 3638,
iast (2.191 ms) : 2128, 2254
. : milestone, 2191,
iast_GLOBAL (2.232 ms) : 2169, 2294
. : milestone, 2232,
profiling (2.049 ms) : 1997, 2100
. : milestone, 2049,
tracing (2.018 ms) : 1969, 2067
. : milestone, 2018,
section candidate
no_agent (1.475 ms) : 1464, 1487
. : milestone, 1475,
appsec (2.403 ms) : 2353, 2453
. : milestone, 2403,
iast (2.18 ms) : 2118, 2242
. : milestone, 2180,
iast_GLOBAL (2.23 ms) : 2168, 2292
. : milestone, 2230,
profiling (2.025 ms) : 1976, 2075
. : milestone, 2025,
tracing (2.005 ms) : 1957, 2053
. : milestone, 2005,
|
@@ -26,12 +26,34 @@ public Rule(List<Selector> selectors, Map<String, Object> configuration) { | |||
} | |||
|
|||
public Rule(Object yaml) { |
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.
Let me know if this constructor is too hard to read and if you recommend I move the "require fields" logic into helper functions (in a utils class to be used by both Rule and Selector?)
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 would rather try to bring type safety to the Java API as soon as possible even if SnakeYaml is returning loose Object
.
An idiomatic Java would be to have a static builder like static Rule from(Map<?, ?> rules)
and check that its parameters is the right time before calling it.
-- Note that a better type for rules
would be Map<String, ?>
but there is no way to enforce / test it due to type erasure in Java.
So far example, I would check the type ahead in StableConfig
before trying to create rule using its fromMap
:
this.apmConfigurationRules = parseRules(yaml);
// with some helper methods in StableConfig
private List<Rule> parseRules(Map<?, ?> yaml) {
Object apmConfigurationDefaultObject = map.get("apm_configuration_default");
if (apmConfigurationDefaultObject instanceof List) {
return ((List<?>) apmConfigurationDefaultObject).stream()
.filter(r -> r instanceof Map)
.map(r -> (Map<?, ?>) r)
.map(Rule::fromMap)
.collect(toList());
} else {
return emptyList();
}
}
internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/Selector.java
Outdated
Show resolved
Hide resolved
@@ -26,12 +26,34 @@ public Rule(List<Selector> selectors, Map<String, Object> configuration) { | |||
} | |||
|
|||
public Rule(Object yaml) { |
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 would rather try to bring type safety to the Java API as soon as possible even if SnakeYaml is returning loose Object
.
An idiomatic Java would be to have a static builder like static Rule from(Map<?, ?> rules)
and check that its parameters is the right time before calling it.
-- Note that a better type for rules
would be Map<String, ?>
but there is no way to enforce / test it due to type erasure in Java.
So far example, I would check the type ahead in StableConfig
before trying to create rule using its fromMap
:
this.apmConfigurationRules = parseRules(yaml);
// with some helper methods in StableConfig
private List<Rule> parseRules(Map<?, ?> yaml) {
Object apmConfigurationDefaultObject = map.get("apm_configuration_default");
if (apmConfigurationDefaultObject instanceof List) {
return ((List<?>) apmConfigurationDefaultObject).stream()
.filter(r -> r instanceof Map)
.map(r -> (Map<?, ?>) r)
.map(Rule::fromMap)
.collect(toList());
} else {
return emptyList();
}
}
return value.contains(match); | ||
default: | ||
return false; | ||
if (comparator.test(match)) { |
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.
👍
Code coverage: total 57.11%, base diff 0.02%, patch 87.91% (view details) This comment will be updated automatically if new data arrives.🔗 Commit SHA: 2706bca | Docs | Was this helpful? Give us feedback! |
.../java/datadog/trace/bootstrap/config/provider/stableconfig/StableConfigMappingException.java
Outdated
Show resolved
Hide resolved
internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/Rule.java
Outdated
Show resolved
Hide resolved
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.
Looking good 👍
One question about testing logging though
internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java
Outdated
Show resolved
Hide resolved
...al-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigSourceTest.groovy
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.
Thanks for the follow up PR and the improvements 👍
internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/Rule.java
Outdated
Show resolved
Hide resolved
internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/Rule.java
Outdated
Show resolved
Hide resolved
internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/Rule.java
Outdated
Show resolved
Hide resolved
internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/Rule.java
Outdated
Show resolved
Hide resolved
internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/Selector.java
Outdated
Show resolved
Hide resolved
...nal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/StableConfig.java
Outdated
Show resolved
Hide resolved
.../java/datadog/trace/bootstrap/config/provider/stableconfig/StableConfigMappingException.java
Outdated
Show resolved
Hide resolved
…ider/stableconfig/Rule.java Co-authored-by: Alexey Kuznetsov <[email protected]>
…ider/stableconfig/Rule.java Co-authored-by: Alexey Kuznetsov <[email protected]>
internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/Rule.java
Outdated
Show resolved
Hide resolved
internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/Selector.java
Outdated
Show resolved
Hide resolved
internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java
Outdated
Show resolved
Hide resolved
internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java
Outdated
Show resolved
Hide resolved
.../java/datadog/trace/bootstrap/config/provider/stableconfig/StableConfigMappingException.java
Outdated
Show resolved
Hide resolved
...nal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/StableConfig.java
Outdated
Show resolved
Hide resolved
internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/Rule.java
Outdated
Show resolved
Hide resolved
internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java
Outdated
Show resolved
Hide resolved
} else if (log.isDebugEnabled()) { | ||
log.error("Unexpected error while reading stable configuration file {}: {}", filePath, e); | ||
} else { |
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.
Not sure I understand this if (log.isDebugEnabled())
...
To me it looks like we can log that error always, because all major cases were processed in previous if
block.
Also, I feel that here we have a very tricky case: you have 2 placeholders {}
, but the second argument e
is actually an exception and it will be treaded in a special way. So you should use only one place holder, or provide as second argument something else, for example e.getMessage()
.
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.
As for the log.isDebugEnabled()
check: this was something @mcculls suggested; we want to print the complete exception if and only if debug is enabled, else just print the exception message. An alternative is to only print the message regardless, I suppose. WDYT?
As for the placeholder: I was not aware, thanks for sharing. 🤔 I will change this accordingly, based on what we decide on the above.
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.
got it! make sense in debug to print stack trace
but code should be with one place holder:
log.error("Unexpected error while reading stable configuration file: {}", filePath, e);
internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/Rule.java
Outdated
Show resolved
Hide resolved
.../java/datadog/trace/bootstrap/config/provider/stableconfig/StableConfigMappingException.java
Outdated
Show resolved
Hide resolved
internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/Selector.java
Outdated
Show resolved
Hide resolved
…ider/stableconfig/Rule.java Co-authored-by: Alexey Kuznetsov <[email protected]>
…-java into mtoff/scfg-improvements
} else if (log.isDebugEnabled()) { | ||
log.error("Unexpected error while reading stable configuration file {}: {}", filePath, e); | ||
} else { |
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.
got it! make sense in debug to print stack trace
but code should be with one place holder:
log.error("Unexpected error while reading stable configuration file: {}", filePath, e);
internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java
Outdated
Show resolved
Hide resolved
…ider/StableConfigSource.java Co-authored-by: Alexey Kuznetsov <[email protected]>
What Does This Do
matchOperator
logic to use method reference comparator for efficiencyStableConfigMappingException
for better exception handling and debug logging when our custom YAML parsing fails on the StableConfig classapm_configuration_rules
is set but none of the rules match the active processRule
andSelector
classes with a static factoring functionfrom(Map<?, ?>)
to enforce type safety at the API levelMotivation
For (1): Suggestion by @PerfectSlayer in previous PR #9201
For (2) & (3): A result of design partner troubleshooting sessions
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]