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

Phased out Grpc package for OTLP Exporter. #3885

Closed
wants to merge 10 commits into from

Conversation

Yun-Ting
Copy link
Contributor

@Yun-Ting Yun-Ting commented Nov 9, 2022

Fixes #3421.

Changes

  • Removed Grpc package.
  • Added HttpClient as a hard requirement for configuring GrpcChannelOptions.

For significant contributions please make sure you have completed the following items:

  • Appropriate CHANGELOG.md updated for non-trivial changes
  • Design discussion issue #
  • Changes in public API reviewed

@codecov
Copy link

codecov bot commented Nov 9, 2022

Codecov Report

Merging #3885 (10f6a34) into main (a8b72df) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 10f6a34 differs from pull request most recent head 982c1dc. Consider uploading reports for the commit 982c1dc to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #3885   +/-   ##
=======================================
  Coverage   87.34%   87.34%           
=======================================
  Files         279      279           
  Lines       10782    10783    +1     
=======================================
+ Hits         9417     9418    +1     
  Misses       1365     1365           
Impacted Files Coverage Δ
...mentation/ExportClient/BaseOtlpGrpcExportClient.cs 83.33% <ø> (-0.88%) ⬇️
...TelemetryProtocol/OtlpExporterOptionsExtensions.cs 97.87% <100.00%> (+1.13%) ⬆️
src/OpenTelemetry/BatchExportProcessor.cs 82.24% <0.00%> (-1.87%) ⬇️
...ZPages/Implementation/ZPagesExporterEventSource.cs 62.50% <0.00%> (+6.25%) ⬆️

@Yun-Ting Yun-Ting marked this pull request as ready for review November 10, 2022 00:23
@Yun-Ting Yun-Ting requested a review from a team November 10, 2022 00:23
@Yun-Ting Yun-Ting changed the title [WIP] Phase out Grpc package for OTLP Exporter. Phased out Grpc package for OTLP Exporter. Nov 10, 2022
<ItemGroup>
<ProjectReference Include="$(RepoRoot)\src\OpenTelemetry\OpenTelemetry.csproj" />
<PackageReference Include="Google.Protobuf" Version="$(GoogleProtobufPkgVer)" />
<PackageReference Include="Grpc.Tools" Version="$(GrpcToolsPkgVer)" PrivateAssets="all" />
<PackageReference Include="Grpc.Net.Client" Version="$(GrpcNetClientPkgVer)" />
Copy link
Member

Choose a reason for hiding this comment

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

I think the primary concern here is that this is probably a breaking change for some users. Consider .NET Framework users for whom things are working just fine today, but with this change they now have to configure the HttpClient for things to work. Worst part is that they'll only find this out at runtime.

Copy link
Member

Choose a reason for hiding this comment

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

One idea we'd discussed in the past is to introduce separate OTLP/gRPC and OTLP/HTTP exporter packages. Something like

  • OpenTelemetry.Exporter.OpenTelemetryProtocol.Grpc: This package would only depend on the new Grpc.Net.Client as you've done here. The benefit of it being a new package is that it won't break existing users.
  • OpenTelemetry.Exporter.OpenTelemetryProtocol.Http: This could be done as a separate effort. Mostly just provides symmetry, but also reduces the number of transitive dependencies for users who don't want to use gRPC.

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.

Use Grpc.Net.Client with netstandard2.0 for OpenTelemetry.Exporter.OpenTelemetryProtocol
2 participants