Fix mixing log/span decoration probes #7246
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
On same location, if we have different probes like log or span decoration probe with different characteristics (capture, snapshot, evaluateAt,...), instrumentation needs the correct definition to apply the shared CapturedContextInstrumentor. We are creating a synthetic definition for applying instrumentation and merging all characteristics required for instrumentation.
BenchmarksStartupLoadParameters
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.36.0-SNAPSHOT~ebff4fad7a, baseline=1.38.0-SNAPSHOT~5ab86d20e4
dateFormat X
axisFormat %s
section baseline
no_agent (1.336 ms) : 1317, 1355
. : milestone, 1336,
appsec (1.726 ms) : 1703, 1749
. : milestone, 1726,
appsec_no_iast (1.712 ms) : 1687, 1736
. : milestone, 1712,
iast (1.465 ms) : 1443, 1488
. : milestone, 1465,
profiling (1.48 ms) : 1455, 1504
. : milestone, 1480,
tracing (1.449 ms) : 1425, 1474
. : milestone, 1449,
section candidate
no_agent (1.335 ms) : 1316, 1354
. : milestone, 1335,
appsec (1.706 ms) : 1683, 1730
. : milestone, 1706,
appsec_no_iast (1.72 ms) : 1696, 1744
. : milestone, 1720,
iast (1.46 ms) : 1437, 1483
. : milestone, 1460,
profiling (1.528 ms) : 1504, 1553
. : milestone, 1528,
tracing (1.459 ms) : 1435, 1482
. : milestone, 1459,
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.36.0-SNAPSHOT~ebff4fad7a, baseline=1.38.0-SNAPSHOT~5ab86d20e4
dateFormat X
axisFormat %s
section baseline
no_agent (361.67 µs) : 342, 381
. : milestone, 362,
iast (469.84 µs) : 449, 491
. : milestone, 470,
iast_FULL (544.224 µs) : 523, 565
. : milestone, 544,
iast_GLOBAL (502.851 µs) : 482, 524
. : milestone, 503,
iast_HARDCODED_SECRET_DISABLED (479.767 µs) : 458, 501
. : milestone, 480,
iast_INACTIVE (456.26 µs) : 435, 477
. : milestone, 456,
iast_TELEMETRY_OFF (463.412 µs) : 443, 484
. : milestone, 463,
tracing (441.332 µs) : 420, 463
. : milestone, 441,
section candidate
no_agent (362.578 µs) : 343, 382
. : milestone, 363,
iast (472.758 µs) : 452, 494
. : milestone, 473,
iast_FULL (541.885 µs) : 521, 563
. : milestone, 542,
iast_GLOBAL (504.852 µs) : 483, 527
. : milestone, 505,
iast_HARDCODED_SECRET_DISABLED (472.337 µs) : 451, 493
. : milestone, 472,
iast_INACTIVE (447.479 µs) : 426, 469
. : milestone, 447,
iast_TELEMETRY_OFF (467.238 µs) : 446, 489
. : milestone, 467,
tracing (437.244 µs) : 416, 459
. : milestone, 437,
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.36.0-SNAPSHOT~ebff4fad7a, baseline=1.38.0-SNAPSHOT~5ab86d20e4
dateFormat X
axisFormat %s
section baseline
no_agent (14.936 s) : 14936000, 14936000
. : milestone, 14936000,
appsec (15.069 s) : 15069000, 15069000
. : milestone, 15069000,
iast (18.739 s) : 18739000, 18739000
. : milestone, 18739000,
iast_GLOBAL (18.039 s) : 18039000, 18039000
. : milestone, 18039000,
profiling (15.52 s) : 15520000, 15520000
. : milestone, 15520000,
tracing (14.795 s) : 14795000, 14795000
. : milestone, 14795000,
section candidate
no_agent (15.549 s) : 15549000, 15549000
. : milestone, 15549000,
appsec (15.13 s) : 15130000, 15130000
. : milestone, 15130000,
iast (18.877 s) : 18877000, 18877000
. : milestone, 18877000,
iast_GLOBAL (17.928 s) : 17928000, 17928000
. : milestone, 17928000,
profiling (15.004 s) : 15004000, 15004000
. : milestone, 15004000,
tracing (15.161 s) : 15161000, 15161000
. : milestone, 15161000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.36.0-SNAPSHOT~ebff4fad7a, baseline=1.38.0-SNAPSHOT~5ab86d20e4
dateFormat X
axisFormat %s
section baseline
no_agent (1.452 ms) : 1441, 1463
. : milestone, 1452,
appsec (2.225 ms) : 2190, 2260
. : milestone, 2225,
iast (1.964 ms) : 1922, 2005
. : milestone, 1964,
iast_GLOBAL (2.017 ms) : 1973, 2060
. : milestone, 2017,
profiling (1.86 ms) : 1826, 1894
. : milestone, 1860,
tracing (1.821 ms) : 1788, 1854
. : milestone, 1821,
section candidate
no_agent (1.45 ms) : 1439, 1461
. : milestone, 1450,
appsec (2.203 ms) : 2169, 2236
. : milestone, 2203,
iast (1.932 ms) : 1892, 1972
. : milestone, 1932,
iast_GLOBAL (1.983 ms) : 1943, 2023
. : milestone, 1983,
profiling (1.828 ms) : 1796, 1860
. : milestone, 1828,
tracing (1.808 ms) : 1778, 1839
. : milestone, 1808,
|
shatzi
approved these changes
Jul 8, 2024
| capture = mergeCapture(capture, logProbe.getCapture()); | ||
| } | ||
| if (definition.getEvaluateAt() == MethodLocation.ENTRY | ||
| || definition.getEvaluateAt() == MethodLocation.DEFAULT) { |
There was a problem hiding this comment.
why we overwrite EXIT with default?
There was a problem hiding this comment.
I think there should be a priority here.. if there are two probes one with DEFAULT and one with ENTRY.. the order of us going over the probe would have determine which one wins.
Does this even matter?
There was a problem hiding this comment.
DEFAULT == ENTRY from the beginning.
The fact that by default, UI put EXIT is independent of what we are doing here.
| .build(); | ||
| } | ||
|
|
||
| private LogProbe.Sampling mergeSampling( |
There was a problem hiding this comment.
why do we care about merge the sampling?
I thought we rate-limit each probe independently.. do we use the sampling of the reference definition?
There was a problem hiding this comment.
left over. this method is not called. removed
| } | ||
|
|
||
| <T extends ProbeDefinition> T createMock( | ||
| Class<T> clazz, List<ProbeDefinition> invocationOrder, String id) { | ||
| ProbeDefinition mock = mock(clazz); | ||
| doAnswer( | ||
| invocation -> { | ||
| invocationOrder.add(mock); | ||
| // invocationOrder.add(mock); |
| .where(CLASS_NAME, "process") | ||
| .captureSnapshot(true) | ||
| .sampling(10.0) | ||
| .capture(5, 200, 200, 30) |
There was a problem hiding this comment.
do we want to test that we merge the sampling and captures limit correctly?
add test for merging capture limits
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
On same location, if we have different probes like log or span decoration probe with different characteristics (capture, snapshot, evaluateAt,...), instrumentation needs the correct definition to apply
the shared CapturedContextInstrumentor.
We are creating a synthetic definition for applying instrumentation and merging all characteristics required for instrumentation.
Motivation
Additional Notes
Jira ticket: DEBUG-2498