[HttpClient] Remove SDK dependency #5077
Conversation
vishweshbankwar
changed the title
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main open-telemetry/opentelemetry-dotnet#5077 +/- ##
==========================================
- Coverage 83.15% 83.06% -0.10%
==========================================
Files 296 296
Lines 12351 12340 -11
==========================================
- Hits 10271 10250 -21
- Misses 2080 2090 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| /// <summary> | ||
| /// Gets or sets a value indicating whether suppresses http instrumentation when the http call is part of grpc client call. | ||
| /// </summary> | ||
| public bool SuppressInstrumentationWhenGrpcIsPresent { get; set; } |
There was a problem hiding this comment.
I think we should ship this new option as experimental. The option is useful as a stop gap, but I do not think it's ultimately what we want. We have an open issue with a different, more generic, proposal #4641 though it is only focused on suppressing tracing. The nice thing about our current suppression functionality is that it works to suppress all signals.
The ability to suppress downstream instrumentation is a long running open issue in the specification. Ultimately, I believe this will be functionality provided by the API not the SDK.
I am ok with this PR if we remove the dependency on the SDK from the HTTP instrumentation and ship this option specifically targeting gRPC as experimental.
There was a problem hiding this comment.
I removed the option proposed for now. Will bring SDK dependency back after the stable release. Was hoping to use EXPOSE_EXPERIMENTAL_FEATURES but it is not possible to use as is without additional changes in build. We can discuss the possible options (the one I proposed and existing one post GA release).
There was a problem hiding this comment.
I'm in favor of removing the SDK dependency. We could address the issue for Grpc instrumentation library users after the stable release.
| @@ -278,7 +278,7 @@ public async Task InjectsHeadersAsync_CustomFormat() | |||
| })); | |||
| } | |||
|
|
|||
| [Fact] | |||
| [Fact(Skip = "Removed for stable release of http instrumentation")] | |||
There was a problem hiding this comment.
lets add a link to a tracking gh issue.
There was a problem hiding this comment.
Yes, that is my TODO on PR desc :D. Wanted to confirm everyone agrees with this proposal.
There was a problem hiding this comment.
Also add in PR desc and/or changelog the full impact of this PR.
| @@ -2,6 +2,14 @@ | |||
|
|
|||
| ## Unreleased | |||
|
|
|||
| * **Breaking Change** : | |||
| [SuppressDownstreamInstrumentation](https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/src/OpenTelemetry.Instrumentation.GrpcNetClient#suppressdownstreaminstrumentation) | |||
| option will no longer be supported when used with certain versions of | |||
There was a problem hiding this comment.
Not for this PR but we should write the exact version here when we do the release.
Fixes #4809
Design discussion issue #
Changes
Removes the SDK dependency from http client instrumentation. Note that with this change, the SuppressDownstreamInstrumentation will no longer work. We can bring the dependency back for pre-release versions to support SuppressDownstreamInstrumentation.
For details on how this change impact users, see open-telemetry/opentelemetry-dotnet-contrib#1727
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changes