custom error to code handler #1330
Conversation
There was a problem hiding this comment.
Instead of returning three values like this:
FindInstance(ctx context.Context, id string) (*Instance, bool, error)we should return two values like this:
FindInstance(ctx context.Context, id string) (*Instance, error)and use drivers.ErrNotFound as a constant error for when the value wasn't found. Then from the caller, instead of doing:
inst, found, err := registry.FindInstance(ctx, req.InstanceId)
if err != nil {
return nil, err
}
if !found {
return nil, status.Error(codes.InvalidArgument, "instance not found")
}we would do:
inst, err := registry.FindInstance(ctx, req.InstanceId)
if err != nil {
if err == drivers.ErrNotFound {
return nil, status.Error(codes.InvalidArgument, "instance not found")
}
return nil, err
}| // CustomErrorToCode returns the Code of the error if it is a Status error | ||
| // otherwise use status.FromContextError to determine the Code. | ||
| // Log level for error codes is defined in logging.DefaultServerCodeToLevel | ||
| func CustomErrorToCode(err error) codes.Code { | ||
| if se, ok := err.(interface { | ||
| GRPCStatus() *status.Status | ||
| }); ok { | ||
| return se.GRPCStatus().Code() | ||
| } | ||
| contextStatus := status.FromContextError(err) | ||
| return contextStatus.Code() | ||
| } |
There was a problem hiding this comment.
Maybe drop the Custom and just call this ErrorToCode?
Also, do you think we should override logging.DefaultServerCodeToLevel and only emit codes.Internal as ERROR and everything else as info (or maybe some others as warning)?
There was a problem hiding this comment.
I think its ok for now since apart from codes.Internal
codes.Unknown, codes.Unimplemented, codes.DataLoss
gets logged as error which seems valid, may be we can wait and change later if needed. Anyways no strong opinions here.
There was a problem hiding this comment.
With the new queries refactor, there will probably be more "unknown" errors returned, so would be nice to make those "info" messages instead
|
changed |
* custom error to code handler * comment * fix test * review comments
codes.Canceledwhich is logged as InfoDefaultServerCodeToLevelfunc inoptions.gofilepanicfrom RegistryStore method implementation to handle context cancelled