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

[OTLP exporter] Remove dependency on deprecated gRPC library #4860

Conversation

alanwest
Copy link
Member

Fixes #3421
Fixes #4613

We have discussed recently dropping our dependency on the old gRPC library. Let's use this PR to make a final decision.

The gRPC C# library may technically still be in "maintenance mode", though from the following comment this may change "any day now" grpc/grpc#32719 (comment):

So in practice, Grpc.Core is still "maintained", but it might get declared deprecated any day now, without any extra notice. There's still some small chance that we'll reconsider (based on the data available to us) and continue supporting Grpc.Core for some more time, but AFAICT this seem unlikely and I definitely wouldn't count on it.

There is a possibility that dropping this dependency will be a breaking change for some .NET Framework users. The newer Grpc.Net.Client library does support .NET Framework under certain scenarios (i.e., Windows 11 / Windows Server 2022 using the WinHttpHandler).

Until very recently, the newer library Grpc.Net.Client did not have a net462 target. To support our .NET Framework users, this would have required us to take a dependency on WinHttpHandler and to construct the gRPC channel to use it. However, Grpc.Net.Client as of version 2.56.0 introduced a net462 target which appears to make it "just work" on .NET Framework (still with the Win 11/Server 2022 requirement). See: grpc/grpc-dotnet#2220 (comment)


I'm just putting this PR out there to start the conversation. I have not yet had a chance to try it out on .NET Framework to validate that using Grpc.Net.Client version 2.56.0 without any special configuration works.

@codecov
Copy link

codecov bot commented Sep 16, 2023

Codecov Report

Merging #4860 (9d33848) into main (fe78453) will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4860      +/-   ##
==========================================
- Coverage   83.91%   83.87%   -0.04%     
==========================================
  Files         293      293              
  Lines       12028    12023       -5     
==========================================
- Hits        10093    10084       -9     
- Misses       1935     1939       +4     
Files Changed Coverage
...mentation/ExportClient/BaseOtlpGrpcExportClient.cs ø
...yProtocol/Implementation/ExportClient/OtlpRetry.cs ø
...TelemetryProtocol/OtlpExporterOptionsExtensions.cs ø

@pengweiqhca
Copy link

Grpc.Net.Client has many limitation, I think should provide a Grpc.Core.Api.CallInvoker or Grpc.Core.ChannelBase factory, it will give the most flexibility with how a user provides their desired gRPC connection.

@Kielek
Copy link
Contributor

Kielek commented Sep 18, 2023

Unsupported scenarios OTLP over GRPC on

I was trying it on Win10 and it is really not working.

@alanwest
Copy link
Member Author

I think should provide a Grpc.Core.Api.CallInvoker or Grpc.Core.ChannelBase factory, it will give the most flexibility with how a user provides their desired gRPC connection.

Unfortunately, we want to avoid exposing either CallInvoker or ChannelBase in our options class as you've done in #4815. The reason is because this creates a coupling between our API and specific gRPC client libraries. We are currently considering if there is a way we can eliminate our dependency on any gRPC library entirely.

@alanwest
Copy link
Member Author

I was trying it on Win10 and it is really not working.

Yes, as I suspected, definitely a breaking change. We'll discuss in the SIG meeting tomorrow. Whether we drop it now or not we need to begin to consider what our plan is if, in the event the library becomes unmaintained, a security issue is found.

@pjanotti
Copy link
Contributor

This may be a moment to bring #2959 into consideration.

@alanwest
Copy link
Member Author

This may be a moment to bring #2959 into consideration.

This crossed my mind, however, changing the default to OTLP/HTTP would expand the impact to all users rather than just users running on older versions of Windows. Nonetheless, we'll discuss today.

@@ -1,6 +1,6 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFrameworks>$(DefaultTargetFrameworks);netstandard2.1</TargetFrameworks>
<TargetFrameworks>$(DefaultTargetFrameworks)</TargetFrameworks>
Copy link
Member

@cijothomas cijothomas Sep 19, 2023

Choose a reason for hiding this comment

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

<DefaultTargetFrameworks>net6.0;netstandard2.0;net462</DefaultTargetFrameworks>

For quick/easy ref while reviewing PR

<DefaultTargetFrameworks>net6.0;netstandard2.0;net462</DefaultTargetFrameworks>

@cijothomas
Copy link
Member

Historically, this repo has dropped support for certain .NET frameworks when the frameworks went out of support itself, and this was done with a minor version bump. This PR is similar to that : OTLP Exporter in .NET Framework needs a particular grpc library, and that library itself is going out of support. Its reasonable for OTel .NET to drop support (and do minor version bump only) as well.

(Similar is lack of Prometheus exporter for .NET Framework that can be used in production. There is no underlying library support for that, and we are not building it ourselves...)

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.

Agree with this direction. Not marking as explicitly approved, as we need to update changelogs/readme etc. to reflect this.

@vishweshbankwar
Copy link
Member

This change looks good to me as well. It is better to do it sooner so that it gives time to users to take appropriate mitigation steps (use http based version or different OS). An announcement for this change could be a good option when we ship the first pre-release version.

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Sep 28, 2023
@Kielek
Copy link
Contributor

Kielek commented Oct 4, 2023

@alanwest, there is new announcement about Grpc.Core package. The maintenance mode is extended about at least one year from now (October 2024).

See https://groups.google.com/g/grpc-io/c/iEalUhV4VrU?pli=1

I suppose we can wait a bit before merging these changes.

@github-actions github-actions bot removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label Oct 5, 2023
@alanwest
Copy link
Member Author

The maintenance mode is extended about at least one year from now (October 2024).

Interesting, thanks for the heads up. Given the strong support expressed in removing the dependency, I'd still be a fan of removing it sooner than later giving folks more time to adjust.

I am moving this PR to draft for now. I have not had the time yet, but from our discussion in our SIG meeting a few weeks ago, my remaining TODO on this PR was to try adding an integration test for .NET Framework.

Even if we ultimately do decide to hold off removing the dependency awhile longer it'll still be nice to have this branch ready for whenever we are.

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Oct 17, 2023
@github-actions
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Oct 24, 2023
@mierzejewsky
Copy link

I have a question about this PR, as October 2024 is soon... Anyone considers reopening this topic?

@cijothomas
Copy link
Member

Tagging @alanwest

@Kielek
Copy link
Contributor

Kielek commented Sep 17, 2024

I think that it was again started by @vishweshbankwar in #5731. I doubt that it will be finished by him. Do you know what is the status of this PR?

@reyang
Copy link
Member

reyang commented Sep 17, 2024

I think there is no plan to reopen/resurrect this PR. The plan is to remove gRPC library dependency entirely as mentioned by @Kielek #4860 (comment).

@alanwest
Copy link
Member Author

I cannot find any updates regarding the end of support extension to October 2024. If we find that it will be extended even further then I do not think there's any rush to remove the dependency to the old gRPC library.

Though, if someone can validate that it truly is ending October 2024, then resurrecting this PR could make sense. It would in no way affect the longer term desire to eliminate dependencies on all gRPC libraries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Stale Issues and pull requests which have been flagged for closing due to inactivity
Projects
None yet
8 participants