L44: New RPC Status API for Python #118

lidizheng · 2018-12-08T00:36:19Z

gRFC: https://github.com/lidizheng/proposal/blob/python-rich-status/L44-python-rich-status.md
Discussion Thread: https://groups.google.com/forum/#!topic/grpc-io/8Ys6ba8gpLc

gnossen · 2018-12-08T00:48:40Z

L44-python-rich-status.md

+            details=[detail]
+        )
+
+        context.set_status(*rpc_status.convert(rich_status))


It's a little bit odd to have to use a splat here. Any way we can eliminate it?

I don't have better idea than this one. If we are going to make two variable mandatory and one optional, this is my best solution. Also, I don't like to make a function accept different set of arguments.

An alternative way can satisfy both criteria is make the function signature like this:

def set_status(self, status_tuple):

# Usage for non-grpcio-status users context.set_status((code, message))

But the additional parentheses doesn't look good.

Do you have any insight?

My thought was to make the function accept different types of arguments and determine at runtime how to unpack them. But there are certainly arguments against that type of approach. The splat should be fine.

* Update our final choice * Not going to swap the two concepts anymore * Mark the client-side API as optional * Correct my terrible grammar

lidizheng · 2018-12-14T19:31:43Z

@srini100 I have updated the gRFC. PTAL

lidizheng · 2019-02-19T20:01:16Z

This API seems break opentracing-contrib/python-grpc, but they got it fixed by opentracing-contrib/python-grpc#17.

ericgribkoff

Looks good overall!

Mostly minor comments, with one exception: I would prefer more precise language in the Abstract section (it's only ~3 sentences, but they are the part most people will read). I tried to provide a sample revision - I don't think my sample abstract is very good itself, so please do improve upon or ignore it and replace with a better version entirely.

ericgribkoff · 2019-02-22T22:38:22Z

L44-python-rich-status.md

+
+## Abstract
+
+The current design of status API doesn't support rich status. Also the concept of status "details" doesn't comply with the gRPC spec. So, after discussion with @gnossen and @ericgribkoff, we think we should add a new set of status APIs to correct them.


AFAIK rich status is not a term used elsewhere, and would need to be defined. I would prefer something more general here, such as:

The current design of the gRPC Python servicer API for RPCs terminated with a non-OK status does not easily allow pairing the failure with associated additional information not expressible by the grpc-status and grpc-message fields. This proposal adds a rich Status class to gRPC Python that allows coupling the failure's status code, error string, and arbitrary additional information about the failure (sent as trailing metadata) in a unified interface.

(and please remove the second sentence about "after discussions with")

Updated to your version. Thank you for making it more clear.

ericgribkoff · 2019-02-22T22:39:06Z

L44-python-rich-status.md

+
+## Background
+
+The gRPC [Spec](https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#responses) defined two trailing data to indicate the status of an RPC call `Status` and `Status-Message`. However, status code and a text message themselves are insufficient to express more complicated situation like the RPC call failed because of quota check of specific policy, or the stack trace from the crashed server, or obtain retry delay from the server (see [error details](https://github.com/googleapis/api-common-protos/blob/master/google/rpc/error_details.proto)).


Status -> grpc-status
Status-Message -> grpc-message

ericgribkoff · 2019-02-22T22:39:46Z

L44-python-rich-status.md

+
+The gRPC [Spec](https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#responses) defined two trailing data to indicate the status of an RPC call `Status` and `Status-Message`. However, status code and a text message themselves are insufficient to express more complicated situation like the RPC call failed because of quota check of specific policy, or the stack trace from the crashed server, or obtain retry delay from the server (see [error details](https://github.com/googleapis/api-common-protos/blob/master/google/rpc/error_details.proto)).
+
+The one primary use case of this feature is Google Cloud API. The Google Cloud API needs it to transport rich status from the server to the client. Also, they are the owner of those proto files.


Delete the Also sentence (why does it matter who owns "those" proto files mentioned above?)

ericgribkoff · 2019-02-22T22:40:20Z

L44-python-rich-status.md

+
+The one primary use case of this feature is Google Cloud API. The Google Cloud API needs it to transport rich status from the server to the client. Also, they are the owner of those proto files.
+
+So, the gRPC team introduced a new internal trailing metadata entry `grpc-status-details-bin` to serve this purpose. It takes in serialized proto `google.rpc.status.Status` message binary and transmits as binary metadata. However, since gRPC is a ProtoBuf-agnostic framework, it can't enforce either the content of the binary data be set in the trailing metadata entry, nor the consistency of status code/status message with the additional status detail proto message. The result of this unsolvable problem is that for three major implementations of gRPC we have, each of them is slightly different about this behavior.


s/be set/set/

Need to say that google.rpc.status.Status contains its own code and message fields (probably after second sentence here, before "However,"). This lets a new reader understand the problems with consistency that are then described.

The first mistake is corrected.
The description for Status message is added:

Notably, the google.rpc.status.Status message contains its own code and message fields.

ericgribkoff · 2019-02-22T22:41:36Z

L44-python-rich-status.md

+
+So, which paradigm should Python follow?
+
+### The Proto of Status


"Status Proto"

ericgribkoff · 2019-02-22T22:42:24Z

L44-python-rich-status.md

+
+The name of the class is `Status`, because it is shorter and cleaner than `RpcStatus` or `ServerRpcStatus`. In the future, we might want to utilize it at client side as well.
+
+The metadata field of `Status` is `trailing_metadata` instead of `metadata`. The `Status` should be the final status of an RPC, so it should assist developers to get information that only accessible at the end of the RPC. Also, if we want to support it at client side, we need to separate `initial_metadata` and `trailing_metadata`.


I don't understand the last sentence about supporting this at client side, I think it can just be deleted. On the client side the status would not included initial_metadata...but it doesn't matter for this gRFC in either case.

The sentence is removed. I meant there have to be two fields to support both server-side and client-side.

ericgribkoff · 2019-02-22T22:42:55Z

L44-python-rich-status.md

+
+### Server side API
+
+gRPC Python team encountered lots of disagreements about how this part should be implemented. Available options are listed in the `Rationale` section. Here is our final consensus as a team, but feel free to comment about other options.


I suggest removing the first sentence about internal disagreements - I don't think it adds anything for future readers...

ericgribkoff · 2019-02-22T22:43:28Z

L44-python-rich-status.md

+        # The method handler will abort
+```
+
+### Optional: Client side API


Delete this section and leave to a future gRFC?

ericgribkoff · 2019-02-22T22:45:51Z

L44-python-rich-status.md

+
+### Why we need a new package `grpcio-status`?
+
+Although we can't enforce the consistency of status code, status message, and status details, there is still value for the new package to help developers validate that. It demonstrates our recommendation through the design.


Although the grpcio package can't enforce the consistency of status code and status message with the information embedded in the rich Status proto, there is still value to provide an additional package - one which can depend on protobuf and is able to verify that these elements are consistent - to help developers use the API in its intended fashion.. It demonstrates our recommendation through the design.

ericgribkoff · 2019-02-22T22:46:57Z

L44-python-rich-status.md

+Ideally speaking, the **status message** and **status details** are misused in Python implementation, and this proposal is our best chance to get them right. According to spec, the correct definition should be as followed:
+
+* **Status message** should be interpreted as a text (Unicode allowed) field that indicates the status of the RPC call, and it is intended to be read by the developers.
+* **Status details** should be understood as a encoded `google.rpc.status.Status` proto message that serves as rich status error details about the RPC Call.


There is not actually a spec for this, is there? I would just simplify this section by saying what Python calls 'details' actually corresponds to grpc-message, and that this discrepancy is unfortunately deeply embedded in the gRPC Python API and unlikely to be changed.

The specs for both "grpc-message" and "grpc-status-details-bin" fields are not directly describing their usage, and you are right we shouldn't do the definition for them in this gRFC. The last section is rewrote into:

Semantic difference of details between gRPC Python and Status proto

gRPC Python uses the field name details to store data that will later be transmitted in grpc-message header. But the details field in proto Status is a list for error details proto messages. Unfortunately, this discrepancy is deeply embedded in the gRPC Python API and unlikely to be changed.

lidizheng · 2019-02-22T23:11:10Z

@ericgribkoff Thank you for the thorough review! I fixed most issue you posted in the review. The last section rewrite still in progress.

ericgribkoff

LGTM

ericgribkoff · 2019-02-22T23:47:51Z

L44-python-rich-status.md


 ## Background

-The gRPC [Spec](https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#responses) defined two trailing data to indicate the status of an RPC call `Status` and `Status-Message`. However, status code and a text message themselves are insufficient to express more complicated situation like the RPC call failed because of quota check of specific policy, or the stack trace from the crashed server, or obtain retry delay from the server (see [error details](https://github.com/googleapis/api-common-protos/blob/master/google/rpc/error_details.proto)).
+The gRPC [Spec](https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#responses) defined two trailing data to indicate the status of an RPC call `grpc-status` and `grpc-Message`. However, status code and a text message themselves are insufficient to express more complicated situation like the RPC call failed because of quota check of specific policy, or the stack trace from the crashed server, or obtain retry delay from the server (see [error details](https://github.com/googleapis/api-common-protos/blob/master/google/rpc/error_details.proto)).


nit: grpc-message (lower case m)

Typo fixed.

New RPC Status API for Python

12d4b30

lidizheng added the enhancement label Dec 8, 2018

lidizheng assigned gnossen, ericgribkoff and srini100 Dec 8, 2018

lidizheng requested review from gnossen, ericgribkoff and srini100 December 8, 2018 00:36

Learn how to count five...

694a88d

gnossen reviewed Dec 8, 2018

View reviewed changes

lidizheng changed the title ~~New RPC Status API for Python~~ L44: New RPC Status API for Python Dec 10, 2018

Add grpc-io thread

b5f9b1a

lidizheng mentioned this pull request Dec 11, 2018

Rich status transport implementation grpc/grpc#17384

Closed

lidizheng added 2 commits December 14, 2018 11:15

Update the design doc

a6fb004

* Update our final choice * Not going to swap the two concepts anymore * Mark the client-side API as optional * Correct my terrible grammar

Update the pull requests

b7ef8c4

This was referenced Dec 14, 2018

New abort with grpc.Status API grpc/grpc#17481

Merged

Add grpcio-status extension package grpc/grpc#17490

Merged

lidizheng mentioned this pull request Dec 28, 2018

Python - Support send structured error details grpc/grpc#17601

Closed

Minor fix of the snippet

8f14ebf

Update the reviewer to ericgribkoff@

468c114

ericgribkoff reviewed Feb 22, 2019

View reviewed changes

Adopt reviewer's suggestions

2013adb

Rewrite the last section

1fcd3e9

ericgribkoff approved these changes Feb 22, 2019

View reviewed changes

Fix typo

3787bda

lidizheng merged commit 105bf98 into grpc:master Feb 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

L44: New RPC Status API for Python #118

L44: New RPC Status API for Python #118

lidizheng commented Dec 8, 2018 •

edited

Loading

gnossen Dec 8, 2018

lidizheng Dec 8, 2018

gnossen Dec 10, 2018

lidizheng commented Dec 14, 2018

lidizheng commented Feb 19, 2019

ericgribkoff left a comment

ericgribkoff Feb 22, 2019

lidizheng Feb 22, 2019

ericgribkoff Feb 22, 2019

lidizheng Feb 22, 2019

ericgribkoff Feb 22, 2019

lidizheng Feb 22, 2019

ericgribkoff Feb 22, 2019

ericgribkoff Feb 22, 2019

lidizheng Feb 22, 2019

ericgribkoff Feb 22, 2019

lidizheng Feb 22, 2019

ericgribkoff Feb 22, 2019

lidizheng Feb 22, 2019

ericgribkoff Feb 22, 2019

lidizheng Feb 22, 2019

ericgribkoff Feb 22, 2019

lidizheng Feb 22, 2019

ericgribkoff Feb 22, 2019

lidizheng Feb 22, 2019

ericgribkoff Feb 22, 2019

lidizheng Feb 22, 2019

lidizheng commented Feb 22, 2019

ericgribkoff left a comment

ericgribkoff Feb 22, 2019

lidizheng Feb 22, 2019


		## Abstract

		The current design of status API doesn't support rich status. Also the concept of status "details" doesn't comply with the gRPC spec. So, after discussion with @gnossen and @ericgribkoff, we think we should add a new set of status APIs to correct them.


		## Background

		The gRPC [Spec](https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#responses) defined two trailing data to indicate the status of an RPC call `Status` and `Status-Message`. However, status code and a text message themselves are insufficient to express more complicated situation like the RPC call failed because of quota check of specific policy, or the stack trace from the crashed server, or obtain retry delay from the server (see [error details](https://github.com/googleapis/api-common-protos/blob/master/google/rpc/error_details.proto)).


		The gRPC [Spec](https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#responses) defined two trailing data to indicate the status of an RPC call `Status` and `Status-Message`. However, status code and a text message themselves are insufficient to express more complicated situation like the RPC call failed because of quota check of specific policy, or the stack trace from the crashed server, or obtain retry delay from the server (see [error details](https://github.com/googleapis/api-common-protos/blob/master/google/rpc/error_details.proto)).

		The one primary use case of this feature is Google Cloud API. The Google Cloud API needs it to transport rich status from the server to the client. Also, they are the owner of those proto files.


		The one primary use case of this feature is Google Cloud API. The Google Cloud API needs it to transport rich status from the server to the client. Also, they are the owner of those proto files.

		So, the gRPC team introduced a new internal trailing metadata entry `grpc-status-details-bin` to serve this purpose. It takes in serialized proto `google.rpc.status.Status` message binary and transmits as binary metadata. However, since gRPC is a ProtoBuf-agnostic framework, it can't enforce either the content of the binary data be set in the trailing metadata entry, nor the consistency of status code/status message with the additional status detail proto message. The result of this unsolvable problem is that for three major implementations of gRPC we have, each of them is slightly different about this behavior.


		So, which paradigm should Python follow?

		### The Proto of Status


		The name of the class is `Status`, because it is shorter and cleaner than `RpcStatus` or `ServerRpcStatus`. In the future, we might want to utilize it at client side as well.

		The metadata field of `Status` is `trailing_metadata` instead of `metadata`. The `Status` should be the final status of an RPC, so it should assist developers to get information that only accessible at the end of the RPC. Also, if we want to support it at client side, we need to separate `initial_metadata` and `trailing_metadata`.


		### Server side API

		gRPC Python team encountered lots of disagreements about how this part should be implemented. Available options are listed in the `Rationale` section. Here is our final consensus as a team, but feel free to comment about other options.


		### Why we need a new package `grpcio-status`?

		Although we can't enforce the consistency of status code, status message, and status details, there is still value for the new package to help developers validate that. It demonstrates our recommendation through the design.

L44: New RPC Status API for Python #118

L44: New RPC Status API for Python #118

Conversation

lidizheng commented Dec 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidizheng commented Dec 14, 2018

lidizheng commented Feb 19, 2019

ericgribkoff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Semantic difference of details between gRPC Python and Status proto

lidizheng commented Feb 22, 2019

ericgribkoff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidizheng commented Dec 8, 2018 •

edited

Loading

Semantic difference of `details` between gRPC Python and `Status` proto