Reduce scope listener API #7296
Merged
Conversation
This file contains 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
PerfectSlayer
added
comp: core
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 54 metrics, 9 unstable metrics. Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.38.0-SNAPSHOT~086ab609b9, baseline=1.38.0-SNAPSHOT~f88def8618
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.062 s) : 0, 1062019
Total [baseline] (8.503 s) : 0, 8502727
Agent [candidate] (1.074 s) : 0, 1074111
Total [candidate] (8.516 s) : 0, 8515702
section iast
Agent [baseline] (1.17 s) : 0, 1169730
Total [baseline] (8.947 s) : 0, 8946543
Agent [candidate] (1.171 s) : 0, 1171176
Total [candidate] (8.921 s) : 0, 8920595
section iast_HARDCODED_SECRET_DISABLED
Agent [baseline] (1.183 s) : 0, 1182881
Total [baseline] (8.943 s) : 0, 8943053
Agent [candidate] (1.17 s) : 0, 1170336
Total [candidate] (8.909 s) : 0, 8908661
section iast_TELEMETRY_OFF
Agent [baseline] (1.176 s) : 0, 1175792
Total [baseline] (8.943 s) : 0, 8942718
Agent [candidate] (1.166 s) : 0, 1166214
Total [candidate] (8.937 s) : 0, 8937104
gantt
title insecure-bank - break down per module: candidate=1.38.0-SNAPSHOT~086ab609b9, baseline=1.38.0-SNAPSHOT~f88def8618
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (665.082 ms) : 0, 665082
BytebuddyAgent [candidate] (673.983 ms) : 0, 673983
GlobalTracer [baseline] (304.137 ms) : 0, 304137
GlobalTracer [candidate] (306.426 ms) : 0, 306426
AppSec [baseline] (49.946 ms) : 0, 49946
AppSec [candidate] (50.379 ms) : 0, 50379
Remote Config [baseline] (684.743 µs) : 0, 685
Remote Config [candidate] (670.55 µs) : 0, 671
Telemetry [baseline] (7.721 ms) : 0, 7721
Telemetry [candidate] (7.615 ms) : 0, 7615
section iast
BytebuddyAgent [baseline] (779.041 ms) : 0, 779041
BytebuddyAgent [candidate] (779.821 ms) : 0, 779821
GlobalTracer [baseline] (294.931 ms) : 0, 294931
GlobalTracer [candidate] (295.27 ms) : 0, 295270
AppSec [baseline] (48.652 ms) : 0, 48652
AppSec [candidate] (47.858 ms) : 0, 47858
IAST [baseline] (26.13 ms) : 0, 26130
IAST [candidate] (27.337 ms) : 0, 27337
Remote Config [baseline] (573.68 µs) : 0, 574
Remote Config [candidate] (552.348 µs) : 0, 552
Telemetry [baseline] (6.942 ms) : 0, 6942
Telemetry [candidate] (6.87 ms) : 0, 6870
section iast_HARDCODED_SECRET_DISABLED
BytebuddyAgent [baseline] (789.959 ms) : 0, 789959
BytebuddyAgent [candidate] (778.789 ms) : 0, 778789
GlobalTracer [baseline] (296.707 ms) : 0, 296707
GlobalTracer [candidate] (295.377 ms) : 0, 295377
AppSec [baseline] (48.454 ms) : 0, 48454
AppSec [candidate] (47.297 ms) : 0, 47297
IAST [baseline] (26.497 ms) : 0, 26497
IAST [candidate] (27.875 ms) : 0, 27875
Remote Config [baseline] (564.685 µs) : 0, 565
Remote Config [candidate] (561.121 µs) : 0, 561
Telemetry [baseline] (6.971 ms) : 0, 6971
Telemetry [candidate] (6.926 ms) : 0, 6926
section iast_TELEMETRY_OFF
BytebuddyAgent [baseline] (782.339 ms) : 0, 782339
BytebuddyAgent [candidate] (776.261 ms) : 0, 776261
GlobalTracer [baseline] (296.738 ms) : 0, 296738
GlobalTracer [candidate] (294.877 ms) : 0, 294877
AppSec [baseline] (47.719 ms) : 0, 47719
AppSec [candidate] (47.319 ms) : 0, 47319
IAST [baseline] (27.959 ms) : 0, 27959
IAST [candidate] (25.985 ms) : 0, 25985
Remote Config [baseline] (573.76 µs) : 0, 574
Remote Config [candidate] (614.125 µs) : 0, 614
Telemetry [baseline] (6.87 ms) : 0, 6870
Telemetry [candidate] (7.62 ms) : 0, 7620
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.38.0-SNAPSHOT~086ab609b9, baseline=1.38.0-SNAPSHOT~f88def8618
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.066 s) : 0, 1066074
Total [baseline] (10.359 s) : 0, 10359110
Agent [candidate] (1.061 s) : 0, 1060825
Total [candidate] (10.356 s) : 0, 10355620
section appsec
Agent [baseline] (1.19 s) : 0, 1190489
Total [baseline] (10.504 s) : 0, 10504031
Agent [candidate] (1.187 s) : 0, 1186948
Total [candidate] (10.54 s) : 0, 10540057
section iast
Agent [baseline] (1.171 s) : 0, 1170745
Total [baseline] (10.693 s) : 0, 10692732
Agent [candidate] (1.173 s) : 0, 1173375
Total [candidate] (10.745 s) : 0, 10744775
section profiling
Agent [baseline] (1.272 s) : 0, 1271886
Total [baseline] (10.618 s) : 0, 10617891
Agent [candidate] (1.265 s) : 0, 1264773
Total [candidate] (10.59 s) : 0, 10590048
gantt
title petclinic - break down per module: candidate=1.38.0-SNAPSHOT~086ab609b9, baseline=1.38.0-SNAPSHOT~f88def8618
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (666.714 ms) : 0, 666714
BytebuddyAgent [candidate] (664.014 ms) : 0, 664014
GlobalTracer [baseline] (306.19 ms) : 0, 306190
GlobalTracer [candidate] (304.063 ms) : 0, 304063
AppSec [baseline] (50.196 ms) : 0, 50196
AppSec [candidate] (49.901 ms) : 0, 49901
Remote Config [baseline] (675.055 µs) : 0, 675
Remote Config [candidate] (662.047 µs) : 0, 662
Telemetry [baseline] (7.677 ms) : 0, 7677
Telemetry [candidate] (7.594 ms) : 0, 7594
section appsec
BytebuddyAgent [baseline] (680.365 ms) : 0, 680365
BytebuddyAgent [candidate] (677.998 ms) : 0, 677998
GlobalTracer [baseline] (300.434 ms) : 0, 300434
GlobalTracer [candidate] (299.94 ms) : 0, 299940
AppSec [baseline] (154.19 ms) : 0, 154190
AppSec [candidate] (154.146 ms) : 0, 154146
Remote Config [baseline] (615.402 µs) : 0, 615
Remote Config [candidate] (625.148 µs) : 0, 625
Telemetry [baseline] (8.944 ms) : 0, 8944
Telemetry [candidate] (9.261 ms) : 0, 9261
IAST [baseline] (22.165 ms) : 0, 22165
IAST [candidate] (20.361 ms) : 0, 20361
section iast
BytebuddyAgent [baseline] (780.174 ms) : 0, 780174
BytebuddyAgent [candidate] (780.041 ms) : 0, 780041
GlobalTracer [baseline] (294.865 ms) : 0, 294865
GlobalTracer [candidate] (295.906 ms) : 0, 295906
AppSec [baseline] (48.036 ms) : 0, 48036
AppSec [candidate] (47.33 ms) : 0, 47330
Remote Config [baseline] (591.218 µs) : 0, 591
Remote Config [candidate] (568.269 µs) : 0, 568
Telemetry [baseline] (7.031 ms) : 0, 7031
Telemetry [candidate] (6.995 ms) : 0, 6995
IAST [baseline] (26.492 ms) : 0, 26492
IAST [candidate] (28.955 ms) : 0, 28955
section profiling
BytebuddyAgent [baseline] (666.703 ms) : 0, 666703
BytebuddyAgent [candidate] (662.912 ms) : 0, 662912
GlobalTracer [baseline] (390.109 ms) : 0, 390109
GlobalTracer [candidate] (389.06 ms) : 0, 389060
AppSec [baseline] (52.153 ms) : 0, 52153
AppSec [candidate] (51.774 ms) : 0, 51774
Remote Config [baseline] (661.215 µs) : 0, 661
Remote Config [candidate] (647.354 µs) : 0, 647
Telemetry [baseline] (7.386 ms) : 0, 7386
Telemetry [candidate] (7.416 ms) : 0, 7416
ProfilingAgent [baseline] (97.226 ms) : 0, 97226
ProfilingAgent [candidate] (95.675 ms) : 0, 95675
Profiling [baseline] (97.251 ms) : 0, 97251
Profiling [candidate] (95.699 ms) : 0, 95699
LoadParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 12 metrics, 16 unstable metrics. Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.38.0-SNAPSHOT~086ab609b9, baseline=1.38.0-SNAPSHOT~f88def8618
dateFormat X
axisFormat %s
section baseline
no_agent (1.349 ms) : 1330, 1368
. : milestone, 1349,
appsec (1.703 ms) : 1680, 1727
. : milestone, 1703,
appsec_no_iast (1.727 ms) : 1704, 1751
. : milestone, 1727,
iast (1.491 ms) : 1469, 1514
. : milestone, 1491,
profiling (1.484 ms) : 1459, 1509
. : milestone, 1484,
tracing (1.477 ms) : 1454, 1501
. : milestone, 1477,
section candidate
no_agent (1.344 ms) : 1325, 1364
. : milestone, 1344,
appsec (1.716 ms) : 1693, 1740
. : milestone, 1716,
appsec_no_iast (1.714 ms) : 1689, 1739
. : milestone, 1714,
iast (1.49 ms) : 1468, 1513
. : milestone, 1490,
profiling (1.492 ms) : 1467, 1517
. : milestone, 1492,
tracing (1.47 ms) : 1447, 1494
. : milestone, 1470,
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.38.0-SNAPSHOT~086ab609b9, baseline=1.38.0-SNAPSHOT~f88def8618
dateFormat X
axisFormat %s
section baseline
no_agent (366.756 µs) : 346, 387
. : milestone, 367,
iast (484.553 µs) : 463, 506
. : milestone, 485,
iast_FULL (556.091 µs) : 535, 577
. : milestone, 556,
iast_GLOBAL (511.665 µs) : 489, 534
. : milestone, 512,
iast_HARDCODED_SECRET_DISABLED (478.713 µs) : 458, 499
. : milestone, 479,
iast_INACTIVE (465.652 µs) : 444, 487
. : milestone, 466,
iast_TELEMETRY_OFF (464.044 µs) : 443, 485
. : milestone, 464,
tracing (439.227 µs) : 419, 459
. : milestone, 439,
section candidate
no_agent (366.232 µs) : 345, 387
. : milestone, 366,
iast (487.299 µs) : 466, 509
. : milestone, 487,
iast_FULL (560.994 µs) : 540, 582
. : milestone, 561,
iast_GLOBAL (515.573 µs) : 493, 538
. : milestone, 516,
iast_HARDCODED_SECRET_DISABLED (486.291 µs) : 465, 507
. : milestone, 486,
iast_INACTIVE (449.96 µs) : 429, 471
. : milestone, 450,
iast_TELEMETRY_OFF (472.71 µs) : 451, 494
. : milestone, 473,
tracing (446.117 µs) : 425, 468
. : milestone, 446,
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.38.0-SNAPSHOT~086ab609b9, baseline=1.38.0-SNAPSHOT~f88def8618
dateFormat X
axisFormat %s
section baseline
no_agent (15.466 s) : 15466000, 15466000
. : milestone, 15466000,
appsec (15.221 s) : 15221000, 15221000
. : milestone, 15221000,
iast (18.806 s) : 18806000, 18806000
. : milestone, 18806000,
iast_GLOBAL (18.029 s) : 18029000, 18029000
. : milestone, 18029000,
profiling (15.271 s) : 15271000, 15271000
. : milestone, 15271000,
tracing (15.114 s) : 15114000, 15114000
. : milestone, 15114000,
section candidate
no_agent (14.96 s) : 14960000, 14960000
. : milestone, 14960000,
appsec (14.904 s) : 14904000, 14904000
. : milestone, 14904000,
iast (18.61 s) : 18610000, 18610000
. : milestone, 18610000,
iast_GLOBAL (18.052 s) : 18052000, 18052000
. : milestone, 18052000,
profiling (15.188 s) : 15188000, 15188000
. : milestone, 15188000,
tracing (15.039 s) : 15039000, 15039000
. : milestone, 15039000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.38.0-SNAPSHOT~086ab609b9, baseline=1.38.0-SNAPSHOT~f88def8618
dateFormat X
axisFormat %s
section baseline
no_agent (1.457 ms) : 1445, 1468
. : milestone, 1457,
appsec (2.193 ms) : 2158, 2228
. : milestone, 2193,
iast (1.955 ms) : 1913, 1997
. : milestone, 1955,
iast_GLOBAL (1.996 ms) : 1954, 2038
. : milestone, 1996,
profiling (1.838 ms) : 1804, 1871
. : milestone, 1838,
tracing (1.829 ms) : 1796, 1862
. : milestone, 1829,
section candidate
no_agent (1.453 ms) : 1442, 1465
. : milestone, 1453,
appsec (2.199 ms) : 2164, 2233
. : milestone, 2199,
iast (1.959 ms) : 1917, 2000
. : milestone, 1959,
iast_GLOBAL (1.998 ms) : 1957, 2040
. : milestone, 1998,
profiling (1.853 ms) : 1817, 1889
. : milestone, 1853,
tracing (1.82 ms) : 1788, 1853
. : milestone, 1820,
|
PerfectSlayer
force-pushed
the
bbujon/refactor-scope-listener
branch
2 times, most recently
from
26a1c8e
to
341f749
Compare
mcculls
reviewed
Jul 9, 2024
| add(CorrelationIdentifier.getSpanIdKey(), DDSpanId.toString(spanId)); | ||
| } | ||
| public void afterScopeActivated() { | ||
| add(CorrelationIdentifier.getTraceIdKey(), CorrelationIdentifier.getTraceId()); |
There was a problem hiding this comment.
This removes support for dynamically turning log injection on/off - was that intentional?
There was a problem hiding this comment.
Not really. My question is how is it supposed to work?
Because it looks only log4j2 uses it where all other logging frameworks don't support the feature 🤔
Should not the agent helper class be a listener of the remote config instead?
There was a problem hiding this comment.
git log is your friend ;)
LogContextScopeListener was already used by the log4j2 instrumentation to inject the active span into the log4j2 MDC. PR #5221 updated this existing listener so it only injected the active span when log injection was dynamically enabled.
Other logging instrumentations take a different approach and instrument the appropriate record/event classes to inject the active span when the MDC is being updated. They do the same isLogsInjectionEnabled check, but at a different point.
So if you want to simplify the listener without breaking support for dynamically turning log injection on/off in log4j2 then you have 2 options:
- find a different instrumentation point for
log4j2which avoids the need to use a scope listener - add a check for
AgentTracer.traceConfig().isLogsInjectionEnabled()in the log scope listener
There was a problem hiding this comment.
Also note for consistency reasons the dynamic config for a trace is locked in when that trace starts. This is to avoid issues where the sampling config changes mid-trace, because otherwise you would be applying different rules to different parts of the same trace.
This is done by using a copy-on-write approach for dynamic config when the remote config changes, with each trace taking a reference to the current dynamic config snapshot when it starts. This avoids having to take a copy on every trace, because traces occur frequently whereas remote config changes happen infrequently.
AgentTracer.traceConfig() accesses the current snapshot, which involves at most a couple of volatile reads (one to get the tracer, another to get its latest dynamic config) plus a few field accesses to get the log injection state. This isn't much more than you'd need if the log scope listener had its own remote-config listener.
There was a problem hiding this comment.
Thanks for the detailed feedback.
For now, I will simply use the AgentTracer.traceConfig().isLogsInjectionEnabled() check.
I had a look at how to change the instrumentation for Log4j2 prior to 2.7 (when ContextDataInjector was introduced). It should be doable to it instrumenting ThreadContext or ThreadContextMap.
But I also saw that ContextDataInjector is deprecated since 2.13 in favor of ContextDataProvider:
NOTE: It is no longer recommended that custom implementations of this interface be provided as it is difficult to do. Instead, provide a custom ContextDataProvider.
From ContextDataInjector Javadoc and:
The ContextDataProvider (introduced in Log4j 2.13.2) is an interface applications and libraries can use to inject additional key-value pairs into the LogEvent's context data.
From Log4j extension documentation. So I wonder if it would better if I pair with @amarziali and get to the bottom of it rather than just patching <2.7 only.
PerfectSlayer
force-pushed
the
bbujon/refactor-scope-listener
branch
from
341f749
to
086ab60
Compare
mcculls
approved these changes
Jul 17, 2024
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
This PR reduces the API of the
ExtendedScopeListener.It also replaces the implementation of the
LogContextScopeListenerto use the commonCorrelationIdentifierthat already support 128-bit TID correlation.Motivation
Limit the internal API to what’s needed.
Additional Notes
Now, only CWS-TLS needs the
ExtendedScopeListener.I kept it for performance reason as it needs ID using the
longform.Jira ticket: [PROJ-IDENT]