-
Notifications
You must be signed in to change notification settings - Fork 147
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CI Visibility] Add Xunit.v3 support #6573
Conversation
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:
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 (6573) - mean (79ms) : 72, 86
. : milestone, 79,
master - mean (69ms) : 66, 72
. : milestone, 69,
section CallTarget+Inlining+NGEN
This PR (6573) - mean (1,028ms) : 921, 1135
. : milestone, 1028,
master - mean (990ms) : 968, 1011
. : milestone, 990,
gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6573) - mean (103ms) : 100, 105
. : milestone, 103,
master - mean (102ms) : 100, 105
. : milestone, 102,
section CallTarget+Inlining+NGEN
This PR (6573) - mean (674ms) : 656, 692
. : milestone, 674,
master - mean (675ms) : 660, 691
. : milestone, 675,
gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6573) - mean (90ms) : 88, 92
. : milestone, 90,
master - mean (93ms) : 83, 103
. : milestone, 93,
section CallTarget+Inlining+NGEN
This PR (6573) - mean (635ms) : 619, 650
. : milestone, 635,
master - mean (632ms) : 612, 651
. : milestone, 632,
gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6573) - mean (191ms) : 186, 196
. : milestone, 191,
master - mean (190ms) : 187, 193
. : milestone, 190,
section CallTarget+Inlining+NGEN
This PR (6573) - mean (1,109ms) : 1070, 1147
. : milestone, 1109,
master - mean (1,100ms) : 1068, 1131
. : milestone, 1100,
gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6573) - mean (271ms) : 266, 277
. : milestone, 271,
master - mean (269ms) : 265, 273
. : milestone, 269,
section CallTarget+Inlining+NGEN
This PR (6573) - mean (863ms) : 830, 897
. : milestone, 863,
master - mean (861ms) : 838, 884
. : milestone, 861,
gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6573) - mean (262ms) : 258, 266
. : milestone, 262,
master - mean (262ms) : 257, 267
. : milestone, 262,
section CallTarget+Inlining+NGEN
This PR (6573) - mean (841ms) : 801, 881
. : milestone, 841,
master - mean (843ms) : 811, 874
. : milestone, 843,
|
Benchmarks Report for tracer 🐌Benchmarks for #6573 compared to master:
The following thresholds were used for comparing the benchmark speeds:
Allocation changes below 0.5% are ignored. Benchmark detailsBenchmarks.Trace.ActivityBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.GraphQLBenchmark - Faster 🎉 Same allocations ✔️
|
Benchmark | base/diff | Base Median (ns) | Diff Median (ns) | Modality |
---|---|---|---|---|
Benchmarks.Trace.GraphQLBenchmark.ExecuteAsync‑net6.0 | 1.208 | 1,519.19 | 1,258.01 |
Raw results
Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
---|---|---|---|---|---|---|---|---|---|
master | ExecuteAsync |
net6.0 | 1.52μs | 0.958ns | 3.71ns | 0.013 | 0 | 0 | 952 B |
master | ExecuteAsync |
netcoreapp3.1 | 1.6μs | 0.69ns | 2.67ns | 0.0128 | 0 | 0 | 952 B |
master | ExecuteAsync |
net472 | 1.83μs | 0.234ns | 0.874ns | 0.145 | 0 | 0 | 915 B |
#6573 | ExecuteAsync |
net6.0 | 1.26μs | 0.453ns | 1.7ns | 0.0132 | 0 | 0 | 952 B |
#6573 | ExecuteAsync |
netcoreapp3.1 | 1.65μs | 0.611ns | 2.2ns | 0.0124 | 0 | 0 | 952 B |
#6573 | ExecuteAsync |
net472 | 1.83μs | 0.631ns | 2.44ns | 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.38μs | 1.72ns | 6.19ns | 0.0327 | 0 | 0 | 2.31 KB |
master | SendAsync |
netcoreapp3.1 | 5.36μs | 2.67ns | 10.3ns | 0.0376 | 0 | 0 | 2.85 KB |
master | SendAsync |
net472 | 7.44μs | 4.13ns | 15.4ns | 0.492 | 0 | 0 | 3.12 KB |
#6573 | SendAsync |
net6.0 | 4.49μs | 2.07ns | 8.03ns | 0.0315 | 0 | 0 | 2.31 KB |
#6573 | SendAsync |
netcoreapp3.1 | 5.24μs | 2.2ns | 8.24ns | 0.0395 | 0 | 0 | 2.85 KB |
#6573 | SendAsync |
net472 | 7.41μs | 3.1ns | 12ns | 0.496 | 0 | 0 | 3.12 KB |
Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
---|---|---|---|---|---|---|---|---|---|
master | EnrichedLog |
net6.0 | 1.54μs | 0.859ns | 3.1ns | 0.0231 | 0 | 0 | 1.64 KB |
master | EnrichedLog |
netcoreapp3.1 | 2.15μs | 0.703ns | 2.43ns | 0.0226 | 0 | 0 | 1.64 KB |
master | EnrichedLog |
net472 | 2.48μs | 0.699ns | 2.62ns | 0.249 | 0 | 0 | 1.57 KB |
#6573 | EnrichedLog |
net6.0 | 1.63μs | 0.867ns | 3.24ns | 0.0231 | 0 | 0 | 1.64 KB |
#6573 | EnrichedLog |
netcoreapp3.1 | 2.19μs | 1.96ns | 7.08ns | 0.0221 | 0 | 0 | 1.64 KB |
#6573 | EnrichedLog |
net472 | 2.57μs | 1.7ns | 6.57ns | 0.248 | 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 | 111μs | 127ns | 491ns | 0.0556 | 0 | 0 | 4.28 KB |
master | EnrichedLog |
netcoreapp3.1 | 116μs | 103ns | 384ns | 0.058 | 0 | 0 | 4.28 KB |
master | EnrichedLog |
net472 | 151μs | 373ns | 1.44μs | 0.681 | 0.227 | 0 | 4.46 KB |
#6573 | EnrichedLog |
net6.0 | 113μs | 87.8ns | 304ns | 0.0562 | 0 | 0 | 4.28 KB |
#6573 | EnrichedLog |
netcoreapp3.1 | 116μs | 136ns | 528ns | 0.0578 | 0 | 0 | 4.28 KB |
#6573 | EnrichedLog |
net472 | 150μs | 104ns | 389ns | 0.678 | 0.226 | 0 | 4.46 KB |
Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
---|---|---|---|---|---|---|---|---|---|
master | EnrichedLog |
net6.0 | 3.12μs | 3.28ns | 12.3ns | 0.0305 | 0 | 0 | 2.2 KB |
master | EnrichedLog |
netcoreapp3.1 | 4.25μs | 1.31ns | 5.07ns | 0.0298 | 0 | 0 | 2.2 KB |
master | EnrichedLog |
net472 | 4.94μs | 1.45ns | 5.62ns | 0.319 | 0 | 0 | 2.02 KB |
#6573 | EnrichedLog |
net6.0 | 3μs | 0.772ns | 2.99ns | 0.03 | 0 | 0 | 2.2 KB |
#6573 | EnrichedLog |
netcoreapp3.1 | 4.16μs | 1.66ns | 6.21ns | 0.0292 | 0 | 0 | 2.2 KB |
#6573 | EnrichedLog |
net472 | 5.02μs | 1.61ns | 6.24ns | 0.321 | 0 | 0 | 2.02 KB |
Benchmarks.Trace.RedisBenchmark - Faster 🎉 Same allocations ✔️
Faster 🎉 in #6573
Benchmark
base/diff
Base Median (ns)
Diff Median (ns)
Modality
Benchmarks.Trace.RedisBenchmark.SendReceive‑net6.0
1.138
1,452.72
1,277.01
Benchmark | base/diff | Base Median (ns) | Diff Median (ns) | Modality |
---|---|---|---|---|
Benchmarks.Trace.RedisBenchmark.SendReceive‑net6.0 | 1.138 | 1,452.72 | 1,277.01 |
Raw results
Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
---|---|---|---|---|---|---|---|---|---|
master | SendReceive |
net6.0 | 1.45μs | 1.79ns | 6.95ns | 0.0157 | 0 | 0 | 1.14 KB |
master | SendReceive |
netcoreapp3.1 | 1.75μs | 0.79ns | 3.06ns | 0.0154 | 0 | 0 | 1.14 KB |
master | SendReceive |
net472 | 2.13μs | 1.68ns | 6.52ns | 0.184 | 0 | 0 | 1.16 KB |
#6573 | SendReceive |
net6.0 | 1.28μs | 1ns | 3.89ns | 0.016 | 0 | 0 | 1.14 KB |
#6573 | SendReceive |
netcoreapp3.1 | 1.78μs | 0.816ns | 3.05ns | 0.0151 | 0 | 0 | 1.14 KB |
#6573 | SendReceive |
net472 | 2.05μs | 2.39ns | 9.25ns | 0.183 | 0 | 0 | 1.16 KB |
Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
---|---|---|---|---|---|---|---|---|---|
master | EnrichedLog |
net6.0 | 2.68μs | 0.678ns | 2.63ns | 0.0214 | 0 | 0 | 1.6 KB |
master | EnrichedLog |
netcoreapp3.1 | 3.74μs | 7.02ns | 27.2ns | 0.0224 | 0 | 0 | 1.65 KB |
master | EnrichedLog |
net472 | 4.48μs | 2.52ns | 9.74ns | 0.323 | 0 | 0 | 2.04 KB |
#6573 | EnrichedLog |
net6.0 | 2.73μs | 1.5ns | 5.82ns | 0.0219 | 0 | 0 | 1.6 KB |
#6573 | EnrichedLog |
netcoreapp3.1 | 3.95μs | 1.53ns | 5.92ns | 0.0217 | 0 | 0 | 1.65 KB |
#6573 | EnrichedLog |
net472 | 4.43μs | 4.14ns | 16ns | 0.323 | 0 | 0 | 2.04 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 | 447ns | 0.765ns | 2.96ns | 0.00807 | 0 | 0 | 576 B |
master | StartFinishSpan |
netcoreapp3.1 | 565ns | 0.832ns | 3.11ns | 0.0078 | 0 | 0 | 576 B |
master | StartFinishSpan |
net472 | 625ns | 1.81ns | 7ns | 0.0918 | 0 | 0 | 578 B |
master | StartFinishScope |
net6.0 | 489ns | 0.715ns | 2.77ns | 0.00968 | 0 | 0 | 696 B |
master | StartFinishScope |
netcoreapp3.1 | 688ns | 1.65ns | 6.4ns | 0.00937 | 0 | 0 | 696 B |
master | StartFinishScope |
net472 | 751ns | 1.82ns | 6.8ns | 0.105 | 0 | 0 | 658 B |
#6573 | StartFinishSpan |
net6.0 | 470ns | 0.62ns | 2.4ns | 0.00804 | 0 | 0 | 576 B |
#6573 | StartFinishSpan |
netcoreapp3.1 | 579ns | 0.83ns | 3.21ns | 0.00783 | 0 | 0 | 576 B |
#6573 | StartFinishSpan |
net472 | 651ns | 0.377ns | 1.41ns | 0.0916 | 0 | 0 | 578 B |
#6573 | StartFinishScope |
net6.0 | 497ns | 0.864ns | 3.35ns | 0.00974 | 0 | 0 | 696 B |
#6573 | StartFinishScope |
netcoreapp3.1 | 748ns | 0.847ns | 3.28ns | 0.00937 | 0 | 0 | 696 B |
#6573 | StartFinishScope |
net472 | 801ns | 0.438ns | 1.64ns | 0.104 | 0 | 0 | 658 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 | 645ns | 1.11ns | 4.3ns | 0.00974 | 0 | 0 | 696 B |
master | RunOnMethodBegin |
netcoreapp3.1 | 914ns | 1.54ns | 5.95ns | 0.00914 | 0 | 0 | 696 B |
master | RunOnMethodBegin |
net472 | 1.08μs | 2.66ns | 10.3ns | 0.104 | 0 | 0 | 658 B |
#6573 | RunOnMethodBegin |
net6.0 | 646ns | 0.392ns | 1.52ns | 0.00972 | 0 | 0 | 696 B |
#6573 | RunOnMethodBegin |
netcoreapp3.1 | 960ns | 0.446ns | 1.67ns | 0.00911 | 0 | 0 | 696 B |
#6573 | RunOnMethodBegin |
net472 | 1.07μs | 0.543ns | 2.1ns | 0.104 | 0 | 0 | 658 B |
tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/Testing/XUnit/ITestCase.cs
Outdated
Show resolved
Hide resolved
tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/Testing/XUnit/RetryMessageBus.cs
Outdated
Show resolved
Hide resolved
...c/Datadog.Trace/ClrProfiler/AutoInstrumentation/Testing/XUnit/V3/AssemblyMetadataStructV3.cs
Show resolved
Hide resolved
tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/Testing/XUnit/V3/IXunitTestCaseV3.cs
Outdated
Show resolved
Hide resolved
...ofiler/AutoInstrumentation/Testing/XUnit/V3/XunitTestMethodRunnerContextCtorV3Integration.cs
Outdated
Show resolved
Hide resolved
/// Duck type task interface | ||
/// </summary> | ||
/// <typeparam name="T">Type of the result</typeparam> | ||
public interface IDuckTypeTask<out T> : IDuckType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it is pretty much a direct overlap with #6480... I feel like we should maybe try to unify those with these at least? 🤔 Not in this PR, just a thought.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we need to do that in a different PR
|
||
#if NET6_0_OR_GREATER | ||
[UsesVerify] | ||
public class XUnitEvpTestsV3 : TestingFrameworkEvpTest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing there's a lot of duplication here with the xunit tests, right? Rather than duplicate 500 lines of test code, can we derive one from the other or something? FWIW, it feels like a huge number of the asserts are essentially already covered by the snapshots, no? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, I'll work on that in a different PR, because it doesn't affect only xUnit v3 support. It will need a complete refactor of the tests.
tracer/test/Datadog.Trace.TestHelpers.AutoInstrumentation/TestHelper.cs
Outdated
Show resolved
Hide resolved
@@ -43,7 +43,7 @@ internal static CallTargetState OnMethodBegin<TTarget, TContext>(TTarget instanc | |||
var testBundleString = new AssemblyName(assemblyName).Name ?? string.Empty; | |||
|
|||
// Extract the version of the framework from the TestClassRunner base class | |||
var frameworkType = instance.GetType(); | |||
var frameworkType = instance!.GetType(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha, I guess this is a consequence of me suggesting removing the instance
check 😅 can probably fix it with it a where TTarget : notnull
, but whether we want/should do that is a different question 😅 I still think you should have a Sorry, you did in the next commit!context.Instance is null
check, but I don't know...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Looks like v1.1.0 came out a few days ago, but I think this should be fine, not seeing anything in it.
|
||
public ref TReturn? GetInternalDuckTypedInstance<TReturn>() | ||
{ | ||
throw new NotImplementedException(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering if this is intentional
Summary of changes
This PR adds support for xUnit.v3
Reason for change
We were missing the support for this new xUnit nuget package
Implementation details
It's a new integration that uses some of the existing xUnit integration code.
This PR also contains:
dotnet exec
Test coverage
Two new test classes were added to test Evp Proxy, EFD and ATR.
Other details