Skip to content

Add proper error checking around GetModuleMetadata#5985

Merged
robertpi merged 1 commit into
masterfrom
kevin/table_flip
Sep 19, 2024
Merged

Add proper error checking around GetModuleMetadata#5985
robertpi merged 1 commit into
masterfrom
kevin/table_flip

Conversation

@kevingosse

@kevingosse kevingosse commented Sep 4, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary of changes

Explicitly check for S_OK when calling GetModuleMetadata.

Reason for change

GetModuleMetadata can return S_FALSE (one known case is if the module is a resource, but there might be other). We've been using the SUCCEEDED/FAILED macros to check the error code, but S_FALSE is not considered as an error so we would mistakenly assume that the call succeeded.

Implementation details

Note that I only updated the error checks. There are a couple of places where we call GetModuleMetadata without checking the error code, I haven't touched those.

For GetModuleInterfaces in IAST, I also moved the call to Release to only do it if the call succeeded.

@kevingosse
kevingosse requested review from a team as code owners September 4, 2024 17:25
@andrewlock

andrewlock commented Sep 4, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

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 (5985) - mean (70ms)  : 67, 73
     .   : milestone, 70,
    master - mean (69ms)  : 67, 71
     .   : milestone, 69,

    section CallTarget+Inlining+NGEN
    This PR (5985) - mean (1,085ms)  : 1055, 1114
     .   : milestone, 1085,
    master - mean (1,077ms)  : 1053, 1101
     .   : milestone, 1077,
Loading 
gantt
    title Execution time (ms) FakeDbCommand (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5985) - mean (109ms)  : 104, 115
     .   : milestone, 109,
    master - mean (109ms)  : 105, 112
     .   : milestone, 109,

    section CallTarget+Inlining+NGEN
    This PR (5985) - mean (762ms)  : 746, 778
     .   : milestone, 762,
    master - mean (758ms)  : 739, 778
     .   : milestone, 758,
Loading 
gantt
    title Execution time (ms) FakeDbCommand (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5985) - mean (93ms)  : 89, 96
     .   : milestone, 93,
    master - mean (92ms)  : 90, 95
     .   : milestone, 92,

    section CallTarget+Inlining+NGEN
    This PR (5985) - mean (712ms)  : 690, 733
     .   : milestone, 712,
    master - mean (710ms)  : 697, 724
     .   : milestone, 710,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5985) - mean (190ms)  : 187, 193
     .   : milestone, 190,
    master - mean (190ms)  : 187, 192
     .   : milestone, 190,

    section CallTarget+Inlining+NGEN
    This PR (5985) - mean (1,159ms)  : 1126, 1192
     .   : milestone, 1159,
    master - mean (1,160ms)  : 1137, 1184
     .   : milestone, 1160,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5985) - mean (275ms)  : 269, 281
     .   : milestone, 275,
    master - mean (274ms)  : 270, 279
     .   : milestone, 274,

    section CallTarget+Inlining+NGEN
    This PR (5985) - mean (920ms)  : 903, 938
     .   : milestone, 920,
    master - mean (922ms)  : 902, 942
     .   : milestone, 922,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5985) - mean (263ms)  : 259, 268
     .   : milestone, 263,
    master - mean (264ms)  : 259, 268
     .   : milestone, 264,

    section CallTarget+Inlining+NGEN
    This PR (5985) - mean (906ms)  : 885, 928
     .   : milestone, 906,
    master - mean (902ms)  : 883, 922
     .   : milestone, 902,
Loading

dromanol
dromanol approved these changes Sep 4, 2024
View reviewed changes

@dromanol dromanol left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks a lot

@andrewlock

andrewlock commented Sep 4, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Benchmarks Report for appsec 🐌

Benchmarks for #5985 compared to master:

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

Slower ⚠️ in #5985

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.Asm.AppSecBodyBenchmark.ObjectExtractorSimpleBody‑netcoreapp3.1 1.253 198.55 248.73
Benchmarks.Trace.Asm.AppSecBodyBenchmark.ObjectExtractorSimpleBody‑net472 1.130 171.34 193.66

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master AllCycleSimpleBody net6.0 72.4μs 81.3ns 304ns 0.0723 0 0 6 KB
master AllCycleSimpleBody netcoreapp3.1 62.9μs 113ns 438ns 0.094 0 0 6.95 KB
master AllCycleSimpleBody net472 49.6μs 30.6ns 115ns 1.31 0 0 8.34 KB
master AllCycleMoreComplexBody net6.0 78.5μs 50.2ns 188ns 0.118 0 0 9.51 KB
master AllCycleMoreComplexBody netcoreapp3.1 69μs 86.1ns 322ns 0.138 0 0 10.37 KB
master AllCycleMoreComplexBody net472 55.4μs 41ns 159ns 1.88 0.0277 0 11.85 KB
master ObjectExtractorSimpleBody net6.0 140ns 0.125ns 0.435ns 0.00397 0 0 280 B
master ObjectExtractorSimpleBody netcoreapp3.1 199ns 0.131ns 0.473ns 0.00371 0 0 272 B
master ObjectExtractorSimpleBody net472 171ns 0.158ns 0.59ns 0.0446 0 0 281 B
master ObjectExtractorMoreComplexBody net6.0 3.04μs 3.38ns 12.6ns 0.0533 0 0 3.78 KB
master ObjectExtractorMoreComplexBody netcoreapp3.1 3.96μs 3.18ns 11.9ns 0.0494 0 0 3.69 KB
master ObjectExtractorMoreComplexBody net472 3.81μs 1.98ns 7.67ns 0.603 0.00572 0 3.8 KB
#5985 AllCycleSimpleBody net6.0 72.6μs 72.6ns 281ns 0.0728 0 0 6.01 KB
#5985 AllCycleSimpleBody netcoreapp3.1 62.3μs 71.1ns 275ns 0.093 0 0 6.95 KB
#5985 AllCycleSimpleBody net472 49.4μs 72.3ns 280ns 1.32 0 0 8.34 KB
#5985 AllCycleMoreComplexBody net6.0 77.7μs 110ns 425ns 0.116 0 0 9.51 KB
#5985 AllCycleMoreComplexBody netcoreapp3.1 69.5μs 30.4ns 105ns 0.138 0 0 10.36 KB
#5985 AllCycleMoreComplexBody net472 56.5μs 76.4ns 296ns 1.88 0.028 0 11.85 KB
#5985 ObjectExtractorSimpleBody net6.0 142ns 0.302ns 1.13ns 0.00398 0 0 280 B
#5985 ObjectExtractorSimpleBody netcoreapp3.1 249ns 0.67ns 2.6ns 0.0037 0 0 272 B
#5985 ObjectExtractorSimpleBody net472 193ns 0.308ns 1.19ns 0.0446 0 0 281 B
#5985 ObjectExtractorMoreComplexBody net6.0 3.28μs 6.54ns 25.3ns 0.0537 0 0 3.78 KB
#5985 ObjectExtractorMoreComplexBody netcoreapp3.1 4.13μs 12.7ns 49.3ns 0.0507 0 0 3.69 KB
#5985 ObjectExtractorMoreComplexBody net472 3.92μs 3.74ns 14.5ns 0.603 0.00584 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 36.7μs 17ns 65.9ns 0.446 0 0 32.4 KB
master EncodeArgs netcoreapp3.1 54.1μs 27ns 101ns 0.433 0 0 32.4 KB
master EncodeArgs net472 66μs 26.8ns 104ns 5.15 0.0661 0 32.5 KB
master EncodeLegacyArgs net6.0 72.8μs 106ns 412ns 0 0 0 2.14 KB
master EncodeLegacyArgs netcoreapp3.1 106μs 383ns 1.48μs 0 0 0 2.15 KB
master EncodeLegacyArgs net472 151μs 128ns 495ns 0.304 0 0 2.15 KB
#5985 EncodeArgs net6.0 39.1μs 83.8ns 313ns 0.442 0 0 32.4 KB
#5985 EncodeArgs netcoreapp3.1 56μs 237ns 916ns 0.44 0 0 32.4 KB
#5985 EncodeArgs net472 66.1μs 57.9ns 209ns 5.16 0.0662 0 32.5 KB
#5985 EncodeLegacyArgs net6.0 73.8μs 140ns 543ns 0 0 0 2.14 KB
#5985 EncodeLegacyArgs netcoreapp3.1 105μs 391ns 1.52μs 0 0 0 2.14 KB
#5985 EncodeLegacyArgs net472 152μs 84.9ns 306ns 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 184μs 63.1ns 244ns 0 0 0 2.42 KB
master RunWafRealisticBenchmark netcoreapp3.1 194μs 255ns 986ns 0 0 0 2.37 KB
master RunWafRealisticBenchmark net472 209μs 137ns 532ns 0.314 0 0 2.43 KB
master RunWafRealisticBenchmarkWithAttack net6.0 122μs 74.3ns 288ns 0 0 0 1.46 KB
master RunWafRealisticBenchmarkWithAttack netcoreapp3.1 130μs 193ns 747ns 0 0 0 1.45 KB
master RunWafRealisticBenchmarkWithAttack net472 139μs 209ns 783ns 0.208 0 0 1.48 KB
#5985 RunWafRealisticBenchmark net6.0 185μs 58.6ns 211ns 0 0 0 2.42 KB
#5985 RunWafRealisticBenchmark netcoreapp3.1 196μs 209ns 784ns 0 0 0 2.37 KB
#5985 RunWafRealisticBenchmark net472 209μs 45.6ns 171ns 0.315 0 0 2.43 KB
#5985 RunWafRealisticBenchmarkWithAttack net6.0 122μs 44.8ns 168ns 0 0 0 1.46 KB
#5985 RunWafRealisticBenchmarkWithAttack netcoreapp3.1 131μs 129ns 484ns 0 0 0 1.45 KB
#5985 RunWafRealisticBenchmarkWithAttack net472 140μs 52.6ns 190ns 0.209 0 0 1.48 KB
Benchmarks.Trace.Iast.StringAspectsBenchmark - Faster 🎉 Fewer allocations 🎉

Faster 🎉 in #5985

Benchmark base/diff Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑net6.0 1.180 60,900.00 51,600.00 multimodal

Fewer allocations 🎉 in #5985

Benchmark Base Allocated Diff Allocated Change Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑netcoreapp3.1 255.52 KB 254.02 KB -1.5 KB -0.59%
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑net6.0 255.88 KB 252.47 KB -3.41 KB -1.33%
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑net472 62.13 KB 59.1 KB -3.02 KB -4.87%

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StringConcatBenchmark net6.0 60.8μs 838ns 8.29μs 0 0 0 43.44 KB
master StringConcatBenchmark netcoreapp3.1 53.1μs 219ns 930ns 0 0 0 42.64 KB
master StringConcatBenchmark net472 37.3μs 96.2ns 360ns 0 0 0 62.13 KB
master StringConcatAspectBenchmark net6.0 323μs 1.83μs 12.5μs 0 0 0 255.88 KB
master StringConcatAspectBenchmark netcoreapp3.1 345μs 2μs 16.5μs 0 0 0 255.52 KB
master StringConcatAspectBenchmark net472 266μs 4.34μs 42.3μs 0 0 0 278.53 KB
#5985 StringConcatBenchmark net6.0 51.5μs 171ns 706ns 0 0 0 43.44 KB
#5985 StringConcatBenchmark netcoreapp3.1 53μs 229ns 856ns 0 0 0 42.64 KB
#5985 StringConcatBenchmark net472 37μs 82.2ns 307ns 0 0 0 59.1 KB
#5985 StringConcatAspectBenchmark net6.0 294μs 1.64μs 10.5μs 0 0 0 252.47 KB
#5985 StringConcatAspectBenchmark netcoreapp3.1 334μs 2.33μs 21.8μs 0 0 0 254.02 KB
#5985 StringConcatAspectBenchmark net472 295μs 7.33μs 72.2μs 0 0 0 278.53 KB

@andrewlock

andrewlock commented Sep 4, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Benchmarks Report for tracer 🐌

Benchmarks for #5985 compared to master:

  • 1 benchmarks are faster, with geometric mean 1.154
  • 2 benchmarks are slower, with geometric mean 1.200
  • All benchmarks have the same 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.81μs 43.8ns 291ns 0.0151 0.00753 0 5.42 KB
master StartStopWithChild netcoreapp3.1 10.2μs 53.7ns 263ns 0.0201 0.0101 0 5.62 KB
master StartStopWithChild net472 16.4μs 69.4ns 269ns 1.01 0.291 0.097 6.06 KB
#5985 StartStopWithChild net6.0 7.82μs 44.7ns 332ns 0.0163 0.00406 0 5.43 KB
#5985 StartStopWithChild netcoreapp3.1 9.81μs 55.1ns 348ns 0.019 0.00949 0 5.62 KB
#5985 StartStopWithChild net472 16.1μs 48ns 186ns 1 0.282 0.0806 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 459μs 336ns 1.3μs 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 623μs 513ns 1.99μs 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces net472 833μs 533ns 2.07μs 0.411 0 0 3.3 KB
#5985 WriteAndFlushEnrichedTraces net6.0 486μs 544ns 2.11μs 0 0 0 2.7 KB
#5985 WriteAndFlushEnrichedTraces netcoreapp3.1 627μs 424ns 1.64μs 0 0 0 2.7 KB
#5985 WriteAndFlushEnrichedTraces net472 833μs 475ns 1.71μs 0.417 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 206μs 1.2μs 10.8μs 0.201 0 0 18.45 KB
master SendRequest netcoreapp3.1 228μs 1.31μs 10.5μs 0.214 0 0 20.61 KB
master SendRequest net472 0.00371ns 0.00125ns 0.00483ns 0 0 0 0 b
#5985 SendRequest net6.0 204μs 1.17μs 10.8μs 0.195 0 0 18.45 KB
#5985 SendRequest netcoreapp3.1 222μs 1.22μs 8.65μs 0.22 0 0 20.61 KB
#5985 SendRequest net472 0.000912ns 0.000441ns 0.00165ns 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 585μs 3.29μs 21.6μs 0.598 0 0 41.65 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 686μs 3.75μs 21.2μs 0.326 0 0 41.64 KB
master WriteAndFlushEnrichedTraces net472 887μs 4.29μs 18.2μs 8.13 2.57 0.428 53.31 KB
#5985 WriteAndFlushEnrichedTraces net6.0 597μs 3.34μs 22.4μs 0.556 0 0 41.72 KB
#5985 WriteAndFlushEnrichedTraces netcoreapp3.1 708μs 3.64μs 18.5μs 0.342 0 0 41.79 KB
#5985 WriteAndFlushEnrichedTraces net472 875μs 3.92μs 15.2μs 8.36 2.64 0.44 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.34μs 2.17ns 8.42ns 0.0147 0 0 1.02 KB
master ExecuteNonQuery netcoreapp3.1 1.73μs 1.41ns 5.09ns 0.0139 0 0 1.02 KB
master ExecuteNonQuery net472 2.04μs 2.09ns 8.11ns 0.156 0 0 987 B
#5985 ExecuteNonQuery net6.0 1.28μs 1.95ns 7.57ns 0.0141 0 0 1.02 KB
#5985 ExecuteNonQuery netcoreapp3.1 1.73μs 1.34ns 5.19ns 0.0138 0 0 1.02 KB
#5985 ExecuteNonQuery net472 2.18μs 1.94ns 6.72ns 0.156 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.14μs 1.16ns 4.33ns 0.0136 0 0 976 B
master CallElasticsearch netcoreapp3.1 1.54μs 0.544ns 1.96ns 0.0131 0 0 976 B
master CallElasticsearch net472 2.5μs 1.54ns 5.77ns 0.158 0 0 995 B
master CallElasticsearchAsync net6.0 1.24μs 1.16ns 4.48ns 0.013 0 0 952 B
master CallElasticsearchAsync netcoreapp3.1 1.71μs 0.784ns 2.93ns 0.0138 0 0 1.02 KB
master CallElasticsearchAsync net472 2.55μs 1.12ns 4.18ns 0.166 0 0 1.05 KB
#5985 CallElasticsearch net6.0 1.2μs 0.838ns 3.14ns 0.0137 0 0 976 B
#5985 CallElasticsearch netcoreapp3.1 1.54μs 0.941ns 3.52ns 0.013 0 0 976 B
#5985 CallElasticsearch net472 2.44μs 0.806ns 3.02ns 0.157 0 0 995 B
#5985 CallElasticsearchAsync net6.0 1.18μs 0.296ns 1.15ns 0.013 0 0 952 B
#5985 CallElasticsearchAsync netcoreapp3.1 1.68μs 0.861ns 3.22ns 0.0132 0 0 1.02 KB
#5985 CallElasticsearchAsync net472 2.48μs 1.01ns 3.79ns 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.3μs 0.772ns 2.99ns 0.0131 0 0 952 B
master ExecuteAsync netcoreapp3.1 1.71μs 0.564ns 2.11ns 0.0128 0 0 952 B
master ExecuteAsync net472 1.72μs 1.62ns 6.27ns 0.145 0 0 915 B
#5985 ExecuteAsync net6.0 1.3μs 1.13ns 4.06ns 0.0133 0 0 952 B
#5985 ExecuteAsync netcoreapp3.1 1.67μs 3.29ns 12.7ns 0.0125 0 0 952 B
#5985 ExecuteAsync net472 1.77μs 0.498ns 1.86ns 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.2μs 1.06ns 3.97ns 0.0315 0 0 2.22 KB
master SendAsync netcoreapp3.1 5.11μs 2.11ns 7.9ns 0.0384 0 0 2.76 KB
master SendAsync net472 7.74μs 3.5ns 12.1ns 0.497 0 0 3.15 KB
#5985 SendAsync net6.0 4.16μs 1.41ns 5.28ns 0.0313 0 0 2.22 KB
#5985 SendAsync netcoreapp3.1 5.11μs 1.7ns 5.88ns 0.0357 0 0 2.76 KB
#5985 SendAsync net472 7.72μs 2.97ns 11.5ns 0.496 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.56μs 0.729ns 2.82ns 0.0227 0 0 1.64 KB
master EnrichedLog netcoreapp3.1 2.18μs 1.06ns 3.97ns 0.0218 0 0 1.64 KB
master EnrichedLog net472 2.7μs 2.47ns 8.91ns 0.249 0 0 1.57 KB
#5985 EnrichedLog net6.0 1.51μs 6.55ns 25.4ns 0.0231 0 0 1.64 KB
#5985 EnrichedLog netcoreapp3.1 2.35μs 0.982ns 3.54ns 0.0223 0 0 1.64 KB
#5985 EnrichedLog net472 2.64μs 2.27ns 8.78ns 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 120μs 270ns 1.04μs 0.057 0 0 4.28 KB
master EnrichedLog netcoreapp3.1 119μs 283ns 1.1μs 0.0597 0 0 4.28 KB
master EnrichedLog net472 147μs 130ns 503ns 0.656 0.219 0 4.46 KB
#5985 EnrichedLog net6.0 116μs 275ns 1.03μs 0.0579 0 0 4.28 KB
#5985 EnrichedLog netcoreapp3.1 119μs 263ns 1.02μs 0.0592 0 0 4.28 KB
#5985 EnrichedLog net472 150μs 211ns 819ns 0.669 0.223 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 2.99μs 1.1ns 4.11ns 0.03 0 0 2.2 KB
master EnrichedLog netcoreapp3.1 4.13μs 2.4ns 9.29ns 0.029 0 0 2.2 KB
master EnrichedLog net472 4.95μs 1.36ns 5.27ns 0.319 0 0 2.02 KB
#5985 EnrichedLog net6.0 3.03μs 0.909ns 3.52ns 0.0305 0 0 2.2 KB
#5985 EnrichedLog netcoreapp3.1 4.14μs 4.91ns 19ns 0.0288 0 0 2.2 KB
#5985 EnrichedLog net472 4.89μs 1.54ns 5.97ns 0.319 0 0 2.02 KB
Benchmarks.Trace.RedisBenchmark - Faster 🎉 Same allocations ✔️

Faster 🎉 in #5985

Benchmark base/diff Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.RedisBenchmark.SendReceive‑net6.0 1.154 1,448.50 1,255.59

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendReceive net6.0 1.45μs 0.807ns 3.13ns 0.0159 0 0 1.14 KB
master SendReceive netcoreapp3.1 1.85μs 5.26ns 20.4ns 0.0157 0 0 1.14 KB
master SendReceive net472 2.22μs 1.59ns 6.16ns 0.183 0.00111 0 1.16 KB
#5985 SendReceive net6.0 1.26μs 0.923ns 3.58ns 0.0158 0 0 1.14 KB
#5985 SendReceive netcoreapp3.1 1.79μs 1.11ns 4.31ns 0.0152 0 0 1.14 KB
#5985 SendReceive net472 2.28μs 2.08ns 8.07ns 0.183 0.00114 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.923ns 3.58ns 0.0225 0 0 1.6 KB
master EnrichedLog netcoreapp3.1 3.82μs 1.68ns 6.49ns 0.021 0 0 1.65 KB
master EnrichedLog net472 4.4μs 1.84ns 7.11ns 0.323 0 0 2.04 KB
#5985 EnrichedLog net6.0 2.87μs 0.787ns 3.05ns 0.023 0 0 1.6 KB
#5985 EnrichedLog netcoreapp3.1 3.74μs 1.83ns 7.07ns 0.0224 0 0 1.65 KB
#5985 EnrichedLog net472 4.24μs 1.53ns 5.71ns 0.323 0 0 2.04 KB
Benchmarks.Trace.SpanBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #5985

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.SpanBenchmark.StartFinishScope‑netcoreapp3.1 1.184 657.36 778.21

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartFinishSpan net6.0 430ns 0.391ns 1.46ns 0.00794 0 0 576 B
master StartFinishSpan netcoreapp3.1 557ns 0.261ns 1.01ns 0.00779 0 0 576 B
master StartFinishSpan net472 621ns 0.2ns 0.749ns 0.0916 0 0 578 B
master StartFinishScope net6.0 532ns 0.322ns 1.25ns 0.00984 0 0 696 B
master StartFinishScope netcoreapp3.1 660ns 1.3ns 5.05ns 0.00948 0 0 696 B
master StartFinishScope net472 850ns 0.909ns 3.52ns 0.104 0 0 658 B
#5985 StartFinishSpan net6.0 401ns 0.241ns 0.933ns 0.00812 0 0 576 B
#5985 StartFinishSpan netcoreapp3.1 569ns 0.499ns 1.93ns 0.00776 0 0 576 B
#5985 StartFinishSpan net472 616ns 0.51ns 1.97ns 0.0918 0 0 578 B
#5985 StartFinishScope net6.0 560ns 0.393ns 1.52ns 0.00974 0 0 696 B
#5985 StartFinishScope netcoreapp3.1 778ns 0.623ns 2.41ns 0.00936 0 0 696 B
#5985 StartFinishScope net472 856ns 1.31ns 5.06ns 0.105 0 0 658 B
Benchmarks.Trace.TraceAnnotationsBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #5985

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.TraceAnnotationsBenchmark.RunOnMethodBegin‑net6.0 1.216 601.97 732.21

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master RunOnMethodBegin net6.0 602ns 0.275ns 1.07ns 0.00972 0 0 696 B
master RunOnMethodBegin netcoreapp3.1 898ns 0.268ns 1ns 0.00945 0 0 696 B
master RunOnMethodBegin net472 1.06μs 0.522ns 2.02ns 0.104 0 0 658 B
#5985 RunOnMethodBegin net6.0 732ns 0.508ns 1.97ns 0.00987 0 0 696 B
#5985 RunOnMethodBegin netcoreapp3.1 950ns 1.2ns 4.66ns 0.0095 0 0 696 B
#5985 RunOnMethodBegin net472 1.14μs 0.788ns 2.95ns 0.104 0 0 658 B

@datadog-ddstaging

datadog-ddstaging Bot commented Sep 4, 2024 •
edited
Loading

Copy link
Copy Markdown

Datadog Report

Branch report: kevin/table_flip
Commit report: 82a0277
Test service: dd-trace-dotnet

✅ 0 Failed, 372531 Passed, 2984 Skipped, 27h 52m 13.41s Total Time

andrewlock pushed a commit that referenced this pull request Sep 6, 2024
Add proper error checking around GetModuleMetadata (#5985 => v2) (#5992)
ff4bb6d 
## Summary of changes

Explicitly check for `S_OK` when calling `GetModuleMetadata`.

## Reason for change

`GetModuleMetadata` can return `S_FALSE` (one known case is if the
module is a resource, but there might be other). We've been using the
`SUCCEEDED`/`FAILED` macros to check the error code, but `S_FALSE` is
not considered as an error so we would mistakenly assume that the call
succeeded.

## Implementation details

Note that I only updated the error checks. There are a couple of places
where we call `GetModuleMetadata` without checking the error code, I
haven't touched those.

For `GetModuleInterfaces` in IAST, I also moved the call to `Release` to
only do it if the call succeeded.
GreenMatan
GreenMatan approved these changes Sep 19, 2024
View reviewed changes

@GreenMatan GreenMatan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice catch! LGTM

@robertpi
robertpi merged commit 53e09d3 into master Sep 19, 2024
@robertpi
robertpi deleted the kevin/table_flip branch September 19, 2024 12:38
@github-actions github-actions Bot added this to the vNext-v3 milestone Sep 19, 2024
@andrewlock andrewlock added area:native-library Automatic instrumentation native C++ code (Datadog.Trace.ClrProfiler.Native) area:shared-components type:bug labels Oct 23, 2024
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
@andrewlock andrewlock andrewlock approved these changes
@dromanol dromanol dromanol approved these changes
+1 more reviewer
@GreenMatan GreenMatan GreenMatan approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area:native-library Automatic instrumentation native C++ code (Datadog.Trace.ClrProfiler.Native) area:shared-components type:bug

Projects

None yet

Milestone

3.4.0

Development

Successfully merging this pull request may close these issues.

5 participants

@kevingosse @andrewlock @robertpi @GreenMatan @dromanol