-
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
Add support for [DuckPropertyOrField]
#6463
Conversation
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 one nit.
tracer/test/Datadog.Trace.DuckTyping.Tests/PropertyOrFieldTests.cs
Outdated
Show resolved
Hide resolved
f390184
to
1baeece
Compare
Datadog ReportBranch report: ✅ 0 Failed, 455578 Passed, 2806 Skipped, 19h 29m 33.97s Total Time |
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 (6463) - mean (69ms) : 65, 73
. : milestone, 69,
master - mean (68ms) : 66, 71
. : milestone, 68,
section CallTarget+Inlining+NGEN
This PR (6463) - mean (980ms) : 953, 1007
. : milestone, 980,
master - mean (974ms) : 954, 995
. : milestone, 974,
gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6463) - mean (107ms) : 105, 109
. : milestone, 107,
master - mean (107ms) : 105, 110
. : milestone, 107,
section CallTarget+Inlining+NGEN
This PR (6463) - mean (679ms) : 664, 694
. : milestone, 679,
master - mean (676ms) : 665, 688
. : milestone, 676,
gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6463) - mean (91ms) : 90, 93
. : milestone, 91,
master - mean (91ms) : 89, 93
. : milestone, 91,
section CallTarget+Inlining+NGEN
This PR (6463) - mean (633ms) : 611, 654
. : milestone, 633,
master - mean (631ms) : 611, 650
. : milestone, 631,
gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6463) - mean (193ms) : 189, 197
. : milestone, 193,
master - mean (194ms) : 189, 199
. : milestone, 194,
section CallTarget+Inlining+NGEN
This PR (6463) - mean (1,100ms) : 1072, 1127
. : milestone, 1100,
master - mean (1,102ms) : 1070, 1133
. : milestone, 1102,
gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6463) - mean (279ms) : 275, 284
. : milestone, 279,
master - mean (279ms) : 274, 284
. : milestone, 279,
section CallTarget+Inlining+NGEN
This PR (6463) - mean (872ms) : 843, 902
. : milestone, 872,
master - mean (872ms) : 839, 905
. : milestone, 872,
gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6463) - mean (268ms) : 263, 272
. : milestone, 268,
master - mean (267ms) : 263, 271
. : milestone, 267,
section CallTarget+Inlining+NGEN
This PR (6463) - mean (850ms) : 817, 883
. : milestone, 850,
master - mean (851ms) : 812, 890
. : milestone, 851,
|
Benchmarks Report for tracer 🐌Benchmarks for #6463 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 - Slower
|
Benchmark | diff/base | Base Median (ns) | Diff Median (ns) | Modality |
---|---|---|---|---|
Benchmarks.Trace.ElasticsearchBenchmark.CallElasticsearch‑net6.0 | 1.138 | 1,189.89 | 1,354.24 |
Raw results
Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
---|---|---|---|---|---|---|---|---|---|
master | CallElasticsearch |
net6.0 | 1.19μs | 0.601ns | 2.33ns | 0.0137 | 0 | 0 | 976 B |
master | CallElasticsearch |
netcoreapp3.1 | 1.53μs | 1.08ns | 4.06ns | 0.0129 | 0 | 0 | 976 B |
master | CallElasticsearch |
net472 | 2.53μs | 1.49ns | 5.58ns | 0.157 | 0 | 0 | 995 B |
master | CallElasticsearchAsync |
net6.0 | 1.29μs | 0.591ns | 2.21ns | 0.013 | 0 | 0 | 952 B |
master | CallElasticsearchAsync |
netcoreapp3.1 | 1.6μs | 1.01ns | 3.79ns | 0.0137 | 0 | 0 | 1.02 KB |
master | CallElasticsearchAsync |
net472 | 2.69μs | 1.17ns | 4.52ns | 0.167 | 0 | 0 | 1.05 KB |
#6463 | CallElasticsearch |
net6.0 | 1.35μs | 0.423ns | 1.58ns | 0.0136 | 0 | 0 | 976 B |
#6463 | CallElasticsearch |
netcoreapp3.1 | 1.54μs | 1.83ns | 6.85ns | 0.013 | 0 | 0 | 976 B |
#6463 | CallElasticsearch |
net472 | 2.51μs | 1.49ns | 5.77ns | 0.157 | 0 | 0 | 995 B |
#6463 | CallElasticsearchAsync |
net6.0 | 1.34μs | 0.98ns | 3.79ns | 0.0134 | 0 | 0 | 952 B |
#6463 | CallElasticsearchAsync |
netcoreapp3.1 | 1.64μs | 1.09ns | 4.21ns | 0.014 | 0 | 0 | 1.02 KB |
#6463 | CallElasticsearchAsync |
net472 | 2.66μs | 1.06ns | 3.96ns | 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.26μs | 0.717ns | 2.78ns | 0.0132 | 0 | 0 | 952 B |
master | ExecuteAsync |
netcoreapp3.1 | 1.7μs | 1.22ns | 4.55ns | 0.0128 | 0 | 0 | 952 B |
master | ExecuteAsync |
net472 | 1.77μs | 0.597ns | 2.31ns | 0.145 | 0 | 0 | 915 B |
#6463 | ExecuteAsync |
net6.0 | 1.32μs | 0.995ns | 3.85ns | 0.0132 | 0 | 0 | 952 B |
#6463 | ExecuteAsync |
netcoreapp3.1 | 1.66μs | 0.82ns | 3.07ns | 0.0126 | 0 | 0 | 952 B |
#6463 | ExecuteAsync |
net472 | 1.88μs | 1.23ns | 4.75ns | 0.144 | 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.23μs | 0.846ns | 3.05ns | 0.0316 | 0 | 0 | 2.31 KB |
master | SendAsync |
netcoreapp3.1 | 5.34μs | 5.13ns | 19.9ns | 0.0373 | 0 | 0 | 2.85 KB |
master | SendAsync |
net472 | 7.27μs | 2.65ns | 9.9ns | 0.493 | 0 | 0 | 3.12 KB |
#6463 | SendAsync |
net6.0 | 4.52μs | 2.8ns | 9.68ns | 0.0316 | 0 | 0 | 2.31 KB |
#6463 | SendAsync |
netcoreapp3.1 | 5.2μs | 1.19ns | 4.44ns | 0.0368 | 0 | 0 | 2.85 KB |
#6463 | SendAsync |
net472 | 7.37μs | 3.12ns | 12.1ns | 0.493 | 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.61μs | 0.949ns | 3.55ns | 0.0233 | 0 | 0 | 1.64 KB |
master | EnrichedLog |
netcoreapp3.1 | 2.18μs | 1.53ns | 5.73ns | 0.0219 | 0 | 0 | 1.64 KB |
master | EnrichedLog |
net472 | 2.72μs | 1.3ns | 4.85ns | 0.249 | 0 | 0 | 1.57 KB |
#6463 | EnrichedLog |
net6.0 | 1.46μs | 0.311ns | 1.12ns | 0.0235 | 0 | 0 | 1.64 KB |
#6463 | EnrichedLog |
netcoreapp3.1 | 2.19μs | 0.91ns | 3.52ns | 0.0225 | 0 | 0 | 1.64 KB |
#6463 | EnrichedLog |
net472 | 2.57μs | 1.44ns | 5.58ns | 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 | 119μs | 161ns | 622ns | 0.0593 | 0 | 0 | 4.28 KB |
master | EnrichedLog |
netcoreapp3.1 | 122μs | 181ns | 702ns | 0 | 0 | 0 | 4.28 KB |
master | EnrichedLog |
net472 | 152μs | 145ns | 562ns | 0.683 | 0.228 | 0 | 4.46 KB |
#6463 | EnrichedLog |
net6.0 | 118μs | 311ns | 1.2μs | 0.0586 | 0 | 0 | 4.28 KB |
#6463 | EnrichedLog |
netcoreapp3.1 | 122μs | 195ns | 755ns | 0 | 0 | 0 | 4.28 KB |
#6463 | EnrichedLog |
net472 | 153μs | 71.2ns | 276ns | 0.688 | 0.229 | 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.09μs | 0.693ns | 2.68ns | 0.0309 | 0 | 0 | 2.2 KB |
master | EnrichedLog |
netcoreapp3.1 | 4.31μs | 1.01ns | 3.9ns | 0.0303 | 0 | 0 | 2.2 KB |
master | EnrichedLog |
net472 | 4.76μs | 1.2ns | 4.66ns | 0.319 | 0 | 0 | 2.02 KB |
#6463 | EnrichedLog |
net6.0 | 3.09μs | 0.702ns | 2.53ns | 0.031 | 0 | 0 | 2.2 KB |
#6463 | EnrichedLog |
netcoreapp3.1 | 4.08μs | 2.16ns | 8.08ns | 0.0286 | 0 | 0 | 2.2 KB |
#6463 | EnrichedLog |
net472 | 4.84μs | 1.24ns | 4.62ns | 0.32 | 0 | 0 | 2.02 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 | 1.39μs | 0.686ns | 2.47ns | 0.0161 | 0 | 0 | 1.14 KB |
master | SendReceive |
netcoreapp3.1 | 1.81μs | 0.995ns | 3.86ns | 0.0153 | 0 | 0 | 1.14 KB |
master | SendReceive |
net472 | 2.17μs | 3.28ns | 12.7ns | 0.184 | 0 | 0 | 1.16 KB |
#6463 | SendReceive |
net6.0 | 1.35μs | 0.592ns | 2.29ns | 0.0161 | 0 | 0 | 1.14 KB |
#6463 | SendReceive |
netcoreapp3.1 | 1.76μs | 1.16ns | 4.35ns | 0.0158 | 0 | 0 | 1.14 KB |
#6463 | SendReceive |
net472 | 2.14μs | 0.941ns | 3.65ns | 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.83μs | 1.09ns | 4.07ns | 0.0213 | 0 | 0 | 1.6 KB |
master | EnrichedLog |
netcoreapp3.1 | 4.08μs | 2.22ns | 8.59ns | 0.0204 | 0 | 0 | 1.65 KB |
master | EnrichedLog |
net472 | 4.49μs | 11.6ns | 45ns | 0.323 | 0 | 0 | 2.04 KB |
#6463 | EnrichedLog |
net6.0 | 2.77μs | 1.25ns | 4.83ns | 0.0221 | 0 | 0 | 1.6 KB |
#6463 | EnrichedLog |
netcoreapp3.1 | 3.72μs | 1.79ns | 6.94ns | 0.0223 | 0 | 0 | 1.65 KB |
#6463 | EnrichedLog |
net472 | 4.38μs | 2.31ns | 8.94ns | 0.324 | 0 | 0 | 2.04 KB |
Benchmarks.Trace.SpanBenchmark - Slower ⚠️ Same allocations ✔️
Slower ⚠️ in #6463
Benchmark
diff/base
Base Median (ns)
Diff Median (ns)
Modality
Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑net6.0
1.210
410.73
497.11
Benchmark | diff/base | Base Median (ns) | Diff Median (ns) | Modality |
---|---|---|---|---|
Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑net6.0 | 1.210 | 410.73 | 497.11 |
Raw results
Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
---|---|---|---|---|---|---|---|---|---|
master | StartFinishSpan |
net6.0 | 411ns | 0.263ns | 1.02ns | 0.00804 | 0 | 0 | 576 B |
master | StartFinishSpan |
netcoreapp3.1 | 637ns | 0.814ns | 3.15ns | 0.00775 | 0 | 0 | 576 B |
master | StartFinishSpan |
net472 | 669ns | 0.276ns | 1.03ns | 0.0915 | 0 | 0 | 578 B |
master | StartFinishScope |
net6.0 | 493ns | 0.287ns | 1.11ns | 0.00969 | 0 | 0 | 696 B |
master | StartFinishScope |
netcoreapp3.1 | 733ns | 0.971ns | 3.76ns | 0.00959 | 0 | 0 | 696 B |
master | StartFinishScope |
net472 | 930ns | 0.523ns | 2.03ns | 0.104 | 0 | 0 | 658 B |
#6463 | StartFinishSpan |
net6.0 | 497ns | 0.276ns | 1.07ns | 0.00803 | 0 | 0 | 576 B |
#6463 | StartFinishSpan |
netcoreapp3.1 | 632ns | 0.58ns | 2.17ns | 0.00777 | 0 | 0 | 576 B |
#6463 | StartFinishSpan |
net472 | 631ns | 0.408ns | 1.58ns | 0.0915 | 0 | 0 | 578 B |
#6463 | StartFinishScope |
net6.0 | 485ns | 0.233ns | 0.871ns | 0.00976 | 0 | 0 | 696 B |
#6463 | StartFinishScope |
netcoreapp3.1 | 713ns | 0.545ns | 2.11ns | 0.00949 | 0 | 0 | 696 B |
#6463 | StartFinishScope |
net472 | 914ns | 0.391ns | 1.51ns | 0.104 | 0 | 0 | 658 B |
Benchmarks.Trace.TraceAnnotationsBenchmark - Faster 🎉 Same allocations ✔️
Faster 🎉 in #6463
Benchmark
base/diff
Base Median (ns)
Diff Median (ns)
Modality
Benchmarks.Trace.TraceAnnotationsBenchmark.RunOnMethodBegin‑net6.0
1.121
721.85
644.10
Benchmark | base/diff | Base Median (ns) | Diff Median (ns) | Modality |
---|---|---|---|---|
Benchmarks.Trace.TraceAnnotationsBenchmark.RunOnMethodBegin‑net6.0 | 1.121 | 721.85 | 644.10 |
Raw results
Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
---|---|---|---|---|---|---|---|---|---|
master | RunOnMethodBegin |
net6.0 | 722ns | 0.335ns | 1.25ns | 0.0098 | 0 | 0 | 696 B |
master | RunOnMethodBegin |
netcoreapp3.1 | 956ns | 0.649ns | 2.51ns | 0.00945 | 0 | 0 | 696 B |
master | RunOnMethodBegin |
net472 | 1.17μs | 0.343ns | 1.33ns | 0.104 | 0 | 0 | 658 B |
#6463 | RunOnMethodBegin |
net6.0 | 644ns | 0.315ns | 1.22ns | 0.00971 | 0 | 0 | 696 B |
#6463 | RunOnMethodBegin |
netcoreapp3.1 | 964ns | 0.519ns | 2.01ns | 0.00922 | 0 | 0 | 696 B |
#6463 | RunOnMethodBegin |
net472 | 1.1μs | 0.45ns | 1.74ns | 0.104 | 0 | 0 | 658 B |
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 (6463) (11.161M) : 0, 11160510
master (11.161M) : 0, 11161110
benchmarks/2.9.0 (11.033M) : 0, 11032866
section Automatic
This PR (6463) (7.110M) : 0, 7109966
master (7.229M) : 0, 7229017
benchmarks/2.9.0 (7.786M) : 0, 7785853
section Trace stats
master (7.435M) : 0, 7435310
section Manual
master (11.146M) : 0, 11146248
section Manual + Automatic
This PR (6463) (6.706M) : 0, 6705894
master (6.610M) : 0, 6610117
section DD_TRACE_ENABLED=0
master (10.159M) : 0, 10159204
gantt
title Throughput Linux arm64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (6463) (9.481M) : 0, 9480657
master (9.601M) : 0, 9600540
benchmarks/2.9.0 (9.495M) : 0, 9494821
section Automatic
This PR (6463) (6.357M) : 0, 6356955
master (6.366M) : 0, 6365756
section Trace stats
master (6.424M) : 0, 6424147
section Manual
master (9.621M) : 0, 9620773
section Manual + Automatic
This PR (6463) (6.001M) : 0, 6001230
master (5.938M) : 0, 5937506
section DD_TRACE_ENABLED=0
master (8.890M) : 0, 8889593
gantt
title Throughput Windows x64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (6463) (10.245M) : 0, 10244522
master (10.145M) : 0, 10145415
benchmarks/2.9.0 (10.020M) : 0, 10019592
section Automatic
This PR (6463) (6.634M) : 0, 6633933
master (6.521M) : 0, 6521495
benchmarks/2.9.0 (7.255M) : 0, 7255257
section Trace stats
master (7.245M) : 0, 7245193
section Manual
master (10.135M) : 0, 10135197
section Manual + Automatic
This PR (6463) (6.011M) : 0, 6011368
master (6.058M) : 0, 6057608
section DD_TRACE_ENABLED=0
master (9.431M) : 0, 9430769
|
## 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.
Summary of changes
Allows you to specify that a duck type member can match a property or a field - whatever's available.
Reason for change
In the recent version of RabbitMQ, they changed some properties into identically-named fields. Without this change, we'd have to duplicate the whole duck chain hierarchy.
Implementation details
If the
DuckKind
isPropertyOrField
, and we fail to find a property, then try to look for a matching field instead.Test coverage
Added some simple tests, that it works as expected, but I don't think we need to go as far as duplicating the extensive suite we have for Property or Field matching.
Other details
Required for RabbitMQ v7 support