uploadpack: Convert packfile negotiation counters to use CounterVecs

We currently set up a different counter for every packfile negotiation
feature we want to track, which is kind of tedious and requires quite a
lot of plumbing. Convert the code to use CounterVecs instead where we
can simply set up labels for each of the features we track.

7 files changed, 50 insertions, 122 deletions

diff --git a/internal/service/register.go b/internal/service/register.go
index eb5c9c2cd..412defcf3 100644
--- a/internal/service/register.go
+++ b/internal/service/register.go
@@ -2,6 +2,7 @@ package service
 
 import (
 	"github.com/prometheus/client_golang/prometheus"
+	"github.com/prometheus/client_golang/prometheus/promauto"
 	"gitlab.com/gitlab-org/gitaly/internal/config"
 	"gitlab.com/gitlab-org/gitaly/internal/rubyserver"
 	"gitlab.com/gitlab-org/gitaly/internal/service/blob"
@@ -28,72 +29,27 @@ import (
 )
 
 var (
-	smarthttpDeepensMetric = prometheus.NewCounter(
+	smarthttpPackfileNegotiationMetrics = promauto.NewCounterVec(
 		prometheus.CounterOpts{
 			Namespace: "gitaly",
 			Subsystem: "smarthttp",
-			Name:      "deepen_requests_total",
-			Help:      "Number of git-upload-pack requests processed that contained a 'deepen' message",
+			Name:      "packfile_negotiation_requests_total",
+			Help:      "Total number of features used for packfile negotiations",
 		},
+		[]string{"git_negotiation_feature"},
 	)
 
-	smarthttpFiltersMetric = prometheus.NewCounter(
-		prometheus.CounterOpts{
-			Namespace: "gitaly",
-			Subsystem: "smarthttp",
-			Name:      "filter_requests_total",
-			Help:      "Number of git-upload-pack requests processed that contained a 'filter' message",
-		},
-	)
-
-	smarthttpHavesMetric = prometheus.NewCounter(
-		prometheus.CounterOpts{
-			Namespace: "gitaly",
-			Subsystem: "smarthttp",
-			Name:      "haves_requests_total",
-			Help:      "Number of git-upload-pack requests processed that contained a 'have' message",
-		},
-	)
-
-	sshDeepensMetric = prometheus.NewCounter(
+	sshPackfileNegotiationMetrics = promauto.NewCounterVec(
 		prometheus.CounterOpts{
 			Namespace: "gitaly",
 			Subsystem: "ssh",
-			Name:      "deepen_requests_total",
-			Help:      "Number of git-upload-pack requests processed that contained a 'deepen' message",
-		},
-	)
-
-	sshFiltersMetric = prometheus.NewCounter(
-		prometheus.CounterOpts{
-			Namespace: "gitaly",
-			Subsystem: "ssh",
-			Name:      "filter_requests_total",
-			Help:      "Number of git-upload-pack requests processed that contained a 'filter' message",
-		},
-	)
-
-	sshHavesMetric = prometheus.NewCounter(
-		prometheus.CounterOpts{
-			Namespace: "gitaly",
-			Subsystem: "ssh",
-			Name:      "haves_requests_total",
-			Help:      "Number of git-upload-pack requests processed that contained a 'have' message",
+			Name:      "packfile_negotiation_requests_total",
+			Help:      "Total number of features used for packfile negotiations",
 		},
+		[]string{"git_negotiation_feature"},
 	)
 )
 
-func init() {
-	prometheus.MustRegister(
-		smarthttpDeepensMetric,
-		smarthttpFiltersMetric,
-		smarthttpHavesMetric,
-		sshDeepensMetric,
-		sshFiltersMetric,
-		sshHavesMetric,
-	)
-}
-
 // RegisterAll will register all the known grpc services with
 // the specified grpc service instance
 func RegisterAll(grpcServer *grpc.Server, rubyServer *rubyserver.Server) {
@@ -106,14 +62,10 @@ func RegisterAll(grpcServer *grpc.Server, rubyServer *rubyserver.Server) {
 	gitalypb.RegisterRefServiceServer(grpcServer, ref.NewServer())
 	gitalypb.RegisterRepositoryServiceServer(grpcServer, repository.NewServer(rubyServer, config.GitalyInternalSocketPath()))
 	gitalypb.RegisterSSHServiceServer(grpcServer, ssh.NewServer(
-		ssh.WithDeepensMetric(sshDeepensMetric),
-		ssh.WithHavesMetric(sshHavesMetric),
-		ssh.WithFiltersMetric(sshFiltersMetric),
+		ssh.WithPackfileNegotiationMetrics(sshPackfileNegotiationMetrics),
 	))
 	gitalypb.RegisterSmartHTTPServiceServer(grpcServer, smarthttp.NewServer(
-		smarthttp.WithDeepensMetric(smarthttpDeepensMetric),
-		smarthttp.WithFiltersMetric(smarthttpFiltersMetric),
-		smarthttp.WithHavesMetric(smarthttpHavesMetric),
+		smarthttp.WithPackfileNegotiationMetrics(smarthttpPackfileNegotiationMetrics),
 	))
 	gitalypb.RegisterWikiServiceServer(grpcServer, wiki.NewServer(rubyServer))
 	gitalypb.RegisterConflictsServiceServer(grpcServer, conflicts.NewServer(rubyServer))
diff --git a/internal/service/smarthttp/server.go b/internal/service/smarthttp/server.go
index b00dbb043..76ab4b36e 100644
--- a/internal/service/smarthttp/server.go
+++ b/internal/service/smarthttp/server.go
@@ -2,23 +2,21 @@ package smarthttp
 
 import (
 	"github.com/prometheus/client_golang/prometheus"
-	"gitlab.com/gitlab-org/gitaly/internal/prometheus/metrics"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 )
 
 type server struct {
-	deepensMetric metrics.Counter
-	filtersMetric metrics.Counter
-	havesMetric   metrics.Counter
+	packfileNegotiationMetrics *prometheus.CounterVec
 	gitalypb.UnimplementedSmartHTTPServiceServer
 }
 
 // NewServer creates a new instance of a grpc SmartHTTPServer
 func NewServer(serverOpts ...ServerOpt) gitalypb.SmartHTTPServiceServer {
 	s := &server{
-		deepensMetric: prometheus.NewCounter(prometheus.CounterOpts{}),
-		filtersMetric: prometheus.NewCounter(prometheus.CounterOpts{}),
-		havesMetric:   prometheus.NewCounter(prometheus.CounterOpts{}),
+		packfileNegotiationMetrics: prometheus.NewCounterVec(
+			prometheus.CounterOpts{},
+			[]string{"git_negotiation_feature"},
+		),
 	}
 
 	for _, serverOpt := range serverOpts {
@@ -31,20 +29,8 @@ func NewServer(serverOpts ...ServerOpt) gitalypb.SmartHTTPServiceServer {
 // ServerOpt is a self referential option for server
 type ServerOpt func(s *server)
 
-func WithDeepensMetric(c metrics.Counter) ServerOpt {
+func WithPackfileNegotiationMetrics(c *prometheus.CounterVec) ServerOpt {
 	return func(s *server) {
-		s.deepensMetric = c
-	}
-}
-
-func WithFiltersMetric(c metrics.Counter) ServerOpt {
-	return func(s *server) {
-		s.filtersMetric = c
-	}
-}
-
-func WithHavesMetric(c metrics.Counter) ServerOpt {
-	return func(s *server) {
-		s.havesMetric = c
+		s.packfileNegotiationMetrics = c
 	}
 }
diff --git a/internal/service/smarthttp/upload_pack.go b/internal/service/smarthttp/upload_pack.go
index db18176e4..04792ba8e 100644
--- a/internal/service/smarthttp/upload_pack.go
+++ b/internal/service/smarthttp/upload_pack.go
@@ -52,13 +52,13 @@ func (s *server) PostUploadPack(stream gitalypb.SmartHTTPService_PostUploadPackS
 		}
 
 		if stats.Deepen != "" {
-			s.deepensMetric.Inc()
+			s.packfileNegotiationMetrics.WithLabelValues("deepen").Inc()
 		}
 		if stats.Filter != "" {
-			s.filtersMetric.Inc()
+			s.packfileNegotiationMetrics.WithLabelValues("filter").Inc()
 		}
 		if len(stats.Haves) > 0 {
-			s.havesMetric.Inc()
+			s.packfileNegotiationMetrics.WithLabelValues("have").Inc()
 		}
 
 		statsCh <- stats
diff --git a/internal/service/smarthttp/upload_pack_test.go b/internal/service/smarthttp/upload_pack_test.go
index 57a0f04f5..26d078846 100644
--- a/internal/service/smarthttp/upload_pack_test.go
+++ b/internal/service/smarthttp/upload_pack_test.go
@@ -12,6 +12,8 @@ import (
 	"testing"
 	"time"
 
+	"github.com/prometheus/client_golang/prometheus"
+	promtest "github.com/prometheus/client_golang/prometheus/testutil"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
@@ -19,7 +21,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
-	"gitlab.com/gitlab-org/gitaly/internal/testhelper/promtest"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/streamio"
 	"google.golang.org/grpc/codes"
@@ -30,10 +31,10 @@ const (
 )
 
 func TestSuccessfulUploadPackRequest(t *testing.T) {
-	haves := &promtest.MockCounter{}
+	negotiationMetrics := prometheus.NewCounterVec(prometheus.CounterOpts{}, []string{"feature"})
 
 	serverSocketPath, stop := runSmartHTTPServer(
-		t, WithHavesMetric(haves),
+		t, WithPackfileNegotiationMetrics(negotiationMetrics),
 	)
 	defer stop()
 
@@ -98,7 +99,9 @@ func TestSuccessfulUploadPackRequest(t *testing.T) {
 	// The fact that this command succeeds means that we got the commit correctly, no further checks should be needed.
 	testhelper.MustRunCommand(t, nil, "git", "-C", localRepoPath, "show", string(newHead))
 
-	require.Equal(t, 1.0, haves.Value())
+	metric, err := negotiationMetrics.GetMetricWithLabelValues("have")
+	require.NoError(t, err)
+	require.Equal(t, 1.0, promtest.ToFloat64(metric))
 }
 
 func TestUploadPackRequestWithGitConfigOptions(t *testing.T) {
@@ -331,10 +334,10 @@ func extractPackDataFromResponse(t *testing.T, buf *bytes.Buffer) ([]byte, int,
 }
 
 func TestUploadPackRequestForPartialCloneSuccess(t *testing.T) {
-	filter := &promtest.MockCounter{}
+	negotiationMetrics := prometheus.NewCounterVec(prometheus.CounterOpts{}, []string{"feature"})
 
 	serverSocketPath, stop := runSmartHTTPServer(
-		t, WithFiltersMetric(filter),
+		t, WithPackfileNegotiationMetrics(negotiationMetrics),
 	)
 	defer stop()
 
@@ -430,5 +433,7 @@ func TestUploadPackRequestForPartialCloneSuccess(t *testing.T) {
 	_, err = makePostUploadPackRequest(ctx, t, serverSocketPath, req, &requestBuffer)
 	require.NoError(t, err)
 
-	require.Equal(t, 2.0, filter.Value())
+	metric, err := negotiationMetrics.GetMetricWithLabelValues("filter")
+	require.NoError(t, err)
+	require.Equal(t, 2.0, promtest.ToFloat64(metric))
 }
diff --git a/internal/service/ssh/server.go b/internal/service/ssh/server.go
index 808877f1f..abe01ad98 100644
--- a/internal/service/ssh/server.go
+++ b/internal/service/ssh/server.go
@@ -4,7 +4,6 @@ import (
 	"time"
 
 	"github.com/prometheus/client_golang/prometheus"
-	"gitlab.com/gitlab-org/gitaly/internal/prometheus/metrics"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 )
 
@@ -16,9 +15,7 @@ var (
 type server struct {
 	uploadPackRequestTimeout    time.Duration
 	uploadArchiveRequestTimeout time.Duration
-	havesMetric                 metrics.Counter
-	deepensMetric               metrics.Counter
-	filtersMetric               metrics.Counter
+	packfileNegotiationMetrics  *prometheus.CounterVec
 	gitalypb.UnimplementedSSHServiceServer
 }
 
@@ -27,9 +24,10 @@ func NewServer(serverOpts ...ServerOpt) gitalypb.SSHServiceServer {
 	s := &server{
 		uploadPackRequestTimeout:    defaultUploadPackRequestTimeout,
 		uploadArchiveRequestTimeout: defaultUploadArchiveRequestTimeout,
-		deepensMetric:               prometheus.NewCounter(prometheus.CounterOpts{}),
-		filtersMetric:               prometheus.NewCounter(prometheus.CounterOpts{}),
-		havesMetric:                 prometheus.NewCounter(prometheus.CounterOpts{}),
+		packfileNegotiationMetrics: prometheus.NewCounterVec(
+			prometheus.CounterOpts{},
+			[]string{"git_negotiation_feature"},
+		),
 	}
 
 	for _, serverOpt := range serverOpts {
@@ -56,20 +54,8 @@ func WithArchiveRequestTimeout(d time.Duration) ServerOpt {
 	}
 }
 
-func WithHavesMetric(c metrics.Counter) ServerOpt {
+func WithPackfileNegotiationMetrics(c *prometheus.CounterVec) ServerOpt {
 	return func(s *server) {
-		s.havesMetric = c
-	}
-}
-
-func WithDeepensMetric(c metrics.Counter) ServerOpt {
-	return func(s *server) {
-		s.deepensMetric = c
-	}
-}
-
-func WithFiltersMetric(c metrics.Counter) ServerOpt {
-	return func(s *server) {
-		s.filtersMetric = c
+		s.packfileNegotiationMetrics = c
 	}
 }
diff --git a/internal/service/ssh/upload_pack.go b/internal/service/ssh/upload_pack.go
index 854eff492..5618190d3 100644
--- a/internal/service/ssh/upload_pack.go
+++ b/internal/service/ssh/upload_pack.go
@@ -104,13 +104,13 @@ func (s *server) sshUploadPack(stream gitalypb.SSHService_SSHUploadPackServer, r
 			return
 		}
 		if stats.Deepen != "" {
-			s.deepensMetric.Inc()
+			s.packfileNegotiationMetrics.WithLabelValues("deepen").Inc()
 		}
 		if stats.Filter != "" {
-			s.filtersMetric.Inc()
+			s.packfileNegotiationMetrics.WithLabelValues("filter").Inc()
 		}
 		if len(stats.Haves) > 0 {
-			s.havesMetric.Inc()
+			s.packfileNegotiationMetrics.WithLabelValues("have").Inc()
 		}
 	}()
 
diff --git a/internal/service/ssh/upload_pack_test.go b/internal/service/ssh/upload_pack_test.go
index 447b9c8df..a7307fd84 100644
--- a/internal/service/ssh/upload_pack_test.go
+++ b/internal/service/ssh/upload_pack_test.go
@@ -12,12 +12,13 @@ import (
 	"time"
 
 	"github.com/golang/protobuf/jsonpb"
+	"github.com/prometheus/client_golang/prometheus"
+	promtest "github.com/prometheus/client_golang/prometheus/testutil"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
 	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
-	"gitlab.com/gitlab-org/gitaly/internal/testhelper/promtest"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
 )
@@ -186,12 +187,10 @@ func TestFailedUploadPackRequestDueToValidationError(t *testing.T) {
 }
 
 func TestUploadPackCloneSuccess(t *testing.T) {
-	deepen := &promtest.MockCounter{}
-	filter := &promtest.MockCounter{}
-	haves := &promtest.MockCounter{}
+	negotiationMetrics := prometheus.NewCounterVec(prometheus.CounterOpts{}, []string{"feature"})
 
 	serverSocketPath, stop := runSSHServer(
-		t, WithDeepensMetric(deepen), WithFiltersMetric(filter), WithHavesMetric(haves),
+		t, WithPackfileNegotiationMetrics(negotiationMetrics),
 	)
 	defer stop()
 
@@ -224,9 +223,9 @@ func TestUploadPackCloneSuccess(t *testing.T) {
 			lHead, rHead, _, _ := cmd.test(t, localRepoPath)
 			require.Equal(t, lHead, rHead, "local and remote head not equal")
 
-			require.Equal(t, deepen.Value(), tc.deepen)
-			require.Equal(t, filter.Value(), 0.0)
-			require.Equal(t, haves.Value(), 0.0)
+			metric, err := negotiationMetrics.GetMetricWithLabelValues("deepen")
+			require.NoError(t, err)
+			require.Equal(t, tc.deepen, promtest.ToFloat64(metric))
 		})
 	}
 }