From d8343f8132d9eb3a48aa0eca2d350cbe247517f2 Mon Sep 17 00:00:00 2001 From: Martin Chodur Date: Sat, 28 Sep 2019 00:03:57 +0200 Subject: [PATCH] feat: added error unwrapping Signed-off-by: Martin Chodur --- CHANGELOG.md | 3 ++ makefile | 2 - packages/grpcstatus/grpcstatus.go | 57 +++++++++++++++++++++ packages/grpcstatus/grpcstatus1.13+_test.go | 26 ++++++++++ packages/grpcstatus/grpcstatus_test.go | 34 ++++++++++++ packages/grpcstatus/native_unwrap1.12-.go | 11 ++++ packages/grpcstatus/native_unwrap1.13+.go | 17 ++++++ server_metrics.go | 8 +-- 8 files changed, 152 insertions(+), 6 deletions(-) create mode 100644 packages/grpcstatus/grpcstatus.go create mode 100644 packages/grpcstatus/grpcstatus1.13+_test.go create mode 100644 packages/grpcstatus/grpcstatus_test.go create mode 100644 packages/grpcstatus/native_unwrap1.12-.go create mode 100644 packages/grpcstatus/native_unwrap1.13+.go diff --git a/CHANGELOG.md b/CHANGELOG.md index b351def..14e8869 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ## [Unreleased] +### Added +* Support for error unwrapping. (Supported for `github.com/pkg/errors` and native wrapping added in go1.13) + ## [1.2.0](https://github.com/grpc-ecosystem/go-grpc-prometheus/releases/tag/v1.2.0) - 2018-06-04 ### Added diff --git a/makefile b/makefile index 74c0842..6cfc8d2 100644 --- a/makefile +++ b/makefile @@ -1,5 +1,3 @@ -SHELL="/bin/bash" - GOFILES_NOVENDOR = $(shell go list ./... | grep -v /vendor/) all: vet fmt test diff --git a/packages/grpcstatus/grpcstatus.go b/packages/grpcstatus/grpcstatus.go new file mode 100644 index 0000000..6c0f285 --- /dev/null +++ b/packages/grpcstatus/grpcstatus.go @@ -0,0 +1,57 @@ +package grpcstatus + +import ( + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +type gRPCStatus interface { + GRPCStatus() *status.Status +} + +func unwrapPkgErrorsGRPCStatus(err error) (*status.Status, bool) { + type causer interface { + Cause() error + } + + // Unwrapping the github.com/pkg/errors causer interface, using `Cause` directly could miss some error implementing + // the `GRPCStatus` function so we have to check it on our selves. + unwrappedCauser := err + for unwrappedCauser != nil { + if s, ok := unwrappedCauser.(gRPCStatus); ok { + return s.GRPCStatus(), true + } + cause, ok := unwrappedCauser.(causer) + if !ok { + break + } + unwrappedCauser = cause.Cause() + } + return nil, false +} + +// Since error can be wrapped and the `FromError` function only checks for `GRPCStatus` function +// and as a fallback uses the `Unknown` gRPC status we need to unwrap the error if possible to get the original status. +// pkg/errors and Go native errors packages have two different approaches so we try to unwrap both types. +// Eventually should be implemented in the go-grpc status function `FromError`. See https://github.com/grpc/grpc-go/issues/2934 +func FromError(err error) (s *status.Status, ok bool) { + s, ok = status.FromError(err) + if ok { + return s, true + } + + // Try to unwrap `github.com/pkg/errors` wrapped error + s, ok = unwrapPkgErrorsGRPCStatus(err) + if ok { + return s, true + } + + // Try to unwrap native wrapped errors using `fmt.Errorf` and `%w` + s, ok = unwrapNativeWrappedGRPCStatus(err) + if ok { + return s, true + } + + // We failed to unwrap any GRPSStatus so return default `Unknown` + return status.New(codes.Unknown, err.Error()), false +} diff --git a/packages/grpcstatus/grpcstatus1.13+_test.go b/packages/grpcstatus/grpcstatus1.13+_test.go new file mode 100644 index 0000000..1e531a4 --- /dev/null +++ b/packages/grpcstatus/grpcstatus1.13+_test.go @@ -0,0 +1,26 @@ +// +build go1.13 + +package grpcstatus + +import ( + "fmt" + "github.com/stretchr/testify/require" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + "testing" +) + +func TestNativeErrorUnwrapping(t *testing.T) { + gRPCCode := codes.FailedPrecondition + gRPCError := status.Errorf(gRPCCode, "Userspace error.") + expectedGRPCStatus, _ := status.FromError(gRPCError) + testedErrors := []error{ + fmt.Errorf("go native wrapped error: %w", gRPCError), + } + + for _, e := range testedErrors { + resultingStatus, ok := FromError(e) + require.True(t, ok) + require.Equal(t, expectedGRPCStatus, resultingStatus) + } +} diff --git a/packages/grpcstatus/grpcstatus_test.go b/packages/grpcstatus/grpcstatus_test.go new file mode 100644 index 0000000..3c4afa9 --- /dev/null +++ b/packages/grpcstatus/grpcstatus_test.go @@ -0,0 +1,34 @@ +package grpcstatus + +import ( + "github.com/stretchr/testify/require" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + "testing" +) + +// Own implementation of pkg/errors withStack to avoid additional dependency +type wrappedError struct { + cause error + msg string +} + +func (w *wrappedError) Error() string { return w.msg + ": " + w.cause.Error() } + +func (w *wrappedError) Cause() error { return w.cause } + +func TestErrorUnwrapping(t *testing.T) { + gRPCCode := codes.FailedPrecondition + gRPCError := status.Errorf(gRPCCode, "Userspace error.") + expectedGRPCStatus, _ := status.FromError(gRPCError) + testedErrors := []error{ + gRPCError, + &wrappedError{cause: gRPCError, msg: "pkg/errors wrapped error: "}, + } + + for _, e := range testedErrors { + resultingStatus, ok := FromError(e) + require.True(t, ok) + require.Equal(t, expectedGRPCStatus, resultingStatus) + } +} diff --git a/packages/grpcstatus/native_unwrap1.12-.go b/packages/grpcstatus/native_unwrap1.12-.go new file mode 100644 index 0000000..0f575fe --- /dev/null +++ b/packages/grpcstatus/native_unwrap1.12-.go @@ -0,0 +1,11 @@ +// +build !go1.13 + +package grpcstatus + +import ( + "google.golang.org/grpc/status" +) + +func unwrapNativeWrappedGRPCStatus(err error) (*status.Status, bool) { + return nil, false +} diff --git a/packages/grpcstatus/native_unwrap1.13+.go b/packages/grpcstatus/native_unwrap1.13+.go new file mode 100644 index 0000000..216d057 --- /dev/null +++ b/packages/grpcstatus/native_unwrap1.13+.go @@ -0,0 +1,17 @@ +// +build go1.13 + +package grpcstatus + +import ( + "errors" + "google.golang.org/grpc/status" +) + +func unwrapNativeWrappedGRPCStatus(err error) (*status.Status, bool) { + // Unwrapping the native Go unwrap interface + var unwrappedStatus gRPCStatus + if ok := errors.As(err, &unwrappedStatus); ok { + return unwrappedStatus.GRPCStatus(), true + } + return nil, false +} diff --git a/server_metrics.go b/server_metrics.go index cf2e045..d28a46e 100644 --- a/server_metrics.go +++ b/server_metrics.go @@ -2,10 +2,10 @@ package grpc_prometheus import ( "context" - + "github.com/grpc-ecosystem/go-grpc-prometheus/packages/grpcstatus" prom "github.com/prometheus/client_golang/prometheus" + "google.golang.org/grpc" - "google.golang.org/grpc/status" ) // ServerMetrics represents a collection of metrics to be registered on a @@ -106,7 +106,7 @@ func (m *ServerMetrics) UnaryServerInterceptor() func(ctx context.Context, req i monitor := newServerReporter(m, Unary, info.FullMethod) monitor.ReceivedMessage() resp, err := handler(ctx, req) - st, _ := status.FromError(err) + st, _ := grpcstatus.FromError(err) monitor.Handled(st.Code()) if err == nil { monitor.SentMessage() @@ -120,7 +120,7 @@ func (m *ServerMetrics) StreamServerInterceptor() func(srv interface{}, ss grpc. return func(srv interface{}, ss grpc.ServerStream, info *grpc.StreamServerInfo, handler grpc.StreamHandler) error { monitor := newServerReporter(m, streamRPCType(info), info.FullMethod) err := handler(srv, &monitoredServerStream{ss, monitor}) - st, _ := status.FromError(err) + st, _ := grpcstatus.FromError(err) monitor.Handled(st.Code()) return err }