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.

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

Reenable tracer flare and handle debug request #5040

Merged
merged 2 commits into from
Jan 15, 2024

Conversation

andrewlock
Copy link
Member

Summary of changes

  • Reenable the tracer flare
  • Restrict handling of AGENT_CONFIG remote configuration to only flare-log-level.debug and flare-log-level.trace

Reason for change

When we subscribe to AGENT_CONFIG, you always receive a config on app start that describes the different log levels available, regardless of whether a tracer flare is activated. We should not enable debug mode when we receive that initial config.

Implementation details

Check the Id of the AGENT_CONFIG details to listen for flare-log-level.debug and flare-log-level.trace. Only treat the addition (or removal) of those configs as the tracer-flare ones we want

Test coverage

Updated integration test to cover this. Considered refactoring the TracerFlareManager to test more thoroughly, but the logic is simple enough (and the complexity is more around what we actually receive, which these wouldn't test for) so it didn't seem worth it 🤷‍♂️

Other details

We may well want to handle other flare-log-level.* AGENT_CONFIG events (e.g. flare-log-level.warn) and treat those in the opposite way (i.e. disabled debug when received, and enable debug when removed). But they're usage as part of the flare isn't clear/specified currently, so deferring that till we have more concrete cases.

@andrewlock andrewlock requested a review from a team as a code owner January 10, 2024 14:54
@datadog-ddstaging
Copy link

datadog-ddstaging bot commented Jan 10, 2024

Datadog Report

Branch report: andrew/tracer_flare/fix_debug_mode
Commit report: 8d6ea3b
Test service: dd-trace-dotnet

✅ 0 Failed, 310330 Passed, 1002 Skipped, 33m 46.84s Wall Time

@andrewlock
Copy link
Member Author

Execution-Time Benchmarks Report ⏱️

Execution-time results for samples comparing the following branches/commits:

Execution-time benchmarks measure the whole time it takes to execute a program. And are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are shown in red. The following thresholds were used for comparing the execution times:

  • Welch test with statistical test for significance of 5%
  • Only results indicating a difference greater than 5% and 5 ms are considered.

Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard.

Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph).

gantt
    title Execution time (ms) FakeDbCommand (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5040) - mean (71ms)  : 62, 80
     .   : milestone, 71,
    master - mean (71ms)  : 64, 77
     .   : milestone, 71,

    section CallTarget+Inlining+NGEN
    This PR (5040) - mean (1,025ms)  : 993, 1057
     .   : milestone, 1025,
    master - mean (1,048ms)  : 1031, 1066
     .   : milestone, 1048,

Loading
gantt
    title Execution time (ms) FakeDbCommand (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5040) - mean (105ms)  : 99, 110
     .   : milestone, 105,
    master - mean (106ms)  : 103, 109
     .   : milestone, 106,

    section CallTarget+Inlining+NGEN
    This PR (5040) - mean (729ms)  : 709, 749
     .   : milestone, 729,
    master - mean (741ms)  : 724, 759
     .   : milestone, 741,

Loading
gantt
    title Execution time (ms) FakeDbCommand (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5040) - mean (89ms)  : 84, 93
     .   : milestone, 89,
    master - mean (90ms)  : 88, 92
     .   : milestone, 90,

    section CallTarget+Inlining+NGEN
    This PR (5040) - mean (688ms)  : 664, 712
     .   : milestone, 688,
    master - mean (701ms)  : 681, 722
     .   : milestone, 701,

Loading
gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5040) - mean (187ms)  : 184, 190
     .   : milestone, 187,
    master - mean (187ms)  : 185, 190
     .   : milestone, 187,

    section CallTarget+Inlining+NGEN
    This PR (5040) - mean (1,138ms)  : 1117, 1158
     .   : milestone, 1138,
    master - mean (1,146ms)  : 1132, 1161
     .   : milestone, 1146,

Loading
gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5040) - mean (271ms)  : 267, 275
     .   : milestone, 271,
    master - mean (270ms)  : 267, 274
     .   : milestone, 270,

    section CallTarget+Inlining+NGEN
    This PR (5040) - mean (1,097ms)  : 1073, 1120
     .   : milestone, 1097,
    master - mean (1,099ms)  : 1072, 1127
     .   : milestone, 1099,

Loading
gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5040) - mean (261ms)  : 258, 264
     .   : milestone, 261,
    master - mean (261ms)  : 256, 266
     .   : milestone, 261,

    section CallTarget+Inlining+NGEN
    This PR (5040) - mean (1,061ms)  : 1041, 1082
     .   : milestone, 1061,
    master - mean (1,067ms)  : 1040, 1094
     .   : milestone, 1067,

Loading

@andrewlock
Copy link
Member Author

Throughput/Crank Report:zap:

Throughput results for AspNetCoreSimpleController comparing the following branches/commits:

Cases where throughput results for the PR are worse than latest master (5% drop or greater), results are shown in red.

Note that these results are based on a single point-in-time result for each branch. For full results, see one of the many, many dashboards!

gantt
    title Throughput Linux x64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (5040) (11.695M)   : 0, 11694691
    master (11.037M)   : 0, 11037087
    benchmarks/2.9.0 (11.276M)   : 0, 11275582

    section Automatic
    This PR (5040) (7.900M)   : 0, 7899732
    master (7.494M)   : 0, 7494167
    benchmarks/2.9.0 (8.217M)   : 0, 8216570

    section Trace stats
    This PR (5040) (8.386M)   : 0, 8386240
    master (7.846M)   : 0, 7846190

    section Manual
    This PR (5040) (9.989M)   : 0, 9989301
    master (9.649M)   : 0, 9649292

    section Manual + Automatic
    This PR (5040) (7.507M)   : 0, 7507119
    master (7.071M)   : 0, 7070720

    section Version Conflict
    This PR (5040) (6.772M)   : 0, 6771631
    master (6.433M)   : 0, 6433352

Loading
gantt
    title Throughput Linux arm64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (5040) (9.555M)   : 0, 9554904
    master (9.556M)   : 0, 9556222
    benchmarks/2.9.0 (9.573M)   : 0, 9572514

    section Automatic
    This PR (5040) (6.683M)   : 0, 6682910
    master (6.710M)   : 0, 6710388

    section Trace stats
    This PR (5040) (6.887M)   : 0, 6887438
    master (6.774M)   : 0, 6774287

    section Manual
    This PR (5040) (8.286M)   : 0, 8286462
    master (8.112M)   : 0, 8112282

    section Manual + Automatic
    This PR (5040) (6.219M)   : 0, 6219497
    master (6.232M)   : 0, 6232469

    section Version Conflict
    This PR (5040) (5.762M)   : 0, 5762469
    master (5.822M)   : 0, 5821633

Loading
gantt
    title Throughput Windows x64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (5040) (10.461M)   : 0, 10461009
    master (10.964M)   : 0, 10963844
    benchmarks/2.9.0 (9.528M)   : 0, 9527529

    section Automatic
    This PR (5040) (7.233M)   : crit ,0, 7233428
    master (7.814M)   : 0, 7813512
    benchmarks/2.9.0 (7.277M)   : 0, 7277162

    section Trace stats
    This PR (5040) (7.537M)   : crit ,0, 7537285
    master (8.109M)   : 0, 8109324

    section Manual
    This PR (5040) (8.923M)   : crit ,0, 8922966
    master (9.637M)   : 0, 9636722

    section Manual + Automatic
    This PR (5040) (6.926M)   : crit ,0, 6925577
    master (7.379M)   : 0, 7379241

    section Version Conflict
    This PR (5040) (6.379M)   : crit ,0, 6378961
    master (6.735M)   : 0, 6735312

Loading
gantt
    title Throughput Linux x64 (ASM) (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    master (7.431M)   : 0, 7430935
    benchmarks/2.9.0 (7.942M)   : 0, 7942095

    section No attack
    master (1.774M)   : 0, 1774264
    benchmarks/2.9.0 (3.272M)   : 0, 3271840

    section Attack
    master (1.420M)   : 0, 1419925
    benchmarks/2.9.0 (2.547M)   : 0, 2546976

    section Blocking
    master (3.138M)   : 0, 3138188

    section IAST default
    master (6.435M)   : 0, 6435429

    section IAST full
    master (5.725M)   : 0, 5724886

    section Base vuln
    master (0.965M)   : 0, 965154

    section IAST vuln
    master (0.886M)   : 0, 886079

Loading

@andrewlock andrewlock merged commit 7db6320 into master Jan 15, 2024
53 of 55 checks passed
@andrewlock andrewlock deleted the andrew/tracer_flare/fix_debug_mode branch January 15, 2024 09:38
@github-actions github-actions bot added this to the vNext milestone Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants