-
Notifications
You must be signed in to change notification settings - Fork 773
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
bump grpc client version to fix unobserved exception #4573
bump grpc client version to fix unobserved exception #4573
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4573 +/- ##
=======================================
Coverage 84.86% 84.87%
=======================================
Files 313 313
Lines 12604 12604
=======================================
+ Hits 10697 10698 +1
+ Misses 1907 1906 -1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@masonyc - Could you please also update the changelog for otlp exporter https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/CHANGELOG.md
Lets wait a bit, I do not have a chance to verify impact on .NET AutoInstrumentation. I suppose it will be significant. |
Finally, I have find the time to review impact. For Auto Instrumentation, it will leads to reduce set of application which can be auto-instrumented without adjustment for dependencies. Similar discussion was done for #4407 Especially, we will have to increase minimal instrumented version Grpc.Client.Net from 2.43.0 to the upgraded one. As 2.45 contains obvious bug-fix, I see the reason to do this, but I have doubts if it should go up to 2.53.0. @alanwest, you were mainly checking what we can do for protobuf. I suppose that you have also good insight for this particulat package. EDIT: Similar thing for #4576 (if the user is using .NET version which brings 7.0.1 or 7.0.0). |
@masonyc thanks for your patience! We spoke about this PR today in our weekly meeting. Regarding @Kielek's comment:
We agreed a more conservative bump to 2.45 is preferable, but before we do that, I wanted to understand the issue a bit more. I haven't studied this issue closely enough to understand in what circumstance this bug manifests itself with OpenTelemetry .NET. My thought is that if we understand the bug a bit more then maybe we could find a way to fix it by changing how we use gRPC client to get around the bug rather than bumping the package version. I've briefly reviewed grpc/grpc-dotnet#1656, but the repro it provides does not resemble how we currently invoke gRPC client, so it left us wondering how this bug is manifested in OpenTelemetry .NET. @masonyc do you have a repro you could share generating this bug using OpenTelemetry .NET? |
It isn’t specific to how the gRPC client is being used. It’s a Task inside the internal client code that isn’t explicitly observed when there is an exception making a call (such as a transient network issue). If you’re making a call, then the internal code path that triggers the gRCP client behavior mentioned in the issue will be exercised. |
Gotcha. I've had a chance to look more closely and this makes sense now. @masonyc please change to 2.45 and I think we're good to merge. |
Fixes #4570
Changes
Bump grpc net client to 2.53
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial changes