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

[Http Instrumentation] Unify exposed public API #3793

Merged

Conversation

CodeBlanch
Copy link
Member

@CodeBlanch CodeBlanch commented Oct 19, 2022

Fixes #3434 (really mean it this time)
Relates to #3787

Changes

  • Exposes the same API for all targets to fix package resolution issues when consuming Http instrumentation into netstandard2.0 projects.

  • We used to only run HttpWebRequest tests for .NET Framework & HttpClient tests for .NET/Core. Now we run the full suite for all runtimes.

Public API Changes

namespace OpenTelemetry.Instrumentation.Http
{
-#if NETFRAMEWORK
-    public class HttpWebRequestInstrumentationOptions
-    {
-        public Func<HttpWebRequest, bool> Filter { get; set; }
-        // omitted other stuff but whole class removed
-    }
-#else
    public class HttpClientInstrumentationOptions
    {
-        public Func<HttpRequestMessage, bool> Filter { get; set; }
+        public Func<HttpRequestMessage, bool> FilterHttpRequestMessage { get; set; }
+        public Func<HttpWebRequest, bool> FilterHttpWebRequest { get; set; }
    }
-#endif
}

Not shown: Removed the AddHttpClientInstrumentation extensions using HttpWebRequestInstrumentationOptions and made the ones using HttpClientInstrumentationOptions available to NETFRAMEWORK targets.

TODOs

  • Appropriate CHANGELOG.md updated for non-trivial changes
  • Unit tests
  • Changes in public API reviewed

@codecov
Copy link

codecov bot commented Oct 19, 2022

Codecov Report

Merging #3793 (9a37673) into main (893332e) will decrease coverage by 0.19%.
The diff coverage is 100.00%.

❗ Current head 9a37673 differs from pull request most recent head a1445e2. Consider uploading reports for the commit a1445e2 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3793      +/-   ##
==========================================
- Coverage   87.46%   87.27%   -0.20%     
==========================================
  Files         280      279       -1     
  Lines       10763    10755       -8     
==========================================
- Hits         9414     9386      -28     
- Misses       1349     1369      +20     
Impacted Files Coverage Δ
...mentation.Http/HttpClientInstrumentationOptions.cs 100.00% <100.00%> (ø)
...tp/Implementation/HttpHandlerDiagnosticListener.cs 73.68% <100.00%> (ø)
...plementation/HttpWebRequestActivitySource.netfx.cs 80.14% <100.00%> (+0.04%) ⬆️
...umentation.Http/TracerProviderBuilderExtensions.cs 100.00% <100.00%> (ø)
...entation/ExportClient/OtlpGrpcTraceExportClient.cs 50.00% <0.00%> (-28.58%) ⬇️
...xporter.OpenTelemetryProtocol/OtlpTraceExporter.cs 65.38% <0.00%> (-15.39%) ⬇️
...porter.OpenTelemetryProtocol/OtlpMetricExporter.cs 72.72% <0.00%> (-13.64%) ⬇️
...tation/OpenTelemetryProtocolExporterEventSource.cs 85.00% <0.00%> (-10.00%) ⬇️
...metryProtocol/Implementation/ActivityExtensions.cs 91.89% <0.00%> (-3.25%) ⬇️
...tpListener/Internal/PrometheusCollectionManager.cs 73.62% <0.00%> (-2.20%) ⬇️
... and 3 more

@CodeBlanch
Copy link
Member Author

@vishweshbankwar FYI I updated this to include #3792.

@CodeBlanch CodeBlanch mentioned this pull request Oct 26, 2022
1 task
@CodeBlanch CodeBlanch marked this pull request as ready for review October 28, 2022 21:12
@CodeBlanch CodeBlanch requested a review from a team October 28, 2022 21:12
`HttpClientInstrumentationOptions`. It is important to note that there are
differences between .NET Framework and newer .NET/.NET Core runtimes which
govern what options are used. On .NET Framework, `HttpClient` uses the
`HttpWebRequest` API. On .NET & .NET Core, `HttpWebRequest` uses the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
`HttpWebRequest` API. On .NET & .NET Core, `HttpWebRequest` uses the
`HttpWebRequest` API. On .NET & .NET Core, `HttpClient` uses the

Copy link
Member Author

@CodeBlanch CodeBlanch Nov 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do this, it will say:

On .NET & .NET Core, HttpClient uses the HttpClient API.

Which is kind of silly 😜

What I was trying to highlight here is the behavior is reversed, depending on the runtime.

  • .NET Framework: HttpClient & HttpWebRequest implemented using HttpWebRequest.
  • .NET/Core: HttpClient & HttpWebRequest implemented using HttpClient.

Not sure how to best articulate that!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea i can see this is not easy to articulate. Lets merge this now, and come back to fixing the wording.

@CodeBlanch CodeBlanch merged commit 399fbcf into open-telemetry:main Nov 2, 2022
@CodeBlanch CodeBlanch deleted the http-instrumentation-publicapi branch November 2, 2022 18:48
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.

[HttpClient instrumentation] Runtime failures with a mix of NetFramework and NetStandard
5 participants