sentryhandler: Allow overriding function that reports events

We don't have any tests that verify that the sentryhandler interceptors
work as expected. With the current layout this is also quite impossible
to do, because the event that is to be reported will be passed to the
`sentry.CaptureEvent()` function without giving us a way to intercept
the captured event.

Refactor the code such that it becomes possible to replace the function
that reports events. Like that, we can intercept the capture function
and thus exercise the code more readily.

4 files changed, 59 insertions, 21 deletions

diff --git a/internal/gitaly/server/server.go b/internal/gitaly/server/server.go
index 594f9dced..9ffb88c65 100644
--- a/internal/gitaly/server/server.go
+++ b/internal/gitaly/server/server.go
@@ -117,7 +117,7 @@ func (s *GitalyServerFactory) New(secure bool, opts ...Option) (*grpc.Server, er
 			gitalylog.DeciderOption(),
 		),
 		gitalylog.StreamLogDataCatcherServerInterceptor(),
-		sentryhandler.StreamLogHandler,
+		sentryhandler.StreamLogHandler(),
 		statushandler.Stream, // Should be below LogHandler
 		auth.StreamServerInterceptor(s.cfg.Auth),
 	}
@@ -133,7 +133,7 @@ func (s *GitalyServerFactory) New(secure bool, opts ...Option) (*grpc.Server, er
 			gitalylog.DeciderOption(),
 		),
 		gitalylog.UnaryLogDataCatcherServerInterceptor(),
-		sentryhandler.UnaryLogHandler,
+		sentryhandler.UnaryLogHandler(),
 		statushandler.Unary, // Should be below LogHandler
 		auth.UnaryServerInterceptor(s.cfg.Auth),
 	}
diff --git a/internal/grpc/middleware/sentryhandler/sentryhandler.go b/internal/grpc/middleware/sentryhandler/sentryhandler.go
index 5b3263a72..133aa9eb0 100644
--- a/internal/grpc/middleware/sentryhandler/sentryhandler.go
+++ b/internal/grpc/middleware/sentryhandler/sentryhandler.go
@@ -37,26 +37,64 @@ var (
 	}
 )
 
-// UnaryLogHandler handles access times and errors for unary RPC's
-func UnaryLogHandler(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) {
-	start := time.Now()
-	resp, err := handler(ctx, req)
-	if err != nil {
-		logGrpcErrorToSentry(ctx, info.FullMethod, start, err)
+// Option is an option that can be passed to UnaryLogHandler or StreamLogHandler in order to modify their default
+// behaviour.
+type Option func(cfg *config)
+
+// WithEventReporter overrides the function that is used to report events to Sentry. The only intended purpose of this
+// function is to override this function during tests.
+func WithEventReporter(reporter func(*sentry.Event) *sentry.EventID) Option {
+	return func(cfg *config) {
+		cfg.eventReporter = reporter
 	}
+}
 
-	return resp, err
+type config struct {
+	eventReporter func(*sentry.Event) *sentry.EventID
 }
 
-// StreamLogHandler handles access times and errors for stream RPC's
-func StreamLogHandler(srv interface{}, stream grpc.ServerStream, info *grpc.StreamServerInfo, handler grpc.StreamHandler) error {
-	start := time.Now()
-	err := handler(srv, stream)
-	if err != nil {
-		logGrpcErrorToSentry(stream.Context(), info.FullMethod, start, err)
+func configFromOptions(opts ...Option) config {
+	cfg := config{
+		eventReporter: sentry.CaptureEvent,
+	}
+
+	for _, opt := range opts {
+		opt(&cfg)
 	}
 
-	return err
+	return cfg
+}
+
+// UnaryLogHandler handles access times and errors for unary RPC's. Its default behaviour can be changed by passing
+// Options.
+func UnaryLogHandler(opts ...Option) grpc.UnaryServerInterceptor {
+	cfg := configFromOptions(opts...)
+
+	return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) {
+		start := time.Now()
+		resp, err := handler(ctx, req)
+		if err != nil {
+			logGrpcErrorToSentry(ctx, info.FullMethod, start, err, cfg.eventReporter)
+		}
+
+		return resp, err
+	}
+}
+
+// StreamLogHandler handles access times and errors for stream RPC's. Its default behaviour can be changed by passing
+// options.
+func StreamLogHandler(opts ...Option) grpc.StreamServerInterceptor {
+	cfg := configFromOptions(opts...)
+
+	return func(srv interface{}, stream grpc.ServerStream, info *grpc.StreamServerInfo, handler grpc.StreamHandler) error {
+		start := time.Now()
+		err := handler(srv, stream)
+		if err != nil {
+			logGrpcErrorToSentry(stream.Context(), info.FullMethod, start, err, cfg.eventReporter)
+		}
+
+		return err
+	}
 }
 
 func stringMap(incoming map[string]interface{}) map[string]string {
@@ -133,13 +171,13 @@ func generateSentryEvent(ctx context.Context, method string, duration time.Durat
 	return event
 }
 
-func logGrpcErrorToSentry(ctx context.Context, method string, start time.Time, err error) {
+func logGrpcErrorToSentry(ctx context.Context, method string, start time.Time, err error, reporter func(*sentry.Event) *sentry.EventID) {
 	event := generateSentryEvent(ctx, method, time.Since(start), err)
 	if event == nil {
 		return
 	}
 
-	sentry.CaptureEvent(event)
+	reporter(event)
 }
 
 var errorMsgPattern = regexp.MustCompile(`\A(\w+): (.+)\z`)
diff --git a/internal/grpc/proxy/proxy_test_testhelper_test.go b/internal/grpc/proxy/proxy_test_testhelper_test.go
index 2c3493743..7a1b0f13d 100644
--- a/internal/grpc/proxy/proxy_test_testhelper_test.go
+++ b/internal/grpc/proxy/proxy_test_testhelper_test.go
@@ -63,7 +63,7 @@ func newProxy(tb testing.TB, ctx context.Context, director proxy.StreamDirector,
 			// context tags usage is required by sentryhandler.StreamLogHandler
 			grpcmwtags.StreamServerInterceptor(grpcmwtags.WithFieldExtractorForInitialReq(fieldextractors.FieldExtractor)),
 			// sentry middleware to capture errors
-			sentryhandler.StreamLogHandler,
+			sentryhandler.StreamLogHandler(),
 		),
 		grpc.UnknownServiceHandler(proxy.TransparentHandler(director)),
 	)
diff --git a/internal/praefect/server.go b/internal/praefect/server.go
index 6738715df..066f2bf78 100644
--- a/internal/praefect/server.go
+++ b/internal/praefect/server.go
@@ -76,7 +76,7 @@ func commonUnaryServerInterceptors(logger log.Logger, messageProducer grpcmwlogr
 			grpcmwlogrus.WithMessageProducer(messageProducer),
 			log.DeciderOption(),
 		),
-		sentryhandler.UnaryLogHandler,
+		sentryhandler.UnaryLogHandler(),
 		statushandler.Unary, // Should be below LogHandler
 		grpctracing.UnaryServerTracingInterceptor(),
 		// Panic handler should remain last so that application panics will be
@@ -146,7 +146,7 @@ func NewGRPCServer(
 			grpcmwlogrus.WithMessageProducer(logMsgProducer),
 			log.DeciderOption(),
 		),
-		sentryhandler.StreamLogHandler,
+		sentryhandler.StreamLogHandler(),
 		statushandler.Stream, // Should be below LogHandler
 		grpctracing.StreamServerTracingInterceptor(),
 		auth.StreamServerInterceptor(deps.Config.Auth),