Skip to content

[IAST] Add support for Dotnet 9 ReadOnlySpan String overloads #6846

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
Merged
merged 4 commits into from
Apr 22, 2025
Merged

[IAST] Add support for Dotnet 9 ReadOnlySpan String overloads #6846

merged 4 commits into from
Apr 22, 2025

Conversation

daniel-romano-DD
Copy link
Contributor

@daniel-romano-DD daniel-romano-DD commented Apr 11, 2025 •
edited
Loading

Summary of changes

Added IAST support for string overloads using ReadOnlySpan s

Reason for change

Dotnet 6 introduced some overloads in Concat function, accepting ReadOnlySpan<char> as parameters instead of strings. Dotnet 9 introduced other overloads in Join and Split, getting ReadOnlySpan<string> instead of params string[]. Add support for both cases.

Implementation details

For the first case, implement a resolve function that returns the original string address from the first char pointed to by the ReadOnlySpan<char>.

For the second cases, and due to the fact that they are dotnet 9 and there is no tracer for that framework, the ReadOnlySpan is converted and the Array overload is called.

Test coverage

New instrumented tests have been deployed.

Other details

@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented Apr 11, 2025 •
edited
Loading

Datadog Report

All test runs 76ea175 🔗

❌ 2 Total Test Services: 1 Failed, 1 Passed

Test Services
Service Name Failed Known Flaky New Flaky Passed Skipped Total Time Test Service View
dd-trace-dotnet 1 0 0 587097 5602 48h 23m 8.93s Link
exploration_tests 0 0 0 22085 3 12m 14.58s Link

❌ Failed Tests (1)

  • MethodProbeTest - Datadog.Trace.Debugger.IntegrationTests.ProbesTests - Details

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

@andrewlock
Copy link
Member

andrewlock commented Apr 11, 2025 •
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 (6846) - mean (69ms)  : 67, 71
     .   : milestone, 69,
    master - mean (70ms)  : 64, 77
     .   : milestone, 70,

    section CallTarget+Inlining+NGEN
    This PR (6846) - mean (1,010ms)  : 990, 1030
     .   : milestone, 1010,
    master - mean (1,010ms)  : 984, 1036
     .   : milestone, 1010,
Loading 
gantt
    title Execution time (ms) FakeDbCommand (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6846) - mean (103ms)  : 100, 105
     .   : milestone, 103,
    master - mean (103ms)  : 100, 105
     .   : milestone, 103,

    section CallTarget+Inlining+NGEN
    This PR (6846) - mean (692ms)  : 666, 718
     .   : milestone, 692,
    master - mean (697ms)  : 682, 712
     .   : milestone, 697,
Loading 
gantt
    title Execution time (ms) FakeDbCommand (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6846) - mean (89ms)  : 87, 91
     .   : milestone, 89,
    master - mean (89ms)  : 86, 92
     .   : milestone, 89,

    section CallTarget+Inlining+NGEN
    This PR (6846) - mean (650ms)  : 630, 670
     .   : milestone, 650,
    master - mean (655ms)  : 632, 678
     .   : milestone, 655,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6846) - mean (190ms)  : 186, 193
     .   : milestone, 190,
    master - mean (190ms)  : 186, 194
     .   : milestone, 190,

    section CallTarget+Inlining+NGEN
    This PR (6846) - mean (1,107ms)  : 1079, 1136
     .   : milestone, 1107,
    master - mean (1,111ms)  : 1085, 1138
     .   : milestone, 1111,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6846) - mean (269ms)  : 265, 272
     .   : milestone, 269,
    master - mean (268ms)  : 265, 272
     .   : milestone, 268,

    section CallTarget+Inlining+NGEN
    This PR (6846) - mean (880ms)  : 849, 910
     .   : milestone, 880,
    master - mean (881ms)  : 848, 914
     .   : milestone, 881,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6846) - mean (260ms)  : 256, 264
     .   : milestone, 260,
    master - mean (261ms)  : 256, 266
     .   : milestone, 261,

    section CallTarget+Inlining+NGEN
    This PR (6846) - mean (864ms)  : 837, 890
     .   : milestone, 864,
    master - mean (876ms)  : 845, 907
     .   : milestone, 876,
Loading

@datadog-ddstaging
Copy link

datadog-ddstaging bot commented Apr 11, 2025 •
edited
Loading

Datadog Report

Branch report: dani/iast/donet_9_overloads
Commit report: 1ad929c
Test service: dd-trace-dotnet

✅ 0 Failed, 6 Passed, 0 Skipped, 0s Total Time

@andrewlock
Copy link
Member

andrewlock commented Apr 11, 2025 •
edited
Loading

Benchmarks Report for appsec 🐌

Benchmarks for #6846 compared to master:

  • 3 benchmarks are slower, with geometric mean 1.189
  • 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.Asm.AppSecBodyBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #6846

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.Asm.AppSecBodyBenchmark.ObjectExtractorMoreComplexBody‑net472 1.196 3,542.11 4,238.11

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master AllCycleSimpleBody net6.0 211μs 179ns 646ns 2.13 0 0 196.71 KB
master AllCycleSimpleBody netcoreapp3.1 313μs 347ns 1.34μs 1.99 0 0 204.37 KB
master AllCycleSimpleBody net472 284μs 524ns 2.03μs 37.1 1.43 0 237.3 KB
master AllCycleMoreComplexBody net6.0 213μs 139ns 518ns 2.14 0 0 200.21 KB
master AllCycleMoreComplexBody netcoreapp3.1 329μs 451ns 1.63μs 1.81 0 0 207.78 KB
master AllCycleMoreComplexBody net472 290μs 272ns 1.05μs 37.4 1.44 0 240.82 KB
master ObjectExtractorSimpleBody net6.0 145ns 0.187ns 0.7ns 0.00374 0 0 280 B
master ObjectExtractorSimpleBody netcoreapp3.1 199ns 0.187ns 0.723ns 0.00296 0 0 272 B
master ObjectExtractorSimpleBody net472 168ns 0.195ns 0.756ns 0.0444 0 0 281 B
master ObjectExtractorMoreComplexBody net6.0 2.96μs 3.19ns 11.9ns 0.0443 0 0 3.78 KB
master ObjectExtractorMoreComplexBody netcoreapp3.1 3.8μs 4.31ns 15.5ns 0.038 0 0 3.69 KB
master ObjectExtractorMoreComplexBody net472 3.55μs 4.73ns 18.3ns 0.603 0 0 3.8 KB
#6846 AllCycleSimpleBody net6.0 208μs 181ns 652ns 2.08 0 0 196.71 KB
#6846 AllCycleSimpleBody netcoreapp3.1 318μs 313ns 1.21μs 1.73 0 0 204.37 KB
#6846 AllCycleSimpleBody net472 280μs 335ns 1.3μs 36.7 1.41 0 237.31 KB
#6846 AllCycleMoreComplexBody net6.0 223μs 91ns 341ns 2.2 0 0 200.21 KB
#6846 AllCycleMoreComplexBody netcoreapp3.1 316μs 383ns 1.43μs 1.73 0 0 207.78 KB
#6846 AllCycleMoreComplexBody net472 290μs 359ns 1.34μs 37.6 1.45 0 240.82 KB
#6846 ObjectExtractorSimpleBody net6.0 135ns 0.245ns 0.949ns 0.00342 0 0 280 B
#6846 ObjectExtractorSimpleBody netcoreapp3.1 197ns 0.273ns 1.02ns 0.00296 0 0 272 B
#6846 ObjectExtractorSimpleBody net472 176ns 0.523ns 2.02ns 0.0441 0 0 281 B
#6846 ObjectExtractorMoreComplexBody net6.0 2.92μs 3.02ns 11.7ns 0.0437 0 0 3.78 KB
#6846 ObjectExtractorMoreComplexBody netcoreapp3.1 3.8μs 3.57ns 13.4ns 0.038 0 0 3.69 KB
#6846 ObjectExtractorMoreComplexBody net472 4.24μs 3.44ns 12.4ns 0.593 0 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 37.4μs 54.8ns 212ns 0.374 0 0 32.4 KB
master EncodeArgs netcoreapp3.1 53.3μs 30.7ns 106ns 0.266 0 0 32.4 KB
master EncodeArgs net472 65.6μs 97.2ns 377ns 4.85 0 0 32.51 KB
master EncodeLegacyArgs net6.0 73.1μs 38.4ns 149ns 0 0 0 2.14 KB
master EncodeLegacyArgs netcoreapp3.1 107μs 70ns 262ns 0 0 0 2.14 KB
master EncodeLegacyArgs net472 152μs 49ns 183ns 0 0 0 2.15 KB
#6846 EncodeArgs net6.0 38.5μs 38ns 137ns 0.383 0 0 32.4 KB
#6846 EncodeArgs netcoreapp3.1 54.1μs 57.1ns 206ns 0.269 0 0 32.4 KB
#6846 EncodeArgs net472 65.2μs 101ns 391ns 4.86 0 0 32.51 KB
#6846 EncodeLegacyArgs net6.0 71.3μs 28.2ns 105ns 0 0 0 2.14 KB
#6846 EncodeLegacyArgs netcoreapp3.1 105μs 226ns 847ns 0 0 0 2.14 KB
#6846 EncodeLegacyArgs net472 151μs 59ns 229ns 0 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 174μs 190ns 736ns 0 0 0 2.54 KB
master RunWafRealisticBenchmark netcoreapp3.1 184μs 307ns 1.19μs 0 0 0 2.49 KB
master RunWafRealisticBenchmark net472 199μs 136ns 508ns 0 0 0 2.55 KB
master RunWafRealisticBenchmarkWithAttack net6.0 114μs 256ns 993ns 0 0 0 1.57 KB
master RunWafRealisticBenchmarkWithAttack netcoreapp3.1 124μs 398ns 1.54μs 0 0 0 1.55 KB
master RunWafRealisticBenchmarkWithAttack net472 131μs 58.1ns 209ns 0 0 0 1.58 KB
#6846 RunWafRealisticBenchmark net6.0 173μs 102ns 367ns 0 0 0 2.54 KB
#6846 RunWafRealisticBenchmark netcoreapp3.1 183μs 276ns 996ns 0 0 0 2.49 KB
#6846 RunWafRealisticBenchmark net472 199μs 366ns 1.32μs 0 0 0 2.56 KB
#6846 RunWafRealisticBenchmarkWithAttack net6.0 114μs 69.3ns 259ns 0 0 0 1.57 KB
#6846 RunWafRealisticBenchmarkWithAttack netcoreapp3.1 122μs 226ns 876ns 0 0 0 1.55 KB
#6846 RunWafRealisticBenchmarkWithAttack net472 131μs 224ns 774ns 0 0 0 1.58 KB
Benchmarks.Trace.Iast.StringAspectsBenchmark - Slower ⚠️ More allocations ⚠️

Slower ⚠️ in #6846

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑net6.0 1.200 52,200.00 62,650.00 bimodal
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑netcoreapp3.1 1.169 53,450.00 62,500.00 bimodal

More allocations ⚠️ in #6846

Benchmark Base Allocated Diff Allocated Change Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑net472 58.04 KB 61.7 KB 3.66 KB 6.31%

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StringConcatBenchmark net6.0 52.4μs 263ns 1.14μs 0 0 0 43.44 KB
master StringConcatBenchmark netcoreapp3.1 53.4μs 252ns 1.01μs 0 0 0 42.64 KB
master StringConcatBenchmark net472 37.1μs 60.4ns 218ns 0 0 0 58.04 KB
master StringConcatAspectBenchmark net6.0 325μs 1.67μs 12.4μs 0 0 0 256.13 KB
master StringConcatAspectBenchmark netcoreapp3.1 345μs 1.88μs 11.1μs 0 0 0 255.18 KB
master StringConcatAspectBenchmark net472 283μs 6.43μs 61.7μs 0 0 0 278.53 KB
#6846 StringConcatBenchmark net6.0 63.7μs 988ns 9.78μs 0 0 0 43.44 KB
#6846 StringConcatBenchmark netcoreapp3.1 63.5μs 824ns 8.08μs 0 0 0 42.64 KB
#6846 StringConcatBenchmark net472 36μs 87.4ns 327ns 0 0 0 61.7 KB
#6846 StringConcatAspectBenchmark net6.0 294μs 6.36μs 62.7μs 0 0 0 255.94 KB
#6846 StringConcatAspectBenchmark netcoreapp3.1 355μs 1.76μs 10.6μs 0 0 0 255.36 KB
#6846 StringConcatAspectBenchmark net472 303μs 8.06μs 79.8μs 0 0 0 278.53 KB

@andrewlock
Copy link
Member

andrewlock commented Apr 11, 2025 •
edited
Loading

Benchmarks Report for tracer 🐌

Benchmarks for #6846 compared to master:

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

Fewer allocations 🎉 in #6846

Benchmark Base Allocated Diff Allocated Change Change %
Benchmarks.Trace.ActivityBenchmark.StartStopWithChild‑netcoreapp3.1 5.8 KB 5.76 KB -42 B -0.72%

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartStopWithChild net6.0 7.65μs 99.5ns 995ns 0 0 0 5.58 KB
master StartStopWithChild netcoreapp3.1 10.3μs 87.9ns 848ns 0 0 0 5.8 KB
master StartStopWithChild net472 15.4μs 84.9ns 537ns 0.971 0.299 0.0747 6.11 KB
#6846 StartStopWithChild net6.0 8.36μs 154ns 1.54μs 0 0 0 5.58 KB
#6846 StartStopWithChild netcoreapp3.1 10μs 143ns 1.43μs 0 0 0 5.76 KB
#6846 StartStopWithChild net472 15.4μs 84.7ns 479ns 1 0.308 0.0771 6.1 KB
Benchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Fewer allocations 🎉

Fewer allocations 🎉 in #6846

Benchmark Base Allocated Diff Allocated Change Change %
Benchmarks.Trace.AgentWriterBenchmark.WriteAndFlushEnrichedTraces‑net472 3.35 KB 3.31 KB -34 B -1.02%

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master WriteAndFlushEnrichedTraces net6.0 500μs 450ns 1.56μs 0 0 0 2.71 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 666μs 3.42μs 15.3μs 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces net472 849μs 1.18μs 4.41μs 0 0 0 3.35 KB
#6846 WriteAndFlushEnrichedTraces net6.0 505μs 1.19μs 4.46μs 0 0 0 2.7 KB
#6846 WriteAndFlushEnrichedTraces netcoreapp3.1 674μs 842ns 3.15μs 0 0 0 2.7 KB
#6846 WriteAndFlushEnrichedTraces net472 854μs 1.88μs 7.03μs 0 0 0 3.31 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 126μs 732ns 6.04μs 0 0 0 14.48 KB
master SendRequest netcoreapp3.1 146μs 803ns 4.54μs 0 0 0 17.28 KB
master SendRequest net472 0.00235ns 0.00077ns 0.00298ns 0 0 0 0 b
#6846 SendRequest net6.0 130μs 684ns 3.42μs 0 0 0 14.48 KB
#6846 SendRequest netcoreapp3.1 143μs 780ns 4.74μs 0 0 0 17.28 KB
#6846 SendRequest net472 0.00372ns 0.0012ns 0.00464ns 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 646μs 7.81μs 77.7μs 0 0 0 41.56 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 754μs 10.7μs 106μs 0 0 0 41.67 KB
master WriteAndFlushEnrichedTraces net472 886μs 5.4μs 54μs 8.33 4.17 0 53.39 KB
#6846 WriteAndFlushEnrichedTraces net6.0 661μs 9.34μs 93.4μs 0 0 0 41.55 KB
#6846 WriteAndFlushEnrichedTraces netcoreapp3.1 815μs 14μs 140μs 0 0 0 41.81 KB
#6846 WriteAndFlushEnrichedTraces net472 908μs 7.32μs 72.9μs 7.81 3.91 0 53.38 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.38μs 2.96ns 11.1ns 0.0136 0 0 1.03 KB
master ExecuteNonQuery netcoreapp3.1 1.76μs 5.04ns 18.9ns 0.00874 0 0 1.02 KB
master ExecuteNonQuery net472 2.07μs 3.48ns 13ns 0.154 0.0103 0 995 B
#6846 ExecuteNonQuery net6.0 1.39μs 5.34ns 20.7ns 0.0137 0 0 1.03 KB
#6846 ExecuteNonQuery netcoreapp3.1 1.82μs 3.85ns 14.4ns 0.00906 0 0 1.02 KB
#6846 ExecuteNonQuery net472 2.13μs 5.15ns 19.3ns 0.148 0.0106 0 995 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.25μs 1.08ns 4.2ns 0.0127 0 0 984 B
master CallElasticsearch netcoreapp3.1 1.59μs 1.46ns 5.25ns 0.008 0 0 984 B
master CallElasticsearch net472 2.6μs 1.75ns 6.79ns 0.156 0 0 1 KB
master CallElasticsearchAsync net6.0 1.22μs 0.757ns 2.93ns 0.0122 0 0 960 B
master CallElasticsearchAsync netcoreapp3.1 1.67μs 1.76ns 6.6ns 0.00847 0 0 1.03 KB
master CallElasticsearchAsync net472 2.71μs 1.37ns 5.29ns 0.164 0 0 1.06 KB
#6846 CallElasticsearch net6.0 1.19μs 4.89ns 17.6ns 0.0118 0 0 984 B
#6846 CallElasticsearch netcoreapp3.1 1.61μs 1.56ns 5.39ns 0.00796 0 0 984 B
#6846 CallElasticsearch net472 2.54μs 1.32ns 4.92ns 0.152 0 0 1 KB
#6846 CallElasticsearchAsync net6.0 1.35μs 2.56ns 9.23ns 0.0135 0 0 960 B
#6846 CallElasticsearchAsync netcoreapp3.1 1.63μs 2.16ns 7.8ns 0.00818 0 0 1.03 KB
#6846 CallElasticsearchAsync net472 2.81μs 0.992ns 3.71ns 0.155 0 0 1.06 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.3μs 1.66ns 6.42ns 0.013 0 0 960 B
master ExecuteAsync netcoreapp3.1 1.68μs 1.28ns 4.95ns 0.00839 0 0 960 B
master ExecuteAsync net472 1.88μs 1.11ns 4.32ns 0.141 0 0 923 B
#6846 ExecuteAsync net6.0 1.36μs 1.79ns 6.71ns 0.0135 0 0 960 B
#6846 ExecuteAsync netcoreapp3.1 1.69μs 1.2ns 4.33ns 0.00837 0 0 960 B
#6846 ExecuteAsync net472 1.84μs 0.571ns 2.14ns 0.138 0 0 923 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.42μs 4.17ns 15ns 0.0221 0 0 2.32 KB
master SendAsync netcoreapp3.1 5.35μs 5.7ns 21.3ns 0.0267 0 0 2.86 KB
master SendAsync net472 7.59μs 7.6ns 29.4ns 0.492 0 0 3.13 KB
#6846 SendAsync net6.0 4.42μs 1.6ns 5.98ns 0.022 0 0 2.32 KB
#6846 SendAsync netcoreapp3.1 5.61μs 20.4ns 76.2ns 0.0271 0 0 2.86 KB
#6846 SendAsync net472 7.45μs 5.49ns 21.3ns 0.483 0 0 3.13 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.65μs 1.59ns 6.15ns 0.0248 0 0 1.71 KB
master EnrichedLog netcoreapp3.1 2.35μs 2.1ns 7.85ns 0.0118 0 0 1.71 KB
master EnrichedLog net472 2.56μs 2.09ns 8.08ns 0.256 0 0 1.64 KB
#6846 EnrichedLog net6.0 1.67μs 1.32ns 5.13ns 0.0168 0 0 1.71 KB
#6846 EnrichedLog netcoreapp3.1 2.19μs 2.5ns 9.36ns 0.0217 0 0 1.71 KB
#6846 EnrichedLog net472 2.46μs 1.38ns 4.96ns 0.259 0 0 1.64 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 112μs 211ns 760ns 0 0 0 4.32 KB
master EnrichedLog netcoreapp3.1 116μs 245ns 948ns 0 0 0 4.32 KB
master EnrichedLog net472 153μs 376ns 1.46μs 0 0 0 4.51 KB
#6846 EnrichedLog net6.0 112μs 374ns 1.45μs 0 0 0 4.32 KB
#6846 EnrichedLog netcoreapp3.1 116μs 180ns 697ns 0 0 0 4.32 KB
#6846 EnrichedLog net472 150μs 214ns 831ns 0 0 0 4.51 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μs 4.34ns 16.8ns 0.0301 0 0 2.26 KB
master EnrichedLog netcoreapp3.1 4.09μs 4.5ns 17.4ns 0.0205 0 0 2.26 KB
master EnrichedLog net472 4.97μs 3.15ns 12.2ns 0.323 0 0 2.09 KB
#6846 EnrichedLog net6.0 3.18μs 1.83ns 6.83ns 0.0318 0 0 2.26 KB
#6846 EnrichedLog netcoreapp3.1 4.28μs 1.86ns 6.97ns 0.0217 0 0 2.26 KB
#6846 EnrichedLog net472 4.74μs 3.36ns 13ns 0.309 0 0 2.09 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.29μs 0.856ns 3.31ns 0.0129 0 0 1.15 KB
master SendReceive netcoreapp3.1 1.8μs 2.61ns 9.77ns 0.00892 0 0 1.15 KB
master SendReceive net472 2.07μs 0.899ns 3.48ns 0.176 0 0 1.16 KB
#6846 SendReceive net6.0 1.27μs 0.524ns 1.89ns 0.0126 0 0 1.15 KB
#6846 SendReceive netcoreapp3.1 1.83μs 1.34ns 5.01ns 0.00909 0 0 1.15 KB
#6846 SendReceive net472 2.07μs 0.915ns 3.42ns 0.175 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.78μs 1.59ns 6.15ns 0.014 0 0 1.64 KB
master EnrichedLog netcoreapp3.1 3.98μs 1.71ns 5.93ns 0.0198 0 0 1.69 KB
master EnrichedLog net472 4.5μs 4.44ns 17.2ns 0.315 0 0 2.08 KB
#6846 EnrichedLog net6.0 2.73μs 3.93ns 14.2ns 0.0136 0 0 1.64 KB
#6846 EnrichedLog netcoreapp3.1 3.99μs 1.95ns 7.28ns 0.02 0 0 1.69 KB
#6846 EnrichedLog net472 4.47μs 2.63ns 9.84ns 0.312 0 0 2.08 KB
Benchmarks.Trace.SpanBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartFinishSpan net6.0 421ns 0.288ns 1.12ns 0.0084 0 0 584 B
master StartFinishSpan netcoreapp3.1 637ns 0.455ns 1.7ns 0.00635 0 0 584 B
master StartFinishSpan net472 674ns 0.157ns 0.568ns 0.0912 0 0 586 B
master StartFinishScope net6.0 517ns 0.31ns 1.2ns 0.00774 0 0 704 B
master StartFinishScope netcoreapp3.1 734ns 1.03ns 3.72ns 0.00729 0 0 704 B
master StartFinishScope net472 827ns 0.448ns 1.74ns 0.104 0 0 666 B
#6846 StartFinishSpan net6.0 448ns 0.191ns 0.713ns 0.00673 0 0 584 B
#6846 StartFinishSpan netcoreapp3.1 619ns 0.451ns 1.69ns 0.00626 0 0 584 B
#6846 StartFinishSpan net472 632ns 0.438ns 1.7ns 0.0923 0 0 586 B
#6846 StartFinishScope net6.0 479ns 0.364ns 1.31ns 0.00952 0 0 704 B
#6846 StartFinishScope netcoreapp3.1 768ns 0.724ns 2.81ns 0.00776 0 0 704 B
#6846 StartFinishScope net472 854ns 0.584ns 2.26ns 0.102 0 0 666 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 628ns 0.836ns 3.24ns 0.00938 0 0 704 B
master RunOnMethodBegin netcoreapp3.1 970ns 1.72ns 6.67ns 0.00487 0 0 704 B
master RunOnMethodBegin net472 1.14μs 0.881ns 3.41ns 0.102 0 0 666 B
#6846 RunOnMethodBegin net6.0 659ns 0.53ns 1.91ns 0.00977 0 0 704 B
#6846 RunOnMethodBegin netcoreapp3.1 950ns 1.25ns 4.67ns 0.00479 0 0 704 B
#6846 RunOnMethodBegin net472 1.07μs 0.39ns 1.46ns 0.101 0 0 666 B

@daniel-romano-DD daniel-romano-DD marked this pull request as ready for review April 14, 2025 15:00
@daniel-romano-DD daniel-romano-DD requested review from a team as code owners April 14, 2025 15:00
@robertpi robertpi modified the milestone: vNext-v3 Apr 18, 2025
e-n-0
e-n-0 approved these changes Apr 18, 2025
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.

Thanks a lot 😄
Left a few nits

tracer/src/Datadog.Trace/Iast/Aspects/System/StringAspects.cs Outdated
}
catch (Exception ex)
{
IastModule.LogAspectException(ex, $"{nameof(StringAspects)}.{nameof(Join)}");
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 the method reported in the error here should be Concat instead of Join?

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
IastModule.LogAspectException(ex, $"{nameof(StringAspects)}.{nameof(Join)}");
IastModule.LogAspectException(ex, $"{nameof(StringAspects)}.{nameof(Concat)}");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixing it

tracer/src/Datadog.Trace/Iast/Aspects/System/StringAspects.cs Outdated
}
catch (Exception ex)
{
IastModule.LogAspectException(ex, $"{nameof(StringAspects)}.{nameof(Join)}");
Copy link
Member

Choose a reason for hiding this comment

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

same here

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
IastModule.LogAspectException(ex, $"{nameof(StringAspects)}.{nameof(Join)}");
IastModule.LogAspectException(ex, $"{nameof(StringAspects)}.{nameof(Concat)}");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixing it

tracer/src/Datadog.Trace/Iast/Aspects/System/StringAspects.cs Outdated
}
catch (Exception ex)
{
IastModule.LogAspectException(ex, $"{nameof(StringAspects)}.{nameof(Join)}");
Copy link
Member

Choose a reason for hiding this comment

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

same here

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
IastModule.LogAspectException(ex, $"{nameof(StringAspects)}.{nameof(Join)}");
IastModule.LogAspectException(ex, $"{nameof(StringAspects)}.{nameof(Concat)}");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixing it

...st/test-applications/integrations/Samples.InstrumentedTests/Samples.InstrumentedTests.csproj Outdated
@@ -22,6 +22,7 @@
<!-- as those force choosing the span overloads (which we currently don't support) -->
<!-- Once we add support for those overloads, we should remove this -->
<LangVersion>12.0</LangVersion>
<AllowUnsafeBlocks>True</AllowUnsafeBlocks>
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 really needed? Maybe an artefact but might not needed anymore?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 I'm surprised to see this here since none of your updates to the test code seem to require 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.

Yep, a leftover from initial implementation testing. Removing it

zacharycmontoya
zacharycmontoya reviewed Apr 18, 2025
View reviewed changes
tracer/src/Datadog.Trace/Iast/Aspects/System/StringAspects.cs
public static string Join(string separator, ReadOnlySpan<string> values)
#pragma warning disable DD0005 // Aspect is in incorrect format
{
return Join(separator, values.ToArray());
Copy link
Contributor

@zacharycmontoya zacharycmontoya Apr 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Perhaps this can be done as a follow-up, but can you write a NET6_0_OR_GREATER version of Join / OnStringJoin<T> to accept and iterate over a ReadOnlySpan<T>? I can't see an obvious reason why you couldn't, but maybe I'm missing something

Copy link
Member

Choose a reason for hiding this comment

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

I was trying to establish the same thing for Concat(ReadOnlySpan<string>) above but I think actually the "trivial" implementation is basically this which is far from trivial 😅

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 agree. But right now these methods will be called very seldom (if anytime at all), and it would imply duplicating code, so I prefer the lesser evil of an extra alloc for the sake of simplicity. But in the future, when these overloads become more "popular" it's something to consider.

zacharycmontoya
zacharycmontoya approved these changes Apr 18, 2025
View reviewed changes
Copy link
Contributor

@zacharycmontoya zacharycmontoya left a comment

Choose a reason for hiding this comment

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

LGTM. I left a couple of comments on the implementation, but nothing is blocking from me

andrewlock
andrewlock approved these changes Apr 22, 2025
View reviewed changes
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.

LGTM, though it's a shame to have to trash the performance by calling ToArray everywhere 😢 That would definitely be an unexpected side effect for customers I think, because they could potentially go from using ReadOnlySpan<T> for performance reasons, to suddenly getting worse performance than if they'd used the original collection

tracer/src/Datadog.Trace/Iast/Aspects/System/StringAspects.cs Outdated
}
catch (Exception ex)
{
IastModule.LogAspectException(ex, $"{nameof(StringAspects)}.{nameof(Join)}");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
IastModule.LogAspectException(ex, $"{nameof(StringAspects)}.{nameof(Join)}");
IastModule.LogAspectException(ex, $"{nameof(StringAspects)}.{nameof(Concat)}");

tracer/src/Datadog.Trace/Iast/Aspects/System/StringAspects.cs Outdated
}
catch (Exception ex)
{
IastModule.LogAspectException(ex, $"{nameof(StringAspects)}.{nameof(Join)}");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
IastModule.LogAspectException(ex, $"{nameof(StringAspects)}.{nameof(Join)}");
IastModule.LogAspectException(ex, $"{nameof(StringAspects)}.{nameof(Concat)}");

tracer/src/Datadog.Trace/Iast/Aspects/System/StringAspects.cs Outdated
}
catch (Exception ex)
{
IastModule.LogAspectException(ex, $"{nameof(StringAspects)}.{nameof(Join)}");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
IastModule.LogAspectException(ex, $"{nameof(StringAspects)}.{nameof(Join)}");
IastModule.LogAspectException(ex, $"{nameof(StringAspects)}.{nameof(Concat)}");

tracer/src/Datadog.Trace/Iast/Aspects/System/StringAspects.cs
#pragma warning disable DD0005 // Aspect is in incorrect format
{
// TODO: use propper overload when dotnet 9 or later target is available
var values = param.ToArray();
Copy link
Member

Choose a reason for hiding this comment

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

How concerned should we be about the fact that this allocates an array, when the whole purpose of the Concat(System.ReadOnlySpan1<System.String>)` call is to not allocate 🤔

Somewhat unrelated, but I was trying to find the source code for the Concat(System.ReadOnlySpan1<System.String>)` call (to see if we could trivially implement it here instead) and couldn't find it anywhere 🤔

tracer/src/Datadog.Trace/Iast/Aspects/System/StringAspects.cs
public static string Join(string separator, ReadOnlySpan<string> values)
#pragma warning disable DD0005 // Aspect is in incorrect format
{
return Join(separator, values.ToArray());
Copy link
Member

Choose a reason for hiding this comment

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

I was trying to establish the same thing for Concat(ReadOnlySpan<string>) above but I think actually the "trivial" implementation is basically this which is far from trivial 😅

...st-applications/integrations/Samples.InstrumentedTests/Propagation/String/StringJoinTests.cs
var span = new ReadOnlySpan<string>(values);
AssertTaintedFormatWithOriginalCallCheck(":+-tainted-+:a:+-tainted-+:",
System.String.Join('a', span),
() => System.String.Join('a', values)); // Use values here as we can not use span in the lambda expression
Copy link
Member

Choose a reason for hiding this comment

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

// Use values here as we can not use span in the lambda expression

So are we actually testing the code we think we are? 🤔 The lambda is for calculating the "expected" value right? If so then this is fine 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the case.

@daniel-romano-DD daniel-romano-DD merged commit ef4da48 into master Apr 22, 2025
129 of 133 checks passed
@daniel-romano-DD daniel-romano-DD deleted the dani/iast/donet_9_overloads branch April 22, 2025 11:15
@github-actions github-actions bot added this to the vNext-v3 milestone Apr 22, 2025
@andrewlock andrewlock added the type:enhancement Improvement to an existing feature label May 6, 2025
chojomok pushed a commit that referenced this pull request Jul 15, 2025
@daniel-romano-DD @chojomok
[IAST] Add support for Dotnet 9 ReadOnlySpan String overloads (#6846)
67481fe 
## Summary of changes
Added IAST support for string overloads using `ReadOnlySpan` s

## Reason for change
Dotnet 6 introduced some overloads in `Concat `function, accepting
`ReadOnlySpan<char>` as parameters instead of strings. Dotnet 9
introduced other overloads in Join and Split, getting
`ReadOnlySpan<string>` instead of `params string[]`. Add support for
both cases.

## Implementation details
For the first case, implement a resolve function that returns the
original string address from the first char pointed to by the
`ReadOnlySpan<char>`.

For the second cases, and due to the fact that they are dotnet 9 and
there is no tracer for that framework, the ReadOnlySpan is converted and
the Array overload is called.

## Test coverage
New instrumented tests have been deployed.

## 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

@robertpi robertpi robertpi approved these changes

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

@zacharycmontoya zacharycmontoya zacharycmontoya approved these changes

@andrewlock andrewlock andrewlock approved these changes

Assignees
No one assigned
Labels
area:asm-iast type:enhancement Improvement to an existing feature
Projects
None yet
Milestone
3.16.0
Development

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