-
Notifications
You must be signed in to change notification settings - Fork 70
feat: add client debug logging support for streaming gRPC calls #2336
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
base: main
Are you sure you want to change the base?
Conversation
d69564c
to
27463b2
Compare
9519eda
to
d190cba
Compare
@@ -59,7 +59,12 @@ except ImportError: # pragma: NO COVER | |||
_LOGGER = std_logging.getLogger(__name__) | |||
|
|||
|
|||
class _LoggingClientInterceptor(grpc.UnaryUnaryClientInterceptor): # pragma: NO COVER | |||
class _LoggingClientInterceptor(grpc.UnaryUnaryClientInterceptor, grpc.UnaryStreamClientInterceptor): # pragma: NO COVER | |||
def intercept_unary_stream(self, continuation, client_call_details, request): |
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.
Can we add a note here to clarify that:
- request logging for streaming calls lives in the generator but response logging for streaming calls lives in python-api-core, briefly explaining why.
- request / response logging for unary calls lives in the generator.
This'll make it easier for us to track down the implementation in the future.
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.
Fixed in 84c7986
I don't think I've encountered the term "unary-stream" before, and it took me seeing the examples in the PR description to be sure I understood what you meant. I suggest using standard terminology, "server streaming", or clarifying that that's what you mean by "unary-stream". Furthermore, this "logging-stream-from-the-server" functionality should also apply to bidi streaming. Does it? Do we have an example/golden? (Apologies if I've missed it; I focused this review on just looking at the redis golden files) |
Unary Stream is a term used in gRPC. I can't change any references to that but I have updated terminology in this PR which can be changed to use |
Fixes #2289
Also see necessary changes in googleapis/python-api-core#794 to add logging in the response iterator each time a chunk is received in the stream.
Sync Server Streaming
Async Server Streaming