-
Notifications
You must be signed in to change notification settings - Fork 283
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
Adds resource name matching to the trace sampling rules #5900
Conversation
This changes adds resource name matching to the trace sampling rules. This change also changes matching on service and operation name to use glob matching -- which does introduce a small breaking change if someone used * or ? in their name previously In addition to the core change to trace sampling rules, this code also cleans up the handling of glob matching New util classes Matcher and Matchers have now been introduced that handle most of the glob related code This clean-up allowed me to eliminate some existing duplication in the single span code In a subsequent changeset, I may replace the pattern based glob matching with a proper glob matcher.
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 54 cases. Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.25.0-SNAPSHOT~c642f50b85, baseline=1.25.0-SNAPSHOT~743bacde52
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.039 s) : 0, 1038656
Total [baseline] (8.809 s) : 0, 8808947
Agent [candidate] (1.033 s) : 0, 1033283
Total [candidate] (8.776 s) : 0, 8775544
section iast
Agent [baseline] (1.151 s) : 0, 1150508
Total [baseline] (9.357 s) : 0, 9356576
Agent [candidate] (1.152 s) : 0, 1151940
Total [candidate] (9.322 s) : 0, 9322250
section iast_TELEMETRY_OFF
Agent [baseline] (1.15 s) : 0, 1149979
Total [baseline] (9.3 s) : 0, 9300291
Agent [candidate] (1.145 s) : 0, 1144647
Total [candidate] (9.307 s) : 0, 9307023
gantt
title insecure-bank - break down per module: candidate=1.25.0-SNAPSHOT~c642f50b85, baseline=1.25.0-SNAPSHOT~743bacde52
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (650.279 ms) : 0, 650279
BytebuddyAgent [candidate] (646.94 ms) : 0, 646940
GlobalTracer [baseline] (297.194 ms) : 0, 297194
GlobalTracer [candidate] (295.446 ms) : 0, 295446
AppSec [baseline] (48.73 ms) : 0, 48730
AppSec [candidate] (48.62 ms) : 0, 48620
Remote Config [baseline] (668.389 µs) : 0, 668
Remote Config [candidate] (662.293 µs) : 0, 662
Telemetry [baseline] (7.208 ms) : 0, 7208
Telemetry [candidate] (7.286 ms) : 0, 7286
section iast
BytebuddyAgent [baseline] (767.447 ms) : 0, 767447
BytebuddyAgent [candidate] (768.557 ms) : 0, 768557
GlobalTracer [baseline] (274.936 ms) : 0, 274936
GlobalTracer [candidate] (275.897 ms) : 0, 275897
AppSec [baseline] (46.418 ms) : 0, 46418
AppSec [candidate] (47.065 ms) : 0, 47065
IAST [baseline] (18.121 ms) : 0, 18121
IAST [candidate] (17.479 ms) : 0, 17479
Remote Config [baseline] (592.702 µs) : 0, 593
Remote Config [candidate] (579.188 µs) : 0, 579
Telemetry [baseline] (8.52 ms) : 0, 8520
Telemetry [candidate] (7.831 ms) : 0, 7831
section iast_TELEMETRY_OFF
BytebuddyAgent [baseline] (766.517 ms) : 0, 766517
BytebuddyAgent [candidate] (762.175 ms) : 0, 762175
GlobalTracer [baseline] (276.865 ms) : 0, 276865
GlobalTracer [candidate] (276.224 ms) : 0, 276224
AppSec [baseline] (46.934 ms) : 0, 46934
AppSec [candidate] (46.89 ms) : 0, 46890
IAST [baseline] (17.341 ms) : 0, 17341
IAST [candidate] (17.188 ms) : 0, 17188
Remote Config [baseline] (585.456 µs) : 0, 585
Remote Config [candidate] (583.625 µs) : 0, 584
Telemetry [baseline] (7.035 ms) : 0, 7035
Telemetry [candidate] (7.113 ms) : 0, 7113
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.25.0-SNAPSHOT~c642f50b85, baseline=1.25.0-SNAPSHOT~743bacde52
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.033 s) : 0, 1033340
Total [baseline] (9.322 s) : 0, 9322213
Agent [candidate] (1.035 s) : 0, 1035381
Total [candidate] (9.354 s) : 0, 9354267
section appsec
Agent [baseline] (1.121 s) : 0, 1121327
Total [baseline] (9.358 s) : 0, 9358073
Agent [candidate] (1.123 s) : 0, 1123301
Total [candidate] (9.4 s) : 0, 9400181
section iast
Agent [baseline] (1.15 s) : 0, 1149868
Total [baseline] (9.518 s) : 0, 9517942
Agent [candidate] (1.154 s) : 0, 1153823
Total [candidate] (9.551 s) : 0, 9551438
section profiling
Agent [baseline] (1.216 s) : 0, 1216343
Total [baseline] (9.504 s) : 0, 9504093
Agent [candidate] (1.227 s) : 0, 1227028
Total [candidate] (9.559 s) : 0, 9558872
gantt
title petclinic - break down per module: candidate=1.25.0-SNAPSHOT~c642f50b85, baseline=1.25.0-SNAPSHOT~743bacde52
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (647.012 ms) : 0, 647012
BytebuddyAgent [candidate] (647.851 ms) : 0, 647851
GlobalTracer [baseline] (295.218 ms) : 0, 295218
GlobalTracer [candidate] (296.034 ms) : 0, 296034
AppSec [baseline] (48.872 ms) : 0, 48872
AppSec [candidate] (49.057 ms) : 0, 49057
Remote Config [baseline] (656.958 µs) : 0, 657
Remote Config [candidate] (662.775 µs) : 0, 663
Telemetry [baseline] (7.246 ms) : 0, 7246
Telemetry [candidate] (7.286 ms) : 0, 7286
section appsec
BytebuddyAgent [baseline] (646.804 ms) : 0, 646804
BytebuddyAgent [candidate] (646.544 ms) : 0, 646544
GlobalTracer [baseline] (294.411 ms) : 0, 294411
GlobalTracer [candidate] (296.288 ms) : 0, 296288
AppSec [baseline] (138.416 ms) : 0, 138416
AppSec [candidate] (138.724 ms) : 0, 138724
Remote Config [baseline] (635.675 µs) : 0, 636
Remote Config [candidate] (636.538 µs) : 0, 637
Telemetry [baseline] (6.73 ms) : 0, 6730
Telemetry [candidate] (6.753 ms) : 0, 6753
section iast
BytebuddyAgent [baseline] (767.079 ms) : 0, 767079
BytebuddyAgent [candidate] (770.209 ms) : 0, 770209
GlobalTracer [baseline] (274.809 ms) : 0, 274809
GlobalTracer [candidate] (275.853 ms) : 0, 275853
AppSec [baseline] (46.471 ms) : 0, 46471
AppSec [candidate] (46.75 ms) : 0, 46750
Remote Config [baseline] (617.998 µs) : 0, 618
Remote Config [candidate] (559.039 µs) : 0, 559
Telemetry [baseline] (7.746 ms) : 0, 7746
Telemetry [candidate] (6.999 ms) : 0, 6999
IAST [baseline] (18.811 ms) : 0, 18811
IAST [candidate] (18.908 ms) : 0, 18908
section profiling
ProfilingAgent [baseline] (87.473 ms) : 0, 87473
ProfilingAgent [candidate] (88.313 ms) : 0, 88313
BytebuddyAgent [baseline] (657.531 ms) : 0, 657531
BytebuddyAgent [candidate] (664.197 ms) : 0, 664197
GlobalTracer [baseline] (360.343 ms) : 0, 360343
GlobalTracer [candidate] (362.789 ms) : 0, 362789
AppSec [baseline] (48.681 ms) : 0, 48681
AppSec [candidate] (48.791 ms) : 0, 48791
Remote Config [baseline] (652.995 µs) : 0, 653
Remote Config [candidate] (652.076 µs) : 0, 652
Telemetry [baseline] (7.37 ms) : 0, 7370
Telemetry [candidate] (7.469 ms) : 0, 7469
Profiling [baseline] (87.497 ms) : 0, 87497
Profiling [candidate] (88.336 ms) : 0, 88336
LoadParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 22 cases. Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.25.0-SNAPSHOT~c642f50b85, baseline=1.25.0-SNAPSHOT~743bacde52
dateFormat X
axisFormat %s
section baseline
no_agent (362.408 µs) : 342, 382
. : milestone, 362,
iast (468.117 µs) : 447, 489
. : milestone, 468,
iast_FULL (533.476 µs) : 513, 554
. : milestone, 533,
iast_INACTIVE (444.804 µs) : 424, 466
. : milestone, 445,
iast_TELEMETRY_OFF (465.494 µs) : 444, 487
. : milestone, 465,
tracing (438.575 µs) : 417, 460
. : milestone, 439,
section candidate
no_agent (362.345 µs) : 342, 383
. : milestone, 362,
iast (466.286 µs) : 446, 487
. : milestone, 466,
iast_FULL (535.142 µs) : 514, 556
. : milestone, 535,
iast_INACTIVE (443.438 µs) : 423, 464
. : milestone, 443,
iast_TELEMETRY_OFF (473.018 µs) : 452, 494
. : milestone, 473,
tracing (438.836 µs) : 417, 460
. : milestone, 439,
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.25.0-SNAPSHOT~c642f50b85, baseline=1.25.0-SNAPSHOT~743bacde52
dateFormat X
axisFormat %s
section baseline
no_agent (1.327 ms) : 1308, 1346
. : milestone, 1327,
appsec (1.718 ms) : 1694, 1743
. : milestone, 1718,
iast (1.491 ms) : 1467, 1515
. : milestone, 1491,
profiling (1.492 ms) : 1466, 1519
. : milestone, 1492,
tracing (1.474 ms) : 1450, 1499
. : milestone, 1474,
section candidate
no_agent (1.352 ms) : 1333, 1372
. : milestone, 1352,
appsec (1.717 ms) : 1692, 1741
. : milestone, 1717,
iast (1.489 ms) : 1465, 1514
. : milestone, 1489,
profiling (1.493 ms) : 1467, 1519
. : milestone, 1493,
tracing (1.484 ms) : 1460, 1509
. : milestone, 1484,
|
|| operationExactName.contentEquals(span.getOperationName())) | ||
&& (operationPattern == null | ||
|| operationPattern.matcher(span.getOperationName()).matches()); | ||
return Matchers.matches(serviceMatcher, span.getServiceName()) |
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 clean up!
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 to me
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
|
||
import java.util.regex.Pattern; | ||
|
||
public final class Matchers { |
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.
Can we add some unit tests here to codify the edge cases?
I don't have many concerns about the correctness of the code, but it would be nice to have our edge cases explicitly defined
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.
Tests are ready for another review
@@ -19,15 +19,15 @@ public static final Matcher compileGlob(String glob) { | |||
} | |||
} | |||
|
|||
public static final boolean matches(Matcher matcher, String str) { |
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.
Unnecessary final
in the final class
16cece8
to
0602486
Compare
@@ -5,20 +5,11 @@ | |||
public final class GlobPattern { | |||
|
|||
public static Pattern globToRegexPattern(String globPattern) { | |||
if (globPattern == 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.
Removed now unnecessary checks in these methods
What Does This Do
This changes adds resource name matching to the trace sampling rules.
This change also changes matching on service and operation name to use glob matching -- which does introduce a small breaking change if someone used * or ? in their name previously
The breakage is small, since a glob pattern always matches itself. Any existing rules will continue match all the traces that they matched previously, but they may match additional traces now.
Motivation
This change will allow users to sample traces using a resource pattern match
Additional Notes
In addition to the core change to trace sampling rules, this code also cleans up the handling of glob matching
New util classes Matcher and Matchers have now been introduced that handle most of the glob related code
This clean-up allowed me to eliminate some existing duplication in the single span code.
In a subsequent changeset, I may replace the pattern based glob matching with a proper glob matcher.
Parametric tests: DataDog/system-tests#1816