yarpc / yarpc-go

A message passing platform for Go
MIT License
412 stars 103 forks source link

gRPC encoding application errors #905

Closed kriskowal closed 7 years ago

kriskowal commented 7 years ago

I propose that we add an Rpc-Error header on the wire for both HTTP and gRPC, containing the name of the error case, e.g., bogus, instead of Yarpc-Protobuf-Raw-Response: 1. The case name would need to be expressed with yarpc.Error(caseName, message) or yarpc.Errorf in the handler. The absense of this error class would imply Unknown error for gRPC, which would have to fall back to an empty response body.

The yarpc.Error could be expressed for any encoding and would be inherent to ThriftRW codegen, naming the field of the result struct. As far as a gRPC client would be concerned, an application error response would only be distinguishable from a transport error only be examining the response message, so this would only be useful to Protobuf clients if they rolled the Result pattern by hand.

func handle(ctx context.Context, req *Request) {
    return yarpc.Error("bogus", "bogus reason for application error"), &Result{
        Success: nil,
        BogusError: &BogusError{
             Message: "bogus reason for application error",
             Severity: 10,
        },
    }
}

Over TChannel, yarpc.Error would indicate that the application error bit should be set in addition to the Rpc-Error response header.

This implies some work on the yarpc spec repository. https://github.com/yarpc/yarpc

@abhinav Please check for viability.

bufdev commented 7 years ago

The raw response header is the only way to accomplish this between encodings and transports given the current API, without adding a dependency on the grpc transport in the protobuf encoding (which would be a circular dependency, as this currently stands).

See https://github.com/yarpc/yarpc-go/commit/0ce6d0d6c61f49e74b973fe5f4ab6cd69a2de103 - I previously tried to accomplish this using Context, but you can't do this two-ways, and have to propagate this on the headers.

kriskowal commented 7 years ago

@peter-edge This is a tricky problem. It might be that we need to treat all Thrift application errors as successes (with an Rpc-Error header) over gRPC. Services using Protobuf to transport a success/errors result union are hypothetical, so no existing gRPC client/service expects them.