Skip to content
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

Conversation

utpilla
Copy link
Contributor

@utpilla utpilla commented Apr 15, 2021

Currently, AddHttpClientInstrumentation extension method for .NET Framework uses both HttpClientInstrumentationOptions and HttpWebRequestInstrumentationOptions as arguments. This causes confusion to a lot of users who want to configure the HttpWebRequestInstrumentationOptions but end up configuring HttpClientInstrumentationOptions 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 uses HttpWebRequest so we only need HttpWebRequestInstrumentationOptions. There is a possibility of someone using a .NET Core version of HttpClient in a .NET Framework application in which case we might need the HttpClientInstrumentationOptions but using HttpClient in that way is not recommended.

Changes

  • Update AddHttpClientInstrumentation extension method for .NET Framework to only use HttpWebRequestInstrumentationOptions as an argument

Please 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
  • Changes in public API reviewed

@codecov
Copy link

codecov bot commented Apr 15, 2021

Codecov Report

Merging #1982 (4e96d28) into main (6dfe944) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1982   +/-   ##
=======================================
  Coverage   83.86%   83.86%           
=======================================
  Files         192      192           
  Lines        6179     6179           
=======================================
  Hits         5182     5182           
  Misses        997      997           
Impacted Files Coverage Δ
...umentation.Http/TracerProviderBuilderExtensions.cs 100.00% <ø> (ø)

Copy link
Member

@cijothomas cijothomas left a 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.

@cijothomas cijothomas marked this pull request as ready for review April 15, 2021 22:12
@cijothomas cijothomas requested a review from a team as a code owner April 15, 2021 22:12
@alanwest
Copy link
Member

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.

Copy link
Member

@CodeBlanch CodeBlanch left a 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.

@cijothomas
Copy link
Member

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

@cijothomas
Copy link
Member

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.

@cijothomas cijothomas merged commit 4a44bed into open-telemetry:main Apr 17, 2021
@utpilla utpilla deleted the utpilla/Update-HttpClientInstrumentation branch June 3, 2021 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants