Merge branch 'revert-8bf7e67d' into 'master'

Revert "Merge branch 'pks-log-self-contained-middleware' into 'master'"

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/6422

Merged-by: Toon Claes <toon@gitlab.com>
Approved-by: Toon Claes <toon@gitlab.com>
Co-authored-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>

8 files changed, 105 insertions, 72 deletions

diff --git a/internal/gitaly/server/server.go b/internal/gitaly/server/server.go
index ac7606601..594f9dced 100644
--- a/internal/gitaly/server/server.go
+++ b/internal/gitaly/server/server.go
@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"time"
 
+	grpcmwlogrus "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus"
 	grpcmwtags "github.com/grpc-ecosystem/go-grpc-middleware/tags"
 	grpcprometheus "github.com/grpc-ecosystem/go-grpc-prometheus"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/server/auth"
@@ -21,7 +22,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v16/internal/grpc/middleware/statushandler"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/grpc/protoregistry"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/helper/fieldextractors"
-	"gitlab.com/gitlab-org/gitaly/v16/internal/log"
+	gitalylog "gitlab.com/gitlab-org/gitaly/v16/internal/log"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/structerr"
 	grpccorrelation "gitlab.com/gitlab-org/labkit/correlation/grpc"
 	grpctracing "gitlab.com/gitlab-org/labkit/tracing/grpc"
@@ -94,12 +95,15 @@ func (s *GitalyServerFactory) New(secure bool, opts ...Option) (*grpc.Server, er
 		[]grpc.DialOption{client.UnaryInterceptor()},
 	))
 
-	logFieldsProducers := []log.FieldsProducer{
-		customfieldshandler.FieldsProducer,
-		grpcstats.FieldsProducer,
-		featureflag.FieldsProducer,
-		structerr.FieldsProducer,
-	}
+	logMsgProducer := grpcmwlogrus.WithMessageProducer(
+		gitalylog.MessageProducer(
+			gitalylog.PropagationMessageProducer(grpcmwlogrus.DefaultMessageProducer),
+			customfieldshandler.FieldsProducer,
+			grpcstats.FieldsProducer,
+			featureflag.FieldsProducer,
+			structerr.FieldsProducer,
+		),
+	)
 
 	streamServerInterceptors := []grpc.StreamServerInterceptor{
 		grpcmwtags.StreamServerInterceptor(ctxTagOpts...),
@@ -107,8 +111,12 @@ func (s *GitalyServerFactory) New(secure bool, opts ...Option) (*grpc.Server, er
 		metadatahandler.StreamInterceptor,
 		grpcprometheus.StreamServerInterceptor,
 		customfieldshandler.StreamInterceptor,
-		s.logger.WithField("component", "gitaly.StreamServerInterceptor").StreamServerInterceptor(logFieldsProducers...),
-		log.StreamLogDataCatcherServerInterceptor(),
+		s.logger.WithField("component", "gitaly.StreamServerInterceptor").StreamServerInterceptor(
+			grpcmwlogrus.WithTimestampFormat(gitalylog.LogTimestampFormat),
+			logMsgProducer,
+			gitalylog.DeciderOption(),
+		),
+		gitalylog.StreamLogDataCatcherServerInterceptor(),
 		sentryhandler.StreamLogHandler,
 		statushandler.Stream, // Should be below LogHandler
 		auth.StreamServerInterceptor(s.cfg.Auth),
@@ -119,8 +127,12 @@ func (s *GitalyServerFactory) New(secure bool, opts ...Option) (*grpc.Server, er
 		metadatahandler.UnaryInterceptor,
 		grpcprometheus.UnaryServerInterceptor,
 		customfieldshandler.UnaryInterceptor,
-		s.logger.WithField("component", "gitaly.UnaryServerInterceptor").UnaryServerInterceptor(logFieldsProducers...),
-		log.UnaryLogDataCatcherServerInterceptor(),
+		s.logger.WithField("component", "gitaly.UnaryServerInterceptor").UnaryServerInterceptor(
+			grpcmwlogrus.WithTimestampFormat(gitalylog.LogTimestampFormat),
+			logMsgProducer,
+			gitalylog.DeciderOption(),
+		),
+		gitalylog.UnaryLogDataCatcherServerInterceptor(),
 		sentryhandler.UnaryLogHandler,
 		statushandler.Unary, // Should be below LogHandler
 		auth.UnaryServerInterceptor(s.cfg.Auth),
@@ -151,9 +163,9 @@ func (s *GitalyServerFactory) New(secure bool, opts ...Option) (*grpc.Server, er
 	unaryServerInterceptors = append(unaryServerInterceptors, cfg.unaryInterceptors...)
 
 	serverOptions := []grpc.ServerOption{
-		grpc.StatsHandler(log.PerRPCLogHandler{
+		grpc.StatsHandler(gitalylog.PerRPCLogHandler{
 			Underlying:     &grpcstats.PayloadBytes{},
-			FieldProducers: []log.FieldsProducer{grpcstats.FieldsProducer},
+			FieldProducers: []gitalylog.FieldsProducer{grpcstats.FieldsProducer},
 		}),
 		grpc.Creds(lm),
 		grpc.ChainStreamInterceptor(streamServerInterceptors...),
diff --git a/internal/grpc/grpcstats/stats_test.go b/internal/grpc/grpcstats/stats_test.go
index 5a86ba2d0..b3bfa881c 100644
--- a/internal/grpc/grpcstats/stats_test.go
+++ b/internal/grpc/grpcstats/stats_test.go
@@ -8,6 +8,7 @@ import (
 	"sync"
 	"testing"
 
+	grpcmwlogrus "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/grpc/client"
@@ -61,11 +62,25 @@ func TestPayloadBytes(t *testing.T) {
 			FieldProducers: []log.FieldsProducer{FieldsProducer},
 		}),
 		grpc.ChainUnaryInterceptor(
-			logger.UnaryServerInterceptor(FieldsProducer),
+			logger.UnaryServerInterceptor(
+				grpcmwlogrus.WithMessageProducer(
+					log.MessageProducer(
+						log.PropagationMessageProducer(grpcmwlogrus.DefaultMessageProducer),
+						FieldsProducer,
+					),
+				),
+			),
 			log.UnaryLogDataCatcherServerInterceptor(),
 		),
 		grpc.ChainStreamInterceptor(
-			logger.StreamServerInterceptor(FieldsProducer),
+			logger.StreamServerInterceptor(
+				grpcmwlogrus.WithMessageProducer(
+					log.MessageProducer(
+						log.PropagationMessageProducer(grpcmwlogrus.DefaultMessageProducer),
+						FieldsProducer,
+					),
+				),
+			),
 			log.StreamLogDataCatcherServerInterceptor(),
 		),
 	}
diff --git a/internal/grpc/middleware/customfieldshandler/customfields_handler_test.go b/internal/grpc/middleware/customfieldshandler/customfields_handler_test.go
index 2ea6464ae..15895b7ba 100644
--- a/internal/grpc/middleware/customfieldshandler/customfields_handler_test.go
+++ b/internal/grpc/middleware/customfieldshandler/customfields_handler_test.go
@@ -6,6 +6,7 @@ import (
 	"net"
 	"testing"
 
+	grpcmwlogrus "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/catfile"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest"
@@ -14,7 +15,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/service/ref"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/grpc/backchannel"
-	"gitlab.com/gitlab-org/gitaly/v16/internal/grpc/grpcstats"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/log"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/testhelper/testcfg"
@@ -27,20 +27,20 @@ import (
 func createNewServer(t *testing.T, cfg config.Cfg, logger log.Logger) *grpc.Server {
 	t.Helper()
 
-	logger = logger.WithField("test", t.Name())
+	logrusEntry := logger.WithField("test", t.Name())
 
 	opts := []grpc.ServerOption{
-		grpc.StatsHandler(log.PerRPCLogHandler{
-			Underlying:     &grpcstats.PayloadBytes{},
-			FieldProducers: []log.FieldsProducer{grpcstats.FieldsProducer},
-		}),
 		grpc.ChainStreamInterceptor(
 			StreamInterceptor,
-			logger.StreamServerInterceptor(FieldsProducer),
+			logrusEntry.StreamServerInterceptor(
+				grpcmwlogrus.WithTimestampFormat(log.LogTimestampFormat),
+				grpcmwlogrus.WithMessageProducer(log.MessageProducer(grpcmwlogrus.DefaultMessageProducer, FieldsProducer))),
 		),
 		grpc.ChainUnaryInterceptor(
 			UnaryInterceptor,
-			logger.UnaryServerInterceptor(FieldsProducer),
+			logrusEntry.UnaryServerInterceptor(
+				grpcmwlogrus.WithTimestampFormat(log.LogTimestampFormat),
+				grpcmwlogrus.WithMessageProducer(log.MessageProducer(grpcmwlogrus.DefaultMessageProducer, FieldsProducer))),
 		),
 	}
 
diff --git a/internal/grpc/middleware/featureflag/featureflag_handler_test.go b/internal/grpc/middleware/featureflag/featureflag_handler_test.go
index e398cd541..45eb31d80 100644
--- a/internal/grpc/middleware/featureflag/featureflag_handler_test.go
+++ b/internal/grpc/middleware/featureflag/featureflag_handler_test.go
@@ -5,9 +5,9 @@ import (
 	"net"
 	"testing"
 
+	grpcmwlogrus "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/featureflag"
-	"gitlab.com/gitlab-org/gitaly/v16/internal/grpc/grpcstats"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/log"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/structerr"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/testhelper"
@@ -37,11 +37,17 @@ func TestFeatureFlagLogs(t *testing.T) {
 
 	service := &mockService{}
 	server := grpc.NewServer(
-		grpc.StatsHandler(log.PerRPCLogHandler{
-			Underlying:     &grpcstats.PayloadBytes{},
-			FieldProducers: []log.FieldsProducer{grpcstats.FieldsProducer},
-		}),
-		grpc.ChainUnaryInterceptor(logger.UnaryServerInterceptor(FieldsProducer)),
+		grpc.ChainUnaryInterceptor(
+			grpcmwlogrus.UnaryServerInterceptor(
+				logger.LogrusEntry(), //nolint:staticcheck
+				grpcmwlogrus.WithMessageProducer(
+					log.MessageProducer(
+						grpcmwlogrus.DefaultMessageProducer,
+						FieldsProducer,
+					),
+				),
+			),
+		),
 	)
 	grpc_testing.RegisterTestServiceServer(server, service)
 
diff --git a/internal/log/logger.go b/internal/log/logger.go
index af7579f30..753ac3c30 100644
--- a/internal/log/logger.go
+++ b/internal/log/logger.go
@@ -23,8 +23,8 @@ type Logger interface {
 	Warn(msg string)
 	Error(msg string)
 
-	StreamServerInterceptor(...FieldsProducer) grpc.StreamServerInterceptor
-	UnaryServerInterceptor(...FieldsProducer) grpc.UnaryServerInterceptor
+	StreamServerInterceptor(...grpcmwlogrus.Option) grpc.StreamServerInterceptor
+	UnaryServerInterceptor(...grpcmwlogrus.Option) grpc.UnaryServerInterceptor
 }
 
 // LogrusLogger is an implementation of the Logger interface that is implemented via a `logrus.FieldLogger`.
@@ -87,27 +87,13 @@ func (l LogrusLogger) ToContext(ctx context.Context) context.Context {
 }
 
 // StreamServerInterceptor creates a gRPC interceptor that generates log messages for streaming RPC calls.
-func (l LogrusLogger) StreamServerInterceptor(fieldsProducers ...FieldsProducer) grpc.StreamServerInterceptor {
-	return grpcmwlogrus.StreamServerInterceptor(l.entry,
-		grpcmwlogrus.WithTimestampFormat(LogTimestampFormat),
-		grpcmwlogrus.WithMessageProducer(MessageProducer(
-			PropagationMessageProducer(grpcmwlogrus.DefaultMessageProducer),
-			fieldsProducers...,
-		)),
-		DeciderOption(),
-	)
+func (l LogrusLogger) StreamServerInterceptor(opts ...grpcmwlogrus.Option) grpc.StreamServerInterceptor {
+	return grpcmwlogrus.StreamServerInterceptor(l.entry, opts...)
 }
 
 // UnaryServerInterceptor creates a gRPC interceptor that generates log messages for unary RPC calls.
-func (l LogrusLogger) UnaryServerInterceptor(fieldsProducers ...FieldsProducer) grpc.UnaryServerInterceptor {
-	return grpcmwlogrus.UnaryServerInterceptor(l.entry,
-		grpcmwlogrus.WithTimestampFormat(LogTimestampFormat),
-		grpcmwlogrus.WithMessageProducer(MessageProducer(
-			PropagationMessageProducer(grpcmwlogrus.DefaultMessageProducer),
-			fieldsProducers...,
-		)),
-		DeciderOption(),
-	)
+func (l LogrusLogger) UnaryServerInterceptor(opts ...grpcmwlogrus.Option) grpc.UnaryServerInterceptor {
+	return grpcmwlogrus.UnaryServerInterceptor(l.entry, opts...)
 }
 
 // FromContext extracts the logger from the context. If no logger has been injected then this will return a discarding
diff --git a/internal/praefect/delete_object_pool_test.go b/internal/praefect/delete_object_pool_test.go
index bc77a26aa..573bbb3b7 100644
--- a/internal/praefect/delete_object_pool_test.go
+++ b/internal/praefect/delete_object_pool_test.go
@@ -4,9 +4,9 @@ import (
 	"context"
 	"testing"
 
+	grpcmwlogrus "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest"
-	"gitlab.com/gitlab-org/gitaly/v16/internal/grpc/grpcstats"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/log"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/praefect/datastore"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/testhelper"
@@ -71,13 +71,9 @@ func TestDeleteObjectPoolHandler(t *testing.T) {
 	praefectLn, praefectAddr := testhelper.GetLocalhostListener(t)
 	logger := testhelper.NewLogger(t)
 	hook := testhelper.AddLoggerHook(logger)
-	praefectSrv := grpc.NewServer(
-		grpc.StatsHandler(log.PerRPCLogHandler{
-			Underlying:     &grpcstats.PayloadBytes{},
-			FieldProducers: []log.FieldsProducer{grpcstats.FieldsProducer},
-		}),
-		grpc.ChainStreamInterceptor(logger.StreamServerInterceptor()),
-	)
+	praefectSrv := grpc.NewServer(grpc.ChainStreamInterceptor(
+		logger.StreamServerInterceptor(grpcmwlogrus.WithTimestampFormat(log.LogTimestampFormat)),
+	))
 	praefectSrv.RegisterService(&grpc.ServiceDesc{
 		ServiceName: "gitaly.ObjectPoolService",
 		HandlerType: (*interface{})(nil),
diff --git a/internal/praefect/server.go b/internal/praefect/server.go
index 0ebee1420..823ed02ec 100644
--- a/internal/praefect/server.go
+++ b/internal/praefect/server.go
@@ -7,6 +7,7 @@ package praefect
 import (
 	"time"
 
+	grpcmwlogrus "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus"
 	grpcmwtags "github.com/grpc-ecosystem/go-grpc-middleware/tags"
 	grpcprometheus "github.com/grpc-ecosystem/go-grpc-prometheus"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/server/auth"
@@ -41,19 +42,20 @@ import (
 	"google.golang.org/grpc/keepalive"
 )
 
-var logFieldsProducers = []log.FieldsProducer{
-	structerr.FieldsProducer,
-}
-
 // NewBackchannelServerFactory returns a ServerFactory that serves the RefTransactionServer on the backchannel
 // connection.
 func NewBackchannelServerFactory(logger log.Logger, refSvc gitalypb.RefTransactionServer, registry *sidechannel.Registry) backchannel.ServerFactory {
+	logMsgProducer := log.MessageProducer(
+		log.PropagationMessageProducer(grpcmwlogrus.DefaultMessageProducer),
+		structerr.FieldsProducer,
+	)
+
 	return func() backchannel.Server {
 		lm := listenmux.New(insecure.NewCredentials())
 		lm.Register(sidechannel.NewServerHandshaker(registry))
 		srv := grpc.NewServer(
 			grpc.ChainUnaryInterceptor(
-				commonUnaryServerInterceptors(logger.WithField("component", "backchannel.PraefectServer"))...,
+				commonUnaryServerInterceptors(logger.WithField("component", "backchannel.PraefectServer"), logMsgProducer)...,
 			),
 			grpc.Creds(lm),
 		)
@@ -63,13 +65,17 @@ func NewBackchannelServerFactory(logger log.Logger, refSvc gitalypb.RefTransacti
 	}
 }
 
-func commonUnaryServerInterceptors(logger log.Logger) []grpc.UnaryServerInterceptor {
+func commonUnaryServerInterceptors(logger log.Logger, messageProducer grpcmwlogrus.MessageProducer) []grpc.UnaryServerInterceptor {
 	return []grpc.UnaryServerInterceptor{
 		grpcmwtags.UnaryServerInterceptor(ctxtagsInterceptorOption()),
 		grpccorrelation.UnaryServerCorrelationInterceptor(), // Must be above the metadata handler
 		metadatahandler.UnaryInterceptor,
 		grpcprometheus.UnaryServerInterceptor,
-		logger.UnaryServerInterceptor(logFieldsProducers...),
+		logger.UnaryServerInterceptor(
+			grpcmwlogrus.WithTimestampFormat(log.LogTimestampFormat),
+			grpcmwlogrus.WithMessageProducer(messageProducer),
+			log.DeciderOption(),
+		),
 		sentryhandler.UnaryLogHandler,
 		statushandler.Unary, // Should be below LogHandler
 		grpctracing.UnaryServerTracingInterceptor(),
@@ -117,8 +123,13 @@ func NewGRPCServer(
 		opt(&serverCfg)
 	}
 
+	logMsgProducer := log.MessageProducer(
+		log.PropagationMessageProducer(grpcmwlogrus.DefaultMessageProducer),
+		structerr.FieldsProducer,
+	)
+
 	unaryInterceptors := append(
-		commonUnaryServerInterceptors(deps.Logger.WithField("component", "praefect.UnaryServerInterceptor")),
+		commonUnaryServerInterceptors(deps.Logger.WithField("component", "praefect.UnaryServerInterceptor"), logMsgProducer),
 		middleware.MethodTypeUnaryInterceptor(deps.Registry, deps.Logger),
 		auth.UnaryServerInterceptor(deps.Config.Auth),
 	)
@@ -130,7 +141,11 @@ func NewGRPCServer(
 		middleware.MethodTypeStreamInterceptor(deps.Registry, deps.Logger),
 		metadatahandler.StreamInterceptor,
 		grpcprometheus.StreamServerInterceptor,
-		deps.Logger.WithField("component", "praefect.StreamServerInterceptor").StreamServerInterceptor(logFieldsProducers...),
+		deps.Logger.WithField("component", "praefect.StreamServerInterceptor").StreamServerInterceptor(
+			grpcmwlogrus.WithTimestampFormat(log.LogTimestampFormat),
+			grpcmwlogrus.WithMessageProducer(logMsgProducer),
+			log.DeciderOption(),
+		),
 		sentryhandler.StreamLogHandler,
 		statushandler.Stream, // Should be below LogHandler
 		grpctracing.StreamServerTracingInterceptor(),
diff --git a/internal/testhelper/testserver/structerr_interceptors_test.go b/internal/testhelper/testserver/structerr_interceptors_test.go
index 7a057fe39..a0453e7ed 100644
--- a/internal/testhelper/testserver/structerr_interceptors_test.go
+++ b/internal/testhelper/testserver/structerr_interceptors_test.go
@@ -7,8 +7,8 @@ import (
 	"net"
 	"testing"
 
+	grpcmwlogrus "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus"
 	"github.com/stretchr/testify/require"
-	"gitlab.com/gitlab-org/gitaly/v16/internal/grpc/grpcstats"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/log"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/structerr"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/testhelper"
@@ -94,12 +94,15 @@ func TestFieldsProducer(t *testing.T) {
 
 	service := &mockService{}
 	server := grpc.NewServer(
-		grpc.StatsHandler(log.PerRPCLogHandler{
-			Underlying:     &grpcstats.PayloadBytes{},
-			FieldProducers: []log.FieldsProducer{grpcstats.FieldsProducer},
-		}),
 		grpc.ChainUnaryInterceptor(
-			logger.UnaryServerInterceptor(structerr.FieldsProducer),
+			logger.UnaryServerInterceptor(
+				grpcmwlogrus.WithMessageProducer(
+					log.MessageProducer(
+						grpcmwlogrus.DefaultMessageProducer,
+						structerr.FieldsProducer,
+					),
+				),
+			),
 		),
 	)
 	grpc_testing.RegisterTestServiceServer(server, service)