Use the functional options pattern to configure interceptor

Also adresse other minor/style issues.
getsentry · Jan 20, 2021 · 456ebca · 456ebca
1 parent a960775
commit 456ebca
Show file tree

Hide file tree

Showing 3 changed files with 92 additions and 62 deletions.
diff --git a/grpc/options.go b/grpc/options.go
@@ -0,0 +1,63 @@
+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
+	}
+}
+
+// WithReportOn configures whether to report on errors.
+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
@@ -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,53 +30,20 @@ func UnaryServerInterceptor(opts UnaryServerInterceptorOptions) grpc.UnaryServer
 			if r := recover(); r != nil {
 				hub.RecoverWithContext(ctx, r)
 
-				if opts.Repanic {
+				if opts.repanic {
 					panic(r)
 				}
 
 				err = status.Errorf(codes.Internal, "%s", r)
 			}
 		}()
 
-		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
@@ -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()