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

Fix grpc middleware interceptor not PostCall-ing when a streaming RPC with non-streaming server finishes successfully. #725

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alexandrupitis1
Copy link

Current Client-side Stream Interceptor implementation is bugged - it assumes that if the RPC is streaming, then the Server must be streaming responses too. This is incorrect, as a Streaming RPC can be either a Bidi stream ( which are currently handled properly ), or a Client-side only stream. In this case, if the RPC finished successfully, then PostCall never fires as Interceptor assumes it should get an io.EOF ( which obviously does not make sense when Server is not sending a response stream ).

This most obviously manifests when using it with Prometheus reporter - client side metrics for such RPCs that depend on PostCall ( grpc_client_handled_total, grpc_client_handling_seconds ) are not exported.

Changes

  • Client StreamInterceptor now handles cases when Server is streaming or non-streaming separately.

Verification

  • I created a POC with a Client-streaming, Server non-streaming RPC and modified the example in this repo to run it. Verified that the example does not export certain prometheus metrics ( grpc_client_handled_total, grpc_client_handling_seconds ) for said RPC.
  • Applied the fix, verified again that the metrics are now exported

.

Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

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

Nice! Could you add a test for this?

@alexandrupitis1
Copy link
Author

Hi there Johan,

Sure - I'll take a stab next week, pretty loaded right now with other things.

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.

3 participants