Merge branch 'wc/repo-count-dedup' into 'master'

counter: Key metric on storage path instead of name

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

Merged-by: Justin Tobler <jtobler@gitlab.com>
Approved-by: Steve Xuereb <sxuereb@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: Justin Tobler <jtobler@gitlab.com>
Reviewed-by: Steve Xuereb <sxuereb@gitlab.com>
Reviewed-by: Will Chandler <wchandler@gitlab.com>
Reviewed-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Co-authored-by: Will Chandler <wchandler@gitlab.com>

7 files changed, 123 insertions, 57 deletions

diff --git a/internal/backup/backup_test.go b/internal/backup/backup_test.go
index 46a8e840d..af905e9d0 100644
--- a/internal/backup/backup_test.go
+++ b/internal/backup/backup_test.go
@@ -70,7 +70,7 @@ func TestManager_Create(t *testing.T) {
 	cfg.SocketPath = testserver.RunGitalyServer(t, cfg, setup.RegisterAll)
 
 	ctx := testhelper.Context(t)
-	repoCounter := counter.NewRepositoryCounter()
+	repoCounter := counter.NewRepositoryCounter(cfg.Storages)
 
 	for _, managerTC := range []struct {
 		desc  string
@@ -238,7 +238,7 @@ func TestManager_Create_incremental(t *testing.T) {
 
 	cfg.SocketPath = testserver.RunGitalyServer(t, cfg, setup.RegisterAll)
 	ctx := testhelper.Context(t)
-	repoCounter := counter.NewRepositoryCounter()
+	repoCounter := counter.NewRepositoryCounter(cfg.Storages)
 
 	for _, managerTC := range []struct {
 		desc  string
@@ -382,7 +382,7 @@ func TestManager_Restore_latest(t *testing.T) {
 	cfg := testcfg.Build(t)
 	testcfg.BuildGitalyHooks(t, cfg)
 	cfg.SocketPath = testserver.RunGitalyServer(t, cfg, setup.RegisterAll)
-	repoCounter := counter.NewRepositoryCounter()
+	repoCounter := counter.NewRepositoryCounter(cfg.Storages)
 
 	for _, managerTC := range []struct {
 		desc  string
@@ -710,7 +710,7 @@ func TestManager_Restore_specific(t *testing.T) {
 	cfg := testcfg.Build(t)
 	testcfg.BuildGitalyHooks(t, cfg)
 	cfg.SocketPath = testserver.RunGitalyServer(t, cfg, setup.RegisterAll)
-	repoCounter := counter.NewRepositoryCounter()
+	repoCounter := counter.NewRepositoryCounter(cfg.Storages)
 
 	for _, managerTC := range []struct {
 		desc  string
diff --git a/internal/cli/gitaly/serve.go b/internal/cli/gitaly/serve.go
index b0a0c3833..5f80d28e5 100644
--- a/internal/cli/gitaly/serve.go
+++ b/internal/cli/gitaly/serve.go
@@ -230,9 +230,9 @@ func run(cfg config.Cfg) error {
 
 	locator := config.NewLocator(cfg)
 
-	repoCounter := counter.NewRepositoryCounter()
+	repoCounter := counter.NewRepositoryCounter(cfg.Storages)
 	prometheus.MustRegister(repoCounter)
-	repoCounter.StartCountingRepositories(ctx, locator, cfg.Storages, log.StandardLogger())
+	repoCounter.StartCountingRepositories(ctx, locator, log.StandardLogger())
 
 	tempdir.StartCleaning(locator, cfg.Storages, time.Hour)
 
diff --git a/internal/gitaly/repoutil/create_test.go b/internal/gitaly/repoutil/create_test.go
index 614782342..08d2d43bc 100644
--- a/internal/gitaly/repoutil/create_test.go
+++ b/internal/gitaly/repoutil/create_test.go
@@ -322,7 +322,7 @@ func TestCreate(t *testing.T) {
 			// manager's data.
 			ctx := ctx
 			*txManager = transaction.MockManager{}
-			repoCounter := counter.NewRepositoryCounter()
+			repoCounter := counter.NewRepositoryCounter(cfg.Storages)
 
 			repo := &gitalypb.Repository{
 				StorageName:  cfg.Storages[0].Name,
diff --git a/internal/gitaly/repoutil/remove_test.go b/internal/gitaly/repoutil/remove_test.go
index 7d59b78c2..ae46b4d25 100644
--- a/internal/gitaly/repoutil/remove_test.go
+++ b/internal/gitaly/repoutil/remove_test.go
@@ -94,7 +94,7 @@ func TestRemove(t *testing.T) {
 			cfg := testcfg.Build(t)
 			locator := config.NewLocator(cfg)
 			txManager := transaction.NewTrackingManager()
-			repoCounter := counter.NewRepositoryCounter()
+			repoCounter := counter.NewRepositoryCounter(cfg.Storages)
 
 			repo, repoPath := tc.createRepo(t, ctx, cfg)
 
diff --git a/internal/gitaly/storage/counter/counter.go b/internal/gitaly/storage/counter/counter.go
index 117e21dfb..86accfeb6 100644
--- a/internal/gitaly/storage/counter/counter.go
+++ b/internal/gitaly/storage/counter/counter.go
@@ -22,20 +22,23 @@ import (
 type RepositoryCounter struct {
 	reposTotal     *prometheus.GaugeVec
 	suppressMetric atomic.Bool
+	// Lookup path associated with a storage.
+	storageNameToPath map[string]string
 }
 
 // NewRepositoryCounter constructs a RepositoryCounter object.
-func NewRepositoryCounter() *RepositoryCounter {
+func NewRepositoryCounter(storages []config.Storage) *RepositoryCounter {
 	c := &RepositoryCounter{
 		reposTotal: prometheus.NewGaugeVec(
 			prometheus.GaugeOpts{
 				Name: "gitaly_total_repositories_count",
-				Help: "Gauge of number of repositories by storage and path",
+				Help: "Gauge of number of repositories by storage path and repository prefix",
 			},
-			[]string{"storage", "prefix"},
+			[]string{"path", "prefix"},
 		),
 	}
 	c.suppressMetric.Store(true)
+	c.storageNameToPath = nameToPath(storages)
 
 	return c
 }
@@ -58,29 +61,36 @@ func (c *RepositoryCounter) Collect(metrics chan<- prometheus.Metric) {
 func (c *RepositoryCounter) StartCountingRepositories(
 	ctx context.Context,
 	locator storage.Locator,
-	storages []config.Storage,
 	logger log.FieldLogger,
 ) {
 	dontpanic.Go(func() {
-		c.countRepositories(ctx, locator, storages, logger)
+		c.countRepositories(ctx, locator, logger)
 	})
 }
 
 func (c *RepositoryCounter) countRepositories(
 	ctx context.Context,
 	locator storage.Locator,
-	storages []config.Storage,
 	logger log.FieldLogger,
 ) {
 	defer func() {
 		c.suppressMetric.Store(false)
 	}()
 
+	// Multiple storage names may share a single path, ensure we walk each path only once.
+	uniquePaths := make(map[string]string)
+	for name, path := range c.storageNameToPath {
+		_, ok := uniquePaths[path]
+		if !ok {
+			uniquePaths[path] = name
+		}
+	}
+
 	logger.Info("counting repositories")
 	totalStart := time.Now()
 
-	for _, stor := range storages {
-		logger.Infof("starting to count repositories in storage %q", stor.Name)
+	for storPath, name := range uniquePaths {
+		logger.Infof("starting to count repositories in path %q", storPath)
 		storageStart := time.Now()
 
 		paths := make(map[string]float64)
@@ -89,24 +99,23 @@ func (c *RepositoryCounter) countRepositories(
 			if err != nil {
 				// Encountering a malformed path should not block us from continuing
 				// to count.
-				logger.WithError(err).Warnf("counting repositories: walking storage %q", stor)
+				logger.WithError(err).Warnf("counting repositories: walking path %q", storPath)
 				return nil
 			}
 
 			paths[prefix]++
 			return nil
 		}
-		ctx := context.Background()
 
-		if err := walk.FindRepositories(ctx, locator, stor.Name, incrementPrefix); err != nil {
-			log.WithError(err).Errorf("failed to count repositories in storage %q", stor.Name)
+		if err := walk.FindRepositories(ctx, locator, name, incrementPrefix); err != nil {
+			log.WithError(err).Errorf("failed to count repositories in path %q", storPath)
 		}
 
 		for prefix, ct := range paths {
-			c.reposTotal.WithLabelValues(stor.Name, prefix).Add(ct)
+			c.reposTotal.WithLabelValues(storPath, prefix).Add(ct)
 		}
 
-		logger.Infof("completed counting repositories in storage %q after %s", stor.Name, time.Since(storageStart))
+		logger.Infof("completed counting repositories in path %q after %s", storPath, time.Since(storageStart))
 	}
 
 	logger.Infof("completed counting all repositories after %s", time.Since(totalStart))
@@ -123,8 +132,10 @@ func (c *RepositoryCounter) Decrement(repo storage.Repository) {
 }
 
 // DeleteStorage removes metrics associated with a storage.
-func (c *RepositoryCounter) DeleteStorage(storage string) {
-	c.reposTotal.DeletePartialMatch(prometheus.Labels{"storage": storage})
+func (c *RepositoryCounter) DeleteStorage(storageName string) {
+	if path, exist := c.storageNameToPath[storageName]; exist {
+		c.reposTotal.DeletePartialMatch(prometheus.Labels{"path": path})
+	}
 }
 
 func (c *RepositoryCounter) add(repo storage.Repository, ct float64) {
@@ -132,8 +143,20 @@ func (c *RepositoryCounter) add(repo storage.Repository, ct float64) {
 	if err != nil {
 		return
 	}
+	if path, exist := c.storageNameToPath[repo.GetStorageName()]; exist {
+		c.reposTotal.WithLabelValues(path, prefix).Add(ct)
+	}
+}
+
+// Create a map of storage name to paths for quick lookup.
+func nameToPath(storages []config.Storage) map[string]string {
+	m := make(map[string]string)
+
+	for _, stor := range storages {
+		m[stor.Name] = stor.Path
+	}
 
-	c.reposTotal.WithLabelValues(repo.GetStorageName(), prefix).Add(ct)
+	return m
 }
 
 func getPrefix(path string) (string, error) {
diff --git a/internal/gitaly/storage/counter/counter_test.go b/internal/gitaly/storage/counter/counter_test.go
index f06692041..78d80e0cb 100644
--- a/internal/gitaly/storage/counter/counter_test.go
+++ b/internal/gitaly/storage/counter/counter_test.go
@@ -16,8 +16,8 @@ import (
 )
 
 type metric struct {
-	storage, prefix string
-	count           int
+	path, prefix string
+	count        int
 }
 
 func TestCountStorages(t *testing.T) {
@@ -26,46 +26,79 @@ func TestCountStorages(t *testing.T) {
 	for _, tc := range []struct {
 		name            string
 		storageNames    []string
+		sharedPath      bool
 		repos           []string
 		files           []string
-		expectedMetrics []metric
+		expectedMetrics func(map[string]string) []metric
 	}{
 		{
 			name:         "non-praefect paths",
 			storageNames: []string{"foo"},
 			repos:        []string{"@hashed/aa/bb/repo-1.git", "@hashed/01/23/repo-2.git"},
-			expectedMetrics: []metric{
-				{storage: "foo", prefix: "@hashed", count: 2},
+			expectedMetrics: func(storagePaths map[string]string) []metric {
+				path, ok := storagePaths["foo"]
+				require.True(t, ok)
+				return []metric{{path: path, prefix: "@hashed", count: 2}}
 			},
 		},
 		{
 			name:         "multiple prefixes",
 			storageNames: []string{"foo"},
 			repos:        []string{"@hashed/aa/bb/repo-1.git", "@snippets/01/23/repo-2.git", "@groups/ee/ff/wiki.git"},
-			expectedMetrics: []metric{
-				{storage: "foo", prefix: "@hashed", count: 1},
-				{storage: "foo", prefix: "@snippets", count: 1},
-				{storage: "foo", prefix: "@groups", count: 1},
+			expectedMetrics: func(storagePaths map[string]string) []metric {
+				path, ok := storagePaths["foo"]
+				require.True(t, ok)
+				return []metric{
+					{path: path, prefix: "@hashed", count: 1},
+					{path: path, prefix: "@snippets", count: 1},
+					{path: path, prefix: "@groups", count: 1},
+				}
 			},
 		},
 		{
 			name:         "@cluster paths",
 			storageNames: []string{"foo"},
 			repos:        []string{"@cluster/bar/01/23/1234", "@cluster/baz/45/67/89ab"},
-			expectedMetrics: []metric{
-				{storage: "foo", prefix: "bar", count: 1},
-				{storage: "foo", prefix: "baz", count: 1},
+			expectedMetrics: func(storagePaths map[string]string) []metric {
+				path, ok := storagePaths["foo"]
+				require.True(t, ok)
+				return []metric{
+					{path: path, prefix: "bar", count: 1},
+					{path: path, prefix: "baz", count: 1},
+				}
 			},
 		},
 		{
 			name:         "multiple storages",
 			storageNames: []string{"foo", "bar"},
 			repos:        []string{"@hashed/aa/bb/repo-1.git", "@snippets/01/23/repo-2.git"},
-			expectedMetrics: []metric{
-				{storage: "foo", prefix: "@hashed", count: 1},
-				{storage: "foo", prefix: "@snippets", count: 1},
-				{storage: "bar", prefix: "@hashed", count: 1},
-				{storage: "bar", prefix: "@snippets", count: 1},
+			expectedMetrics: func(storagePaths map[string]string) []metric {
+				fooPath, ok := storagePaths["foo"]
+				require.True(t, ok)
+				barPath, ok := storagePaths["bar"]
+				require.True(t, ok)
+
+				return []metric{
+					{path: fooPath, prefix: "@hashed", count: 1},
+					{path: fooPath, prefix: "@snippets", count: 1},
+					{path: barPath, prefix: "@hashed", count: 1},
+					{path: barPath, prefix: "@snippets", count: 1},
+				}
+			},
+		},
+		{
+			name:         "storages sharing a path are deduped",
+			storageNames: []string{"foo", "bar"},
+			sharedPath:   true,
+			repos:        []string{"@hashed/aa/bb/repo-1.git", "@snippets/01/23/repo-2.git"},
+			expectedMetrics: func(storagePaths map[string]string) []metric {
+				fooPath, ok := storagePaths["foo"]
+				require.True(t, ok)
+
+				return []metric{
+					{path: fooPath, prefix: "@hashed", count: 1},
+					{path: fooPath, prefix: "@snippets", count: 1},
+				}
 			},
 		},
 	} {
@@ -73,12 +106,15 @@ func TestCountStorages(t *testing.T) {
 
 		t.Run(tc.name, func(t *testing.T) {
 			t.Parallel()
-
 			ctx := testhelper.Context(t)
 			cfg := testcfg.Build(t, testcfg.WithStorages(tc.storageNames[0], tc.storageNames[1:]...))
 			locator := config.NewLocator(cfg)
 			logger := testhelper.NewDiscardingLogger(t)
 
+			if tc.sharedPath {
+				cfg.Storages[1].Path = cfg.Storages[0].Path
+			}
+
 			for _, storage := range cfg.Storages {
 				for _, path := range tc.repos {
 					_, _ = gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{
@@ -89,12 +125,14 @@ func TestCountStorages(t *testing.T) {
 				}
 			}
 
-			c := NewRepositoryCounter()
+			c := NewRepositoryCounter(cfg.Storages)
+
+			c.countRepositories(ctx, locator, logger)
 
-			c.countRepositories(ctx, locator, cfg.Storages, logger)
+			storages := nameToPath(cfg.Storages)
 
 			require.NoError(t, testutil.CollectAndCompare(
-				c, buildMetrics(t, tc.expectedMetrics), "gitaly_total_repositories_count"))
+				c, buildMetrics(t, tc.expectedMetrics(storages)), "gitaly_total_repositories_count"))
 		})
 	}
 }
@@ -103,6 +141,7 @@ func TestCounter(t *testing.T) {
 	t.Parallel()
 
 	const storageName = "default"
+	const storagePath = "/repos"
 	repo := &gitalypb.Repository{
 		StorageName:  storageName,
 		RelativePath: gittest.NewRepositoryName(t),
@@ -119,14 +158,14 @@ func TestCounter(t *testing.T) {
 			setup: func(t *testing.T, c *RepositoryCounter) {
 				c.Increment(repo)
 			},
-			expectedMetrics: []metric{{storage: storageName, prefix: "@hashed", count: 1}},
+			expectedMetrics: []metric{{path: storagePath, prefix: "@hashed", count: 1}},
 		},
 		{
 			name: "decrement",
 			setup: func(t *testing.T, c *RepositoryCounter) {
 				c.Decrement(repo)
 			},
-			expectedMetrics: []metric{{storage: storageName, prefix: "@hashed", count: -1}},
+			expectedMetrics: []metric{{path: storagePath, prefix: "@hashed", count: -1}},
 		},
 		{
 			name: "object pool",
@@ -137,7 +176,7 @@ func TestCounter(t *testing.T) {
 				}
 				c.Increment(poolRepo)
 			},
-			expectedMetrics: []metric{{storage: storageName, prefix: "@pools", count: 1}},
+			expectedMetrics: []metric{{path: storagePath, prefix: "@pools", count: 1}},
 		},
 		{
 			name: "praefect",
@@ -154,8 +193,8 @@ func TestCounter(t *testing.T) {
 				c.Increment(praefectPool)
 			},
 			expectedMetrics: []metric{
-				{storage: storageName, prefix: "repositories", count: 1},
-				{storage: storageName, prefix: "pools", count: 1},
+				{path: storagePath, prefix: "repositories", count: 1},
+				{path: storagePath, prefix: "pools", count: 1},
 			},
 		},
 		{
@@ -170,7 +209,7 @@ func TestCounter(t *testing.T) {
 
 				c.DeleteStorage(storageName)
 			},
-			expectedMetrics: []metric{{storage: "other", prefix: "@hashed", count: 1}},
+			expectedMetrics: []metric{{path: "/other", prefix: "@hashed", count: 1}},
 		},
 		{
 			name: "invalid path ignored",
@@ -195,7 +234,11 @@ func TestCounter(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			t.Parallel()
 
-			c := NewRepositoryCounter()
+			storages := []config.Storage{
+				{Name: storageName, Path: storagePath},
+				{Name: "other", Path: "/other"},
+			}
+			c := NewRepositoryCounter(storages)
 
 			c.suppressMetric.Store(tc.suppress)
 			tc.setup(t, c)
@@ -210,14 +253,14 @@ func buildMetrics(t *testing.T, metrics []metric) *strings.Reader {
 	t.Helper()
 
 	var builder strings.Builder
-	_, err := builder.WriteString("# HELP gitaly_total_repositories_count Gauge of number of repositories by storage and path\n")
+	_, err := builder.WriteString("# HELP gitaly_total_repositories_count Gauge of number of repositories by storage path and repository prefix\n")
 	require.NoError(t, err)
 	_, err = builder.WriteString("# TYPE gitaly_total_repositories_count gauge\n")
 	require.NoError(t, err)
 
 	for _, item := range metrics {
-		_, err = builder.WriteString(fmt.Sprintf("gitaly_total_repositories_count{prefix=%q, storage=%q} %d\n",
-			item.prefix, item.storage, item.count))
+		_, err = builder.WriteString(fmt.Sprintf("gitaly_total_repositories_count{path=%q, prefix=%q} %d\n",
+			item.path, item.prefix, item.count))
 		require.NoError(t, err)
 	}
 	return strings.NewReader(builder.String())
diff --git a/internal/testhelper/testserver/gitaly.go b/internal/testhelper/testserver/gitaly.go
index 4f17f66c4..0ce7dbb59 100644
--- a/internal/testhelper/testserver/gitaly.go
+++ b/internal/testhelper/testserver/gitaly.go
@@ -341,7 +341,7 @@ func (gsd *gitalyServerDeps) createDependencies(ctx context.Context, tb testing.
 	}
 
 	if gsd.repositoryCounter == nil {
-		gsd.repositoryCounter = counter.NewRepositoryCounter()
+		gsd.repositoryCounter = counter.NewRepositoryCounter(cfg.Storages)
 	}
 
 	if gsd.git2goExecutor == nil {