Add CallTarget support for `ValueTask` in .NET FX and < .NET Core 3.1 #6480

andrewlock · 2024-12-23T15:19:31Z

Summary of changes

Add support for correct CallTarget instrumentation of methods that return ValueTask in < .NET Core 3.1 or .NET Framework

Reason for change

We already support instrumenting methods that returns ValueTask in .NET Core 3.1. However, in .NET Framework or .NET Standard 2.0, this support is provided by a package, and we currently don't support instrumenting these methods. Or rather, we just ignore the OnAsyncMethodEnd in integrations in these cases.

Implementation details

We already support ValueTask in more recent frameworks, and the support is very similar to our Task support. Unfortunately, in .NET FX we can't reference the ValueTask type itself.

To work around this, we do the following:

Detect that the return type is ValueTask or ValueTask<T> either by loading the type directly (.NET Core) or checking the type name (.NET Framework)
Duck-type the ValueTask to read the IsCompletedSuccessfully value.
- If this is true, the task is already completed synchronously, and we can simply call the target method.
- For ValueTask<T> we duck type Result and read that directly too.
- If it hasn't completed, we need to extract the Task from it.
Duck typing is used again to extract the Task() for uncompleted ValueTask
- At this point, it's mostly a copy-paste of the existing Task integrations
Once the integration returns, we need to create a "new" ValueTask instead from the previous one
- The semantics of ValueTask require that we create a "fresh" one, we can't just "reuse" the one we got originally, because we've already retrieved the result/ awaited the inner task
- Have to use Activator/DynamicMethod for this

Warning

The existing ValueTask/ValueTask<> and Task/Task<> integrations are written quite differently, and I'm not entirely sure why 🤔 Given the ContinuationAction() methods for both these cases operate on Task, I based on the new ValueTask integrations on the Task integrations, but if anyone has reasons why it shouldn't be, I'm all ears!

Test coverage

Added Task and ValueTask tests to the CallTargetNativeTests integration tests. Previously we were only testing a single Task example, and that was somewhat insufficient
Update the CallTargetNativeTests to explicitly assert that the OnAsyncMethodEnd methods are called for Task / ValueTask method integrations. As we provide both methods in our target integration, we were silently calling the wrong one for .NET FX
- Prior to the fix in this PR, these updated tests would fail on < .NET Core 3.0 and .NET FX
Run the ValueTaskAsyncContinutationGenerator unit tests on all TFMs, not just .NET Core 3.1
Unit tests for the ValueTaskHelper for checking if a type is a ValueTask
Unit tests for the ValueTaskActivator for creating a ValueTask from a Task or Task<T>
Verified it fixes the issues I was seeing in the RabbitMQ integration
- Add support for RabbitMQ v7 #6479

Other details

Required for

Add support for RabbitMQ v7 #6479

datadog-ddstaging · 2024-12-23T15:40:40Z

Datadog Report

Branch report: andrew/value-task-support
Commit report: a73c2c7
Test service: dd-trace-dotnet

✅ 0 Failed, 234987 Passed, 2074 Skipped, 18h 31m 10.03s Total Time

andrewlock · 2024-12-23T15:53:26Z

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 (6480) - mean (68ms)  : 66, 71
     .   : milestone, 68,
    master - mean (69ms)  : 66, 72
     .   : milestone, 69,

    section CallTarget+Inlining+NGEN
    This PR (6480) - mean (978ms)  : 955, 1002
     .   : milestone, 978,
    master - mean (981ms)  : 954, 1008
     .   : milestone, 981,

gantt
    title Execution time (ms) FakeDbCommand (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6480) - mean (107ms)  : 105, 110
     .   : milestone, 107,
    master - mean (107ms)  : 105, 110
     .   : milestone, 107,

    section CallTarget+Inlining+NGEN
    This PR (6480) - mean (676ms)  : 661, 690
     .   : milestone, 676,
    master - mean (678ms)  : 662, 695
     .   : milestone, 678,

gantt
    title Execution time (ms) FakeDbCommand (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6480) - mean (91ms)  : 89, 93
     .   : milestone, 91,
    master - mean (91ms)  : 90, 93
     .   : milestone, 91,

    section CallTarget+Inlining+NGEN
    This PR (6480) - mean (630ms)  : 613, 647
     .   : milestone, 630,
    master - mean (633ms)  : 616, 651
     .   : milestone, 633,

gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6480) - mean (194ms)  : 190, 198
     .   : milestone, 194,
    master - mean (195ms)  : 189, 202
     .   : milestone, 195,

    section CallTarget+Inlining+NGEN
    This PR (6480) - mean (1,099ms)  : 1064, 1133
     .   : milestone, 1099,
    master - mean (1,104ms)  : 1076, 1132
     .   : milestone, 1104,

gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6480) - mean (277ms)  : 272, 282
     .   : milestone, 277,
    master - mean (278ms)  : 273, 282
     .   : milestone, 278,

    section CallTarget+Inlining+NGEN
    This PR (6480) - mean (869ms)  : 838, 901
     .   : milestone, 869,
    master - mean (874ms)  : 847, 901
     .   : milestone, 874,

gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6480) - mean (267ms)  : 264, 270
     .   : milestone, 267,
    master - mean (268ms)  : 264, 272
     .   : milestone, 268,

    section CallTarget+Inlining+NGEN
    This PR (6480) - mean (848ms)  : 815, 881
     .   : milestone, 848,
    master - mean (857ms)  : 829, 885
     .   : milestone, 857,

andrewlock · 2024-12-23T20:53:25Z

Benchmarks Report for tracer 🐌

Benchmarks for #6480 compared to master:

1 benchmarks are faster, with geometric mean 1.127
4 benchmarks are slower, with geometric mean 1.153
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	8.11μs	45.6ns	329ns	0.0166	0.0083	0	5.61 KB
master	`StartStopWithChild`	netcoreapp3.1	10.2μs	50ns	320ns	0.0157	0.00524	0	5.8 KB
master	`StartStopWithChild`	net472	16.6μs	67.3ns	261ns	1.06	0.331	0.0889	6.21 KB
#6480	`StartStopWithChild`	net6.0	8.07μs	45.1ns	299ns	0.0132	0.00439	0	5.61 KB
#6480	`StartStopWithChild`	netcoreapp3.1	10.2μs	55.9ns	311ns	0.0155	0.00518	0	5.8 KB
#6480	`StartStopWithChild`	net472	16.5μs	38.6ns	150ns	1.04	0.306	0.0909	6.22 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`WriteAndFlushEnrichedTraces`	net6.0	480μs	324ns	1.26μs	0	2.7 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	654μs	328ns	1.18μs	0	2.7 KB
master	`WriteAndFlushEnrichedTraces`	net472	855μs	841ns	3.26μs	0.422	3.3 KB
#6480	`WriteAndFlushEnrichedTraces`	net6.0	473μs	305ns	1.14μs	0	2.7 KB
#6480	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	641μs	404ns	1.46μs	0	2.7 KB
#6480	`WriteAndFlushEnrichedTraces`	net472	854μs	671ns	2.6μs	0.425	3.3 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendRequest`	net6.0	156μs	957ns	9.52μs	0.152	14.47 KB
master	`SendRequest`	netcoreapp3.1	172μs	1.01μs	9.35μs	0.165	17.27 KB
master	`SendRequest`	net472	0.00178ns	0.000725ns	0.00281ns	0	0 b
#6480	`SendRequest`	net6.0	151μs	834ns	6.82μs	0.151	14.47 KB
#6480	`SendRequest`	netcoreapp3.1	175μs	1.02μs	9.92μs	0.2	17.27 KB
#6480	`SendRequest`	net472	0.00153ns	0.00108ns	0.00389ns	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	581μs	3.27μs	22μs	0.587	0	0	41.6 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	671μs	3.72μs	22μs	0.322	0	0	41.7 KB
master	`WriteAndFlushEnrichedTraces`	net472	867μs	3.9μs	15.1μs	8.19	2.59	0.431	53.33 KB
#6480	`WriteAndFlushEnrichedTraces`	net6.0	558μs	2.86μs	14.3μs	0.576	0	0	41.6 KB
#6480	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	651μs	3.3μs	16.2μs	0.343	0	0	41.66 KB
#6480	`WriteAndFlushEnrichedTraces`	net472	828μs	3.56μs	14.7μs	8.17	2.45	0.408	53.26 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`ExecuteNonQuery`	net6.0	1.29μs	1.29ns	4.84ns	0.014	0	1.02 KB
master	`ExecuteNonQuery`	netcoreapp3.1	1.8μs	1.33ns	4.97ns	0.0135	0	1.02 KB
master	`ExecuteNonQuery`	net472	2.13μs	2.18ns	8.44ns	0.157	0.00106	987 B
#6480	`ExecuteNonQuery`	net6.0	1.22μs	1.26ns	4.71ns	0.0141	0	1.02 KB
#6480	`ExecuteNonQuery`	netcoreapp3.1	1.78μs	2.02ns	7.54ns	0.0136	0	1.02 KB
#6480	`ExecuteNonQuery`	net472	2.1μs	2.05ns	7.68ns	0.156	0.00104	987 B

Benchmarks.Trace.ElasticsearchBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #6480

Benchmark	diff/base	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.ElasticsearchBenchmark.CallElasticsearch‑net6.0	1.161	1,158.77	1,345.44

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`CallElasticsearch`	net6.0	1.16μs	0.665ns	2.49ns	0.0139	976 B
master	`CallElasticsearch`	netcoreapp3.1	1.54μs	1.66ns	6.21ns	0.0132	976 B
master	`CallElasticsearch`	net472	2.53μs	2.45ns	9.16ns	0.158	995 B
master	`CallElasticsearchAsync`	net6.0	1.23μs	0.618ns	2.31ns	0.013	952 B
master	`CallElasticsearchAsync`	netcoreapp3.1	1.69μs	0.65ns	2.43ns	0.0136	1.02 KB
master	`CallElasticsearchAsync`	net472	2.68μs	1.79ns	6.94ns	0.167	1.05 KB
#6480	`CallElasticsearch`	net6.0	1.35μs	0.508ns	1.97ns	0.0133	976 B
#6480	`CallElasticsearch`	netcoreapp3.1	1.54μs	1.09ns	4.07ns	0.013	976 B
#6480	`CallElasticsearch`	net472	2.58μs	1.72ns	6.2ns	0.158	995 B
#6480	`CallElasticsearchAsync`	net6.0	1.25μs	0.464ns	1.8ns	0.0132	952 B
#6480	`CallElasticsearchAsync`	netcoreapp3.1	1.58μs	0.774ns	2.9ns	0.0134	1.02 KB
#6480	`CallElasticsearchAsync`	net472	2.68μs	1.5ns	5.6ns	0.167	1.05 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`ExecuteAsync`	net6.0	1.28μs	0.864ns	3.23ns	0.0134	952 B
master	`ExecuteAsync`	netcoreapp3.1	1.65μs	0.581ns	2.18ns	0.0124	952 B
master	`ExecuteAsync`	net472	1.82μs	1.09ns	3.78ns	0.145	915 B
#6480	`ExecuteAsync`	net6.0	1.36μs	1.24ns	4.65ns	0.0134	952 B
#6480	`ExecuteAsync`	netcoreapp3.1	1.57μs	0.677ns	2.53ns	0.0126	952 B
#6480	`ExecuteAsync`	net472	1.76μs	0.614ns	2.3ns	0.145	915 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendAsync`	net6.0	4.45μs	1.73ns	6.68ns	0.0315	2.31 KB
master	`SendAsync`	netcoreapp3.1	5.29μs	2.82ns	10.9ns	0.0371	2.85 KB
master	`SendAsync`	net472	7.36μs	3.05ns	11ns	0.494	3.12 KB
#6480	`SendAsync`	net6.0	4.45μs	1.88ns	7.28ns	0.031	2.31 KB
#6480	`SendAsync`	netcoreapp3.1	5.24μs	1.21ns	4.54ns	0.0367	2.85 KB
#6480	`SendAsync`	net472	7.35μs	1.71ns	6.62ns	0.494	3.12 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net6.0	1.54μs	0.653ns	2.45ns	0.0231	1.64 KB
master	`EnrichedLog`	netcoreapp3.1	2.09μs	0.997ns	3.73ns	0.0225	1.64 KB
master	`EnrichedLog`	net472	2.69μs	2.29ns	8.57ns	0.249	1.57 KB
#6480	`EnrichedLog`	net6.0	1.53μs	2.21ns	8.57ns	0.0232	1.64 KB
#6480	`EnrichedLog`	netcoreapp3.1	2.22μs	1.22ns	4.73ns	0.0216	1.64 KB
#6480	`EnrichedLog`	net472	2.72μs	1.61ns	6.23ns	0.248	1.57 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EnrichedLog`	net6.0	118μs	195ns	756ns	0	0	4.28 KB
master	`EnrichedLog`	netcoreapp3.1	120μs	164ns	635ns	0.0603	0	4.28 KB
master	`EnrichedLog`	net472	151μs	173ns	648ns	0.671	0.224	4.46 KB
#6480	`EnrichedLog`	net6.0	119μs	258ns	1E+03ns	0.0593	0	4.28 KB
#6480	`EnrichedLog`	netcoreapp3.1	120μs	210ns	814ns	0	0	4.28 KB
#6480	`EnrichedLog`	net472	152μs	120ns	465ns	0.684	0.228	4.46 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net6.0	2.92μs	1.13ns	4.38ns	0.0306	2.2 KB
master	`EnrichedLog`	netcoreapp3.1	4.2μs	1.84ns	6.88ns	0.0294	2.2 KB
master	`EnrichedLog`	net472	4.88μs	1.69ns	6.54ns	0.319	2.02 KB
#6480	`EnrichedLog`	net6.0	3.12μs	3.15ns	12.2ns	0.0308	2.2 KB
#6480	`EnrichedLog`	netcoreapp3.1	4.42μs	1.82ns	7.05ns	0.0287	2.2 KB
#6480	`EnrichedLog`	net472	4.89μs	0.664ns	2.57ns	0.319	2.02 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendReceive`	net6.0	1.41μs	0.818ns	3.17ns	0.0162	1.14 KB
master	`SendReceive`	netcoreapp3.1	1.76μs	0.633ns	2.28ns	0.0152	1.14 KB
master	`SendReceive`	net472	2.13μs	1.58ns	5.92ns	0.184	1.16 KB
#6480	`SendReceive`	net6.0	1.37μs	0.634ns	2.29ns	0.0158	1.14 KB
#6480	`SendReceive`	netcoreapp3.1	1.8μs	0.627ns	2.35ns	0.0152	1.14 KB
#6480	`SendReceive`	net472	2.16μs	0.993ns	3.58ns	0.183	1.16 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net6.0	2.79μs	0.881ns	3.3ns	0.0223	1.6 KB
master	`EnrichedLog`	netcoreapp3.1	3.95μs	1.6ns	6.19ns	0.0219	1.65 KB
master	`EnrichedLog`	net472	4.24μs	3.28ns	11.8ns	0.322	2.04 KB
#6480	`EnrichedLog`	net6.0	2.77μs	1.7ns	6.37ns	0.022	1.6 KB
#6480	`EnrichedLog`	netcoreapp3.1	3.86μs	2.39ns	9.26ns	0.0213	1.65 KB
#6480	`EnrichedLog`	net472	4.5μs	3.32ns	12.9ns	0.322	2.04 KB

Benchmarks.Trace.SpanBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #6480

Benchmark	diff/base	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑netcoreapp3.1	1.206	538.86	650.01
Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑net6.0	1.126	402.00	452.76

Faster 🎉 in #6480

Benchmark	base/diff	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.SpanBenchmark.StartFinishScope‑net6.0	1.127	562.66	499.30

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`StartFinishSpan`	net6.0	402ns	0.511ns	1.98ns	0.0081	576 B
master	`StartFinishSpan`	netcoreapp3.1	539ns	0.687ns	2.66ns	0.00777	576 B
master	`StartFinishSpan`	net472	650ns	1.43ns	5.56ns	0.0917	578 B
master	`StartFinishScope`	net6.0	563ns	0.836ns	3.24ns	0.00971	696 B
master	`StartFinishScope`	netcoreapp3.1	713ns	1.11ns	4.31ns	0.00965	696 B
master	`StartFinishScope`	net472	885ns	1.17ns	4.53ns	0.104	658 B
#6480	`StartFinishSpan`	net6.0	453ns	0.36ns	1.3ns	0.00806	576 B
#6480	`StartFinishSpan`	netcoreapp3.1	650ns	1.05ns	4.08ns	0.0077	576 B
#6480	`StartFinishSpan`	net472	617ns	1.16ns	4.49ns	0.0918	578 B
#6480	`StartFinishScope`	net6.0	499ns	0.459ns	1.78ns	0.00969	696 B
#6480	`StartFinishScope`	netcoreapp3.1	677ns	1.08ns	4.18ns	0.00915	696 B
#6480	`StartFinishScope`	net472	830ns	1.47ns	5.7ns	0.104	658 B

Benchmarks.Trace.TraceAnnotationsBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #6480

Benchmark	diff/base	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.TraceAnnotationsBenchmark.RunOnMethodBegin‑net6.0	1.120	600.43	672.69

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`RunOnMethodBegin`	net6.0	600ns	1.14ns	4.4ns	0.00973	696 B
master	`RunOnMethodBegin`	netcoreapp3.1	896ns	1.28ns	4.96ns	0.00933	696 B
master	`RunOnMethodBegin`	net472	1.09μs	1.93ns	7.46ns	0.104	658 B
#6480	`RunOnMethodBegin`	net6.0	671ns	1.61ns	6.03ns	0.00985	696 B
#6480	`RunOnMethodBegin`	netcoreapp3.1	993ns	1.02ns	3.97ns	0.00935	696 B
#6480	`RunOnMethodBegin`	net472	1.13μs	1.7ns	6.59ns	0.104	658 B

andrewlock · 2025-01-02T11:48:22Z

Throughput/Crank Report ⚡

Throughput results for AspNetCoreSimpleController comparing the following branches/commits:

Cases where throughput results for the PR are worse than latest master (5% drop or greater), results are shown in red.

Note that these results are based on a single point-in-time result for each branch. For full results, see one of the many, many dashboards!

gantt
    title Throughput Linux x64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (6480) (11.113M)   : 0, 11112891
    master (11.278M)   : 0, 11278464
    benchmarks/2.9.0 (11.144M)   : 0, 11143547

    section Automatic
    This PR (6480) (7.431M)   : 0, 7431434
    master (7.310M)   : 0, 7310284
    benchmarks/2.9.0 (7.982M)   : 0, 7981525

    section Trace stats
    master (7.466M)   : 0, 7466025

    section Manual
    master (11.098M)   : 0, 11098414

    section Manual + Automatic
    This PR (6480) (6.769M)   : 0, 6769440
    master (6.596M)   : 0, 6596135

    section DD_TRACE_ENABLED=0
    master (10.177M)   : 0, 10177094

gantt
    title Throughput Linux arm64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (6480) (9.563M)   : 0, 9563388
    master (9.615M)   : 0, 9615070
    benchmarks/2.9.0 (9.621M)   : 0, 9621397

    section Automatic
    This PR (6480) (6.501M)   : 0, 6500795
    master (6.452M)   : 0, 6452148

    section Trace stats
    master (6.369M)   : 0, 6369273

    section Manual
    master (9.539M)   : 0, 9539051

    section Manual + Automatic
    This PR (6480) (5.924M)   : 0, 5924116
    master (6.003M)   : 0, 6003427

    section DD_TRACE_ENABLED=0
    master (8.846M)   : 0, 8846180

gantt
    title Throughput Windows x64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (6480) (10.316M)   : 0, 10316293
    master (9.954M)   : 0, 9954268

    section Automatic
    This PR (6480) (6.707M)   : 0, 6706701
    master (6.674M)   : 0, 6674018

    section Trace stats
    master (7.283M)   : 0, 7282769

    section Manual
    master (10.371M)   : 0, 10370838

    section Manual + Automatic
    This PR (6480) (6.238M)   : 0, 6237748
    master (6.188M)   : 0, 6187851

    section DD_TRACE_ENABLED=0
    master (9.569M)   : 0, 9568617

kevingosse · 2025-01-03T14:10:25Z

The existing ValueTask/ValueTask<> and Task/Task<> integrations are written quite differently, and I'm not entirely sure why 🤔 Given the ContinuationAction() methods for both these cases operate on Task, I based on the new ValueTask integrations on the Task integrations, but if anyone has reasons why it shouldn't be, I'm all ears!

Initially we wrote the Task integration in such a way to avoid declaring ContinuationAction as async (to save the cost of the state-machine). Eventually we ran into an issue (#1634) and I rewrote it to async. While doing so, I added some optimizations to avoid throwing an exception whenever possible. Looks like I didn't port those optimizations to the ValueTask implementation. I don't know if I just forgot or didn't think it was worth it, or if there was a technical reason 🤔

tonyredondo · 2025-01-08T15:30:11Z

...race/ClrProfiler/CallTarget/Handlers/Continuations/ValueTaskContinuationGenerator`1.NetFx.cs

+                var unwrappedResult = previousValueTask.Result;
+                _continuation(instance, unwrappedResult, exception, in state);
+                return returnValue;


My only concern is that here we are consuming the result of the value task, and returning the same value task. I guess it's ok, but looks like a double usage.

oooh, yeah, good spot, that's not OK:

Awaiting the instance multiple times.

Calling AsTask multiple times.

Using .Result or .GetAwaiter().GetResult() when the operation hasn't yet completed, or using them multiple times.

Using more than one of these techniques to consume the instance.

I think we need to create a new instance here, e.g.

Suggested change

var unwrappedResult = previousValueTask.Result;

_continuation(instance, unwrappedResult, exception, in state);

return returnValue;

var unwrappedResult = previousValueTask.Result;

_continuation(instance, unwrappedResult, exception, in state);

return ValueTaskActivator<TReturn, TResult?>.CreateInstance(unwrappedResult);

(above doesn't compile yet, need to expose the CreateInstance that takes a result)

Fixed in a73c2c7

## Summary of changes Adds support for RabbitMQ v7 ## Reason for change We want to support the latest versions of packages. 7.x.x was released ~6 weeks ago, with _significant_ changes to both the public API and the internal API (which is probably why it hasn't been explicitly requested yet). ## Implementation details This one took a _lot_ of work. In summary: - Basically all the integrations had to be rewritten to work with the new async APIs - Where possible, simply called into the original sync integration (other than where the work is trivial) - Some challenges, in that the duck types had to change. - Most problematic was the `BasicProperties` because now `IReadOnlyBasicProperties` is null, which means we can't easily add the headers if they're not provided (to propagate context). - Worked around it with an additional instrumentation, but I'm not super happy about it 😅 - Had to completely rewrite the sample app to use the new async APIs - Originally I tried to `#if RABBITMQ_7_0` my way in the existing methods, but was impossible to follow, so split into -pre and -post v7 methods instead. - Tweaked some of the pre-V7 methods in the sample to be async to improve reuse ## Test coverage We get the same overall snapshots for v7 as we do for v6, so 🤞 Did an "all TFMs, all versions" run [here](https://dev.azure.com/datadoghq/dd-trace-dotnet/_build/results?buildId=170428&view=results) (and a previous one for DSM) ## Other details Stacked on: - #6463 - #6480 > [!NOTE] > This PR actually highlighted a current limitation in our CallTarget instrumentation. If an API uses `ValueTask`, which comes from a _library_ rather than being built-in (i.e. it's netfx or < .NET Core 3.1) then we _don't_ instrument it correctly. Originally I worked around it, but there's a "proper" fix in #6480. Assuming that actually works and is merged, then we don't need the workaround, otherwise I can re-enable it.

andrewlock requested a review from a team as a code owner December 23, 2024 15:19

andrewlock added area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) area:automatic-instrumentation Automatic instrumentation managed C# code (Datadog.Trace.ClrProfiler.Managed) labels Dec 23, 2024

andrewlock requested review from tonyredondo and kevingosse December 23, 2024 15:20

andrewlock mentioned this pull request Dec 23, 2024

Add support for RabbitMQ v7 #6479

Merged

andrewlock force-pushed the andrew/value-task-support branch from ffc922e to 457a93f Compare December 23, 2024 17:06

andrewlock force-pushed the andrew/value-task-support branch from 457a93f to 6c8dbe5 Compare December 30, 2024 09:47

kevingosse approved these changes Jan 3, 2025

View reviewed changes

tonyredondo approved these changes Jan 8, 2025

View reviewed changes

tonyredondo reviewed Jan 8, 2025

View reviewed changes

andrewlock added 4 commits January 8, 2025 17:30

Update CallTargetNativeTests to test Task and ValueTask scenarios

a85161a

Add support for ValueTask instrumentations for < NET Core 3.1

b022a2a

Base on Task instead of ValueTask

c96ac50

Don't reuse a ValueTask

a73c2c7

andrewlock force-pushed the andrew/value-task-support branch from 6c8dbe5 to a73c2c7 Compare January 8, 2025 17:33

andrewlock merged commit 2fbf8d0 into master Jan 9, 2025
60 of 63 checks passed

andrewlock deleted the andrew/value-task-support branch January 9, 2025 15:16

github-actions bot added this to the vNext-v3 milestone Jan 9, 2025

andrewlock mentioned this pull request Feb 5, 2025

[CI Visibility] Add Xunit.v3 support #6573

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CallTarget support for `ValueTask` in .NET FX and < .NET Core 3.1 #6480

Add CallTarget support for `ValueTask` in .NET FX and < .NET Core 3.1 #6480

andrewlock commented Dec 23, 2024

datadog-ddstaging bot commented Dec 23, 2024 •

edited

Loading

andrewlock commented Dec 23, 2024 •

edited

Loading

andrewlock commented Dec 23, 2024 •

edited

Loading

Raw results

Raw results

Raw results

Raw results

Raw results

Slower ⚠️ in #6480

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Slower ⚠️ in #6480

Faster 🎉 in #6480

Raw results

Slower ⚠️ in #6480

Raw results

andrewlock commented Jan 2, 2025

kevingosse commented Jan 3, 2025

tonyredondo Jan 8, 2025

andrewlock Jan 8, 2025

andrewlock Jan 8, 2025

Add CallTarget support for ValueTask in .NET FX and < .NET Core 3.1 #6480

Add CallTarget support for ValueTask in .NET FX and < .NET Core 3.1 #6480

Conversation

andrewlock commented Dec 23, 2024

Summary of changes

Reason for change

Implementation details

Test coverage

Other details

datadog-ddstaging bot commented Dec 23, 2024 • edited Loading

Datadog Report

andrewlock commented Dec 23, 2024 • edited Loading

Execution-Time Benchmarks Report ⏱️

andrewlock commented Dec 23, 2024 • edited Loading

Benchmarks Report for tracer 🐌

Benchmark details

Raw results

Raw results

Raw results

Raw results

Raw results

Slower ⚠️ in #6480

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Slower ⚠️ in #6480

Faster 🎉 in #6480

Raw results

Slower ⚠️ in #6480

Raw results

andrewlock commented Jan 2, 2025

Throughput/Crank Report ⚡

kevingosse commented Jan 3, 2025

tonyredondo Jan 8, 2025

Choose a reason for hiding this comment

andrewlock Jan 8, 2025

Choose a reason for hiding this comment

andrewlock Jan 8, 2025

Choose a reason for hiding this comment

Add CallTarget support for `ValueTask` in .NET FX and < .NET Core 3.1 #6480

Add CallTarget support for `ValueTask` in .NET FX and < .NET Core 3.1 #6480

datadog-ddstaging bot commented Dec 23, 2024 •

edited

Loading

andrewlock commented Dec 23, 2024 •

edited

Loading

andrewlock commented Dec 23, 2024 •

edited

Loading