-
Notifications
You must be signed in to change notification settings - Fork 765
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
Update AddHttpClientInstrumentation for .NET Framework #1982
Update AddHttpClientInstrumentation for .NET Framework #1982
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1982 +/- ##
=======================================
Coverage 83.86% 83.86%
=======================================
Files 192 192
Lines 6179 6179
=======================================
Hits 5182 5182
Misses 997 997
|
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.
Thanks! This looks good.
I definitely think this improves things for usability in what we think is the most common case. Worst case if it does turn out that supporting configuring options for .NET Framework apps that consume the HttpClient nuget package is needed then maybe it could be a separate extension method. |
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.
LGTM
Might be worth noting in the docs for the instrumentation that HttpClient
on .NET Framework uses HttpWebRequest
internally and will fire HttpWebRequestInstrumentationOptions.Enrich\Filter
callbacks.
The docs already call out difference between .NET FX and .NET Core cases. https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/src/OpenTelemetry.Instrumentation.Http#filter |
I'll make it more clear that its .NET FX vs .NET Core, and not HttpClient vs HttpWebRequest and mention about the fact that HttpClient internally is HttpWebRequest in .NET Fx. |
Currently,
AddHttpClientInstrumentation
extension method for.NET Framework
uses bothHttpClientInstrumentationOptions
andHttpWebRequestInstrumentationOptions
as arguments. This causes confusion to a lot of users who want to configure theHttpWebRequestInstrumentationOptions
but end up configuringHttpClientInstrumentationOptions
as that's the first argument of the extension method and both the options classes have some common properties with the same types.If
HttpClient
is used in .NET Framework it usesHttpWebRequest
so we only needHttpWebRequestInstrumentationOptions
. There is a possibility of someone using a .NET Core version ofHttpClient
in a .NET Framework application in which case we might need theHttpClientInstrumentationOptions
but usingHttpClient
in that way is not recommended.Changes
AddHttpClientInstrumentation
extension method for .NET Framework to only useHttpWebRequestInstrumentationOptions
as an argumentPlease provide a brief description of the changes here.
For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes