vmagamedov / grpclib

Pure-Python gRPC implementation for asyncio
http://grpclib.readthedocs.io
BSD 3-Clause "New" or "Revised" License
936 stars 92 forks source link

Make _STATUS_DETAILS_KEY public #184

Closed thundergolfer closed 5 months ago

thundergolfer commented 8 months ago

We use grpclib in our Python client but grpcio in a server. In order for GRPC error details to propagate correctly we need this shim:

from grpclib.encoding.proto import ProtoStatusDetailsCodec
GRPC_PROTO_STATUS_DETAILS_CODEC = ProtoStatusDetailsCodec()

trailing_metadata: list[tuple[str, Any]] = []
if exc.details:
    # Translate grpclib's details handling into the form grpcio expects.
    status_details = GRPC_PROTO_STATUS_DETAILS_CODEC.encode(exc.status, exc.message, exc.details)
    trailing_metadata.append((_STATUS_DETAILS_KEY, status_details))

The main problem with this shim in my opinion is the dependence on a private constant. It'd be good if grpclib library consumer could depend on the changing of this constant's value being reflected in semver.

vmagamedov commented 7 months ago

Here is how error details should be encoded on the server-side (grpcio): https://github.com/grpc/grpc/blob/master/examples/python/errors/server.py

And here is an example of how you can read errors on the client-side (grpclib): https://grpclib.readthedocs.io/en/latest/errors.html#error-details

You don't have to encode/decode errors details manually and put them into trailers, this should be done automatically by corresponding libraries.

Also grpc-status-details-bin metadata is not our invention, it is kinda part of the protocol. And grpcio has the same constant in their codebase and looks like that it is also private: grpcio_status/grpc_status/_common.py#L20

You can create this constant in your codebase if you want, its value is not going to change, no need to import it from somewhere. In grpclib we treat this constant as an implementation detail, user should not deal with it so it shouldn't be a part of the public API.

Do you really need this shim? If yes then please explain why existing functionality is not enough.

thundergolfer commented 7 months ago

Thank you for the response. We're using ServicerContext.abort which is possibly why we can't take advantage of the existing mechanism provided by then library. I'll look into this further 👍.

thundergolfer commented 5 months ago

Happy to close this to reduce clutter 🙂