Status of support for Grpc-Status-Details-Bin

[stevvooe]

I am trying to understand the current status of support for sending error details via the server. We have an API that requires mapping specific error codes back to error values on either end. There seem to be two options:

1. Define new, non-standard error codes to map to the conditions.
2. Annotate the errors using Status.Details with project specific detail records.

The option number 2 seems to be very solid but there isn't any support in the server. The areas where support would be have TODOs.

Right now, the current solution is to do some ever-so-light string matching.

What is the current status of error status details? Is all that is required for support is to serialize this structure when the header is encountered or is there more to it?

[dfawley]

Support for status details is present in the default server implementation (ref). Are you using the http.Handler method for serving?

[stevvooe]

I was led astray by the TODO in

grpc-go/transport/handler_server.go

Line 228 in eb7b130

// TODO: Support Grpc-Status-Details-Bin

and wasn't sure whether this is fully supported or not.

I actually went a different direction and ended up just mapping our internal errors to grpc codes, as the error code set is very good.

Perhaps, you need some documentation updates to make it clear that grpc.ErrorDesc et al are deprecated in favor of status, as well some supported paths for including error details.

Is there any way I can help here?

[dfawley]

Using the standard set of codes is always the first thing you should attempt -- having a global set of codes that all applications in your organization support has many benefits.

grpc.ErrorDesc and friends all have "deprecated" in their comments and pointers to the status package. There is also #1233 to make details a bit more friendly to use. Do you have any other ideas that might help?

[stevvooe]

@dfawley We are generally on board with moving to the status package, but most new developers end up discovering the grpc.ErrorXXX functions and just use those. The changes in #1233 look good (WithDetail, specifically).

I think the only real thing missing is the ability to add service-specific error codes, complete with string display (stringer doesn't lend itself well to extendable code sets). The case we needed was to represent a locked resource. We've decided to use Unavailable, as that represents the correct user action upon encountering that condition.

If you're curious about the approach, the PR is containerd/containerd#1115. I am pretty happy with the result but would like to put stack traces from pkg/error into the details. We've basically mapped all of the error values to GRPC equivalents and throw those from deep application code. It avoids the coupling with grpc error codes, while avoiding ambiguity.

For the most part, the struggle is around having application errors become grpc errors, then having those become application errors on the client side, maintaining context and semantics. I think as long as the design keeps that in mind, we'll have what we need.

[dfawley]

I think the only real thing missing is the ability to add service-specific error codes, complete with string display

It's possible to use interceptors on the server and client to convert other types of errors to/from gRPC status errors by marshaling/unmarshaling them into the details. If the status package is insufficient for your needs, this is what I would recommend.

With the Details/WithDetails added in #1233, I don't think there's anything else to be done here. Let me know if you still have any problems or concerns.

[stevvooe]

@dfawley I think these are sufficient. If I find anything that is "impossible", I'll raise an issue and see if we can't find a solution.

[c4milo]

@dfawley, i'm trying to use the new Details/WithDetails added in #1233 with grpc-gateway, using its custom httpError handler. But, I haven't been able to make it work. The grpc client call always returns Details:[]*any.Any(nil). Any help or code sample is very much appreciated.

[dfawley]

@c4milo, what is your server returning? It should do something like:

s := status.New(codes.InvalidArgument, "bad user")
if d, err := s.WithDetails(<detail proto message(s)>); err == nil {
  return d.Err()
}
log.Warning("!?")
return s.Err()

@ghasemloo Hrm, maybe we should make WithDetails return the unmodified s on error. The most common usage pattern is surely:

s, _ := status.New(codes.InvalidArgument, "bad user").WithDetails(<detail proto message(s)>)
// If an error happens, what can you do?  Just ignore it.
return s.Err()

[ghasemloo]

@dfawley seems reasonable to me. I can send a pull request to fix that.
ps: I feel that if WithDetails fails it is most likely indicates a bug in code and I would log it as error and try to continue, not just ignore.

@steeve you may find these useful:
https://github.com/googleapis/googleapis/blob/master/google/rpc/code.proto
https://cloud.google.com/apis/design/errors

If a resource is lock and you want the caller to retry I think Aborted would be the typical choice for error code I think.

@c4milo there are a few examples in the tests in status_test.go, I can add some go examples based on #1233 (comment)

[dfawley]

@ghasemloo, that would be great, thanks. Yes, it's probably best for the user to log or inc a metric on error, but still return s.Err() from the server handler.

[c4milo]

@dfawley, @ghasemloo, that's exactly what I'm returning but I get nil details from the client. I might be missing something important or this just does not work using GRPC's HTTP handler.

Server: https://github.com/c4milo/hello/blob/master/hello_service.go#L47-L53
Client: https://github.com/c4milo/hello-test/blob/master/main.go

Results:

[dfawley]

or this just does not work using GRPC's HTTP handler.

Sorry, yes, that is it. We did not add support for this feature through that path of serving yet. It should be fairly straightforward to add it if you are interested in contributing (or anyone else, if you're not).

[c4milo]

@dfawley, I can give it a try if you are kind to fast-track my way into the code base, please :)

[dfawley]

@c4milo Sure thing. I can't remember why I didn't add this when I implemented status details in grpc's server. But I don't think it was because it was going to be any more difficult.