Small perf tweaks for AspNetCoreDiagnosticObserver#7963
Conversation
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (7963) 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 (7963) - mean (68ms) : 67, 70
master - mean (68ms) : 67, 70
section Bailout
This PR (7963) - mean (72ms) : 71, 74
master - mean (72ms) : 71, 73
section CallTarget+Inlining+NGEN
This PR (7963) - mean (1,013ms) : 959, 1067
master - mean (1,008ms) : 951, 1066
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 (7963) - mean (106ms) : 104, 108
master - mean (106ms) : 103, 108
section Bailout
This PR (7963) - mean (107ms) : 105, 108
master - mean (107ms) : 105, 108
section CallTarget+Inlining+NGEN
This PR (7963) - mean (746ms) : crit, 690, 803
master - mean (705ms) : 676, 735
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7963) - mean (93ms) : 91, 95
master - mean (94ms) : 92, 96
section Bailout
This PR (7963) - mean (94ms) : 93, 95
master - mean (94ms) : 93, 95
section CallTarget+Inlining+NGEN
This PR (7963) - mean (710ms) : crit, 691, 728
master - mean (669ms) : 649, 689
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7963) - mean (92ms) : 90, 94
master - mean (92ms) : 90, 94
section Bailout
This PR (7963) - mean (93ms) : 92, 94
master - mean (93ms) : 92, 94
section CallTarget+Inlining+NGEN
This PR (7963) - mean (632ms) : 619, 646
master - mean (628ms) : 615, 641
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 (7963) - mean (194ms) : 189, 198
master - mean (194ms) : 190, 199
section Bailout
This PR (7963) - mean (197ms) : 194, 200
master - mean (197ms) : 194, 200
section CallTarget+Inlining+NGEN
This PR (7963) - mean (1,126ms) : 1051, 1202
master - mean (1,115ms) : 1052, 1178
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 (7963) - mean (276ms) : 271, 282
master - mean (277ms) : 272, 282
section Bailout
This PR (7963) - mean (278ms) : 274, 283
master - mean (277ms) : 273, 280
section CallTarget+Inlining+NGEN
This PR (7963) - mean (935ms) : 891, 979
master - mean (907ms) : 869, 944
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7963) - mean (270ms) : 266, 274
master - mean (271ms) : 265, 277
section Bailout
This PR (7963) - mean (271ms) : 266, 276
master - mean (270ms) : 266, 274
section CallTarget+Inlining+NGEN
This PR (7963) - mean (934ms) : 898, 970
master - mean (888ms) : 852, 925
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7963) - mean (271ms) : 263, 278
master - mean (270ms) : 265, 275
section Bailout
This PR (7963) - mean (270ms) : 267, 273
master - mean (271ms) : 266, 277
section CallTarget+Inlining+NGEN
This PR (7963) - mean (824ms) : 803, 845
master - mean (826ms) : 802, 851
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
tracer/src/Datadog.Trace/DiagnosticListeners/AspNetCoreDiagnosticObserver.cs
Outdated
Show resolved
Hide resolved
tracer/src/Datadog.Trace/DiagnosticListeners/AspNetCoreDiagnosticObserver.cs
Outdated
Show resolved
Hide resolved
tracer/src/Datadog.Trace/PlatformHelpers/AspNetCoreHttpRequestHandler.cs
Outdated
Show resolved
Hide resolved
## 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
bc14e1f to
accaca2
Compare
BenchmarksBenchmark execution time: 2025-12-23 15:49:05 Comparing candidate commit 383da45 in PR branch Found 11 performance improvements and 4 performance regressions! Performance is the same for 153 metrics, 18 unstable metrics. scenario:Benchmarks.Trace.Asm.AppSecBodyBenchmark.AllCycleMoreComplexBody net6.0
scenario:Benchmarks.Trace.Asm.AppSecBodyBenchmark.AllCycleSimpleBody net472
scenario:Benchmarks.Trace.Asm.AppSecBodyBenchmark.ObjectExtractorSimpleBody netcoreapp3.1
scenario:Benchmarks.Trace.Asm.AppSecEncoderBenchmark.EncodeLegacyArgs netcoreapp3.1
scenario:Benchmarks.Trace.AspNetCoreBenchmark.SendRequest net6.0
scenario:Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark.WriteAndFlushEnrichedTraces net472
scenario:Benchmarks.Trace.CharSliceBenchmark.OptimizedCharSlice netcoreapp3.1
scenario:Benchmarks.Trace.CharSliceBenchmark.OptimizedCharSliceWithPool net6.0
scenario:Benchmarks.Trace.CharSliceBenchmark.OptimizedCharSliceWithPool netcoreapp3.1
scenario:Benchmarks.Trace.ElasticsearchBenchmark.CallElasticsearch net472
scenario:Benchmarks.Trace.Log4netBenchmark.EnrichedLog netcoreapp3.1
scenario:Benchmarks.Trace.RedisBenchmark.SendReceive netcoreapp3.1
scenario:Benchmarks.Trace.SpanBenchmark.StartFinishSpan net472
scenario:Benchmarks.Trace.SpanBenchmark.StartFinishTwoScopes net472
scenario:Benchmarks.Trace.SpanBenchmark.StartFinishTwoScopes net6.0
|
This comment has been minimized.
This comment has been minimized.
accaca2 to
383da45
Compare
| // Iast hasn't yet, but doing it now is fine | ||
| // span origins is _not_ initialized yet, and we can't guarantee it will be | ||
| // so just be lazy instead | ||
| observers.Add(new AspNetCoreDiagnosticObserver(Tracer.Instance, Security.Instance, Iast.Iast.Instance, spanCodeOrigin: null)); |
There was a problem hiding this comment.
This forces the creation of IAST and security instances even if not enabled. On the other hand, we are calling Security.Instance and Iast.Instance in many other places, so, in practical terms, we are going to create the instances anyway. Actually, we need to call Instance to read the IAST and ASM settings. Just wondering if at some point we could separate the security settings and access them without actually creating a (not needed mostly) instance of Security or IAST. We are creating instances just to read the settings. Still, a sensitive change (we need to be careful with RC, for instance)
Anyway, definitely, totally out of the scope of this PR. The performance optimization here is sound given the current architecture.
There was a problem hiding this comment.
This forces the creation of IAST and security instances even if not enabled
So these are actually already created automatically 😅 e.g. we explicitly do _ = Security.Instance; just above.
Actually, we need to call Instance to read the IAST and ASM settings. Just wondering if at some point we could separate the security settings and access them without actually creating a (not needed mostly) instance of Security or IAST.
💯 This is something I'd definitely like to do, and I think is worthwhile, but for the exact reason you state "we need to be careful with RC, for instance", I suspect we will generally need to do a certain amount of initialization no matter what, so I don't think it's a massive issue. And like you say, outside of the scope for this PR, but definitely something for us to think about! 🙂
NachoEchevarria
left a comment
There was a problem hiding this comment.
Nice job! Thanks!
## 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>
## Summary of changes Creates an optimized version of `AspNetCoreResourceNameHelper` for 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 - Use the `ValueStringBuilder` to build a resource name - Don't pre-grab MVC route values, wait to see if we have route params from them first - Write directly into the builder as `ToLowerInvariant`, instead of doing an extra ToLower() at the end ## Test 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 - #7962 - #7963 - #7964 - #7966 👈 - #7965
…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
Small perf tweaks for existing
AspNetCoreDiagnosticObserverReason 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
HttpContext.Items[string]instead ofHttpContext.Features.Get<T>as it's a bit fasterTracer/Security/Iastinputs readonly, instead of using a null-coalesce with every accessAddHeaderTagsToSpangiven common case is to avoid the function callTest coverage
Covered by existing tests
Other details
https://datadoghq.atlassian.net/browse/LANGPLAT-842
Part of a stack
ValueStringBuilderfor .NET 6 #7962AspNetCoreDiagnosticObserver#7963 👈AspNetCoreResourceNameHelper#7966