Call HttpRequest.BuildUrl to avoid URL caching
#6361
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 (6361) - mean (69ms) : 67, 72
. : milestone, 69,
master - mean (69ms) : 67, 72
. : milestone, 69,
section CallTarget+Inlining+NGEN
This PR (6361) - mean (984ms) : 960, 1008
. : milestone, 984,
master - mean (983ms) : 953, 1012
. : milestone, 983,
gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6361) - mean (108ms) : 105, 111
. : milestone, 108,
master - mean (108ms) : 106, 110
. : milestone, 108,
section CallTarget+Inlining+NGEN
This PR (6361) - mean (677ms) : 661, 694
. : milestone, 677,
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 (6361) - mean (91ms) : 89, 93
. : milestone, 91,
master - mean (91ms) : 89, 94
. : milestone, 91,
section CallTarget+Inlining+NGEN
This PR (6361) - mean (633ms) : 616, 650
. : milestone, 633,
master - mean (632ms) : 615, 648
. : milestone, 632,
gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6361) - mean (190ms) : 186, 194
. : milestone, 190,
master - mean (190ms) : 185, 195
. : milestone, 190,
section CallTarget+Inlining+NGEN
This PR (6361) - mean (1,088ms) : 1061, 1114
. : milestone, 1088,
master - mean (1,083ms) : 1060, 1106
. : milestone, 1083,
gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6361) - mean (278ms) : 271, 286
. : milestone, 278,
master - mean (276ms) : 273, 280
. : milestone, 276,
section CallTarget+Inlining+NGEN
This PR (6361) - mean (860ms) : 828, 891
. : milestone, 860,
master - mean (868ms) : 834, 901
. : milestone, 868,
gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6361) - mean (264ms) : 259, 269
. : milestone, 264,
master - mean (264ms) : 259, 268
. : milestone, 264,
section CallTarget+Inlining+NGEN
This PR (6361) - mean (846ms) : 809, 883
. : milestone, 846,
master - mean (842ms) : 809, 875
. : milestone, 842,
|
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 (6361) (11.255M) : 0, 11255395
master (11.198M) : 0, 11198348
benchmarks/2.9.0 (11.033M) : 0, 11032866
section Automatic
This PR (6361) (7.353M) : 0, 7353456
master (7.205M) : 0, 7205466
benchmarks/2.9.0 (7.786M) : 0, 7785853
section Trace stats
master (7.443M) : 0, 7442635
section Manual
master (11.218M) : 0, 11217680
section Manual + Automatic
This PR (6361) (6.745M) : 0, 6744898
master (6.727M) : 0, 6726536
section DD_TRACE_ENABLED=0
master (10.410M) : 0, 10409702
gantt
title Throughput Linux arm64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (6361) (9.629M) : 0, 9628527
master (9.510M) : 0, 9510466
benchmarks/2.9.0 (9.495M) : 0, 9494821
section Automatic
This PR (6361) (6.485M) : 0, 6485329
master (6.331M) : 0, 6331398
section Trace stats
master (6.659M) : 0, 6658730
section Manual
master (9.280M) : 0, 9280107
section Manual + Automatic
This PR (6361) (6.052M) : 0, 6051963
master (5.691M) : 0, 5690770
section DD_TRACE_ENABLED=0
master (8.840M) : 0, 8840434
gantt
title Throughput Windows x64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (6361) (9.764M) : 0, 9763785
master (10.163M) : 0, 10163050
benchmarks/2.9.0 (10.020M) : 0, 10019592
section Automatic
This PR (6361) (6.031M) : crit ,0, 6030603
master (6.829M) : 0, 6828973
benchmarks/2.9.0 (7.255M) : 0, 7255257
section Trace stats
master (7.555M) : 0, 7554541
section Manual
master (10.735M) : 0, 10735441
section Manual + Automatic
This PR (6361) (5.688M) : crit ,0, 5688255
master (6.575M) : 0, 6575410
section DD_TRACE_ENABLED=0
master (10.155M) : 0, 10155272
|
Datadog ReportBranch report: ❌ 2 Failed (0 Known Flaky), 232869 Passed, 2076 Skipped, 18h 33m 12.85s Total Time ❌ Failed Tests (2)
|
Benchmarks Report for tracer 🐌Benchmarks for #6361 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 - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.SpanBenchmark - Slower
|
| Benchmark | diff/base | Base Median (ns) | Diff Median (ns) | Modality |
|---|---|---|---|---|
| Benchmarks.Trace.SpanBenchmark.StartFinishScope‑netcoreapp3.1 | 1.155 | 687.47 | 794.35 | |
| Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑net6.0 | 1.149 | 396.60 | 455.53 |
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | StartFinishSpan |
net6.0 | 396ns | 0.4ns | 1.55ns | 0.0081 | 0 | 0 | 576 B |
| master | StartFinishSpan |
netcoreapp3.1 | 577ns | 0.721ns | 2.79ns | 0.00786 | 0 | 0 | 576 B |
| master | StartFinishSpan |
net472 | 645ns | 1.54ns | 5.97ns | 0.0916 | 0 | 0 | 578 B |
| master | StartFinishScope |
net6.0 | 539ns | 0.536ns | 2.07ns | 0.00986 | 0 | 0 | 696 B |
| master | StartFinishScope |
netcoreapp3.1 | 688ns | 1.1ns | 4.25ns | 0.00943 | 0 | 0 | 696 B |
| master | StartFinishScope |
net472 | 799ns | 2.8ns | 10.8ns | 0.104 | 0 | 0 | 658 B |
| #6361 | StartFinishSpan |
net6.0 | 455ns | 0.662ns | 2.56ns | 0.00804 | 0 | 0 | 576 B |
| #6361 | StartFinishSpan |
netcoreapp3.1 | 636ns | 2.03ns | 7.86ns | 0.00771 | 0 | 0 | 576 B |
| #6361 | StartFinishSpan |
net472 | 645ns | 1.29ns | 4.99ns | 0.0918 | 0 | 0 | 578 B |
| #6361 | StartFinishScope |
net6.0 | 487ns | 0.468ns | 1.81ns | 0.00973 | 0 | 0 | 696 B |
| #6361 | StartFinishScope |
netcoreapp3.1 | 795ns | 0.935ns | 3.62ns | 0.00955 | 0 | 0 | 696 B |
| #6361 | StartFinishScope |
net472 | 780ns | 1.75ns | 6.77ns | 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 | 655ns | 0.62ns | 2.4ns | 0.00988 | 0 | 0 | 696 B |
| master | RunOnMethodBegin |
netcoreapp3.1 | 939ns | 1.56ns | 6.05ns | 0.00936 | 0 | 0 | 696 B |
| master | RunOnMethodBegin |
net472 | 1.05μs | 3.37ns | 12.6ns | 0.104 | 0 | 0 | 658 B |
| #6361 | RunOnMethodBegin |
net6.0 | 599ns | 1.44ns | 5.57ns | 0.00981 | 0 | 0 | 696 B |
| #6361 | RunOnMethodBegin |
netcoreapp3.1 | 942ns | 2.02ns | 7.83ns | 0.00933 | 0 | 0 | 696 B |
| #6361 | RunOnMethodBegin |
net472 | 1.08μs | 2.42ns | 9.38ns | 0.104 | 0 | 0 | 658 B |
|
Based on the benchmark results this appears to have a pretty large hit. |
I don't think any of those benchmarks test this path. That said, you might want to use ducktyping instead of reflection, it's supposed to be faster. |
bouwkast
force-pushed
the
steven/owin-fix
branch
from
d19866b to
e024a84
Compare
bouwkast
changed the title
HttpRequest.Url when we access itHttpRequest.BuildUrl to avoid URL caching
andrewlock
approved these changes
Jan 27, 2025
| @@ -653,5 +653,6 @@ | |||
| "crashtracking_debug_url": "crashtracking_debug_url", | |||
| "debug_stack_enabled": "debug_stack_enabled", | |||
| "DD_TRACE_BAGGAGE_MAX_ITEMS": "trace_baggage_max_items", | |||
| "DD_TRACE_BAGGAGE_MAX_BYTES": "trace_baggage_max_bytes" | |||
| "DD_TRACE_BAGGAGE_MAX_BYTES": "trace_baggage_max_bytes", | |||
| "DD_TRACE_BYPASS_HTTP_REQUEST_URL_CACHING_ENABLED": "trace_bypass_http_request_url_caching_enabled" | |||
There was a problem hiding this comment.
Don't forget to make a request to dd-go for this
Just to confirm, the throughput tests are only for ASP.NET Core, so definitely unrelated |
lucaspimentel
approved these changes
Jan 27, 2025
By swapping to calling BuildUrl we bypass any caching logic for the Url.
This splits them into two: - GetUrl is the old behavior where the URL will be cached in .NET Framework - BuildUrl is the new behavior (controlled by a feature flag) where the URL will not be cached
bouwkast
force-pushed
the
steven/owin-fix
branch
from
4de08bc to
08d7600
Compare
Datadog ReportBranch report: ✅ 0 Failed, 243669 Passed, 2094 Skipped, 18h 54m 30.23s Total Time |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of changes
Manually compute the value of
HttpRequest.Urlfor .NET Framework so that that it won't be cached for other callers due to us.Reason for change
For .NET Framework we access
HttpRequest.Urlwhen adding data to Spans.When called this method will build the URL of the
HttpRequestif we are the first accessors and then the result will be cached in a private field_url.The issue here is that there seems to be a few edge cases where this causes us to build the HttpRequest before it is desired to be built.
Implementation details
Adds a new feature flag:
BypassHttpRequestUrlCachingEnabledto allow bypassing of the current behavior where the URL would be cached.Duck types the
HttpRequestBuildUrlfunction to manually build the URL to avoid caching it when enabled.Test coverage
Added some unit tests for this that rely on reflection and attempted to emulate previous behavior and the behavior we want to fix.
Other details
I'm not confident that this a smart idea.
We will end up with an incorrect value here, but we did previously as well (only in these rare cases) but we will hopefully get ourselves out of the picture of affecting/altering application behavior.
https://datadoghq.atlassian.net/browse/AIDM-500