From 08fc7cdb63ff6202b6cbd6dec029942d28140031 Mon Sep 17 00:00:00 2001 From: Shouichi Kamiya Date: Wed, 20 Jan 2021 16:12:08 +0900 Subject: [PATCH] Use the functional options pattern to configure interceptor Also adresse other minor/style issues. --- grpc/options.go | 64 +++++++++++++++++++++++++++++++++++++++++++++ grpc/server.go | 47 +++++---------------------------- grpc/server_test.go | 44 ++++++++++++++++--------------- 3 files changed, 93 insertions(+), 62 deletions(-) create mode 100644 grpc/options.go diff --git a/grpc/options.go b/grpc/options.go new file mode 100644 index 000000000..2cac4bb13 --- /dev/null +++ b/grpc/options.go @@ -0,0 +1,64 @@ +package sentrygrpc + +import ( + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +type options struct { + repanic bool + reportOn ReportOn +} + +func buildOptions(ff ...Option) options { + opts := options{ + reportOn: ReportAlways, + } + + for _, f := range ff { + f(&opts) + } + + return opts +} + +// Option configures reporting behavior. +type Option func(*options) + +// WithRepanic configures whether to panic again after recovering from +// a panic. Use this option if you have other panic handlers. +func WithRepanic(b bool) Option { + return func(o *options) { + o.repanic = b + } +} + +// ReportOn configures whether to report an error. Defaults to +// ReportAlways. +func WithReportOn(r ReportOn) Option { + return func(o *options) { + o.reportOn = r + } +} + +// ReportOn decides error should be reported to sentry. +type ReportOn func(error) bool + +// ReportAlways returns true if err is non-nil. +func ReportAlways(err error) bool { + return err != nil +} + +// ReportOnCodes returns true if error code matches on of the given codes. +func ReportOnCodes(cc ...codes.Code) ReportOn { + return func(err error) bool { + c := status.Code(err) + for i := range cc { + if c == cc[i] { + return true + } + } + + return false + } +} diff --git a/grpc/server.go b/grpc/server.go index a198c8702..ecab7c747 100644 --- a/grpc/server.go +++ b/grpc/server.go @@ -11,17 +11,15 @@ import ( // UnaryServerInterceptor is a grpc interceptor that reports errors and panics // to sentry. It also sets *sentry.Hub to context. -func UnaryServerInterceptor(opts UnaryServerInterceptorOptions) grpc.UnaryServerInterceptor { - if opts.ReportOn == nil { - opts.ReportOn = ReportAlways - } +func UnaryServerInterceptor(options ...Option) grpc.UnaryServerInterceptor { + opts := buildOptions(options...) return func( ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler, - ) (_ interface{}, err error) { + ) (resp interface{}, err error) { hub := sentry.GetHubFromContext(ctx) if hub == nil { hub = sentry.CurrentHub().Clone() @@ -32,7 +30,7 @@ func UnaryServerInterceptor(opts UnaryServerInterceptorOptions) grpc.UnaryServer if r := recover(); r != nil { hub.RecoverWithContext(ctx, r) - if opts.Repanic { + if opts.repanic { panic(r) } @@ -40,45 +38,12 @@ func UnaryServerInterceptor(opts UnaryServerInterceptorOptions) grpc.UnaryServer } }() - resp, err := handler(ctx, req) + resp, err = handler(ctx, req) - if opts.ReportOn(err) { + if opts.reportOn(err) { hub.CaptureException(err) } return resp, err } } - -// UnaryServerInterceptor configure UnaryServerInterceptor. -type UnaryServerInterceptorOptions struct { - // Repanic configures whether to panic again after recovering from a - // panic. Use this option if you have other panic handlers. - Repanic bool - - // ReportOn configures whether to report an error. Defaults to - // ReportAlways. - ReportOn ReportOn -} - -// ReportOn decides error should be reported to sentry. -type ReportOn func(error) bool - -// ReportAlways returns true if err is non-nil. -func ReportAlways(err error) bool { - return err != nil -} - -// ReportOnCodes returns true if error code matches on of the given codes. -func ReportOnCodes(cc ...codes.Code) ReportOn { - return func(err error) bool { - c := status.Code(err) - for i := range cc { - if c == cc[i] { - return true - } - } - - return false - } -} diff --git a/grpc/server_test.go b/grpc/server_test.go index 18a7171ca..bc97724f3 100644 --- a/grpc/server_test.go +++ b/grpc/server_test.go @@ -21,7 +21,7 @@ func TestUnaryServerInterceptor(t *testing.T) { for _, tt := range []struct { name string ctx context.Context - opts sentrygrpc.UnaryServerInterceptorOptions + opts []sentrygrpc.Option handler func( context.Context, *grpchealth.HealthCheckRequest, @@ -32,7 +32,6 @@ func TestUnaryServerInterceptor(t *testing.T) { { name: "does not report when err is nil", ctx: context.Background(), - opts: sentrygrpc.UnaryServerInterceptorOptions{}, handler: func( ctx context.Context, _ *grpchealth.HealthCheckRequest, @@ -44,7 +43,6 @@ func TestUnaryServerInterceptor(t *testing.T) { { name: "reports all errors by default", ctx: context.Background(), - opts: sentrygrpc.UnaryServerInterceptorOptions{}, handler: func( context.Context, *grpchealth.HealthCheckRequest, @@ -67,10 +65,12 @@ func TestUnaryServerInterceptor(t *testing.T) { { name: "reports errors that ReportOn returns true", ctx: context.Background(), - opts: sentrygrpc.UnaryServerInterceptorOptions{ - ReportOn: func(err error) bool { - return errors.Is(err, grpc.ErrServerStopped) - }, + opts: []sentrygrpc.Option{ + sentrygrpc.WithReportOn( + func(err error) bool { + return errors.Is(err, grpc.ErrServerStopped) + }, + ), }, handler: func( context.Context, @@ -94,10 +94,12 @@ func TestUnaryServerInterceptor(t *testing.T) { { name: "does not report errors that ReportOn returns false", ctx: context.Background(), - opts: sentrygrpc.UnaryServerInterceptorOptions{ - ReportOn: func(err error) bool { - return false - }, + opts: []sentrygrpc.Option{ + sentrygrpc.WithReportOn( + func(err error) bool { + return false + }, + ), }, handler: func( context.Context, @@ -110,7 +112,6 @@ func TestUnaryServerInterceptor(t *testing.T) { { name: "recovers from panic and returns internal error", ctx: context.Background(), - opts: sentrygrpc.UnaryServerInterceptorOptions{}, handler: func( context.Context, *grpchealth.HealthCheckRequest, @@ -128,7 +129,6 @@ func TestUnaryServerInterceptor(t *testing.T) { { name: "sets hub on context", ctx: context.Background(), - opts: sentrygrpc.UnaryServerInterceptorOptions{}, handler: func( ctx context.Context, _ *grpchealth.HealthCheckRequest, @@ -165,7 +165,7 @@ func TestUnaryServerInterceptor(t *testing.T) { } defer lis.Close() - opt := grpc.UnaryInterceptor(sentrygrpc.UnaryServerInterceptor(tt.opts)) + opt := grpc.UnaryInterceptor(sentrygrpc.UnaryServerInterceptor(tt.opts...)) server := grpc.NewServer(opt) defer server.Stop() @@ -267,26 +267,28 @@ func TestReportOnCodes(t *testing.T) { t.Run(tt.name, func(t *testing.T) { if w, g := tt.want, sentrygrpc.ReportOnCodes(tt.codes...)(tt.err); w != g { - t.Fatalf("ReportOnCodes: want %t, got %t", w, g) + t.Fatalf("want %t, got %t", w, g) } }) } } func ExampleUnaryServerInterceptor() { - opts := sentrygrpc.UnaryServerInterceptorOptions{ + opts := []sentrygrpc.Option{ // Reports on OutOfRange or Internal error. - ReportOn: sentrygrpc.ReportOnCodes( - codes.OutOfRange, - codes.Internal, + sentrygrpc.WithReportOn( + sentrygrpc.ReportOnCodes( + codes.OutOfRange, + codes.Internal, + ), ), // Recovers from panic, reports it and returns internal error. - Repanic: false, + sentrygrpc.WithRepanic(false), } // This middleware sets *sentry.Hub to context. You can set user to // hub's scope in the later interceptor for example. - sentry := sentrygrpc.UnaryServerInterceptor(opts) + sentry := sentrygrpc.UnaryServerInterceptor(opts...) server := grpc.NewServer(grpc.UnaryInterceptor(sentry)) defer server.Stop()