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

[IAST] Fix for VS Edit and Continue #6097

Merged
merged 2 commits into from
Oct 1, 2024
Merged

[IAST] Fix for VS Edit and Continue #6097

merged 2 commits into from
Oct 1, 2024

Conversation

daniel-romano-DD
Copy link
Contributor

@daniel-romano-DD daniel-romano-DD commented Sep 27, 2024 •
edited
Loading

Summary of changes

When VS enables Edit and Continue, it sets this env var COMPLUS_ForceEnc=true and some CallSite functions are not instrumented.

Reason for change

When COMPLUS_ForceEnc=true the profiler can skip the call to some Rejit events, leaving some functions (specially in callsite) uninstrumented

Implementation details

If this env var is detected, dataflow will set CallSite edited functionsIL in the JIT event as well as in the REJIT event, if this occurs.

Test coverage

Added integration tests

Other details

@daniel-romano-DD daniel-romano-DD added area:native-library Automatic instrumentation native C++ code (Datadog.Trace.ClrProfiler.Native) area:asm-iast labels Sep 27, 2024
@datadog-ddstaging
Copy link

datadog-ddstaging bot commented Sep 27, 2024 •
edited
Loading

Datadog Report

Branch report: dani/iast/support_edit_and_continue
Commit report: 082a93f
Test service: dd-trace-dotnet

✅ 0 Failed, 372476 Passed, 2721 Skipped, 26h 39m 49.2s Total Time

@daniel-romano-DD daniel-romano-DD force-pushed the dani/iast/support_edit_and_continue branch from 89b492c to 9da943d Compare September 28, 2024 08:06
@andrewlock
Copy link
Member

andrewlock commented Sep 28, 2024 •
edited
Loading

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 (6097) - mean (70ms)  : 68, 72
     .   : milestone, 70,
    master - mean (70ms)  : 68, 72
     .   : milestone, 70,

    section CallTarget+Inlining+NGEN
    This PR (6097) - mean (1,106ms)  : 1090, 1122
     .   : milestone, 1106,
    master - mean (1,108ms)  : 1086, 1130
     .   : milestone, 1108,
Loading 
gantt
    title Execution time (ms) FakeDbCommand (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6097) - mean (109ms)  : 106, 113
     .   : milestone, 109,
    master - mean (109ms)  : 105, 112
     .   : milestone, 109,

    section CallTarget+Inlining+NGEN
    This PR (6097) - mean (769ms)  : 753, 786
     .   : milestone, 769,
    master - mean (772ms)  : 753, 791
     .   : milestone, 772,
Loading 
gantt
    title Execution time (ms) FakeDbCommand (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6097) - mean (93ms)  : 89, 96
     .   : milestone, 93,
    master - mean (93ms)  : 90, 96
     .   : milestone, 93,

    section CallTarget+Inlining+NGEN
    This PR (6097) - mean (730ms)  : 709, 750
     .   : milestone, 730,
    master - mean (731ms)  : 709, 753
     .   : milestone, 731,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6097) - mean (191ms)  : 187, 195
     .   : milestone, 191,
    master - mean (190ms)  : 187, 193
     .   : milestone, 190,

    section CallTarget+Inlining+NGEN
    This PR (6097) - mean (1,194ms)  : 1174, 1215
     .   : milestone, 1194,
    master - mean (1,198ms)  : 1173, 1223
     .   : milestone, 1198,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6097) - mean (277ms)  : 273, 281
     .   : milestone, 277,
    master - mean (276ms)  : 273, 279
     .   : milestone, 276,

    section CallTarget+Inlining+NGEN
    This PR (6097) - mean (936ms)  : 918, 953
     .   : milestone, 936,
    master - mean (938ms)  : 923, 953
     .   : milestone, 938,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6097) - mean (265ms)  : 262, 268
     .   : milestone, 265,
    master - mean (266ms)  : 261, 271
     .   : milestone, 266,

    section CallTarget+Inlining+NGEN
    This PR (6097) - mean (922ms)  : 902, 942
     .   : milestone, 922,
    master - mean (927ms)  : 906, 949
     .   : milestone, 927,
Loading

@andrewlock
Copy link
Member

andrewlock commented Sep 28, 2024 •
edited
Loading

Benchmarks Report for tracer 🐌

Benchmarks for #6097 compared to master:

  • 1 benchmarks are faster, with geometric mean 1.149
  • 1 benchmarks are slower, with geometric mean 1.159
  • 1 benchmarks have more allocations

The following thresholds were used for comparing the benchmark speeds:

  • Mann–Whitney U test with statistical test for significance of 5%
  • Only results indicating a difference greater than 10% and 0.3 ns are considered.

Allocation changes below 0.5% are ignored.

Benchmark details

Benchmarks.Trace.ActivityBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartStopWithChild net6.0 7.96μs 44.2ns 280ns 0.0167 0.00835 0 5.43 KB
master StartStopWithChild netcoreapp3.1 9.72μs 53.5ns 325ns 0.0193 0.00964 0 5.62 KB
master StartStopWithChild net472 16.1μs 49.5ns 192ns 1.03 0.307 0.0971 6.06 KB
#6097 StartStopWithChild net6.0 7.72μs 42.4ns 247ns 0.0157 0.00392 0 5.43 KB
#6097 StartStopWithChild netcoreapp3.1 9.65μs 54.7ns 375ns 0.0185 0.00923 0 5.62 KB
#6097 StartStopWithChild net472 15.7μs 33.4ns 129ns 1.03 0.324 0.108 6.06 KB
Benchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master WriteAndFlushEnrichedTraces net6.0 466μs 162ns 629ns 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 645μs 663ns 2.57μs 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces net472 835μs 405ns 1.57μs 0.414 0 0 3.3 KB
#6097 WriteAndFlushEnrichedTraces net6.0 464μs 247ns 925ns 0 0 0 2.7 KB
#6097 WriteAndFlushEnrichedTraces netcoreapp3.1 632μs 322ns 1.21μs 0 0 0 2.7 KB
#6097 WriteAndFlushEnrichedTraces net472 830μs 391ns 1.46μs 0.414 0 0 3.3 KB
Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendRequest net6.0 181μs 851ns 3.3μs 0.185 0 0 18.45 KB
master SendRequest netcoreapp3.1 210μs 1.08μs 4.97μs 0.2 0 0 20.61 KB
master SendRequest net472 0.0024ns 0.000811ns 0.00314ns 0 0 0 0 b
#6097 SendRequest net6.0 192μs 1.08μs 7.06μs 0.196 0 0 18.45 KB
#6097 SendRequest netcoreapp3.1 214μs 1.06μs 4.86μs 0.213 0 0 20.61 KB
#6097 SendRequest net472 0.000679ns 0.000398ns 0.00154ns 0 0 0 0 b
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark - Same speed ✔️ More allocations ⚠️

More allocations ⚠️ in #6097

Benchmark Base Allocated Diff Allocated Change Change %
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark.WriteAndFlushEnrichedTraces‑net6.0 41.52 KB 41.92 KB 398 B 0.96%

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master WriteAndFlushEnrichedTraces net6.0 543μs 2.18μs 8.45μs 0.558 0 0 41.52 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 682μs 1.79μs 6.94μs 0.343 0 0 41.78 KB
master WriteAndFlushEnrichedTraces net472 850μs 2.74μs 10.6μs 8.56 2.57 0.428 53.28 KB
#6097 WriteAndFlushEnrichedTraces net6.0 571μs 2.54μs 11.9μs 0.561 0 0 41.92 KB
#6097 WriteAndFlushEnrichedTraces netcoreapp3.1 664μs 2.22μs 8.31μs 0.342 0 0 41.64 KB
#6097 WriteAndFlushEnrichedTraces net472 879μs 4.54μs 20.8μs 8.25 2.6 0.434 53.29 KB
Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master ExecuteNonQuery net6.0 1.31μs 3.61ns 13.5ns 0.0144 0 0 1.02 KB
master ExecuteNonQuery netcoreapp3.1 1.86μs 0.599ns 2.24ns 0.0132 0 0 1.02 KB
master ExecuteNonQuery net472 2.15μs 2.93ns 11.3ns 0.157 0 0 987 B
#6097 ExecuteNonQuery net6.0 1.33μs 1.02ns 3.94ns 0.0139 0 0 1.02 KB
#6097 ExecuteNonQuery netcoreapp3.1 1.72μs 1.08ns 4.17ns 0.0138 0 0 1.02 KB
#6097 ExecuteNonQuery net472 2.08μs 1.4ns 4.84ns 0.157 0 0 987 B
Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master CallElasticsearch net6.0 1.21μs 0.591ns 2.29ns 0.0139 0 0 976 B
master CallElasticsearch netcoreapp3.1 1.54μs 0.781ns 2.92ns 0.0132 0 0 976 B
master CallElasticsearch net472 2.44μs 1.93ns 7.46ns 0.158 0 0 995 B
master CallElasticsearchAsync net6.0 1.29μs 0.592ns 2.29ns 0.0136 0 0 952 B
master CallElasticsearchAsync netcoreapp3.1 1.67μs 1.05ns 3.94ns 0.0142 0 0 1.02 KB
master CallElasticsearchAsync net472 2.65μs 1.32ns 5.11ns 0.166 0 0 1.05 KB
#6097 CallElasticsearch net6.0 1.16μs 0.868ns 3.36ns 0.0134 0 0 976 B
#6097 CallElasticsearch netcoreapp3.1 1.5μs 1.16ns 4.19ns 0.0135 0 0 976 B
#6097 CallElasticsearch net472 2.56μs 1.9ns 7.37ns 0.157 0 0 995 B
#6097 CallElasticsearchAsync net6.0 1.31μs 1.62ns 6.28ns 0.0131 0 0 952 B
#6097 CallElasticsearchAsync netcoreapp3.1 1.73μs 0.935ns 3.62ns 0.0138 0 0 1.02 KB
#6097 CallElasticsearchAsync net472 2.65μs 1.93ns 7.49ns 0.166 0 0 1.05 KB
Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master ExecuteAsync net6.0 1.33μs 0.852ns 3.3ns 0.0136 0 0 952 B
master ExecuteAsync netcoreapp3.1 1.62μs 0.994ns 3.85ns 0.0129 0 0 952 B
master ExecuteAsync net472 1.77μs 2.96ns 11.1ns 0.145 0 0 915 B
#6097 ExecuteAsync net6.0 1.26μs 0.484ns 1.87ns 0.0132 0 0 952 B
#6097 ExecuteAsync netcoreapp3.1 1.63μs 0.681ns 2.55ns 0.0131 0 0 952 B
#6097 ExecuteAsync net472 1.78μs 0.58ns 2.25ns 0.145 0 0 915 B
Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendAsync net6.0 4.09μs 1.29ns 4.81ns 0.031 0 0 2.22 KB
master SendAsync netcoreapp3.1 5.14μs 1.53ns 5.53ns 0.036 0 0 2.76 KB
master SendAsync net472 7.83μs 5.43ns 21ns 0.498 0 0 3.15 KB
#6097 SendAsync net6.0 4.15μs 0.905ns 3.5ns 0.0315 0 0 2.22 KB
#6097 SendAsync netcoreapp3.1 5.05μs 1.83ns 6.6ns 0.038 0 0 2.76 KB
#6097 SendAsync net472 7.71μs 1.67ns 6.46ns 0.498 0 0 3.15 KB
Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net6.0 1.62μs 0.718ns 2.78ns 0.0227 0 0 1.64 KB
master EnrichedLog netcoreapp3.1 2.19μs 1.28ns 4.63ns 0.0216 0 0 1.64 KB
master EnrichedLog net472 2.6μs 1.13ns 4.21ns 0.249 0 0 1.57 KB
#6097 EnrichedLog net6.0 1.48μs 0.821ns 3.18ns 0.0229 0 0 1.64 KB
#6097 EnrichedLog netcoreapp3.1 2.1μs 1.21ns 4.69ns 0.0224 0 0 1.64 KB
#6097 EnrichedLog net472 2.68μs 1.13ns 4.23ns 0.249 0 0 1.57 KB
Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net6.0 113μs 108ns 419ns 0.0569 0 0 4.28 KB
master EnrichedLog netcoreapp3.1 118μs 125ns 483ns 0.0589 0 0 4.28 KB
master EnrichedLog net472 146μs 193ns 746ns 0.652 0.217 0 4.46 KB
#6097 EnrichedLog net6.0 114μs 150ns 582ns 0 0 0 4.28 KB
#6097 EnrichedLog netcoreapp3.1 119μs 237ns 916ns 0.0592 0 0 4.28 KB
#6097 EnrichedLog net472 146μs 88.9ns 332ns 0.655 0.218 0 4.46 KB
Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net6.0 3.19μs 0.977ns 3.78ns 0.0303 0 0 2.2 KB
master EnrichedLog netcoreapp3.1 4.1μs 7.73ns 30ns 0.0284 0 0 2.2 KB
master EnrichedLog net472 4.81μs 1.81ns 6.77ns 0.319 0 0 2.02 KB
#6097 EnrichedLog net6.0 3.16μs 0.987ns 3.82ns 0.0315 0 0 2.2 KB
#6097 EnrichedLog netcoreapp3.1 4.31μs 1.11ns 4.3ns 0.0281 0 0 2.2 KB
#6097 EnrichedLog net472 4.8μs 1.77ns 6.38ns 0.319 0 0 2.02 KB
Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendReceive net6.0 1.33μs 0.669ns 2.59ns 0.016 0 0 1.14 KB
master SendReceive netcoreapp3.1 1.81μs 0.943ns 3.65ns 0.0154 0 0 1.14 KB
master SendReceive net472 2.06μs 0.764ns 2.96ns 0.183 0 0 1.16 KB
#6097 SendReceive net6.0 1.28μs 1.51ns 5.65ns 0.0159 0 0 1.14 KB
#6097 SendReceive netcoreapp3.1 1.77μs 1.63ns 6.33ns 0.015 0 0 1.14 KB
#6097 SendReceive net472 2.04μs 0.988ns 3.82ns 0.183 0.00102 0 1.16 KB
Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net6.0 2.81μs 0.888ns 3.44ns 0.0225 0 0 1.6 KB
master EnrichedLog netcoreapp3.1 3.76μs 1.06ns 3.81ns 0.0225 0 0 1.65 KB
master EnrichedLog net472 4.53μs 2.51ns 9.72ns 0.323 0 0 2.04 KB
#6097 EnrichedLog net6.0 2.8μs 0.677ns 2.62ns 0.0225 0 0 1.6 KB
#6097 EnrichedLog netcoreapp3.1 3.93μs 1.26ns 4.89ns 0.0216 0 0 1.65 KB
#6097 EnrichedLog net472 4.38μs 2.96ns 11.5ns 0.323 0 0 2.04 KB
Benchmarks.Trace.SpanBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #6097

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑netcoreapp3.1 1.159 553.62 641.43

Faster 🎉 in #6097

Benchmark base/diff Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑net472 1.149 693.94 604.02

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartFinishSpan net6.0 394ns 0.224ns 0.867ns 0.00813 0 0 576 B
master StartFinishSpan netcoreapp3.1 554ns 0.468ns 1.81ns 0.00771 0 0 576 B
master StartFinishSpan net472 692ns 0.782ns 3.03ns 0.0916 0 0 578 B
master StartFinishScope net6.0 475ns 0.168ns 0.628ns 0.00979 0 0 696 B
master StartFinishScope netcoreapp3.1 722ns 0.53ns 2.05ns 0.00946 0 0 696 B
master StartFinishScope net472 888ns 1.24ns 4.46ns 0.104 0 0 658 B
#6097 StartFinishSpan net6.0 397ns 0.38ns 1.47ns 0.00817 0 0 576 B
#6097 StartFinishSpan netcoreapp3.1 641ns 0.415ns 1.61ns 0.00769 0 0 576 B
#6097 StartFinishSpan net472 605ns 0.765ns 2.96ns 0.0917 0 0 578 B
#6097 StartFinishScope net6.0 478ns 0.228ns 0.882ns 0.00989 0 0 696 B
#6097 StartFinishScope netcoreapp3.1 713ns 0.716ns 2.77ns 0.00965 0 0 696 B
#6097 StartFinishScope net472 870ns 1.25ns 4.84ns 0.104 0 0 658 B
Benchmarks.Trace.TraceAnnotationsBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master RunOnMethodBegin net6.0 657ns 1.05ns 3.91ns 0.00973 0 0 696 B
master RunOnMethodBegin netcoreapp3.1 924ns 0.738ns 2.86ns 0.00922 0 0 696 B
master RunOnMethodBegin net472 1.17μs 0.804ns 3.12ns 0.105 0 0 658 B
#6097 RunOnMethodBegin net6.0 673ns 0.492ns 1.9ns 0.00969 0 0 696 B
#6097 RunOnMethodBegin netcoreapp3.1 836ns 0.694ns 2.69ns 0.00948 0 0 696 B
#6097 RunOnMethodBegin net472 1.11μs 0.91ns 3.52ns 0.104 0 0 658 B

@andrewlock
Copy link
Member

andrewlock commented Sep 28, 2024 •
edited
Loading

Benchmarks Report for appsec 🐌

Benchmarks for #6097 compared to master:

  • 1 benchmarks are faster, with geometric mean 1.147
  • 1 benchmarks are slower, with geometric mean 1.278
  • 2 benchmarks have more allocations

The following thresholds were used for comparing the benchmark speeds:

  • Mann–Whitney U test with statistical test for significance of 5%
  • Only results indicating a difference greater than 10% and 0.3 ns are considered.

Allocation changes below 0.5% are ignored.

Benchmark details

Benchmarks.Trace.Asm.AppSecBodyBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #6097

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.Asm.AppSecBodyBenchmark.ObjectExtractorSimpleBody‑net472 1.278 162.77 208.01

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master AllCycleSimpleBody net6.0 72.4μs 108ns 417ns 0.0723 0 0 6 KB
master AllCycleSimpleBody netcoreapp3.1 62.2μs 76ns 294ns 0.0931 0 0 6.95 KB
master AllCycleSimpleBody net472 49.2μs 46ns 178ns 1.3 0 0 8.34 KB
master AllCycleMoreComplexBody net6.0 77μs 65.9ns 246ns 0.115 0 0 9.51 KB
master AllCycleMoreComplexBody netcoreapp3.1 69μs 146ns 526ns 0.138 0 0 10.37 KB
master AllCycleMoreComplexBody net472 55.5μs 40.4ns 156ns 1.88 0.0277 0 11.85 KB
master ObjectExtractorSimpleBody net6.0 138ns 0.123ns 0.475ns 0.00396 0 0 280 B
master ObjectExtractorSimpleBody netcoreapp3.1 213ns 0.181ns 0.701ns 0.00366 0 0 272 B
master ObjectExtractorSimpleBody net472 163ns 0.226ns 0.815ns 0.0446 0 0 281 B
master ObjectExtractorMoreComplexBody net6.0 3.13μs 3.73ns 12.9ns 0.0531 0 0 3.78 KB
master ObjectExtractorMoreComplexBody netcoreapp3.1 4.2μs 1.79ns 6.94ns 0.0504 0 0 3.69 KB
master ObjectExtractorMoreComplexBody net472 3.84μs 4.32ns 16.7ns 0.602 0.00577 0 3.8 KB
#6097 AllCycleSimpleBody net6.0 71.4μs 90.7ns 351ns 0.0716 0 0 6.01 KB
#6097 AllCycleSimpleBody netcoreapp3.1 62.2μs 112ns 433ns 0.0931 0 0 6.95 KB
#6097 AllCycleSimpleBody net472 49.3μs 55.5ns 215ns 1.32 0 0 8.34 KB
#6097 AllCycleMoreComplexBody net6.0 78.2μs 112ns 418ns 0.117 0 0 9.51 KB
#6097 AllCycleMoreComplexBody netcoreapp3.1 69.7μs 68.9ns 258ns 0.138 0 0 10.36 KB
#6097 AllCycleMoreComplexBody net472 55.6μs 91.9ns 344ns 1.86 0.0278 0 11.85 KB
#6097 ObjectExtractorSimpleBody net6.0 138ns 0.14ns 0.526ns 0.00396 0 0 280 B
#6097 ObjectExtractorSimpleBody netcoreapp3.1 215ns 0.135ns 0.523ns 0.00367 0 0 272 B
#6097 ObjectExtractorSimpleBody net472 208ns 0.247ns 0.955ns 0.0446 0 0 281 B
#6097 ObjectExtractorMoreComplexBody net6.0 3.05μs 1.98ns 7.65ns 0.0533 0 0 3.78 KB
#6097 ObjectExtractorMoreComplexBody netcoreapp3.1 4.13μs 7.07ns 26.5ns 0.0494 0 0 3.69 KB
#6097 ObjectExtractorMoreComplexBody net472 3.76μs 3.24ns 12.5ns 0.603 0.00567 0 3.8 KB
Benchmarks.Trace.Asm.AppSecEncoderBenchmark - Faster 🎉 Same allocations ✔️

Faster 🎉 in #6097

Benchmark base/diff Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.Asm.AppSecEncoderBenchmark.EncodeLegacyArgs‑net6.0 1.147 81,390.56 70,985.06

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EncodeArgs net6.0 36.5μs 21ns 78.7ns 0.459 0 0 32.4 KB
master EncodeArgs netcoreapp3.1 54μs 25.6ns 99.1ns 0.429 0 0 32.4 KB
master EncodeArgs net472 65.8μs 25.7ns 99.4ns 5.13 0.0658 0 32.5 KB
master EncodeLegacyArgs net6.0 81.3μs 70.8ns 274ns 0.0379 0 0 2.14 KB
master EncodeLegacyArgs netcoreapp3.1 106μs 107ns 414ns 0 0 0 2.15 KB
master EncodeLegacyArgs net472 149μs 81.2ns 315ns 0.299 0 0 2.15 KB
#6097 EncodeArgs net6.0 38.4μs 25.5ns 98.8ns 0.46 0 0 32.4 KB
#6097 EncodeArgs netcoreapp3.1 53.9μs 13.2ns 49.4ns 0.432 0 0 32.4 KB
#6097 EncodeArgs net472 66.7μs 17.8ns 68.9ns 5.15 0.0665 0 32.5 KB
#6097 EncodeLegacyArgs net6.0 71.1μs 53.5ns 207ns 0 0 0 2.14 KB
#6097 EncodeLegacyArgs netcoreapp3.1 106μs 373ns 1.44μs 0 0 0 2.15 KB
#6097 EncodeLegacyArgs net472 151μs 331ns 1.28μs 0.305 0 0 2.15 KB
Benchmarks.Trace.Asm.AppSecWafBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master RunWafRealisticBenchmark net6.0 182μs 65.8ns 255ns 0 0 0 2.44 KB
master RunWafRealisticBenchmark netcoreapp3.1 196μs 246ns 952ns 0 0 0 2.39 KB
master RunWafRealisticBenchmark net472 209μs 72.4ns 280ns 0.314 0 0 2.46 KB
master RunWafRealisticBenchmarkWithAttack net6.0 123μs 83.8ns 325ns 0 0 0 1.47 KB
master RunWafRealisticBenchmarkWithAttack netcoreapp3.1 130μs 110ns 425ns 0 0 0 1.46 KB
master RunWafRealisticBenchmarkWithAttack net472 139μs 40.4ns 157ns 0.209 0 0 1.49 KB
#6097 RunWafRealisticBenchmark net6.0 185μs 122ns 439ns 0 0 0 2.44 KB
#6097 RunWafRealisticBenchmark netcoreapp3.1 198μs 93.4ns 350ns 0 0 0 2.39 KB
#6097 RunWafRealisticBenchmark net472 208μs 77.8ns 291ns 0.312 0 0 2.46 KB
#6097 RunWafRealisticBenchmarkWithAttack net6.0 122μs 94.8ns 342ns 0 0 0 1.47 KB
#6097 RunWafRealisticBenchmarkWithAttack netcoreapp3.1 130μs 65.4ns 236ns 0 0 0 1.46 KB
#6097 RunWafRealisticBenchmarkWithAttack net472 140μs 21.2ns 79.2ns 0.21 0 0 1.48 KB
Benchmarks.Trace.Iast.StringAspectsBenchmark - Same speed ✔️ More allocations ⚠️

More allocations ⚠️ in #6097

Benchmark Base Allocated Diff Allocated Change Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑net472 59.07 KB 59.56 KB 488 B 0.83%
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑netcoreapp3.1 254.06 KB 255.62 KB 1.55 KB 0.61%

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StringConcatBenchmark net6.0 60.1μs 754ns 7.54μs 0 0 0 43.44 KB
master StringConcatBenchmark netcoreapp3.1 62μs 869ns 8.56μs 0 0 0 42.64 KB
master StringConcatBenchmark net472 37.1μs 88ns 329ns 0 0 0 59.07 KB
master StringConcatAspectBenchmark net6.0 305μs 1.67μs 9.88μs 0 0 0 254.18 KB
master StringConcatAspectBenchmark netcoreapp3.1 339μs 1.88μs 11.7μs 0 0 0 254.06 KB
master StringConcatAspectBenchmark net472 270μs 5.67μs 55.5μs 0 0 0 278.53 KB
#6097 StringConcatBenchmark net6.0 58.6μs 709ns 7.05μs 0 0 0 43.44 KB
#6097 StringConcatBenchmark netcoreapp3.1 56.1μs 445ns 4.36μs 0 0 0 42.64 KB
#6097 StringConcatBenchmark net472 37.5μs 112ns 405ns 0 0 0 59.56 KB
#6097 StringConcatAspectBenchmark net6.0 307μs 1.72μs 10.7μs 0 0 0 255.28 KB
#6097 StringConcatAspectBenchmark netcoreapp3.1 341μs 1.79μs 9.99μs 0 0 0 255.62 KB
#6097 StringConcatAspectBenchmark net472 269μs 5.1μs 49.4μs 0 0 0 278.53 KB

@andrewlock
Copy link
Member

andrewlock commented Sep 28, 2024 •
edited
Loading

Throughput/Crank Report ⚡

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 (6097) (10.922M)   : 0, 10922314
    master (11.076M)   : 0, 11076208
    benchmarks/2.9.0 (11.081M)   : 0, 11080577

    section Automatic
    This PR (6097) (7.388M)   : 0, 7388327
    master (7.280M)   : 0, 7280356
    benchmarks/2.9.0 (7.732M)   : 0, 7732233

    section Trace stats
    master (7.600M)   : 0, 7599611

    section Manual
    master (11.113M)   : 0, 11112549

    section Manual + Automatic
    This PR (6097) (6.794M)   : 0, 6793963
    master (6.824M)   : 0, 6823624

    section DD_TRACE_ENABLED=0
    master (10.191M)   : 0, 10191346
Loading 
gantt
    title Throughput Linux arm64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (6097) (9.271M)   : 0, 9271045
    master (9.452M)   : 0, 9451595
    benchmarks/2.9.0 (9.798M)   : 0, 9798067

    section Automatic
    This PR (6097) (6.672M)   : 0, 6671570
    master (6.606M)   : 0, 6605774

    section Trace stats
    master (6.844M)   : 0, 6844108

    section Manual
    master (9.542M)   : 0, 9541995

    section Manual + Automatic
    This PR (6097) (6.195M)   : 0, 6195035
    master (6.124M)   : 0, 6123597

    section DD_TRACE_ENABLED=0
    master (8.853M)   : 0, 8853041
Loading 
gantt
    title Throughput Windows x64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (6097) (10.116M)   : 0, 10116088
    master (10.113M)   : 0, 10112732
    benchmarks/2.9.0 (10.067M)   : 0, 10067315

    section Automatic
    This PR (6097) (6.594M)   : 0, 6594447
    master (6.785M)   : 0, 6784538
    benchmarks/2.9.0 (7.552M)   : 0, 7552193

    section Trace stats
    master (7.389M)   : 0, 7388503

    section Manual
    master (10.055M)   : 0, 10055078

    section Manual + Automatic
    This PR (6097) (6.074M)   : 0, 6074436
    master (6.279M)   : 0, 6278671

    section DD_TRACE_ENABLED=0
    master (9.407M)   : 0, 9406897
Loading

@daniel-romano-DD daniel-romano-DD marked this pull request as ready for review September 29, 2024 16:24
andrewlock
andrewlock reviewed Sep 30, 2024
View reviewed changes
tracer/test/Datadog.Trace.Security.IntegrationTests/AspNetBase.cs
@@ -409,7 +409,7 @@ protected IImmutableList<MockSpan> WaitForSpans(MockTracerAgent agent, int expec
agent.SpanFilters.Add(s => s.Tags.ContainsKey("http.url") && s.Tags["http.url"].IndexOf(url, StringComparison.InvariantCultureIgnoreCase) > -1);
}

var spans = agent.WaitForSpans(expectedSpans, minDateTime: minDateTime);
var spans = agent.WaitForSpans(expectedSpans, minDateTime: minDateTime, assertExpectedCount: false);
Copy link
Member

Choose a reason for hiding this comment

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

We should probably not disable this by default 🤔 It was a big effort to enable it as we had a large number of cases where we were failing and we didn't know it 😬

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can reenable it, but the reason for disabling it was that when not all the spans are received, we do not know which ones are missing, because the snapshot difference is skipped, and it is an inconvenience.

Copy link
Member

Choose a reason for hiding this comment

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

I seem to remember we came up with a way assert, keep a log of it, but not fail the test? Is it possible to do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't get your point here. If there are fewer spans verify will fail and show it in the snapshot difference

Copy link
Member

Choose a reason for hiding this comment

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

I don't get your point here. If there are fewer spans verify will fail and show it in the snapshot difference

Unfortunately that assumes

  1. Every test is using Verify
  2. The test does not filter the spans, before sending them to Verify

As for how to do both, you can create an assertion scope, but it's probably not the right option here. Unfortunately, there's not really a great option right now.

kevingosse
kevingosse approved these changes Sep 30, 2024
View reviewed changes
Copy link
Collaborator

@kevingosse kevingosse left a comment

Choose a reason for hiding this comment

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

Should we really support this? I feel like this is a significant change in behavior for a niche scenario that is going to be insufficiently tested.

tracer/src/Datadog.Tracer.Native/environment_variables.h Outdated
@@ -127,6 +127,9 @@ namespace environment
// Enables the workaround for dotnet issue 77973 (https://github.com/dotnet/runtime/issues/77973)
const shared::WSTRING internal_workaround_77973_enabled = WStr("DD_INTERNAL_WORKAROUND_77973_ENABLED");

// IDE Edit and Continue. If enabled, profiler behavior is modified slightly
const shared::WSTRING ide_edit_and_continue = WStr("COMPLUS_ForceEnc");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suppose you would need to look for DOTNET_ForceEnc as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 082a93f

@andrewlock
Copy link
Member

Should we really support this? I feel like this is a significant change in behavior for a niche scenario that is going to be insufficiently tested.

FWIW, this was my gut feeling too. I'm assuming we're only doing this because it makes local testing a pain. The answer in that case would seem to be just disabling edit and continue locally?

@daniel-romano-DD
Copy link
Contributor Author

Should we really support this? I feel like this is a significant change in behavior for a niche scenario that is going to be insufficiently tested.

FWIW, this was my gut feeling too. I'm assuming we're only doing this because it makes local testing a pain. The answer in that case would seem to be just disabling edit and continue locally?

One of the targets of IAST are developers, who are the ones in charge of fixing the discovered vulnerabilities. And having edit and continue enabled completely breaks callsite instrumentation, thus, effectively breaking IAST. That's the motivation of this PR

@daniel-romano-DD daniel-romano-DD changed the title [IAST] Support for VS Edit and Continue [IAST] Fix for VS Edit and Continue Sep 30, 2024
e-n-0
e-n-0 approved these changes Oct 1, 2024
View reviewed changes
Copy link
Member

@e-n-0 e-n-0 left a comment

Choose a reason for hiding this comment

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

Did not really looked at the cpp except the ASM part, and that's looking for for me :)

@daniel-romano-DD daniel-romano-DD merged commit 7893475 into master Oct 1, 2024
80 checks passed
@daniel-romano-DD daniel-romano-DD deleted the dani/iast/support_edit_and_continue branch October 1, 2024 13:33
@github-actions github-actions bot added this to the vNext-v3 milestone Oct 1, 2024
agocs pushed a commit that referenced this pull request Oct 8, 2024
@daniel-romano-DD @agocs
[IAST] Fix for VS Edit and Continue (#6097)
7376508 
## Summary of changes
When VS enables Edit and Continue, it sets this env var
`COMPLUS_ForceEnc=true` and some CallSite functions are not
instrumented.

## Reason for change
When `COMPLUS_ForceEnc=true` the profiler can skip the call to some
Rejit events, leaving some functions (specially in callsite)
uninstrumented

## Implementation details
If this env var is detected, dataflow will set CallSite edited
functionsIL in the JIT event as well as in the REJIT event, if this
occurs.

## Test coverage
Added integration tests

## Other details
<!-- Fixes #{issue} -->

<!-- ⚠️ Note: where possible, please obtain 2 approvals prior to
merging. Unless CODEOWNERS specifies otherwise, for external teams it is
typically best to have one review from a team member, and one review
from apm-dotnet. Trivial changes do not require 2 reviews. -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewlock andrewlock andrewlock left review comments

@robertpi robertpi robertpi left review comments

@e-n-0 e-n-0 e-n-0 approved these changes

@kevingosse kevingosse kevingosse approved these changes

Assignees
No one assigned
Labels
area:asm-iast area:native-library Automatic instrumentation native C++ code (Datadog.Trace.ClrProfiler.Native)
Projects
None yet
Milestone
3.4.0
Development

Successfully merging this pull request may close these issues.
5 participants
@daniel-romano-DD @andrewlock @robertpi @kevingosse @e-n-0