Create a more optimized AspNetCoreResourceNameHelper#7966
Conversation
|
c2c2b8c to
37760e4
Compare
5e4ff26 to
bdd3a0f
Compare
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (7966) and master. ✅ No regressions detected - check the details below Full Metrics ComparisonFakeDbCommand
HttpMessageHandler
Comparison explanationExecution-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 highlighted 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). Duration chartsFakeDbCommand (.NET Framework 4.8)gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7966) - mean (78ms) : 75, 81
master - mean (74ms) : 72, 77
section Bailout
This PR (7966) - mean (82ms) : 80, 85
master - mean (78ms) : 76, 80
section CallTarget+Inlining+NGEN
This PR (7966) - mean (1,091ms) : 1037, 1145
master - mean (1,048ms) : 996, 1100
FakeDbCommand (.NET Core 3.1)gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7966) - mean (121ms) : 116, 127
master - mean (115ms) : 111, 118
section Bailout
This PR (7966) - mean (122ms) : 118, 125
master - mean (116ms) : 113, 119
section CallTarget+Inlining+NGEN
This PR (7966) - mean (797ms) : 740, 855
master - mean (773ms) : 715, 832
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7966) - mean (107ms) : 102, 112
master - mean (101ms) : 98, 104
section Bailout
This PR (7966) - mean (108ms) : 105, 111
master - mean (102ms) : 100, 105
section CallTarget+Inlining+NGEN
This PR (7966) - mean (760ms) : 679, 840
master - mean (737ms) : 670, 805
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7966) - mean (105ms) : 102, 109
master - mean (100ms) : 97, 102
section Bailout
This PR (7966) - mean (106ms) : 103, 108
master - mean (101ms) : 99, 103
section CallTarget+Inlining+NGEN
This PR (7966) - mean (706ms) : crit, 679, 734
master - mean (668ms) : 647, 689
HttpMessageHandler (.NET Framework 4.8)gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7966) - mean (194ms) : 191, 198
master - mean (193ms) : 188, 198
section Bailout
This PR (7966) - mean (197ms) : 195, 199
master - mean (197ms) : 193, 201
section CallTarget+Inlining+NGEN
This PR (7966) - mean (1,138ms) : 1066, 1209
master - mean (1,122ms) : 1054, 1191
HttpMessageHandler (.NET Core 3.1)gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7966) - mean (279ms) : 272, 287
master - mean (276ms) : 269, 282
section Bailout
This PR (7966) - mean (278ms) : 274, 282
master - mean (276ms) : 272, 280
section CallTarget+Inlining+NGEN
This PR (7966) - mean (936ms) : 895, 977
master - mean (931ms) : 890, 972
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7966) - mean (272ms) : 266, 279
master - mean (270ms) : 264, 276
section Bailout
This PR (7966) - mean (272ms) : 267, 278
master - mean (269ms) : 266, 272
section CallTarget+Inlining+NGEN
This PR (7966) - mean (935ms) : 901, 969
master - mean (929ms) : 884, 973
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7966) - mean (271ms) : 266, 276
master - mean (270ms) : 264, 276
section Bailout
This PR (7966) - mean (271ms) : 266, 277
master - mean (269ms) : 265, 273
section CallTarget+Inlining+NGEN
This PR (7966) - mean (831ms) : 813, 849
master - mean (827ms) : 808, 845
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
37760e4 to
65dcd0e
Compare
bdd3a0f to
88205c8
Compare
tracer/src/Datadog.Trace/DiagnosticListeners/AspNetCoreResourceNameHelper.cs
Outdated
Show resolved
Hide resolved
| bool expandRouteParameters) | ||
| { | ||
| var sb = routePattern.RawText?.Length < 512 | ||
| ? new ValueStringBuilder(stackalloc char[512]) |
## Summary of changes Adds a `ValueStringBuilder` implementation, based on the one used internally in the .NET runtime ## Reason for change - We can stackalloc the `Span<char>` - It's a bit faster than our existing `StringBuilderCache` implementation, and in some places that matters. ## Implementation details - Made in .NET 6 only for simplicity. We _could_ expose it earlier, but I wanted this for the updated aspnetcore integration, so .NET6+ only for now is good enough - It's not without risks in its usage, so we have to be careful about things like passing it around (i.e. avoid doing that completely, for safety) - Uses an array pool backed implementation (again, built into .NET 6) ## Test coverage Imported the unit tests from the runtime too ## Other details https://datadoghq.atlassian.net/browse/LANGPLAT-842 Part of a stack - #7962 👈 - #7963 - #7964 - #7966 - #7965
65dcd0e to
303a7ac
Compare
00fe314 to
e8369e2
Compare
BenchmarksBenchmark execution time: 2026-01-13 15:33:43 Comparing candidate commit 2ce366f in PR branch Found 15 performance improvements and 3 performance regressions! Performance is the same for 156 metrics, 12 unstable metrics. scenario:Benchmarks.Trace.ActivityBenchmark.StartStopWithChild net6.0
scenario:Benchmarks.Trace.AgentWriterBenchmark.WriteAndFlushEnrichedTraces net6.0
scenario:Benchmarks.Trace.AgentWriterBenchmark.WriteAndFlushEnrichedTraces netcoreapp3.1
scenario:Benchmarks.Trace.Asm.AppSecEncoderBenchmark.EncodeLegacyArgs netcoreapp3.1
scenario:Benchmarks.Trace.Asm.AppSecWafBenchmark.RunWafRealisticBenchmarkWithAttack netcoreapp3.1
scenario:Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark.WriteAndFlushEnrichedTraces net472
scenario:Benchmarks.Trace.CharSliceBenchmark.OptimizedCharSlice net6.0
scenario:Benchmarks.Trace.CharSliceBenchmark.OptimizedCharSlice netcoreapp3.1
scenario:Benchmarks.Trace.CharSliceBenchmark.OptimizedCharSliceWithPool net6.0
scenario:Benchmarks.Trace.ElasticsearchBenchmark.CallElasticsearchAsync net6.0
scenario:Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark net6.0
scenario:Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark net6.0
scenario:Benchmarks.Trace.SpanBenchmark.StartFinishTwoScopes netcoreapp3.1
|
303a7ac to
8a6357a
Compare
e8369e2 to
bb0fb1b
Compare
## Summary of changes Small perf tweaks for existing `AspNetCoreDiagnosticObserver` ## Reason for change While experimenting with single span observer, noticed some (safe) tweaks we could make to the existing implementation. There are some other improvements we could make as well, but those are a little riskier, so may be best to delay them. ## Implementation details - Use `HttpContext.Items[string]` instead of `HttpContext.Features.Get<T>` as it's a bit faster - Make `Tracer`/`Security`/`Iast` inputs readonly, instead of using a null-coalesce with every access - Move check outside of `AddHeaderTagsToSpan` given common case is to avoid the function call ## Test coverage Covered by existing tests ## Other details https://datadoghq.atlassian.net/browse/LANGPLAT-842 Part of a stack - #7962 - #7963 👈 - #7964 - #7966 - #7965
8a6357a to
4fd083f
Compare
bb0fb1b to
a356866
Compare
4fd083f to
5d97960
Compare
a356866 to
04d208f
Compare
5d97960 to
67ea0dd
Compare
04d208f to
de692e8
Compare
## Summary of changes Adds a new `DD_TRACE_SINGLE_SPAN_ASPNETCORE_ENABLED` in which we omit the MVC span ## Reason for change - Addresses #4093 and partially #1147 - Omitting the MVC span has perf and some usability benefits in some cases (See issues above). It _does_ mean a certain amount of less information, particularly in pipeline re-execution scenarios ## Implementation details - Add a new, disabled by default, `DD_TRACE_SINGLE_SPAN_ASPNETCORE_ENABLED` for opting in to the new behaviour. - .NET 6+ only for various reasons - Cloned the original observer, deleted everything related to the MVC span, and then optimized further based on .NET 6+ and other improvements we can make ## Test coverage - Added in-process "integration" tests for the observer (more like unit tests), which mirror the exisitng. - Added "full" sample-based integration tests for basically all scenarios we currently test (IIS in process/out of process / MVC / minimal APIs / proxy spans etc) - Added snapshots by copying existing, and then updating, so if you look at the final commit, you can see the diff, which is basically just "delete the child span" ## Other details https://datadoghq.atlassian.net/browse/LANGPLAT-842 Part of a stack - #7962 - #7963 - #7964 👈 - #7966 - #7965 I experimented with some other perf improvements, but they didn't provide a significant improvement, so I reverted: - Caching the resource name by saving it into the `IEndpointMetadata` collection. The work to do that was too much, and you _can't_ cache in some cases anyway so not worth it - Splitting `WebTags` out to remove the IP headers unless we need them. They're disabled by default, so it just increase the size of the tags object, but wasn't a big enough improvement to make the complexity worth it. - Tried pooling the `RequestTrackingFeature` objects. This one _may_ actually be worth exploring again later, but I didn't see any improvement in my testing, as they're just not very big and only last 1 request. --------- Co-authored-by: Lucas Pimentel <lucas.pimentel@datadoghq.com>
67ea0dd to
2ce366f
Compare
…7965) ## Summary of changes Adds a micro and macro benchmark for the new observer ## Reason for change Want to verify that it has performance benefits ## Implementation details - Added new scenario for macro - Duplicated existing scenario for micro and run. I would like to have had these as the same methods, but the "global" nature of diagnostic observers makes this difficult I think. ## Test coverage I'll manually trigger a run of the macro ## Other details https://datadoghq.atlassian.net/browse/LANGPLAT-842 Part of a stack - #7962 - #7963 - #7964 - #7966 - #7965 👈
Summary of changes
Creates an optimized version of
AspNetCoreResourceNameHelperfor use by the single-span Diagnostic observer.Reason for change
We have the opportunity for a variety of optimizations thanks to not creating the MVC span, and the fact that we're .NET 6+. We could potentially include some or all of these optimizations in the existing observer path, but for safety/simplicity I'd rather keep it isolated for now, and we can look at expanding it more broadly later
Implementation details
ValueStringBuilderto build a resource nameToLowerInvariant, instead of doing an extra ToLower() at the endTest coverage
Added unit tests that use the existing data for the other implementations, to confirm it's identical. It's also covered indirectly by the integration tests
Other details
https://datadoghq.atlassian.net/browse/LANGPLAT-842
Part of a stack
ValueStringBuilderfor .NET 6 #7962AspNetCoreDiagnosticObserver#7963AspNetCoreResourceNameHelper#7966 👈