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

[IAST] Native CallSites Definitions #6241

Merged
merged 19 commits into from
Nov 21, 2024
Merged

Conversation

daniel-romano-DD
Copy link
Contributor

@daniel-romano-DD daniel-romano-DD commented Nov 5, 2024

Summary of changes

Moved CallSite definitions to the Native library so we can spare the marshalling.
As from V3, there will not be a situation where a Managed V3 will have to specify its definitions to a newer Native library, we can remove the definitions from the managed part.
We must still keep the ability to receive definitions from a V2 managed library.

Reason for change

Marshalling of definitions from managed to native is taxing. Also, embedding the definitions in each managed library takes space, precious in Serverless scenarions

Implementation details

Moved the generation of the CallSites definitions to a nuke step, removing the SourceGenerator.
Added InstrumentationCategory to CallSites, to match what CallTargets have

Test coverage

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 Nov 5, 2024
@andrewlock
Copy link
Member

andrewlock commented Nov 6, 2024

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 (6241) - mean (72ms)  : 63, 80
     .   : milestone, 72,
    master - mean (72ms)  : 64, 80
     .   : milestone, 72,

    section CallTarget+Inlining+NGEN
    This PR (6241) - mean (1,111ms)  : 1091, 1131
     .   : milestone, 1111,
    master - mean (1,107ms)  : 1085, 1128
     .   : milestone, 1107,

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

    section CallTarget+Inlining+NGEN
    This PR (6241) - mean (766ms)  : 752, 780
     .   : milestone, 766,
    master - mean (771ms)  : 757, 785
     .   : milestone, 771,

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

    section CallTarget+Inlining+NGEN
    This PR (6241) - mean (720ms)  : 709, 731
     .   : milestone, 720,
    master - mean (726ms)  : 707, 745
     .   : milestone, 726,

Loading
gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6241) - mean (191ms)  : 185, 196
     .   : milestone, 191,
    master - mean (191ms)  : 186, 195
     .   : milestone, 191,

    section CallTarget+Inlining+NGEN
    This PR (6241) - mean (1,209ms)  : 1182, 1236
     .   : milestone, 1209,
    master - mean (1,213ms)  : 1190, 1236
     .   : milestone, 1213,

Loading
gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6241) - mean (276ms)  : 271, 280
     .   : milestone, 276,
    master - mean (276ms)  : 271, 281
     .   : milestone, 276,

    section CallTarget+Inlining+NGEN
    This PR (6241) - mean (942ms)  : 927, 957
     .   : milestone, 942,
    master - mean (948ms)  : 929, 967
     .   : milestone, 948,

Loading
gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6241) - mean (265ms)  : 261, 269
     .   : milestone, 265,
    master - mean (265ms)  : 261, 270
     .   : milestone, 265,

    section CallTarget+Inlining+NGEN
    This PR (6241) - mean (924ms)  : 907, 941
     .   : milestone, 924,
    master - mean (931ms)  : 913, 950
     .   : milestone, 931,

Loading

@andrewlock
Copy link
Member

andrewlock commented Nov 6, 2024

Benchmarks Report for tracer 🐌

Benchmarks for #6241 compared to master:

  • 3 benchmarks are faster, with geometric mean 1.323
  • 3 benchmarks are slower, with geometric mean 1.147
  • 2 benchmarks have fewer 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 45ns 318ns 0.0118 0.00393 0 5.61 KB
master StartStopWithChild netcoreapp3.1 10.1μs 55.7ns 315ns 0.0195 0.00973 0 5.8 KB
master StartStopWithChild net472 16.1μs 50ns 187ns 1.04 0.294 0.103 6.21 KB
#6241 StartStopWithChild net6.0 8.31μs 45.1ns 243ns 0.0199 0.00796 0 5.62 KB
#6241 StartStopWithChild netcoreapp3.1 9.8μs 50.7ns 254ns 0.0194 0.00969 0 5.8 KB
#6241 StartStopWithChild net472 16.2μs 53.2ns 206ns 1.05 0.321 0.11 6.2 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 479μs 354ns 1.37μs 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 672μs 664ns 2.48μs 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces net472 863μs 545ns 1.97μs 0.431 0 0 3.3 KB
#6241 WriteAndFlushEnrichedTraces net6.0 475μs 140ns 506ns 0 0 0 2.7 KB
#6241 WriteAndFlushEnrichedTraces netcoreapp3.1 637μs 367ns 1.37μs 0 0 0 2.7 KB
#6241 WriteAndFlushEnrichedTraces net472 868μs 753ns 2.82μs 0.431 0 0 3.3 KB
Benchmarks.Trace.AspNetCoreBenchmark - Faster 🎉 Fewer allocations 🎉

Faster 🎉 in #6241

Benchmark base/diff Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.AspNetCoreBenchmark.SendRequest‑net6.0 1.421 214,393.06 150,855.56 bimodal
Benchmarks.Trace.AspNetCoreBenchmark.SendRequest‑netcoreapp3.1 1.407 234,382.55 166,635.19

Fewer allocations 🎉 in #6241

Benchmark Base Allocated Diff Allocated Change Change %
Benchmarks.Trace.AspNetCoreBenchmark.SendRequest‑netcoreapp3.1 20.89 KB 17.27 KB -3.62 KB -17.31%
Benchmarks.Trace.AspNetCoreBenchmark.SendRequest‑net6.0 18.73 KB 14.47 KB -4.26 KB -22.73%

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendRequest net6.0 216μs 1.38μs 13.6μs 0.197 0 0 18.73 KB
master SendRequest netcoreapp3.1 237μs 1.69μs 16.8μs 0.227 0 0 20.89 KB
master SendRequest net472 0.00128ns 0.000428ns 0.00154ns 0 0 0 0 b
#6241 SendRequest net6.0 151μs 846ns 6.66μs 0.151 0 0 14.47 KB
#6241 SendRequest netcoreapp3.1 166μs 602ns 2.09μs 0.161 0 0 17.27 KB
#6241 SendRequest net472 0.00223ns 0.000828ns 0.00321ns 0 0 0 0 b
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master WriteAndFlushEnrichedTraces net6.0 595μs 3.13μs 18.5μs 0.587 0 0 41.76 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 679μs 3.08μs 11.5μs 0.349 0 0 41.66 KB
master WriteAndFlushEnrichedTraces net472 858μs 3.57μs 13.8μs 8.33 2.5 0.417 53.34 KB
#6241 WriteAndFlushEnrichedTraces net6.0 573μs 2.55μs 9.9μs 0.561 0 0 41.65 KB
#6241 WriteAndFlushEnrichedTraces netcoreapp3.1 689μs 3.13μs 11.7μs 0.336 0 0 41.66 KB
#6241 WriteAndFlushEnrichedTraces net472 882μs 4.21μs 17.4μs 8.08 2.55 0.425 53.34 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.37μs 1.08ns 4.05ns 0.0141 0 0 1.02 KB
master ExecuteNonQuery netcoreapp3.1 1.7μs 1.17ns 4.55ns 0.0135 0 0 1.02 KB
master ExecuteNonQuery net472 2.06μs 1.65ns 6.39ns 0.156 0.00103 0 987 B
#6241 ExecuteNonQuery net6.0 1.37μs 0.99ns 3.83ns 0.0138 0 0 1.02 KB
#6241 ExecuteNonQuery netcoreapp3.1 1.77μs 2.62ns 10.1ns 0.014 0 0 1.02 KB
#6241 ExecuteNonQuery net472 2.03μs 1.11ns 4.17ns 0.156 0.00102 0 987 B
Benchmarks.Trace.ElasticsearchBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #6241

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.ElasticsearchBenchmark.CallElasticsearchAsync‑net6.0 1.117 1,274.77 1,424.05

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master CallElasticsearch net6.0 1.13μs 0.736ns 2.75ns 0.0136 0 0 976 B
master CallElasticsearch netcoreapp3.1 1.59μs 0.774ns 3ns 0.0128 0 0 976 B
master CallElasticsearch net472 2.51μs 2.8ns 10.8ns 0.158 0 0 995 B
master CallElasticsearchAsync net6.0 1.27μs 0.629ns 2.35ns 0.0134 0 0 952 B
master CallElasticsearchAsync netcoreapp3.1 1.64μs 0.86ns 3.33ns 0.0139 0 0 1.02 KB
master CallElasticsearchAsync net472 2.6μs 2.22ns 8.61ns 0.167 0 0 1.05 KB
#6241 CallElasticsearch net6.0 1.1μs 0.427ns 1.6ns 0.0138 0 0 976 B
#6241 CallElasticsearch netcoreapp3.1 1.64μs 0.549ns 2.05ns 0.0132 0 0 976 B
#6241 CallElasticsearch net472 2.71μs 1.04ns 3.75ns 0.157 0 0 995 B
#6241 CallElasticsearchAsync net6.0 1.42μs 0.954ns 3.7ns 0.0128 0 0 952 B
#6241 CallElasticsearchAsync netcoreapp3.1 1.67μs 0.686ns 2.66ns 0.014 0 0 1.02 KB
#6241 CallElasticsearchAsync net472 2.66μs 2.64ns 10.2ns 0.167 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.23μs 0.961ns 3.72ns 0.0134 0 0 952 B
master ExecuteAsync netcoreapp3.1 1.66μs 1.06ns 4.11ns 0.0133 0 0 952 B
master ExecuteAsync net472 1.82μs 1.88ns 7.27ns 0.145 0 0 915 B
#6241 ExecuteAsync net6.0 1.21μs 0.884ns 3.31ns 0.0134 0 0 952 B
#6241 ExecuteAsync netcoreapp3.1 1.69μs 0.415ns 1.5ns 0.0127 0 0 952 B
#6241 ExecuteAsync net472 1.79μs 0.683ns 2.55ns 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.47μs 2.75ns 10.3ns 0.0317 0 0 2.31 KB
master SendAsync netcoreapp3.1 5.32μs 2.88ns 11.1ns 0.0373 0 0 2.85 KB
master SendAsync net472 7.25μs 3.7ns 14.3ns 0.495 0 0 3.12 KB
#6241 SendAsync net6.0 4.44μs 5.24ns 20.3ns 0.0334 0 0 2.31 KB
#6241 SendAsync netcoreapp3.1 5.27μs 3.11ns 11.6ns 0.0371 0 0 2.85 KB
#6241 SendAsync net472 7.39μs 1.57ns 5.86ns 0.494 0 0 3.12 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.57μs 0.661ns 2.47ns 0.0227 0 0 1.64 KB
master EnrichedLog netcoreapp3.1 2.17μs 1.4ns 5.04ns 0.0218 0 0 1.64 KB
master EnrichedLog net472 2.62μs 1.51ns 5.86ns 0.249 0 0 1.57 KB
#6241 EnrichedLog net6.0 1.47μs 0.731ns 2.73ns 0.0229 0 0 1.64 KB
#6241 EnrichedLog netcoreapp3.1 2.23μs 1.41ns 5.48ns 0.0216 0 0 1.64 KB
#6241 EnrichedLog net472 2.57μs 1.23ns 4.76ns 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 119μs 266ns 1.03μs 0 0 0 4.28 KB
master EnrichedLog netcoreapp3.1 124μs 281ns 1.09μs 0 0 0 4.28 KB
master EnrichedLog net472 151μs 100ns 387ns 0.676 0.225 0 4.46 KB
#6241 EnrichedLog net6.0 121μs 196ns 758ns 0.0606 0 0 4.28 KB
#6241 EnrichedLog netcoreapp3.1 124μs 137ns 531ns 0.0621 0 0 4.28 KB
#6241 EnrichedLog net472 153μs 111ns 432ns 0.682 0.227 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.17μs 1.81ns 6.99ns 0.0297 0 0 2.2 KB
master EnrichedLog netcoreapp3.1 4.35μs 1.45ns 5.62ns 0.0283 0 0 2.2 KB
master EnrichedLog net472 4.83μs 1.52ns 5.9ns 0.319 0 0 2.02 KB
#6241 EnrichedLog net6.0 3.1μs 4.66ns 18ns 0.03 0 0 2.2 KB
#6241 EnrichedLog netcoreapp3.1 4.2μs 2.37ns 8.55ns 0.0294 0 0 2.2 KB
#6241 EnrichedLog net472 4.96μs 1.21ns 4.69ns 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.31μs 0.65ns 2.43ns 0.0157 0 0 1.14 KB
master SendReceive netcoreapp3.1 1.79μs 0.661ns 2.47ns 0.0151 0 0 1.14 KB
master SendReceive net472 2.04μs 1.01ns 3.79ns 0.183 0 0 1.16 KB
#6241 SendReceive net6.0 1.26μs 0.655ns 2.54ns 0.0157 0 0 1.14 KB
#6241 SendReceive netcoreapp3.1 1.75μs 0.553ns 2.14ns 0.0158 0 0 1.14 KB
#6241 SendReceive net472 2.15μs 1.29ns 4.83ns 0.183 0 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.76μs 1.2ns 4.64ns 0.022 0 0 1.6 KB
master EnrichedLog netcoreapp3.1 3.99μs 3.27ns 12.7ns 0.0218 0 0 1.65 KB
master EnrichedLog net472 4.37μs 2.59ns 10ns 0.322 0 0 2.04 KB
#6241 EnrichedLog net6.0 2.72μs 1.08ns 4.06ns 0.0217 0 0 1.6 KB
#6241 EnrichedLog netcoreapp3.1 3.84μs 0.911ns 3.41ns 0.0213 0 0 1.65 KB
#6241 EnrichedLog net472 4.42μs 3.14ns 12.2ns 0.323 0 0 2.04 KB
Benchmarks.Trace.SpanBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #6241

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑net6.0 1.183 405.10 479.16

Faster 🎉 in #6241

Benchmark base/diff Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.SpanBenchmark.StartFinishScope‑net472 1.158 913.82 788.89

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartFinishSpan net6.0 405ns 0.665ns 2.57ns 0.00806 0 0 576 B
master StartFinishSpan netcoreapp3.1 566ns 0.73ns 2.83ns 0.00792 0 0 576 B
master StartFinishSpan net472 672ns 0.909ns 3.52ns 0.0916 0 0 578 B
master StartFinishScope net6.0 493ns 0.611ns 2.37ns 0.00982 0 0 696 B
master StartFinishScope netcoreapp3.1 654ns 1.08ns 4.2ns 0.00954 0 0 696 B
master StartFinishScope net472 913ns 0.51ns 1.98ns 0.104 0 0 658 B
#6241 StartFinishSpan net6.0 479ns 0.415ns 1.61ns 0.0081 0 0 576 B
#6241 StartFinishSpan netcoreapp3.1 593ns 1.32ns 5.1ns 0.0078 0 0 576 B
#6241 StartFinishSpan net472 631ns 1.33ns 4.99ns 0.0917 0 0 578 B
#6241 StartFinishScope net6.0 512ns 0.771ns 2.99ns 0.00983 0 0 696 B
#6241 StartFinishScope netcoreapp3.1 653ns 1.33ns 5.16ns 0.00926 0 0 696 B
#6241 StartFinishScope net472 784ns 3.22ns 12.5ns 0.104 0 0 658 B
Benchmarks.Trace.TraceAnnotationsBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #6241

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.TraceAnnotationsBenchmark.RunOnMethodBegin‑net6.0 1.142 591.00 675.03

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master RunOnMethodBegin net6.0 591ns 0.63ns 2.44ns 0.0096 0 0 696 B
master RunOnMethodBegin netcoreapp3.1 947ns 1.12ns 4.34ns 0.00945 0 0 696 B
master RunOnMethodBegin net472 1.16μs 2.46ns 9.54ns 0.104 0 0 658 B
#6241 RunOnMethodBegin net6.0 674ns 0.749ns 2.9ns 0.00977 0 0 696 B
#6241 RunOnMethodBegin netcoreapp3.1 986ns 1.62ns 6.06ns 0.00942 0 0 696 B
#6241 RunOnMethodBegin net472 1.13μs 3.82ns 14.8ns 0.104 0 0 658 B

@datadog-ddstaging
Copy link

datadog-ddstaging bot commented Nov 6, 2024

Datadog Report

Branch report: dani/apm/NativeCallSites
Commit report: 23077b9
Test service: dd-trace-dotnet

❌ 6 Failed (0 Known Flaky), 566835 Passed, 4587 Skipped, 48h 7m 42.12s Total Time
❄️ 59 New Flaky

❌ Failed Tests (6)

This report shows up to 5 failed tests.

  • AllCycleMoreComplexBody - Benchmarks.Trace.Asm.AppSecBodyBenchmark - Details

  • AllCycleMoreComplexBody - Benchmarks.Trace.Asm.AppSecBodyBenchmark - Details

  • AllCycleMoreComplexBody - Benchmarks.Trace.Asm.AppSecBodyBenchmark - Details

  • AllCycleSimpleBody - Benchmarks.Trace.Asm.AppSecBodyBenchmark - Details

  • AllCycleSimpleBody - Benchmarks.Trace.Asm.AppSecBodyBenchmark - Details

New Flaky Tests (59)

  • IntegrationDisabled - Datadog.Trace.ClrProfiler.IntegrationTests.AdoNet.MicrosoftDataSqlClientTests - Last Failure

    Expand for error
     Expected exit code: 0, actual exit code: 134.
    
  • SubmitsTraces - Datadog.Trace.ClrProfiler.IntegrationTests.AdoNet.MicrosoftDataSqlClientTests - Last Failure

    Expand for error
     Expected exit code: 0, actual exit code: 134.
    
  • SubmitsTraces - Datadog.Trace.ClrProfiler.IntegrationTests.AdoNet.MicrosoftDataSqlClientTests - Last Failure

    Expand for error
     Expected exit code: 0, actual exit code: 134.
    
  • SubmitsTraces - Datadog.Trace.ClrProfiler.IntegrationTests.AdoNet.MicrosoftDataSqlClientTests - Last Failure

    Expand for error
     Expected exit code: 0, actual exit code: 134.
    
  • SubmitsTraces - Datadog.Trace.ClrProfiler.IntegrationTests.AdoNet.MicrosoftDataSqlClientTests - Last Failure

    Expand for error
     Expected exit code: 0, actual exit code: 134.
    

@daniel-romano-DD daniel-romano-DD changed the title [IAST] Initial implementation [IAST] NAtive CallSites Definitions Nov 6, 2024
@daniel-romano-DD daniel-romano-DD changed the title [IAST] NAtive CallSites Definitions [IAST] Native CallSites Definitions Nov 6, 2024
@andrewlock
Copy link
Member

andrewlock commented Nov 6, 2024

Benchmarks Report for appsec 🐌

Benchmarks for #6241 compared to master:

  • All benchmarks have the same speed
  • 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 - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master AllCycleSimpleBody net6.0 72.3μs 67.8ns 263ns 0.0716 0 0 6 KB
master AllCycleSimpleBody netcoreapp3.1 62.5μs 58.7ns 220ns 0.0938 0 0 6.95 KB
master AllCycleSimpleBody net472 49.1μs 72ns 279ns 1.31 0 0 8.33 KB
master AllCycleMoreComplexBody net6.0 78.4μs 55ns 213ns 0.117 0 0 9.51 KB
master AllCycleMoreComplexBody netcoreapp3.1 69.4μs 56.4ns 211ns 0.139 0 0 10.36 KB
master AllCycleMoreComplexBody net472 56.5μs 85.2ns 330ns 1.87 0.0283 0 11.85 KB
master ObjectExtractorSimpleBody net6.0 145ns 0.12ns 0.448ns 0.00395 0 0 280 B
master ObjectExtractorSimpleBody netcoreapp3.1 209ns 0.137ns 0.494ns 0.00369 0 0 272 B
master ObjectExtractorSimpleBody net472 171ns 0.0861ns 0.334ns 0.0446 0 0 281 B
master ObjectExtractorMoreComplexBody net6.0 3.04μs 2.64ns 9.86ns 0.0533 0 0 3.78 KB
master ObjectExtractorMoreComplexBody netcoreapp3.1 3.91μs 3.08ns 11.9ns 0.0507 0 0 3.69 KB
master ObjectExtractorMoreComplexBody net472 4.36μs 1.92ns 7.44ns 0.601 0.00656 0 3.8 KB
#6241 AllCycleSimpleBody net6.0 72.4μs 45.9ns 172ns 0.072 0 0 6.01 KB
#6241 AllCycleSimpleBody netcoreapp3.1 62.9μs 111ns 430ns 0.0939 0 0 6.95 KB
#6241 AllCycleSimpleBody net472 48.8μs 61ns 236ns 1.31 0 0 8.34 KB
#6241 AllCycleMoreComplexBody net6.0 79.5μs 79.7ns 309ns 0.12 0 0 9.51 KB
#6241 AllCycleMoreComplexBody netcoreapp3.1 70μs 73.2ns 274ns 0.104 0 0 10.36 KB
#6241 AllCycleMoreComplexBody net472 55.8μs 60.1ns 217ns 1.86 0.0278 0 11.85 KB
#6241 ObjectExtractorSimpleBody net6.0 146ns 0.287ns 1.07ns 0.00397 0 0 280 B
#6241 ObjectExtractorSimpleBody netcoreapp3.1 201ns 0.21ns 0.788ns 0.00375 0 0 272 B
#6241 ObjectExtractorSimpleBody net472 165ns 0.107ns 0.399ns 0.0446 0 0 281 B
#6241 ObjectExtractorMoreComplexBody net6.0 3.16μs 2.61ns 9.77ns 0.0533 0 0 3.78 KB
#6241 ObjectExtractorMoreComplexBody netcoreapp3.1 3.9μs 2.38ns 9.2ns 0.0506 0 0 3.69 KB
#6241 ObjectExtractorMoreComplexBody net472 4.64μs 2.86ns 10.7ns 0.602 0.00465 0 3.8 KB
Benchmarks.Trace.Asm.AppSecEncoderBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EncodeArgs net6.0 38.2μs 26.2ns 102ns 0.454 0 0 32.4 KB
master EncodeArgs netcoreapp3.1 54.3μs 36.7ns 142ns 0.432 0 0 32.4 KB
master EncodeArgs net472 66.4μs 27.9ns 104ns 5.15 0.0661 0 32.5 KB
master EncodeLegacyArgs net6.0 76.2μs 407ns 2.11μs 0 0 0 2.14 KB
master EncodeLegacyArgs netcoreapp3.1 109μs 254ns 983ns 0 0 0 2.14 KB
master EncodeLegacyArgs net472 154μs 97.1ns 376ns 0.31 0 0 2.15 KB
#6241 EncodeArgs net6.0 36.7μs 25.7ns 99.5ns 0.444 0 0 32.4 KB
#6241 EncodeArgs netcoreapp3.1 54.9μs 30.4ns 114ns 0.44 0 0 32.4 KB
#6241 EncodeArgs net472 66.2μs 42.5ns 165ns 5.14 0.0659 0 32.5 KB
#6241 EncodeLegacyArgs net6.0 79.2μs 23.7ns 92ns 0.0369 0 0 2.14 KB
#6241 EncodeLegacyArgs netcoreapp3.1 105μs 127ns 493ns 0 0 0 2.15 KB
#6241 EncodeLegacyArgs net472 153μs 51.3ns 192ns 0.328 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 185μs 60.9ns 236ns 0 0 0 2.44 KB
master RunWafRealisticBenchmark netcoreapp3.1 198μs 238ns 921ns 0 0 0 2.39 KB
master RunWafRealisticBenchmark net472 210μs 141ns 545ns 0.315 0 0 2.46 KB
master RunWafRealisticBenchmarkWithAttack net6.0 122μs 106ns 381ns 0 0 0 1.47 KB
master RunWafRealisticBenchmarkWithAttack netcoreapp3.1 130μs 155ns 598ns 0 0 0 1.46 KB
master RunWafRealisticBenchmarkWithAttack net472 138μs 42.5ns 153ns 0.208 0 0 1.49 KB
#6241 RunWafRealisticBenchmark net6.0 186μs 159ns 614ns 0 0 0 2.44 KB
#6241 RunWafRealisticBenchmark netcoreapp3.1 197μs 224ns 869ns 0 0 0 2.39 KB
#6241 RunWafRealisticBenchmark net472 210μs 63.3ns 237ns 0.315 0 0 2.46 KB
#6241 RunWafRealisticBenchmarkWithAttack net6.0 123μs 248ns 961ns 0 0 0 1.47 KB
#6241 RunWafRealisticBenchmarkWithAttack netcoreapp3.1 136μs 496ns 1.92μs 0 0 0 1.46 KB
#6241 RunWafRealisticBenchmarkWithAttack net472 140μs 91.7ns 355ns 0.209 0 0 1.49 KB
Benchmarks.Trace.Iast.StringAspectsBenchmark - Same speed ✔️ More allocations ⚠️

More allocations ⚠️ in #6241

Benchmark Base Allocated Diff Allocated Change Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑net472 60.49 KB 61.83 KB 1.34 KB 2.22%
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑net472 272.91 KB 278.53 KB 5.62 KB 2.06%

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StringConcatBenchmark net6.0 52.9μs 209ns 782ns 0 0 0 43.44 KB
master StringConcatBenchmark netcoreapp3.1 54.2μs 287ns 1.38μs 0 0 0 42.64 KB
master StringConcatBenchmark net472 38μs 165ns 616ns 0 0 0 60.49 KB
master StringConcatAspectBenchmark net6.0 295μs 4.71μs 43.7μs 0 0 0 253.95 KB
master StringConcatAspectBenchmark netcoreapp3.1 354μs 1.97μs 12.1μs 0 0 0 255.27 KB
master StringConcatAspectBenchmark net472 278μs 6.14μs 58.9μs 0 0 0 272.91 KB
#6241 StringConcatBenchmark net6.0 59.8μs 703ns 6.85μs 0 0 0 43.44 KB
#6241 StringConcatBenchmark netcoreapp3.1 61.4μs 838ns 8.25μs 0 0 0 42.64 KB
#6241 StringConcatBenchmark net472 36.7μs 88.8ns 320ns 0 0 0 61.83 KB
#6241 StringConcatAspectBenchmark net6.0 313μs 1.64μs 8.01μs 0 0 0 255.11 KB
#6241 StringConcatAspectBenchmark netcoreapp3.1 350μs 1.7μs 9.29μs 0 0 0 254.23 KB
#6241 StringConcatAspectBenchmark net472 266μs 5.06μs 48.3μs 0 0 0 278.53 KB

Copy link
Contributor Author

daniel-romano-DD commented Nov 6, 2024

@andrewlock
Copy link
Member

andrewlock commented Nov 6, 2024

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 (6241) (11.014M)   : 0, 11014258
    master (11.155M)   : 0, 11154721
    benchmarks/2.9.0 (11.033M)   : 0, 11032866

    section Automatic
    This PR (6241) (7.196M)   : 0, 7196294
    master (7.297M)   : 0, 7297200
    benchmarks/2.9.0 (7.786M)   : 0, 7785853

    section Trace stats
    master (7.684M)   : 0, 7684145

    section Manual
    master (11.069M)   : 0, 11068901

    section Manual + Automatic
    This PR (6241) (6.652M)   : 0, 6651985
    master (6.786M)   : 0, 6785523

    section DD_TRACE_ENABLED=0
    master (10.249M)   : 0, 10249493

Loading
gantt
    title Throughput Linux arm64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (6241) (9.472M)   : 0, 9471816
    master (9.602M)   : 0, 9601778
    benchmarks/2.9.0 (9.495M)   : 0, 9494821

    section Automatic
    This PR (6241) (6.142M)   : 0, 6141782
    master (6.210M)   : 0, 6209560

    section Trace stats
    master (6.728M)   : 0, 6728350

    section Manual
    master (9.437M)   : 0, 9436743

    section Manual + Automatic
    This PR (6241) (5.937M)   : 0, 5937023
    master (5.810M)   : 0, 5809535

    section DD_TRACE_ENABLED=0
    master (8.705M)   : 0, 8705485

Loading
gantt
    title Throughput Windows x64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (6241) (10.109M)   : 0, 10109285
    master (10.329M)   : 0, 10328534
    benchmarks/2.9.0 (10.020M)   : 0, 10019592

    section Automatic
    This PR (6241) (6.344M)   : 0, 6344047
    master (6.617M)   : 0, 6616689
    benchmarks/2.9.0 (7.255M)   : 0, 7255257

    section Trace stats
    master (7.166M)   : 0, 7165623

    section Manual
    master (10.093M)   : 0, 10093300

    section Manual + Automatic
    This PR (6241) (6.036M)   : 0, 6036305
    master (6.311M)   : 0, 6310782

    section DD_TRACE_ENABLED=0
    master (9.459M)   : 0, 9459125

Loading

Copy link
Member

@tonyredondo tonyredondo left a comment

Choose a reason for hiding this comment

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

Nice, thanks!

Comment on lines 75 to 78
<ItemGroup>
<Compile Include="..\..\build\_build\CodeGenerators\InstrumentationCategory.cs" Link="ClrProfiler\InstrumentationCategory.cs" />
<Compile Include="..\..\build\_build\CodeGenerators\TargetFrameworks.cs" Link="ClrProfiler\TargetFrameworks.cs" />
</ItemGroup>
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason to do this in Datadog.Trace.csproj?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the opposite way (files in Datadog.Trace and link in _build) it failed in Linux with a File not found 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

In the opposite way (files in Datadog.Trace and link in _build) it failed in Linux with a File not found 🤷‍♂️

Urgh, I'd really prefer we don't do this 😟 We get into issues of chasing the files around, esp as the build won't explicitly fail with "file not found" or anything.

The reason it fails is because in the docker file we copy only the build files across first, and compile the _build project.

Does the build really need direct references to these files? 🤔 Instead of having a direct reference, I wonder if we can grab the values from the dlls we load even?

Worst case, I'd probably actually prefer we just duplicated the files and added a comment 🙈

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'm duplicating the files and add comments.

@daniel-romano-DD daniel-romano-DD marked this pull request as ready for review November 7, 2024 21:56
@daniel-romano-DD daniel-romano-DD requested review from a team as code owners November 7, 2024 21:56
Copy link
Member

@andrewlock andrewlock left a comment

Choose a reason for hiding this comment

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

Nice work! 😄 90% tiny nits, just a couple of questions build-wise... I didn't review the changes to the aspects very extensively, probably want to get ASM 👀 on that

var tfmCategory = GetCategory(tfm);

// Open dll to extract all AspectsClass attributes.
using var asmDefinition = Mono.Cecil.AssemblyDefinition.ReadAssembly(dllPath);
Copy link
Member

Choose a reason for hiding this comment

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

I guess this is just easier/better than what we were doing in GenerateIntegrationDefinitions, loading into a custom assembly context to read the calltargetdefinitions🤔

tracer/build/_build/CodeGenerators/CallSitesGenerator.cs Outdated Show resolved Hide resolved
tracer/build/_build/CodeGenerators/CallSitesGenerator.cs Outdated Show resolved Hide resolved
tracer/build/_build/CodeGenerators/CallSitesGenerator.cs Outdated Show resolved Hide resolved
namespace trace
{

std::vector<WCHAR*> g_callSites=
Copy link
Member

Choose a reason for hiding this comment

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

Just to clarify, I think we need a github check that this file has been correctly generated and committed:

  • Currently, this file is generated in the CompileManagedSrc target (which is where it has to be generated).
  • However, we build the native tracer, before we build the managed tracer
  • That means the native tracer will be built with the previous generated files.

I think this is already covered by the existing GitHub Actions check for committed source generators that we currently have, but it would be good to double check that 🤔

However, when running locally, there's no guarantee that CompileNativeSrc runs after CompileManagedSrc, which it now requires. That could lead to weird experiences locally, where you make some changes to the aspects, call BuildTracerHome, and it looks like everything is working, but actually your native code doesn't include the definitions...

The question is, can we just add .After(CompileManagedSrc) to the CompileNativeSrc target 🤔 I'm not sure unfortunately... I can't remember...

(WCHAR*)WStr(" [AspectMethodReplace(\"System.Text.Json.JsonDocument::Parse(System.String,System.Text.Json.JsonDocumentOptions)\",\"\",[0],[False],[None],Default,[]);V2.49.0] Parse(System.String,System.Text.Json.JsonDocumentOptions) 160"),
(WCHAR*)WStr(" [AspectMethodReplace(\"System.Text.Json.JsonElement::GetRawText()\",\"\",[0],[True],[None],Default,[]);V2.49.0] GetRawText(System.Object) 160"),
(WCHAR*)WStr(" [AspectMethodReplace(\"System.Text.Json.JsonElement::GetString()\",\"\",[0],[True],[None],Default,[]);V2.49.0] GetString(System.Object) 160"),
(WCHAR*)WStr("[AspectClass(\"System.Web\",[None],Propagation,[])] Datadog.Trace.Iast.Aspects.System.Web.HttpCookieAspect 4"),
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth trying to #if out the aspects which are .NET FX only when we're running on Linux/macOS 🤔 No need to in this PR, but could be an optimisation, both for dll size, runtime memory size, and speed of lookups? 🤷‍♂️

@@ -50,7 +50,7 @@ namespace iast
//------------------------------------
VersionInfo currentVersion = GetVersionInfo(GetDatadogVersion());

DataflowAspectClass::DataflowAspectClass(Dataflow* dataflow, const WSTRING& aspectsAssembly, const WSTRING& line)
DataflowAspectClass::DataflowAspectClass(Dataflow* dataflow, const WSTRING& aspectsAssembly, const WSTRING& line, const UINT32 enabledCategories)
Copy link
Member

Choose a reason for hiding this comment

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

Again, not for this PR, but if we're generating the code directly now instead of marshalling, there's no reason we have to (or should) use strings for everything, which we then have to manipulate, right? We could just embed the already-parsed structures directly? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, agree, but that's a big enough task to do it in another PR

foreach (var line in File.ReadAllLines(path))
{
if (!line.Contains("Aspect")) { continue; }
var aspect = line.Substring(14, line.Length - 17).Replace("\\\"", "\"");
Copy link
Member

Choose a reason for hiding this comment

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

🙈 so many magic numbers

@@ -1447,7 +1447,7 @@ internal enum CallTargetKind
}

[Flags]
internal enum InstrumentationCategory
internal enum InstrumentationCategory : uint
Copy link
Member

Choose a reason for hiding this comment

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

Is this still used? You deleted the generator right? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one is the CallTarget ones. I changed the nature of the enum to uint, and reused it for the CallSites

public Dictionary<string, Aspect> Aspects = new Dictionary<string, Aspect>();
public InstrumentationCategory Categories = InstrumentationCategory.Iast;

public string Subfix()
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo? Should it be Suffix instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True

NET5_0 = 64,
NET6_0 = 128,
NET7_0 = 256,
NET8_0 = 512
Copy link
Contributor

Choose a reason for hiding this comment

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

Since .Net 9 is already on its way, it would be worth to add it here and avoid an extra modification later

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'd rather do it where it's due.

@@ -21,7 +21,4 @@ internal enum AspectType

/// <summary> Source aspect </summary>
Source,

/// <summary> Rasp and Iast Sink aspect </summary>
RaspIastSink,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! It was not very clean having this.

Copy link
Contributor

@NachoEchevarria NachoEchevarria left a comment

Choose a reason for hiding this comment

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

Nice!

@andrewlock andrewlock merged commit 615e9e2 into master Nov 21, 2024
81 of 84 checks passed
@andrewlock andrewlock deleted the dani/apm/NativeCallSites branch November 21, 2024 09:41
@github-actions github-actions bot added this to the vNext-v3 milestone Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:asm-iast area:native-library Automatic instrumentation native C++ code (Datadog.Trace.ClrProfiler.Native)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants