Skip to content

[dd-trace] Improved GAC commands #6995

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 8 commits into from
May 23, 2025
Merged

[dd-trace] Improved GAC commands #6995

merged 8 commits into from
May 23, 2025

Conversation

tonyredondo
Copy link
Member

@tonyredondo tonyredondo commented May 21, 2025 •
edited
Loading

Summary of changes

This PR improves the support in the GAC command, by allowing us to install, uninstall and get info for multiple versions of an assembly.

Reason for change

The current support only checks if an assembly is installed in the GAC no matter version, causing an issue when a previous version of dd-trace installed an old version of Datadog.Trace in the GAC meaning that we bailout and don't install the new version, causing a crash when the application loads.

image

Implementation details

Now we include more unmanaged Fusion apis to list the version of the assemblies installed in the GAC.

Test coverage

Added a new GacWithMultipleVersions test.

Other details

@tonyredondo tonyredondo requested a review from a team as a code owner May 21, 2025 13:02
@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented May 21, 2025 •
edited
Loading

Datadog Report

All test runs 6bcc865 🔗

✅ 2 Total Test Services: 0 Failed, 2 Passed

Test Services
Service Name Failed Known Flaky New Flaky Passed Skipped Total Time Test Service View
dd-trace-dotnet 0 0 0 242919 2282 15h 8m 53.67s Link
exploration_tests 0 0 0 22085 3 2m 15.09s Link

@andrewlock
Copy link
Member

andrewlock commented May 21, 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.8) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6995) - mean (68ms)  : 65, 70
     .   : milestone, 68,
    master - mean (68ms)  : 66, 70
     .   : milestone, 68,

    section CallTarget+Inlining+NGEN
    This PR (6995) - mean (1,010ms)  : 984, 1036
     .   : milestone, 1010,
    master - mean (1,007ms)  : 977, 1036
     .   : milestone, 1007,
Loading 
gantt
    title Execution time (ms) FakeDbCommand (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6995) - mean (102ms)  : 101, 104
     .   : milestone, 102,
    master - mean (102ms)  : 100, 104
     .   : milestone, 102,

    section CallTarget+Inlining+NGEN
    This PR (6995) - mean (693ms)  : 673, 713
     .   : milestone, 693,
    master - mean (691ms)  : 670, 713
     .   : milestone, 691,
Loading 
gantt
    title Execution time (ms) FakeDbCommand (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6995) - mean (89ms)  : 87, 92
     .   : milestone, 89,
    master - mean (89ms)  : 87, 91
     .   : milestone, 89,

    section CallTarget+Inlining+NGEN
    This PR (6995) - mean (647ms)  : 627, 668
     .   : milestone, 647,
    master - mean (653ms)  : 633, 673
     .   : milestone, 653,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.8) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6995) - mean (189ms)  : 186, 192
     .   : milestone, 189,
    master - mean (189ms)  : 185, 193
     .   : milestone, 189,

    section CallTarget+Inlining+NGEN
    This PR (6995) - mean (1,112ms)  : 1080, 1143
     .   : milestone, 1112,
    master - mean (1,109ms)  : 1082, 1136
     .   : milestone, 1109,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6995) - mean (269ms)  : 264, 273
     .   : milestone, 269,
    master - mean (268ms)  : 265, 271
     .   : milestone, 268,

    section CallTarget+Inlining+NGEN
    This PR (6995) - mean (873ms)  : 847, 899
     .   : milestone, 873,
    master - mean (878ms)  : 846, 909
     .   : milestone, 878,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6995) - mean (261ms)  : 258, 264
     .   : milestone, 261,
    master - mean (261ms)  : 257, 265
     .   : milestone, 261,

    section CallTarget+Inlining+NGEN
    This PR (6995) - mean (859ms)  : 835, 884
     .   : milestone, 859,
    master - mean (870ms)  : 843, 896
     .   : milestone, 870,
Loading

@pr-commenter
Copy link

pr-commenter bot commented May 21, 2025 •
edited
Loading

Benchmarks

Benchmarks Report for benchmark platform 🐌

Benchmarks for #6995 compared to master:

  • All benchmarks have the same speed
  • 1 benchmarks have fewer allocations
  • 3 benchmarks have more allocations

The following thresholds were used for comparing the benchmark speeds:

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

Allocation changes below 0.5% are ignored.

Benchmark details

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

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartStopWithChild net6.0 10.4μs 57.7ns 379ns 0 0 0 5.56 KB
master StartStopWithChild netcoreapp3.1 14.1μs 60.2ns 225ns 0 0 0 5.73 KB
master StartStopWithChild net472 22μs 116ns 622ns 0.955 0.212 0 6.19 KB
#6995 StartStopWithChild net6.0 10.4μs 58.6ns 371ns 0 0 0 5.54 KB
#6995 StartStopWithChild netcoreapp3.1 13.6μs 72.1ns 353ns 0 0 0 5.74 KB
#6995 StartStopWithChild net472 21.5μs 114ns 603ns 0.99 0.44 0.11 6.17 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 955μs 139ns 519ns 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 1.01ms 53.8ns 208ns 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces net472 1.22ms 930ns 3.35μs 0 0 0 3.31 KB
#6995 WriteAndFlushEnrichedTraces net6.0 931μs 82.3ns 319ns 0 0 0 2.7 KB
#6995 WriteAndFlushEnrichedTraces netcoreapp3.1 1.01ms 239ns 924ns 0 0 0 2.7 KB
#6995 WriteAndFlushEnrichedTraces net472 1.24ms 89.3ns 334ns 0 0 0 3.31 KB
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 329μs 1.6μs 6.79μs 0 0 0 197.06 KB
master AllCycleSimpleBody netcoreapp3.1 485μs 2.35μs 9.39μs 0 0 0 204.77 KB
master AllCycleSimpleBody net472 436μs 183ns 661ns 36.6 2.16 0 236.35 KB
master AllCycleMoreComplexBody net6.0 343μs 1.5μs 5.8μs 0 0 0 200.56 KB
master AllCycleMoreComplexBody netcoreapp3.1 494μs 2.18μs 8.45μs 0 0 0 208.18 KB
master AllCycleMoreComplexBody net472 452μs 121ns 467ns 37.9 2.23 0 239.88 KB
master ObjectExtractorSimpleBody net6.0 314ns 1.82ns 13.5ns 0 0 0 280 B
master ObjectExtractorSimpleBody netcoreapp3.1 393ns 2.1ns 11.7ns 0 0 0 272 B
master ObjectExtractorSimpleBody net472 301ns 0.0771ns 0.267ns 0.044 0 0 281 B
master ObjectExtractorMoreComplexBody net6.0 6.19μs 28.4ns 110ns 0 0 0 3.78 KB
master ObjectExtractorMoreComplexBody netcoreapp3.1 7.76μs 28ns 109ns 0 0 0 3.69 KB
master ObjectExtractorMoreComplexBody net472 6.83μs 8.69ns 33.7ns 0.578 0 0 3.8 KB
#6995 AllCycleSimpleBody net6.0 334μs 626ns 2.34μs 0 0 0 197.06 KB
#6995 AllCycleSimpleBody netcoreapp3.1 480μs 1.68μs 6.5μs 0 0 0 204.77 KB
#6995 AllCycleSimpleBody net472 436μs 131ns 474ns 36.6 2.16 0 236.35 KB
#6995 AllCycleMoreComplexBody net6.0 339μs 305ns 1.18μs 0 0 0 200.57 KB
#6995 AllCycleMoreComplexBody netcoreapp3.1 501μs 841ns 3.26μs 0 0 0 208.18 KB
#6995 AllCycleMoreComplexBody net472 446μs 176ns 683ns 37.9 2.23 0 239.88 KB
#6995 ObjectExtractorSimpleBody net6.0 318ns 1.22ns 4.74ns 0 0 0 280 B
#6995 ObjectExtractorSimpleBody netcoreapp3.1 397ns 1.91ns 7.4ns 0 0 0 272 B
#6995 ObjectExtractorSimpleBody net472 302ns 0.0127ns 0.0457ns 0.0434 0 0 281 B
#6995 ObjectExtractorMoreComplexBody net6.0 6.25μs 29.3ns 109ns 0 0 0 3.78 KB
#6995 ObjectExtractorMoreComplexBody netcoreapp3.1 7.68μs 28.3ns 110ns 0 0 0 3.69 KB
#6995 ObjectExtractorMoreComplexBody net472 6.71μs 5.68ns 22ns 0.602 0 0 3.8 KB
Benchmarks.Trace.Asm.AppSecEncoderBenchmark - Unknown 🤷 Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EncodeArgs net6.0 N/A N/A N/A NaN NaN NaN 0 b
master EncodeArgs netcoreapp3.1 N/A N/A N/A NaN NaN NaN 0 b
master EncodeArgs net472 N/A N/A N/A NaN NaN NaN 0 b
master EncodeLegacyArgs net6.0 N/A N/A N/A NaN NaN NaN 0 b
master EncodeLegacyArgs netcoreapp3.1 N/A N/A N/A NaN NaN NaN 0 b
master EncodeLegacyArgs net472 N/A N/A N/A NaN NaN NaN 0 b
#6995 EncodeArgs net6.0 N/A N/A N/A NaN NaN NaN 0 b
#6995 EncodeArgs netcoreapp3.1 N/A N/A N/A NaN NaN NaN 0 b
#6995 EncodeArgs net472 N/A N/A N/A NaN NaN NaN 0 b
#6995 EncodeLegacyArgs net6.0 N/A N/A N/A NaN NaN NaN 0 b
#6995 EncodeLegacyArgs netcoreapp3.1 N/A N/A N/A NaN NaN NaN 0 b
#6995 EncodeLegacyArgs net472 N/A N/A N/A NaN NaN NaN 0 b
Benchmarks.Trace.Asm.AppSecWafBenchmark - Unknown 🤷 Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master RunWafRealisticBenchmark net6.0 N/A N/A N/A NaN NaN NaN 0 b
master RunWafRealisticBenchmark netcoreapp3.1 N/A N/A N/A NaN NaN NaN 0 b
master RunWafRealisticBenchmark net472 N/A N/A N/A NaN NaN NaN 0 b
master RunWafRealisticBenchmarkWithAttack net6.0 N/A N/A N/A NaN NaN NaN 0 b
master RunWafRealisticBenchmarkWithAttack netcoreapp3.1 N/A N/A N/A NaN NaN NaN 0 b
master RunWafRealisticBenchmarkWithAttack net472 N/A N/A N/A NaN NaN NaN 0 b
#6995 RunWafRealisticBenchmark net6.0 N/A N/A N/A NaN NaN NaN 0 b
#6995 RunWafRealisticBenchmark netcoreapp3.1 N/A N/A N/A NaN NaN NaN 0 b
#6995 RunWafRealisticBenchmark net472 N/A N/A N/A NaN NaN NaN 0 b
#6995 RunWafRealisticBenchmarkWithAttack net6.0 N/A N/A N/A NaN NaN NaN 0 b
#6995 RunWafRealisticBenchmarkWithAttack netcoreapp3.1 N/A N/A N/A NaN NaN NaN 0 b
#6995 RunWafRealisticBenchmarkWithAttack net472 N/A N/A N/A NaN NaN NaN 0 b
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 61.7μs 62.5ns 242ns 0 0 0 14.53 KB
master SendRequest netcoreapp3.1 71.1μs 180ns 650ns 0 0 0 17.42 KB
master SendRequest net472 0.0106ns 0.00117ns 0.00453ns 0 0 0 0 b
#6995 SendRequest net6.0 60.4μs 64.4ns 232ns 0 0 0 14.53 KB
#6995 SendRequest netcoreapp3.1 70.8μs 43ns 161ns 0 0 0 17.42 KB
#6995 SendRequest net472 0.0073ns 0.0025ns 0.00967ns 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 701μs 1.87μs 6.99μs 0 0 0 41.76 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 663μs 1.07μs 4.14μs 0 0 0 41.84 KB
master WriteAndFlushEnrichedTraces net472 1.02ms 4.29μs 16.6μs 4.81 0 0 56.14 KB
#6995 WriteAndFlushEnrichedTraces net6.0 686μs 1.63μs 6.31μs 0 0 0 41.73 KB
#6995 WriteAndFlushEnrichedTraces netcoreapp3.1 711μs 4.12μs 32.7μs 0 0 0 41.94 KB
#6995 WriteAndFlushEnrichedTraces net472 981μs 2.76μs 9.94μs 8.33 0 0 56.05 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.97μs 9.5ns 38ns 0 0 0 1.03 KB
master ExecuteNonQuery netcoreapp3.1 2.42μs 10.4ns 40.4ns 0 0 0 1.02 KB
master ExecuteNonQuery net472 2.73μs 4.28ns 16.6ns 0.15 0.0136 0 995 B
#6995 ExecuteNonQuery net6.0 1.92μs 7.23ns 28ns 0 0 0 1.03 KB
#6995 ExecuteNonQuery netcoreapp3.1 2.44μs 11.9ns 49ns 0 0 0 1.02 KB
#6995 ExecuteNonQuery net472 2.71μs 5.37ns 20.8ns 0.148 0.0135 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.74μs 9.16ns 47.6ns 0 0 0 1.04 KB
master CallElasticsearch netcoreapp3.1 2.3μs 11.2ns 49ns 0 0 0 1.04 KB
master CallElasticsearch net472 3.43μs 1.55ns 5.8ns 0.155 0 0 1.05 KB
master CallElasticsearchAsync net6.0 1.82μs 0.543ns 2.1ns 0 0 0 1.02 KB
master CallElasticsearchAsync netcoreapp3.1 2.34μs 12.4ns 54.1ns 0 0 0 1.09 KB
master CallElasticsearchAsync net472 3.57μs 0.967ns 3.74ns 0.162 0 0 1.11 KB
#6995 CallElasticsearch net6.0 1.73μs 9.55ns 41.6ns 0 0 0 1.04 KB
#6995 CallElasticsearch netcoreapp3.1 2.35μs 9.95ns 38.5ns 0 0 0 1.04 KB
#6995 CallElasticsearch net472 3.49μs 4.17ns 16.1ns 0.158 0 0 1.05 KB
#6995 CallElasticsearchAsync net6.0 1.81μs 1.12ns 4.34ns 0 0 0 1.02 KB
#6995 CallElasticsearchAsync netcoreapp3.1 2.39μs 8.77ns 34ns 0 0 0 1.09 KB
#6995 CallElasticsearchAsync net472 3.74μs 5.42ns 21ns 0.168 0 0 1.11 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.73μs 1.13ns 4.37ns 0 0 0 960 B
master ExecuteAsync netcoreapp3.1 2.31μs 4.48ns 17.3ns 0 0 0 960 B
master ExecuteAsync net472 2.56μs 3.74ns 14.5ns 0.141 0 0 923 B
#6995 ExecuteAsync net6.0 1.82μs 6.29ns 23.5ns 0 0 0 960 B
#6995 ExecuteAsync netcoreapp3.1 2.28μs 0.391ns 1.51ns 0 0 0 960 B
#6995 ExecuteAsync net472 2.59μs 2.25ns 8.72ns 0.143 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 6.99μs 19.5ns 72.9ns 0 0 0 2.37 KB
master SendAsync netcoreapp3.1 8.45μs 19.7ns 76.3ns 0 0 0 2.9 KB
master SendAsync net472 12.3μs 13.6ns 52.7ns 0.497 0 0 3.19 KB
#6995 SendAsync net6.0 6.83μs 5.57ns 20.8ns 0 0 0 2.37 KB
#6995 SendAsync netcoreapp3.1 8.38μs 30.3ns 117ns 0 0 0 2.9 KB
#6995 SendAsync net472 12.2μs 11.1ns 42.9ns 0.488 0 0 3.19 KB
Benchmarks.Trace.Iast.StringAspectsBenchmark - Same speed ✔️ More allocations ⚠️

More allocations ⚠️ in #6995

Benchmark Base Allocated Diff Allocated Change Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑netcoreapp3.1 42.71 KB 45.55 KB 2.84 KB 6.65%
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑netcoreapp3.1 262.2 KB 276.29 KB 14.09 KB 5.37%
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑net6.0 257.73 KB 261.51 KB 3.78 KB 1.47%

Fewer allocations 🎉 in #6995

Benchmark Base Allocated Diff Allocated Change Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑net472 286.72 KB 278.53 KB -8.19 KB -2.86%

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StringConcatBenchmark net6.0 41.7μs 203ns 787ns 0 0 0 43.85 KB
master StringConcatBenchmark netcoreapp3.1 49.8μs 283ns 1.98μs 0 0 0 42.71 KB
master StringConcatBenchmark net472 56.1μs 261ns 976ns 0 0 0 57.34 KB
master StringConcatAspectBenchmark net6.0 460μs 2μs 8.24μs 0 0 0 257.73 KB
master StringConcatAspectBenchmark netcoreapp3.1 530μs 2.32μs 11.6μs 0 0 0 262.2 KB
master StringConcatAspectBenchmark net472 419μs 1.95μs 7.31μs 0 0 0 286.72 KB
#6995 StringConcatBenchmark net6.0 44.7μs 251ns 1.63μs 0 0 0 43.73 KB
#6995 StringConcatBenchmark netcoreapp3.1 54.3μs 742ns 7.27μs 0 0 0 45.55 KB
#6995 StringConcatBenchmark net472 57.5μs 121ns 435ns 0 0 0 57.34 KB
#6995 StringConcatAspectBenchmark net6.0 463μs 2.02μs 7.27μs 0 0 0 261.51 KB
#6995 StringConcatAspectBenchmark netcoreapp3.1 535μs 2.56μs 10.2μs 0 0 0 276.29 KB
#6995 StringConcatAspectBenchmark net472 413μs 1.98μs 8.42μs 0 0 0 278.53 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 2.63μs 4.39ns 16.4ns 0 0 0 1.76 KB
master EnrichedLog netcoreapp3.1 3.39μs 15.2ns 59ns 0 0 0 1.76 KB
master EnrichedLog net472 3.91μs 2.38ns 9.22ns 0.253 0 0 1.69 KB
#6995 EnrichedLog net6.0 2.6μs 1.87ns 7.24ns 0 0 0 1.76 KB
#6995 EnrichedLog netcoreapp3.1 3.54μs 16ns 62.1ns 0 0 0 1.76 KB
#6995 EnrichedLog net472 3.91μs 3.77ns 14.6ns 0.253 0 0 1.69 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 124μs 37.1ns 139ns 0 0 0 4.37 KB
master EnrichedLog netcoreapp3.1 129μs 44.3ns 166ns 0 0 0 4.37 KB
master EnrichedLog net472 168μs 32.1ns 116ns 0 0 0 4.57 KB
#6995 EnrichedLog net6.0 123μs 97.5ns 378ns 0 0 0 4.37 KB
#6995 EnrichedLog netcoreapp3.1 128μs 329ns 1.23μs 0 0 0 4.37 KB
#6995 EnrichedLog net472 165μs 81.3ns 315ns 0 0 0 4.57 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 5.04μs 8.37ns 31.3ns 0 0 0 2.32 KB
master EnrichedLog netcoreapp3.1 6.84μs 9.14ns 34.2ns 0 0 0 2.32 KB
master EnrichedLog net472 7.29μs 6.59ns 25.5ns 0.329 0 0 2.14 KB
#6995 EnrichedLog net6.0 5.13μs 4.53ns 17.5ns 0 0 0 2.32 KB
#6995 EnrichedLog netcoreapp3.1 6.65μs 16.1ns 60.3ns 0 0 0 2.32 KB
#6995 EnrichedLog net472 7.24μs 6.57ns 25.5ns 0.326 0 0 2.14 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 2.01μs 1.38ns 5.16ns 0 0 0 1.21 KB
master SendReceive netcoreapp3.1 2.7μs 11.3ns 43.9ns 0 0 0 1.21 KB
master SendReceive net472 3.07μs 3.14ns 12.2ns 0.185 0 0 1.21 KB
#6995 SendReceive net6.0 2.01μs 1.42ns 5.5ns 0 0 0 1.21 KB
#6995 SendReceive netcoreapp3.1 2.51μs 9.99ns 38.7ns 0 0 0 1.21 KB
#6995 SendReceive net472 3.14μs 3.79ns 14.7ns 0.188 0 0 1.21 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 4.19μs 17.4ns 62.9ns 0 0 0 1.64 KB
master EnrichedLog netcoreapp3.1 5.68μs 18.9ns 73.1ns 0 0 0 1.69 KB
master EnrichedLog net472 6.49μs 4.34ns 16.2ns 0.326 0 0 2.08 KB
#6995 EnrichedLog net6.0 4.19μs 12.1ns 46.9ns 0 0 0 1.64 KB
#6995 EnrichedLog netcoreapp3.1 5.78μs 19ns 73.5ns 0 0 0 1.69 KB
#6995 EnrichedLog net472 6.51μs 6.69ns 25ns 0.325 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 739ns 4.12ns 26.4ns 0 0 0 584 B
master StartFinishSpan netcoreapp3.1 1.01μs 0.735ns 2.85ns 0 0 0 584 B
master StartFinishSpan net472 910ns 0.25ns 0.902ns 0.0913 0 0 586 B
master StartFinishScope net6.0 892ns 4.54ns 22.3ns 0 0 0 704 B
master StartFinishScope netcoreapp3.1 1.22μs 6.58ns 34.2ns 0 0 0 704 B
master StartFinishScope net472 1.13μs 0.272ns 1.05ns 0.101 0 0 666 B
#6995 StartFinishSpan net6.0 746ns 0.249ns 0.964ns 0 0 0 584 B
#6995 StartFinishSpan netcoreapp3.1 957ns 4.61ns 17.9ns 0 0 0 584 B
#6995 StartFinishSpan net472 924ns 0.289ns 1.12ns 0.0927 0 0 586 B
#6995 StartFinishScope net6.0 902ns 0.287ns 1.11ns 0 0 0 704 B
#6995 StartFinishScope netcoreapp3.1 1.19μs 5.91ns 25.1ns 0 0 0 704 B
#6995 StartFinishScope net472 1.11μs 0.17ns 0.635ns 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 1.01μs 5.32ns 28.7ns 0 0 0 704 B
master RunOnMethodBegin netcoreapp3.1 1.36μs 6.48ns 26.7ns 0 0 0 704 B
master RunOnMethodBegin net472 1.35μs 0.777ns 3.01ns 0.101 0 0 666 B
#6995 RunOnMethodBegin net6.0 1.01μs 4.41ns 15.9ns 0 0 0 704 B
#6995 RunOnMethodBegin netcoreapp3.1 1.36μs 4.14ns 16ns 0 0 0 704 B
#6995 RunOnMethodBegin net472 1.35μs 1.09ns 4.22ns 0.101 0 0 666 B

@tonyredondo tonyredondo self-assigned this May 22, 2025
andrewlock
andrewlock reviewed May 23, 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.

The approach here seems to contradict quite heavily with what we have to do in the FleetInstaller 🤔 In that situation we explicitly want to allow multiple versions of the dll in the GAC, because the files could be in use when we're doing an update. Do you not have a similar scenario here? Would it be preferable to allow multiple versions to be installed? If not, I worry how this is going to interact with SSI in the future :/

I also wonder if you should be using a ref count tied to the file path like we are in the fleet installer, but I'm not sure exactly when you need to call this so I'm not sure 😅

tracer/test/Datadog.Trace.Tools.Runner.IntegrationTests/GacCommandTests.cs
}

// UNINSTALL
using (var consoleUninstall = ConsoleHelper.Redirect())
Copy link
Member

Choose a reason for hiding this comment

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

We should probably uninstall the second one afterwards too?

Copy link
Member Author

Choose a reason for hiding this comment

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

I cannot specify the version to uninstall so currently the Uninstall option uninstall all versions. did you have something in FleetInstaller?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, it's maybe because we're passing in a filepath identifier when we register in the GAC, and we can use that when we uninstall to identify the correct version 🤔

tracer/src/Datadog.Trace.Tools.Runner/GacInstallCommand.cs Outdated
if (hr == 0)
using var gacMethods = GacNativeMethods.Create();
var assemblyCache = gacMethods.CreateAssemblyCache();
var hr = assemblyCache.InstallAssembly(AssemblyCacheInstallFlags.IASSEMBLYCACHE_INSTALL_FLAG_REFRESH, assemblyPath, IntPtr.Zero);
Copy link
Member

Choose a reason for hiding this comment

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

Is IASSEMBLYCACHE_INSTALL_FLAG_REFRESH really what we want? 🤔 Doesn't that try to replace the version in the GAC? Don't we need to install them side-by-side?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've changed to None (Changes from review) but we can use it for side-by-side installation, only tries to refresh if the same version is installed.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder how the fleet installer gacinstall would interact with this tbh. That's something we should probably test (manually), even though I don't think it's an expected scenario 🤔

tracer/src/Datadog.Trace.Tools.Runner/GacUninstallCommand.cs
Utils.WriteWarning($"Assembly '{assemblyName}' is still in use.");
context.ExitCode = hr;
break;
Utils.WriteWarning($"Assembly '{assemblyName}' is not installed the GAC.");
Copy link
Member

Choose a reason for hiding this comment

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

Should this just count as success? Afterall, the file isn't installed, which is what you really want, right? i.e. Idempotency?

Copy link
Member Author

Choose a reason for hiding this comment

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

True, I've made the change here: Add exit code

Copy link
Member

Choose a reason for hiding this comment

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

Wait, but you made it an error code? Shouldn't this be success? 😅

tracer/src/Datadog.Trace.Tools.Runner/GacUninstallCommand.cs

switch (position)
var result = Hresult.S_OK;
var installedAssemblyNames = gacMethods.GetAssemblyNames(assemblyName!);
Copy link
Member

Choose a reason for hiding this comment

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

Assembly name isn't versioned, right? Should this be trying to uninstall a specific version?

Copy link
Member Author

Choose a reason for hiding this comment

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

that line, returns the FullAssemblyName from a Normal name, I'm doing like this to no change the behaviour of the command. I tried to add full assembly name support but I was unable to use that full name with the Uninstall method. How do you do it in the FleetInstaller?

Copy link
Member

Choose a reason for hiding this comment

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

We register the path to the native loader using FUSION_INSTALL_REFERENCE.FUSION_REFCOUNT_FILEPATH_GUID when we GAC install Datadog.Trace.dll

And we can add multiple versions IIRC 🤔
This is our implementation of this:
https://github.com/DataDog/dd-trace-dotnet/blob/master/tracer/src/Datadog.FleetInstaller/GacInstaller.cs

And these are the test cases we check:
https://github.com/DataDog/dd-trace-dotnet/blob/master/tracer/test/Datadog.Trace.IntegrationTests/FleetInstaller/GacInstallerTests.cs

We may not want the same behaviour here, but we should probably make the cases are compatible at least...

lucaspimentel
lucaspimentel reviewed May 23, 2025
View reviewed changes
tracer/src/Datadog.Trace.Tools.Runner/Gac/AsmDisplayFlags.cs
ASM_DISPLAYF_CONFIG_MASK = 0x100,
ASM_DISPLAYF_MVID = 0x200,
ASM_DISPLAYF_CONTENT_TYPE = 0x400,
ASM_DISPLAYF_FULL = (((((ASM_DISPLAYF_VERSION | ASM_DISPLAYF_CULTURE) | ASM_DISPLAYF_PUBLIC_KEY_TOKEN) | ASM_DISPLAYF_RETARGET) | ASM_DISPLAYF_PROCESSORARCHITECTURE) | ASM_DISPLAYF_CONTENT_TYPE)
Copy link
Member

@lucaspimentel lucaspimentel May 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

[formatting nit] What's with all the ((((((? 😅

lucaspimentel
lucaspimentel previously approved these changes May 23, 2025
View reviewed changes
@lucaspimentel lucaspimentel requested review from lucaspimentel and a team and removed request for lucaspimentel May 23, 2025 14:38
@lucaspimentel lucaspimentel dismissed their stale review May 23, 2025 14:40

I defer to Andrew (and all his PR comments)

@tonyredondo tonyredondo merged commit ab7bdfc into master May 23, 2025
129 checks passed
@tonyredondo tonyredondo deleted the tony/improved-gac-commands branch May 23, 2025 15:03
@github-actions github-actions bot added this to the vNext-v3 milestone May 23, 2025
@andrewlock andrewlock mentioned this pull request May 23, 2025
Merged
bouwkast pushed a commit that referenced this pull request May 23, 2025
@andrewlock
Fix exit code from GacUninstallCommand when the assembly is not in …
d11dddc 
…the GAC (#7003)

## Summary of changes

Fixes the return code for the `GacUninstallCommand` when the assembly is
not in the GAC

## Reason for change

If the assembly isn't in the GAC, we shouldn't return error when
uninstalling it, because the desired state is correct. This was changed
in #6995 but is an unintended behaviour change

## Implementation details

- Change the exit code `1` -> `0`
- Write a success message instead of a warning
- Fix typo 

## Test coverage

🙈 

## Other details

Currently some of the other "failure" messages are _effectively_ the
same thing, so it might make change to change those too. However, their
behaviour wasn't changed in #6995 so is non-critical
@andrewlock andrewlock added type:enhancement Improvement to an existing feature area:ci-visibility labels May 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewlock andrewlock andrewlock approved these changes

@lucaspimentel lucaspimentel lucaspimentel left review comments

Assignees

@tonyredondo tonyredondo

Labels
area:ci-visibility type:enhancement Improvement to an existing feature
Projects
None yet
Milestone
3.17.0
Development

Successfully merging this pull request may close these issues.
3 participants
@tonyredondo @andrewlock @lucaspimentel