-
Notifications
You must be signed in to change notification settings - Fork 310
Cleanup InstrumenterModule
enablement
#9027
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
ef6f9ce
to
e311183
Compare
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 45 metrics, 8 unstable metrics. Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.51.0-SNAPSHOT~0e4a61656a, baseline=1.51.0-SNAPSHOT~faeb62cfb1
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (993.343 ms) : 0, 993343
Total [baseline] (8.546 s) : 0, 8545927
Agent [candidate] (1.002 s) : 0, 1002392
Total [candidate] (8.568 s) : 0, 8567886
section iast
Agent [baseline] (1.131 s) : 0, 1131126
Total [baseline] (9.299 s) : 0, 9298696
Agent [candidate] (1.134 s) : 0, 1133831
Total [candidate] (9.25 s) : 0, 9249757
gantt
title insecure-bank - break down per module: candidate=1.51.0-SNAPSHOT~0e4a61656a, baseline=1.51.0-SNAPSHOT~faeb62cfb1
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (685.882 ms) : 0, 685882
BytebuddyAgent [candidate] (691.422 ms) : 0, 691422
GlobalTracer [baseline] (241.631 ms) : 0, 241631
GlobalTracer [candidate] (244.579 ms) : 0, 244579
AppSec [baseline] (30.116 ms) : 0, 30116
AppSec [candidate] (30.402 ms) : 0, 30402
Debugger [baseline] (6.082 ms) : 0, 6082
Debugger [candidate] (6.097 ms) : 0, 6097
Remote Config [baseline] (652.509 µs) : 0, 653
Remote Config [candidate] (653.164 µs) : 0, 653
Telemetry [baseline] (8.281 ms) : 0, 8281
Telemetry [candidate] (8.303 ms) : 0, 8303
section iast
BytebuddyAgent [baseline] (807.908 ms) : 0, 807908
BytebuddyAgent [candidate] (803.444 ms) : 0, 803444
GlobalTracer [baseline] (233.002 ms) : 0, 233002
GlobalTracer [candidate] (240.078 ms) : 0, 240078
IAST [baseline] (28.344 ms) : 0, 28344
IAST [candidate] (28.727 ms) : 0, 28727
AppSec [baseline] (26.757 ms) : 0, 26757
AppSec [candidate] (26.543 ms) : 0, 26543
Debugger [baseline] (5.819 ms) : 0, 5819
Debugger [candidate] (5.844 ms) : 0, 5844
Remote Config [baseline] (581.402 µs) : 0, 581
Remote Config [candidate] (571.539 µs) : 0, 572
Telemetry [baseline] (7.937 ms) : 0, 7937
Telemetry [candidate] (7.862 ms) : 0, 7862
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.51.0-SNAPSHOT~0e4a61656a, baseline=1.51.0-SNAPSHOT~faeb62cfb1
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (997.236 ms) : 0, 997236
Total [baseline] (10.641 s) : 0, 10641459
Agent [candidate] (994.692 ms) : 0, 994692
Total [candidate] (10.708 s) : 0, 10707550
section appsec
Agent [baseline] (1.17 s) : 0, 1170173
Total [baseline] (10.69 s) : 0, 10690303
Agent [candidate] (1.167 s) : 0, 1167160
Total [candidate] (10.717 s) : 0, 10716807
section iast
Agent [baseline] (1.137 s) : 0, 1136991
Total [baseline] (10.838 s) : 0, 10837696
Agent [candidate] (1.128 s) : 0, 1128012
Total [candidate] (10.797 s) : 0, 10796541
section profiling
Agent [baseline] (1.259 s) : 0, 1258634
Total [baseline] (11.154 s) : 0, 11153926
Agent [candidate] (1.251 s) : 0, 1251018
Total [candidate] (11.092 s) : 0, 11091530
gantt
title petclinic - break down per module: candidate=1.51.0-SNAPSHOT~0e4a61656a, baseline=1.51.0-SNAPSHOT~faeb62cfb1
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (688.392 ms) : 0, 688392
BytebuddyAgent [candidate] (686.488 ms) : 0, 686488
GlobalTracer [baseline] (242.932 ms) : 0, 242932
GlobalTracer [candidate] (242.429 ms) : 0, 242429
AppSec [baseline] (30.059 ms) : 0, 30059
AppSec [candidate] (30.124 ms) : 0, 30124
Debugger [baseline] (6.072 ms) : 0, 6072
Debugger [candidate] (6.053 ms) : 0, 6053
Remote Config [baseline] (654.261 µs) : 0, 654
Remote Config [candidate] (637.899 µs) : 0, 638
Telemetry [baseline] (8.181 ms) : 0, 8181
Telemetry [candidate] (8.195 ms) : 0, 8195
section appsec
BytebuddyAgent [baseline] (709.13 ms) : 0, 709130
BytebuddyAgent [candidate] (699.632 ms) : 0, 699632
GlobalTracer [baseline] (235.631 ms) : 0, 235631
GlobalTracer [candidate] (241.421 ms) : 0, 241421
AppSec [baseline] (168.125 ms) : 0, 168125
AppSec [candidate] (168.95 ms) : 0, 168950
Debugger [baseline] (5.847 ms) : 0, 5847
Debugger [candidate] (5.84 ms) : 0, 5840
Remote Config [baseline] (599.714 µs) : 0, 600
Remote Config [candidate] (609.365 µs) : 0, 609
Telemetry [baseline] (8.136 ms) : 0, 8136
Telemetry [candidate] (8.177 ms) : 0, 8177
IAST [baseline] (21.838 ms) : 0, 21838
IAST [candidate] (21.732 ms) : 0, 21732
section iast
BytebuddyAgent [baseline] (813.209 ms) : 0, 813209
BytebuddyAgent [candidate] (799.254 ms) : 0, 799254
GlobalTracer [baseline] (233.161 ms) : 0, 233161
GlobalTracer [candidate] (238.949 ms) : 0, 238949
AppSec [baseline] (27.333 ms) : 0, 27333
AppSec [candidate] (28.912 ms) : 0, 28912
Debugger [baseline] (5.821 ms) : 0, 5821
Debugger [candidate] (5.753 ms) : 0, 5753
Remote Config [baseline] (573.431 µs) : 0, 573
Remote Config [candidate] (577.817 µs) : 0, 578
Telemetry [baseline] (7.917 ms) : 0, 7917
Telemetry [candidate] (7.836 ms) : 0, 7836
IAST [baseline] (28.05 ms) : 0, 28050
IAST [candidate] (25.235 ms) : 0, 25235
section profiling
ProfilingAgent [baseline] (103.896 ms) : 0, 103896
ProfilingAgent [candidate] (103.776 ms) : 0, 103776
BytebuddyAgent [baseline] (687.749 ms) : 0, 687749
BytebuddyAgent [candidate] (682.24 ms) : 0, 682240
GlobalTracer [baseline] (365.132 ms) : 0, 365132
GlobalTracer [candidate] (363.042 ms) : 0, 363042
AppSec [baseline] (32.9 ms) : 0, 32900
AppSec [candidate] (31.381 ms) : 0, 31381
Debugger [baseline] (9.735 ms) : 0, 9735
Debugger [candidate] (7.597 ms) : 0, 7597
Remote Config [baseline] (689.817 µs) : 0, 690
Remote Config [candidate] (2.198 ms) : 0, 2198
Telemetry [baseline] (9.536 ms) : 0, 9536
Telemetry [candidate] (11.882 ms) : 0, 11882
Profiling [baseline] (103.921 ms) : 0, 103921
Profiling [candidate] (103.8 ms) : 0, 103800
LoadParameters
See matching parameters
SummaryFound 2 performance improvements and 0 performance regressions! Performance is the same for 10 metrics, 12 unstable metrics.
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.51.0-SNAPSHOT~0e4a61656a, baseline=1.51.0-SNAPSHOT~faeb62cfb1
dateFormat X
axisFormat %s
section baseline
no_agent (37.318 ms) : 37021, 37615
. : milestone, 37318,
appsec (48.471 ms) : 48039, 48902
. : milestone, 48471,
code_origins (44.234 ms) : 43872, 44596
. : milestone, 44234,
iast (45.007 ms) : 44604, 45411
. : milestone, 45007,
profiling (49.902 ms) : 49463, 50341
. : milestone, 49902,
tracing (43.144 ms) : 42796, 43492
. : milestone, 43144,
section candidate
no_agent (36.871 ms) : 36574, 37168
. : milestone, 36871,
appsec (45.545 ms) : 45142, 45948
. : milestone, 45545,
code_origins (44.724 ms) : 44360, 45088
. : milestone, 44724,
iast (45.491 ms) : 45093, 45888
. : milestone, 45491,
profiling (51.192 ms) : 50692, 51691
. : milestone, 51192,
tracing (41.95 ms) : 41610, 42289
. : milestone, 41950,
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.51.0-SNAPSHOT~0e4a61656a, baseline=1.51.0-SNAPSHOT~faeb62cfb1
dateFormat X
axisFormat %s
section baseline
no_agent (4.312 ms) : 4263, 4361
. : milestone, 4312,
iast (9.177 ms) : 9022, 9332
. : milestone, 9177,
iast_FULL (14.562 ms) : 14271, 14852
. : milestone, 14562,
iast_GLOBAL (9.936 ms) : 9766, 10106
. : milestone, 9936,
profiling (8.67 ms) : 8522, 8818
. : milestone, 8670,
tracing (7.567 ms) : 7457, 7677
. : milestone, 7567,
section candidate
no_agent (4.32 ms) : 4272, 4368
. : milestone, 4320,
iast (9.2 ms) : 9051, 9350
. : milestone, 9200,
iast_FULL (13.627 ms) : 13359, 13895
. : milestone, 13627,
iast_GLOBAL (10.016 ms) : 9841, 10191
. : milestone, 10016,
profiling (8.84 ms) : 8704, 8976
. : milestone, 8840,
tracing (7.43 ms) : 7319, 7540
. : milestone, 7430,
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 biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.51.0-SNAPSHOT~0e4a61656a, baseline=1.51.0-SNAPSHOT~faeb62cfb1
dateFormat X
axisFormat %s
section baseline
no_agent (15.45 s) : 15450000, 15450000
. : milestone, 15450000,
appsec (14.839 s) : 14839000, 14839000
. : milestone, 14839000,
iast (18.222 s) : 18222000, 18222000
. : milestone, 18222000,
iast_GLOBAL (18.18 s) : 18180000, 18180000
. : milestone, 18180000,
profiling (15.576 s) : 15576000, 15576000
. : milestone, 15576000,
tracing (14.917 s) : 14917000, 14917000
. : milestone, 14917000,
section candidate
no_agent (15.6 s) : 15600000, 15600000
. : milestone, 15600000,
appsec (14.853 s) : 14853000, 14853000
. : milestone, 14853000,
iast (18.49 s) : 18490000, 18490000
. : milestone, 18490000,
iast_GLOBAL (17.797 s) : 17797000, 17797000
. : milestone, 17797000,
profiling (15.359 s) : 15359000, 15359000
. : milestone, 15359000,
tracing (14.842 s) : 14842000, 14842000
. : milestone, 14842000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.51.0-SNAPSHOT~0e4a61656a, baseline=1.51.0-SNAPSHOT~faeb62cfb1
dateFormat X
axisFormat %s
section baseline
no_agent (1.469 ms) : 1458, 1481
. : milestone, 1469,
appsec (2.401 ms) : 2351, 2450
. : milestone, 2401,
iast (2.179 ms) : 2117, 2241
. : milestone, 2179,
iast_GLOBAL (2.235 ms) : 2173, 2297
. : milestone, 2235,
profiling (2.043 ms) : 1991, 2094
. : milestone, 2043,
tracing (2.017 ms) : 1969, 2065
. : milestone, 2017,
section candidate
no_agent (1.471 ms) : 1460, 1483
. : milestone, 1471,
appsec (2.395 ms) : 2346, 2444
. : milestone, 2395,
iast (2.189 ms) : 2127, 2251
. : milestone, 2189,
iast_GLOBAL (2.231 ms) : 2168, 2293
. : milestone, 2231,
profiling (2.051 ms) : 2000, 2102
. : milestone, 2051,
tracing (2.006 ms) : 1958, 2053
. : milestone, 2006,
|
Kafka / producer-benchmarkParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 3 metrics, 0 unstable metrics. See unchanged results
|
* All instrumentations now declare a target system which they fall under * We introduce COMMON as the system for instrumentations shared across all systems and SECURITY for instrumentations shared between APPSEC and IAST * We now log when an instrumentation has been disabled * Special enablement of security instrumentations shared between APPSEC and IAST has been moved to `isEnabled` which is now told the enabled systems
…fining instrumentation if you just want the package name
ef452fa
to
0e4a616
Compare
Kafka / consumer-benchmarkParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 3 metrics, 0 unstable metrics. See unchanged results
|
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.
Interesting change for the future! Let me know if I get it right:
We now split if an instrumentation is "applicable" from if it is "enabled".
For this, we only rely on "enabled (target) systems".
To this end, additional compound systems are introduced, like COMMON
and SECURITY
.
To me, it feels like "applicable" related to "product" activation, and "enabled" relates to "feature" activation due to the introduction of compound systems.
But it feels weird having CSI / security instrumentation targeting SECURITY
, and then checking for another sub-system / feature like IAST
at "enabled" check. It looks like there is a mix of concerns for TargetSystem
.
I would rather try to have exclusive systems, like TRACING
, IAST
, APPSEC
, etc... (no compound like COMMON
nor SECURITY
) and have an isApplicableFor(Set<TargetSystem> systems) {}
module method instead, to allow having multiple systems applicable per module.
So every module should declare all systems which they are supposed to run with.
With this, CSI instrumentations could then be applicable for both APPSEC
and IAST
and have its child only enabled for specific system if needed.
I agreed this will be harder to make the instrumentation indexes, as each module won't declare the single system they apply for, so we might need to query them for all know systems and check what they answer first. But sounds like it can avoid some future issues with overlapping systems / features - like an instrumentation that will be both RUM and SECURITY for example - and avoid TargetSystem
mixing product / feature concerns.
Let me know what you think of it and if I get it right?
Otherwise, I added few other comments along the changes.
@@ -132,6 +132,10 @@ class DefaultInstrumenterForkedTest extends DDSpecification { | |||
"PERIOD_TEST" | true | "period.test" | "asdf" | |||
} | |||
|
|||
def assertEnabled(target, enabled = true) { | |||
target.isEnabled(EnumSet.of(TargetSystem.TRACING)) == enabled |
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.
Will it fail with assert
?
target.isEnabled(EnumSet.of(TargetSystem.TRACING)) == enabled | |
assert target.isEnabled(EnumSet.of(TargetSystem.TRACING)) == enabled |
protected boolean defaultEnabled() { | ||
return InstrumenterConfig.get().isIntegrationsEnabled(); | ||
} | ||
|
||
public boolean isEnabled() { | ||
public boolean isEnabled(Set<TargetSystem> enabledSystems) { |
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.
Nitpick: I found the enabledSystems
name a bit confusing for the isEnabled()
method.
"Is the module enabled for those enabled systems"
What about public boolean isEnabledFor(Set<TargetSystem> systems);
?
From the module POV, it does not have to know if the given systems are enabled or not, just to answer "should I be enabled for those systems". That's a nitpick but I feel it would add clarity when there is no Javadoc.
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 necessarily for this PR, but I've been pondering whether annotations would be a better way to specify this type of information.
My thinking is that annotations would force you to only use static information. And we could also index the information via an annotation processor.
public boolean isEnabled(Set<TargetSystem> enabledSystems) { | ||
return super.isEnabled(enabledSystems); |
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.
Could be removed?
public boolean isEnabled(Set<TargetSystem> enabledSystems) { | ||
return super.isEnabled(enabledSystems); |
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.
Could be removed?
public boolean isEnabled(Set<TargetSystem> enabledSystems) { | ||
return super.isEnabled(enabledSystems) |
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.
Could be removed?
public boolean isEnabled(Set<TargetSystem> enabledSystems) { | ||
return super.isEnabled(enabledSystems); |
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.
Could be removed?
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
What Does This Do
COMMON
as the system for instrumentations shared across all systems andSECURITY
for instrumentations shared betweenAPPSEC
andIAST
APPSEC
andIAST
has been moved toisEnabled
which is now told the enabled systemsMotivation
Preparation for encoding the target system of each instrumentation in the instrumentation index. This will let us do early filtering without needing to load the instrumentation.
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: APMAPI-1290