Skip to content

feat: add debug log when sending requests via gRPC#2271

Closed
parthea wants to merge 9 commits intomainfrom
add-debug-log-grpc-transport
Closed

feat: add debug log when sending requests via gRPC#2271
parthea wants to merge 9 commits intomainfrom
add-debug-log-grpc-transport

Conversation

@parthea
Copy link
Contributor

@parthea parthea commented Dec 3, 2024

Towards b/381100907

The changes in this PR are only for unary-unary calls. unary-stream calls still need to be added.

@product-auto-label product-auto-label bot added the size: l Pull request size is large. label Dec 3, 2024
@vchudnov-g vchudnov-g force-pushed the add-debug-log-grpc-transport branch from b3df060 to 4c73a46 Compare December 6, 2024 19:56
grpc_response = {
"payload": type(result).to_json(result),
"metadata": dict(metadata),
"status": "OK",
Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC, if we get here the status is OK. In case of a gRPC error, an exception is thrown...or do we catch in the lower levels? If so, how do we get the status?

if CLIENT_LOGGING_SUPPORTED and _LOGGER.isEnabledFor(logging.DEBUG):
grpc_request = {
"payload": type(request).to_json(request),
"requestMethod": "grpc",
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this for gRPC. method and url are rest only fields.

Copy link
Contributor

Choose a reason for hiding this comment

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

But how will the user looking at the logs know (from the logs only) that this was a gRPC request? They have to reason from not having a URL or method field, or from looking at the metadata. We might as well make it explicit for convenience.

No need to block on this; we can change it later.

@product-auto-label product-auto-label bot added size: xl Pull request size is extra large. and removed size: l Pull request size is large. labels Dec 9, 2024
@vchudnov-g
Copy link
Contributor

As per internal discussions, we'll be splitting this PR into smaller PRs

@ohmayr ohmayr closed this Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size: xl Pull request size is extra large.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants