Closed lucaspimentel closed 2 months ago
datadog-ddstaging[bot]
commented
2 months ago Branch report: lpimentel/backport-pr5910-jira
Commit report: c0ab9ed
Test service: dd-trace-dotnet
:white_check_mark: 0 Failed, 352467 Passed, 2263 Skipped, 23h 8m 8.49s Total Time
andrewlock
commented
2 months ago 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:
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 (5934) - mean (75ms) : 63, 87
. : milestone, 75,
section CallTarget+Inlining+NGEN
This PR (5934) - mean (1,036ms) : 1001, 1071
. : milestone, 1036,
gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5934) - mean (111ms) : 106, 116
. : milestone, 111,
section CallTarget+Inlining+NGEN
This PR (5934) - mean (721ms) : 699, 742
. : milestone, 721,
gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5934) - mean (94ms) : 90, 98
. : milestone, 94,
section CallTarget+Inlining+NGEN
This PR (5934) - mean (671ms) : 652, 691
. : milestone, 671,
gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5934) - mean (190ms) : 187, 193
. : milestone, 190,
section CallTarget+Inlining+NGEN
This PR (5934) - mean (1,115ms) : 1086, 1143
. : milestone, 1115,
gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5934) - mean (275ms) : 271, 279
. : milestone, 275,
section CallTarget+Inlining+NGEN
This PR (5934) - mean (879ms) : 856, 903
. : milestone, 879,
gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5934) - mean (265ms) : 260, 270
. : milestone, 265,
section CallTarget+Inlining+NGEN
This PR (5934) - mean (867ms) : 849, 884
. : milestone, 867,
Benchmarks for #5934 compared to master:
The following thresholds were used for comparing the benchmark speeds:
Allocation changes below 0.5% are ignored.
Summary of changes
For automatic instrumentation, the tracer inserts a startup hook inside the first method that we deem as a "valid" startup hook callsite. This change adds logic so that methods inside the
Costura.AssemblyLoadertype are not "valid" startup hook callsites, so that the insertion of the startup hook can be delayed to a later method call.Reason for change
We've seen a case where a WCF client application uses Costura.Fody, and when instrumentation is added to the application it begins running into assembly loading issues with the following exception:
The issue, it appears, is that the Datadog tracing startup hook is being inserted into either the
Costura.AssemblyLoader..cctor()or theCostura.AssemblyLoader.Attach()methods, which Costura adds into the module initializer of the target assembly. These methods add an AssemblyResolve event handler so that the application can load dependent assemblies from the assembly resources, since they are no longer on disk. Since the tracing startup hook also adds an AssemblyResolve event handler, I experimented with delaying the startup hook insertion so that the Costura AssemblyResolve event handler would be added first and it....worked.Implementation details
Adds more logic to the startup hook logic to check against the
Costura.AssemblyLoadertype when considering if a callsite is valid for startup hook insertion.Test coverage
I was able to reproduce the issue by:
binding=basicHttpsBinding(and using a port on my machine with proper HTTPS permissions)Costura.Fodyv4.0.0 NuGet package to the client application with an empty FodyWeavers.xmlBefore the changes, the application would crash with the above error. After the changes, the application runs without issue.
If needed, I can check in a smoke test but to avoid repository bloat I figured the step-by-step above would be sufficient.
Other details
Internal ticket: APMS-12728 Backporting #5910 from 3.x to 2.x