[ASP.NET Core] Set http.request.method as per spec
#5001
Conversation
Codecov Report
@@ Coverage Diff @@
## main #5001 +/- ##
==========================================
- Coverage 83.59% 83.40% -0.19%
==========================================
Files 295 296 +1
Lines 12343 12380 +37
==========================================
+ Hits 10318 10326 +8
- Misses 2025 2054 +29
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| static RequestMethodHelper() | ||
| { | ||
| #if NET8_0_OR_GREATER | ||
| KnownMethods = FrozenDictionary.ToFrozenDictionary( |
There was a problem hiding this comment.
Nice! We should identify other places in the repo that can use Frozen collections.
test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs
Outdated
Show resolved
Hide resolved
|
|
||
| public static bool TryResolveHttpMethod(string method, out string resolvedMethod) | ||
| { | ||
| if (KnownMethods.TryGetValue(method, out resolvedMethod)) |
There was a problem hiding this comment.
@vishweshbankwar Would you do a bit of benchmarking here? I think a switch (method) might actually be the fastest thing to do here (even faster than frozen dictionary).
Check out: https://devblogs.microsoft.com/dotnet/performance-improvements-in-net-8/#frozen-collections
If you scroll down from that anchor to the "IsMostPopular" example, the compiler generates really smart logic for a string switch. I'm guessing it will be faster because it is customized for the known values and done inline, plus it also doesn't need the processing at startup FrozenDictionary needs.
There was a problem hiding this comment.
@CodeBlanch We need a case insensitive search here. I think we would end up doing either ToUpper or ToLower if we want to use a switch case. That could again add to the cost.
There was a problem hiding this comment.
@CodeBlanch We need case-insensitive look up. https://devblogs.microsoft.com/dotnet/performance-improvements-in-net-8/#frozen-collections:~:text=such%20that%20if%20you%20know%20all%20of%20your%20string%20keys%20at%20compile%2Dtime%2C%20and%20you%20just%20need%20an%20ordinal%2C%20case%2Dsensitive%20lookup%2C%20you%20might%20be%20best%20off%20simply%20writing%20a%20switch%20statement%20or%20expression
There was a problem hiding this comment.
Makes sense however I still feel like we could solve it in a way that would be faster. Given there are only a handful of values and we know them all upfront. Perf challenge for anyone willing to take it on 😄
Towards #4994
Design discussion issue #
Changes
Fixes
http.request.methodattribute value on activity and metric as per spec. For details, see the linked issue.Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changes