Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix sampling for line probe without condition #6135

Merged
merged 3 commits into from
Nov 2, 2023
Merged

Fix sampling for line probe without condition #6135

merged 3 commits into from
Nov 2, 2023

Conversation

jpbempel
Copy link
Member

@jpbempel jpbempel commented Nov 2, 2023 •
edited by jira bot
Loading

What Does This Do

In the case of a line probe MethodLocation is DEFAULT and we should not consider the evaluteAt attribute as it is meant for method probe only

Motivation

no sampling for line probe without condition

Additional Notes

Jira ticket: DEBUG-1908

@jpbempel
Fix sampling for line probe without condition
69b777a 
In the case of a line probe MethodLocation is DEFAULT and we should
not consider the evaluteAt attribute as it is meant for method probe
only
@jpbempel jpbempel requested a review from a team as a code owner November 2, 2023 14:14
@jpbempel jpbempel requested review from ojung and removed request for a team November 2, 2023 14:14
@jpbempel jpbempel added comp: debugger Dynamic Instrumentation type: bug labels Nov 2, 2023
@pr-commenter
Copy link

pr-commenter bot commented Nov 2, 2023 •
edited
Loading

Benchmarks

Startup

Parameters

Baseline Candidate
commit 1.23.0-SNAPSHOT~20a70a4bbf 1.23.0-SNAPSHOT~ef53842d04
config baseline candidate
See matching parameters
Baseline Candidate
application insecure-bank insecure-bank
module Agent Agent
parent None None
variant iast iast

Summary

Found 2 performance improvements and 0 performance regressions! Performance is the same for 52 cases.

scenario Δ mean execution_time candidate mean execution_time baseline mean execution_time
scenario:petclinic:profiling:ProfilingAgent better
[-9.241ms; -4.596ms] or [-10.442%; -5.194%]		 81.575ms 88.493ms
scenario:petclinic:profiling:Profiling better
[-9.241ms; -4.596ms] or [-10.439%; -5.192%]		 81.599ms 88.517ms
Startup time reports for petclinic 
gantt
    title petclinic - global startup overhead: candidate=1.23.0-SNAPSHOT~ef53842d04, baseline=1.23.0-SNAPSHOT~20a70a4bbf

    dateFormat X
    axisFormat %s
section tracing
Agent [baseline] (1.045 s) : 0, 1045146
Total [baseline] (9.453 s) : 0, 9452709
Agent [candidate] (1.038 s) : 0, 1038225
Total [candidate] (9.325 s) : 0, 9325189
section appsec
Agent [baseline] (1.119 s) : 0, 1118740
Total [baseline] (9.405 s) : 0, 9404595
Agent [candidate] (1.118 s) : 0, 1117912
Total [candidate] (9.426 s) : 0, 9426140
section iast
Agent [baseline] (1.159 s) : 0, 1158565
Total [baseline] (9.606 s) : 0, 9605652
Agent [candidate] (1.156 s) : 0, 1156464
Total [candidate] (9.526 s) : 0, 9526237
section profiling
Agent [baseline] (1.223 s) : 0, 1223054
Total [baseline] (9.532 s) : 0, 9532312
Agent [candidate] (1.22 s) : 0, 1220493
Total [candidate] (9.603 s) : 0, 9603179
Loading
  • baseline results
Module Variant Duration Δ tracing
Agent tracing 1.045 s -
Agent appsec 1.119 s 73.593 ms (7.0%)
Agent iast 1.159 s 113.418 ms (10.9%)
Agent profiling 1.223 s 177.908 ms (17.0%)
Total tracing 9.453 s -
Total appsec 9.405 s -48.114 ms (-0.5%)
Total iast 9.606 s 152.943 ms (1.6%)
Total profiling 9.532 s 79.603 ms (0.8%)
  • candidate results
Module Variant Duration Δ tracing
Agent tracing 1.038 s -
Agent appsec 1.118 s 79.687 ms (7.7%)
Agent iast 1.156 s 118.24 ms (11.4%)
Agent profiling 1.22 s 182.268 ms (17.6%)
Total tracing 9.325 s -
Total appsec 9.426 s 100.951 ms (1.1%)
Total iast 9.526 s 201.048 ms (2.2%)
Total profiling 9.603 s 277.99 ms (3.0%) 
gantt
    title petclinic - break down per module: candidate=1.23.0-SNAPSHOT~ef53842d04, baseline=1.23.0-SNAPSHOT~20a70a4bbf

    dateFormat X
    axisFormat %s
section tracing
BytebuddyAgent [baseline] (651.344 ms) : 0, 651344
BytebuddyAgent [candidate] (647.114 ms) : 0, 647114
GlobalTracer [baseline] (297.383 ms) : 0, 297383
GlobalTracer [candidate] (295.652 ms) : 0, 295652
AppSec [baseline] (49.525 ms) : 0, 49525
AppSec [candidate] (48.947 ms) : 0, 48947
Remote Config [baseline] (702.919 µs) : 0, 703
Remote Config [candidate] (702.801 µs) : 0, 703
Telemetry [baseline] (11.454 ms) : 0, 11454
Telemetry [candidate] (11.368 ms) : 0, 11368
section appsec
BytebuddyAgent [baseline] (645.002 ms) : 0, 645002
BytebuddyAgent [candidate] (644.307 ms) : 0, 644307
GlobalTracer [baseline] (293.414 ms) : 0, 293414
GlobalTracer [candidate] (293.601 ms) : 0, 293601
AppSec [baseline] (138.49 ms) : 0, 138490
AppSec [candidate] (138.191 ms) : 0, 138191
Remote Config [baseline] (643.427 µs) : 0, 643
Remote Config [candidate] (650.24 µs) : 0, 650
Telemetry [baseline] (6.88 ms) : 0, 6880
Telemetry [candidate] (6.871 ms) : 0, 6871
section iast
BytebuddyAgent [baseline] (771.183 ms) : 0, 771183
BytebuddyAgent [candidate] (771.035 ms) : 0, 771035
GlobalTracer [baseline] (275.757 ms) : 0, 275757
GlobalTracer [candidate] (275.141 ms) : 0, 275141
AppSec [baseline] (47.007 ms) : 0, 47007
AppSec [candidate] (46.78 ms) : 0, 46780
Remote Config [baseline] (585.005 µs) : 0, 585
Remote Config [candidate] (559.613 µs) : 0, 560
Telemetry [baseline] (12.632 ms) : 0, 12632
Telemetry [candidate] (11.344 ms) : 0, 11344
IAST [baseline] (16.755 ms) : 0, 16755
IAST [candidate] (16.949 ms) : 0, 16949
section profiling
BytebuddyAgent [baseline] (659.096 ms) : 0, 659096
BytebuddyAgent [candidate] (661.538 ms) : 0, 661538
GlobalTracer [baseline] (359.7 ms) : 0, 359700
GlobalTracer [candidate] (360.962 ms) : 0, 360962
AppSec [baseline] (49.261 ms) : 0, 49261
AppSec [candidate] (49.581 ms) : 0, 49581
Remote Config [baseline] (667.632 µs) : 0, 668
Remote Config [candidate] (656.732 µs) : 0, 657
Telemetry [baseline] (11.327 ms) : 0, 11327
Telemetry [candidate] (11.434 ms) : 0, 11434
ProfilingAgent [baseline] (88.493 ms) : 0, 88493
ProfilingAgent [candidate] (81.575 ms) : 0, 81575
Profiling [baseline] (88.517 ms) : 0, 88517
Profiling [candidate] (81.599 ms) : 0, 81599
Loading
Startup time reports for insecure-bank 
gantt
    title insecure-bank - global startup overhead: candidate=1.23.0-SNAPSHOT~ef53842d04, baseline=1.23.0-SNAPSHOT~20a70a4bbf

    dateFormat X
    axisFormat %s
section tracing
Agent [baseline] (1.037 s) : 0, 1037244
Total [baseline] (8.795 s) : 0, 8794582
Agent [candidate] (1.035 s) : 0, 1035295
Total [candidate] (8.78 s) : 0, 8779771
section iast
Agent [baseline] (1.147 s) : 0, 1147177
Total [baseline] (9.312 s) : 0, 9311591
Agent [candidate] (1.158 s) : 0, 1158370
Total [candidate] (9.334 s) : 0, 9334217
section iast_TELEMETRY_OFF
Agent [baseline] (1.143 s) : 0, 1142957
Total [baseline] (9.277 s) : 0, 9277406
Agent [candidate] (1.144 s) : 0, 1143699
Total [candidate] (9.318 s) : 0, 9317669
Loading
  • baseline results
Module Variant Duration Δ tracing
Agent tracing 1.037 s -
Agent iast 1.147 s 109.934 ms (10.6%)
Agent iast_TELEMETRY_OFF 1.143 s 105.714 ms (10.2%)
Total tracing 8.795 s -
Total iast 9.312 s 517.009 ms (5.9%)
Total iast_TELEMETRY_OFF 9.277 s 482.824 ms (5.5%)
  • candidate results
Module Variant Duration Δ tracing
Agent tracing 1.035 s -
Agent iast 1.158 s 123.075 ms (11.9%)
Agent iast_TELEMETRY_OFF 1.144 s 108.404 ms (10.5%)
Total tracing 8.78 s -
Total iast 9.334 s 554.446 ms (6.3%)
Total iast_TELEMETRY_OFF 9.318 s 537.898 ms (6.1%) 
gantt
    title insecure-bank - break down per module: candidate=1.23.0-SNAPSHOT~ef53842d04, baseline=1.23.0-SNAPSHOT~20a70a4bbf

    dateFormat X
    axisFormat %s
section tracing
BytebuddyAgent [baseline] (647.965 ms) : 0, 647965
BytebuddyAgent [candidate] (645.736 ms) : 0, 645736
GlobalTracer [baseline] (293.561 ms) : 0, 293561
GlobalTracer [candidate] (294.14 ms) : 0, 294140
AppSec [baseline] (49.141 ms) : 0, 49141
AppSec [candidate] (48.971 ms) : 0, 48971
Remote Config [baseline] (695.025 µs) : 0, 695
Remote Config [candidate] (703.113 µs) : 0, 703
Telemetry [baseline] (11.282 ms) : 0, 11282
Telemetry [candidate] (11.299 ms) : 0, 11299
section iast
BytebuddyAgent [baseline] (763.243 ms) : 0, 763243
BytebuddyAgent [candidate] (771.994 ms) : 0, 771994
GlobalTracer [baseline] (273.063 ms) : 0, 273063
GlobalTracer [candidate] (273.957 ms) : 0, 273957
AppSec [baseline] (46.502 ms) : 0, 46502
AppSec [candidate] (46.818 ms) : 0, 46818
Remote Config [baseline] (565.876 µs) : 0, 566
Remote Config [candidate] (569.969 µs) : 0, 570
Telemetry [baseline] (11.689 ms) : 0, 11689
Telemetry [candidate] (13.187 ms) : 0, 13187
IAST [baseline] (17.766 ms) : 0, 17766
IAST [candidate] (17.097 ms) : 0, 17097
section iast_TELEMETRY_OFF
BytebuddyAgent [baseline] (758.553 ms) : 0, 758553
BytebuddyAgent [candidate] (758.189 ms) : 0, 758189
GlobalTracer [baseline] (274.058 ms) : 0, 274058
GlobalTracer [candidate] (274.744 ms) : 0, 274744
AppSec [baseline] (46.406 ms) : 0, 46406
AppSec [candidate] (46.674 ms) : 0, 46674
Remote Config [baseline] (556.891 µs) : 0, 557
Remote Config [candidate] (569.886 µs) : 0, 570
Telemetry [baseline] (12.601 ms) : 0, 12601
Telemetry [candidate] (12.818 ms) : 0, 12818
IAST [baseline] (16.381 ms) : 0, 16381
IAST [candidate] (16.245 ms) : 0, 16245
Loading

Load

Parameters

Baseline Candidate
commit 1.23.0-SNAPSHOT~20a70a4bbf 1.23.0-SNAPSHOT~ef53842d04
config baseline candidate
end_time 2023-11-02T17:32:43 2023-11-02T17:49:13
start_time 2023-11-02T17:32:30 2023-11-02T17:49:00
See matching parameters
Baseline Candidate
application insecure-bank insecure-bank
variant iast iast

Summary

Found 0 performance improvements and 0 performance regressions! Performance is the same for 22 cases.

Request duration reports for petclinic 
gantt
    title petclinic - request duration [CI 0.99] : candidate=1.23.0-SNAPSHOT~ef53842d04, baseline=1.23.0-SNAPSHOT~20a70a4bbf
    dateFormat X
    axisFormat %s
section baseline
no_agent (1.367 ms) : 1348, 1385
.   : milestone, 1367,
appsec (1.749 ms) : 1725, 1774
.   : milestone, 1749,
iast (1.491 ms) : 1466, 1515
.   : milestone, 1491,
profiling (1.463 ms) : 1438, 1488
.   : milestone, 1463,
tracing (1.448 ms) : 1424, 1472
.   : milestone, 1448,
section candidate
no_agent (1.355 ms) : 1336, 1374
.   : milestone, 1355,
appsec (1.713 ms) : 1688, 1738
.   : milestone, 1713,
iast (1.483 ms) : 1459, 1506
.   : milestone, 1483,
profiling (1.462 ms) : 1437, 1488
.   : milestone, 1462,
tracing (1.453 ms) : 1429, 1478
.   : milestone, 1453,
Loading
  • baseline results
Variant Request duration [CI 0.99] Δ no_agent
no_agent 1.367 ms [1.348 ms, 1.385 ms] -
appsec 1.749 ms [1.725 ms, 1.774 ms] 382.917 µs (28.0%)
iast 1.491 ms [1.466 ms, 1.515 ms] 124.115 µs (9.1%)
profiling 1.463 ms [1.438 ms, 1.488 ms] 96.223 µs (7.0%)
tracing 1.448 ms [1.424 ms, 1.472 ms] 81.464 µs (6.0%)
  • candidate results
Variant Request duration [CI 0.99] Δ no_agent
no_agent 1.355 ms [1.336 ms, 1.374 ms] -
appsec 1.713 ms [1.688 ms, 1.738 ms] 357.978 µs (26.4%)
iast 1.483 ms [1.459 ms, 1.506 ms] 127.972 µs (9.4%)
profiling 1.462 ms [1.437 ms, 1.488 ms] 107.411 µs (7.9%)
tracing 1.453 ms [1.429 ms, 1.478 ms] 98.63 µs (7.3%)
Request duration reports for insecure-bank 
gantt
    title insecure-bank - request duration [CI 0.99] : candidate=1.23.0-SNAPSHOT~ef53842d04, baseline=1.23.0-SNAPSHOT~20a70a4bbf
    dateFormat X
    axisFormat %s
section baseline
no_agent (363.682 µs) : 344, 384
.   : milestone, 364,
iast (461.703 µs) : 441, 482
.   : milestone, 462,
iast_FULL (530.542 µs) : 510, 551
.   : milestone, 531,
iast_INACTIVE (437.743 µs) : 417, 459
.   : milestone, 438,
iast_TELEMETRY_OFF (462.736 µs) : 441, 484
.   : milestone, 463,
tracing (440.161 µs) : 419, 461
.   : milestone, 440,
section candidate
no_agent (361.439 µs) : 341, 382
.   : milestone, 361,
iast (467.958 µs) : 447, 489
.   : milestone, 468,
iast_FULL (519.105 µs) : 498, 540
.   : milestone, 519,
iast_INACTIVE (446.516 µs) : 425, 468
.   : milestone, 447,
iast_TELEMETRY_OFF (465.947 µs) : 444, 487
.   : milestone, 466,
tracing (436.973 µs) : 416, 458
.   : milestone, 437,
Loading
  • baseline results
Variant Request duration [CI 0.99] Δ no_agent
no_agent 363.682 µs [343.541 µs, 383.823 µs] -
iast 461.703 µs [441.072 µs, 482.334 µs] 98.021 µs (27.0%)
iast_FULL 530.542 µs [509.854 µs, 551.23 µs] 166.859 µs (45.9%)
iast_INACTIVE 437.743 µs [416.877 µs, 458.608 µs] 74.06 µs (20.4%)
iast_TELEMETRY_OFF 462.736 µs [441.365 µs, 484.107 µs] 99.054 µs (27.2%)
tracing 440.161 µs [419.06 µs, 461.261 µs] 76.479 µs (21.0%)
  • candidate results
Variant Request duration [CI 0.99] Δ no_agent
no_agent 361.439 µs [340.771 µs, 382.106 µs] -
iast 467.958 µs [446.898 µs, 489.018 µs] 106.519 µs (29.5%)
iast_FULL 519.105 µs [498.395 µs, 539.815 µs] 157.667 µs (43.6%)
iast_INACTIVE 446.516 µs [425.089 µs, 467.943 µs] 85.078 µs (23.5%)
iast_TELEMETRY_OFF 465.947 µs [444.419 µs, 487.474 µs] 104.508 µs (28.9%)
tracing 436.973 µs [415.55 µs, 458.397 µs] 75.535 µs (20.9%)

shatzi
shatzi approved these changes Nov 2, 2023
View reviewed changes
...bugger/debugger-bootstrap/src/main/java/datadog/trace/bootstrap/debugger/MethodLocation.java
@@ -6,6 +6,10 @@ public enum MethodLocation {
EXIT;

public static boolean isSame(MethodLocation methodLocation, MethodLocation evaluateAt) {
if (methodLocation == MethodLocation.DEFAULT) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we do this only for line probe. maybe the line probe logic should not call this instead of adding this case in the method?

Aka, the evaluate function should already have the probe definition, right? so that function can simply skip this if the probe location does not need to check method location

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it centralizes the logic for both calls:

    LogStatus logStatus = (LogStatus) status;
    if (!hasCondition()) {
      // sample when no condition associated
      sample(logStatus, methodLocation);
    }
    logStatus.setCondition(evaluateCondition(context, logStatus));
    CapturedContext.CapturedThrowable throwable = context.getThrowable();
    if (logStatus.hasConditionErrors() && throwable != null) {
      logStatus.addError(
          new EvaluationError(
              "uncaught exception", throwable.getType() + ": " + throwable.getMessage()));
    }
    if (hasCondition() && logStatus.getCondition()) {
      // sample if probe has condition and condition is true
      sample(logStatus, methodLocation);
    }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually it's better to centralize also the other isSame call

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would explain this in the comment. that MethodLocation.DEFAULT is used to mark that this is a line probe and therefore we don't check the probeLocation as it meaningless.

@jpbempel jpbempel merged commit 00c9ebe into master Nov 2, 2023
67 of 69 checks passed
@jpbempel jpbempel deleted the jpbempel/line-probe-rate-limit branch November 2, 2023 20:34
@github-actions github-actions bot added this to the 1.23.0 milestone Nov 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shatzi shatzi shatzi approved these changes

@ojung ojung Awaiting requested review from ojung ojung is a code owner automatically assigned from DataDog/debugger-java

Assignees
No one assigned
Labels
comp: debugger Dynamic Instrumentation type: bug
Projects
None yet
Milestone
1.23.0
Development

Successfully merging this pull request may close these issues.
2 participants
@jpbempel @shatzi