housekeeping: Add a status to the gitaly_housekeeping_tasks_total metricjc-dont-call-optimize-repo

In order to have visibility into success and failures for housekeeing
tasks, add a label to the gitaly_housekeeping_tasks_total metric.

In optimizeRepository, there is an anonymous struct to keep track of
whether or not a given housekeeping task ran. This added layer of
indirection doesn't help much, however, since we end up mapping it back
to strings anyways when emitting the metric.

Instead, use a plain map keyed on the metric name with a boolean value
for whether or not it succeeded. This way, if the metric exists in the
map, then we know that it ran and whether or not it succeeded. If a
metric does not exist in the map, then that means the task was never
attempted.

4 files changed, 84 insertions, 76 deletions

diff --git a/internal/git/housekeeping/manager.go b/internal/git/housekeeping/manager.go
index 552c153c9..c34aa1330 100644
--- a/internal/git/housekeeping/manager.go
+++ b/internal/git/housekeeping/manager.go
@@ -41,7 +41,7 @@ func NewManager(promCfg gitalycfgprom.Config, txManager transaction.Manager) *Re
 				Name: "gitaly_housekeeping_tasks_total",
 				Help: "Total number of housekeeping tasks performed in the repository",
 			},
-			[]string{"housekeeping_task"},
+			[]string{"housekeeping_task", "status"},
 		),
 		tasksLatency: prometheus.NewHistogramVec(
 			prometheus.HistogramOpts{
diff --git a/internal/git/housekeeping/optimize_repository.go b/internal/git/housekeeping/optimize_repository.go
index 09d923c06..f4fc8419e 100644
--- a/internal/git/housekeeping/optimize_repository.go
+++ b/internal/git/housekeeping/optimize_repository.go
@@ -41,31 +41,18 @@ func (m *RepositoryManager) OptimizeRepository(ctx context.Context, repo *localr
 
 func optimizeRepository(ctx context.Context, m *RepositoryManager, repo *localrepo.Repo) error {
 	totalTimer := prometheus.NewTimer(m.tasksLatency.WithLabelValues("total"))
+	totalStatus := "failure"
 
-	optimizations := struct {
-		PackedObjectsIncremental bool `json:"packed_objects_incremental"`
-		PackedObjectsFull        bool `json:"packed_objects_full"`
-		PrunedObjects            bool `json:"pruned_objects"`
-		PackedRefs               bool `json:"packed_refs"`
-		WrittenBitmap            bool `json:"written_bitmap"`
-	}{}
+	optimizations := make(map[string]string)
 	defer func() {
 		totalTimer.ObserveDuration()
 		ctxlogrus.Extract(ctx).WithField("optimizations", optimizations).Info("optimized repository")
 
-		for task, executed := range map[string]bool{
-			"packed_objects_incremental": optimizations.PackedObjectsIncremental,
-			"packed_objects_full":        optimizations.PackedObjectsFull,
-			"pruned_objects":             optimizations.PrunedObjects,
-			"packed_refs":                optimizations.PackedRefs,
-			"written_bitmap":             optimizations.WrittenBitmap,
-		} {
-			if executed {
-				m.tasksTotal.WithLabelValues(task).Add(1)
-			}
+		for task, status := range optimizations {
+			m.tasksTotal.WithLabelValues(task, status).Inc()
 		}
 
-		m.tasksTotal.WithLabelValues("total").Add(1)
+		m.tasksTotal.WithLabelValues("total", totalStatus).Add(1)
 	}()
 
 	timer := prometheus.NewTimer(m.tasksLatency.WithLabelValues("clean-stale-data"))
@@ -83,30 +70,45 @@ func optimizeRepository(ctx context.Context, m *RepositoryManager, repo *localre
 	timer = prometheus.NewTimer(m.tasksLatency.WithLabelValues("repack"))
 	didRepack, repackCfg, err := repackIfNeeded(ctx, repo)
 	if err != nil {
+		optimizations["packed_objects_full"] = "failure"
+		optimizations["packed_objects_incremental"] = "failure"
+		optimizations["written_bitmap"] = "failure"
 		return fmt.Errorf("could not repack: %w", err)
 	}
 	if didRepack {
-		optimizations.PackedObjectsIncremental = !repackCfg.FullRepack
-		optimizations.PackedObjectsFull = repackCfg.FullRepack
-		optimizations.WrittenBitmap = repackCfg.WriteBitmap
+		if repackCfg.FullRepack {
+			optimizations["packed_objects_full"] = "success"
+		} else {
+			optimizations["packed_objects_incremental"] = "success"
+		}
+		if repackCfg.WriteBitmap {
+			optimizations["written_bitmap"] = "success"
+		}
 	}
+
 	timer.ObserveDuration()
 
 	timer = prometheus.NewTimer(m.tasksLatency.WithLabelValues("prune"))
 	didPrune, err := pruneIfNeeded(ctx, repo)
 	if err != nil {
+		optimizations["pruned_objects"] = "failure"
 		return fmt.Errorf("could not prune: %w", err)
+	} else if didPrune {
+		optimizations["pruned_objects"] = "success"
 	}
-	optimizations.PrunedObjects = didPrune
 	timer.ObserveDuration()
 
 	timer = prometheus.NewTimer(m.tasksLatency.WithLabelValues("pack-refs"))
 	didPackRefs, err := packRefsIfNeeded(ctx, repo)
 	if err != nil {
+		optimizations["packed_refs"] = "failure"
 		return fmt.Errorf("could not pack refs: %w", err)
+	} else if didPackRefs {
+		optimizations["packed_refs"] = "success"
 	}
-	optimizations.PackedRefs = didPackRefs
+
 	timer.ObserveDuration()
+	totalStatus = "success"
 
 	return nil
 }
diff --git a/internal/git/housekeeping/optimize_repository_ext_test.go b/internal/git/housekeeping/optimize_repository_ext_test.go
index b494edba4..723408ec0 100644
--- a/internal/git/housekeeping/optimize_repository_ext_test.go
+++ b/internal/git/housekeeping/optimize_repository_ext_test.go
@@ -119,22 +119,16 @@ func TestPruneIfNeeded(t *testing.T) {
 			ctx := ctxlogrus.ToContext(ctx, logrus.NewEntry(logger))
 
 			require.NoError(t, housekeeping.NewManager(cfg.Prometheus, nil).OptimizeRepository(ctx, repo))
-			require.Equal(t,
-				struct {
-					PackedObjectsIncremental bool `json:"packed_objects_incremental"`
-					PackedObjectsFull        bool `json:"packed_objects_full"`
-					PrunedObjects            bool `json:"pruned_objects"`
-					PackedRefs               bool `json:"packed_refs"`
-					WrittenBitmap            bool `json:"written_bitmap"`
-				}{
-					PackedObjectsIncremental: false,
-					PackedObjectsFull:        true,
-					PrunedObjects:            tc.expectedPrune,
-					PackedRefs:               false,
-					WrittenBitmap:            true,
-				},
-				hook.Entries[len(hook.Entries)-1].Data["optimizations"],
-			)
+			expectedLogEntry := map[string]string{
+				"packed_objects_full": "success",
+				"written_bitmap":      "success",
+			}
+
+			if tc.expectedPrune {
+				expectedLogEntry["pruned_objects"] = "success"
+			}
+
+			require.Equal(t, expectedLogEntry, hook.Entries[len(hook.Entries)-1].Data["optimizations"])
 		})
 	}
 }
diff --git a/internal/git/housekeeping/optimize_repository_test.go b/internal/git/housekeeping/optimize_repository_test.go
index e8bd6f0dd..086e3572e 100644
--- a/internal/git/housekeeping/optimize_repository_test.go
+++ b/internal/git/housekeeping/optimize_repository_test.go
@@ -1,6 +1,7 @@
 package housekeeping
 
 import (
+	"bytes"
 	"context"
 	"fmt"
 	"io"
@@ -503,10 +504,10 @@ func TestOptimizeRepository(t *testing.T) {
 	txManager := transaction.NewManager(cfg, backchannel.NewRegistry())
 
 	for _, tc := range []struct {
-		desc                  string
-		setup                 func(t *testing.T) *gitalypb.Repository
-		expectedErr           error
-		expectedOptimizations map[string]float64
+		desc            string
+		setup           func(t *testing.T) *gitalypb.Repository
+		expectedErr     error
+		expectedMetrics string
 	}{
 		{
 			desc: "empty repository tries to write bitmap",
@@ -514,10 +515,12 @@ func TestOptimizeRepository(t *testing.T) {
 				repo, _ := gittest.InitRepo(t, cfg, cfg.Storages[0])
 				return repo
 			},
-			expectedOptimizations: map[string]float64{
-				"packed_objects_full": 1,
-				"written_bitmap":      1,
-			},
+			expectedMetrics: `# HELP gitaly_housekeeping_tasks_total Total number of housekeeping tasks performed in the repository
+# TYPE gitaly_housekeeping_tasks_total counter
+gitaly_housekeeping_tasks_total{housekeeping_task="packed_objects_full", status="success"} 1
+gitaly_housekeeping_tasks_total{housekeeping_task="written_bitmap", status="success"} 1
+gitaly_housekeeping_tasks_total{housekeeping_task="total", status="success"} 1
+`,
 		},
 		{
 			desc: "repository without bitmap repacks objects",
@@ -525,10 +528,12 @@ func TestOptimizeRepository(t *testing.T) {
 				repo, _ := gittest.CloneRepo(t, cfg, cfg.Storages[0])
 				return repo
 			},
-			expectedOptimizations: map[string]float64{
-				"packed_objects_full": 1,
-				"written_bitmap":      1,
-			},
+			expectedMetrics: `# HELP gitaly_housekeeping_tasks_total Total number of housekeeping tasks performed in the repository
+# TYPE gitaly_housekeeping_tasks_total counter
+gitaly_housekeeping_tasks_total{housekeeping_task="packed_objects_full", status="success"} 1
+gitaly_housekeeping_tasks_total{housekeeping_task="written_bitmap", status="success"} 1
+gitaly_housekeeping_tasks_total{housekeeping_task="total", status="success"} 1
+`,
 		},
 		{
 			desc: "repository without commit-graph repacks objects",
@@ -537,10 +542,12 @@ func TestOptimizeRepository(t *testing.T) {
 				gittest.Exec(t, cfg, "-C", repoPath, "repack", "-A", "--write-bitmap-index")
 				return repo
 			},
-			expectedOptimizations: map[string]float64{
-				"packed_objects_full": 1,
-				"written_bitmap":      1,
-			},
+			expectedMetrics: `# HELP gitaly_housekeeping_tasks_total Total number of housekeeping tasks performed in the repository
+# TYPE gitaly_housekeeping_tasks_total counter
+gitaly_housekeeping_tasks_total{housekeeping_task="packed_objects_full", status="success"} 1
+gitaly_housekeeping_tasks_total{housekeeping_task="written_bitmap", status="success"} 1
+gitaly_housekeeping_tasks_total{housekeeping_task="total", status="success"} 1
+`,
 		},
 		{
 			desc: "well-packed repository does not optimize",
@@ -550,6 +557,10 @@ func TestOptimizeRepository(t *testing.T) {
 				gittest.Exec(t, cfg, "-C", repoPath, "commit-graph", "write", "--split", "--changed-paths")
 				return repo
 			},
+			expectedMetrics: `# HELP gitaly_housekeeping_tasks_total Total number of housekeeping tasks performed in the repository
+# TYPE gitaly_housekeeping_tasks_total counter
+gitaly_housekeeping_tasks_total{housekeeping_task="total", status="success"} 1
+`,
 		},
 		{
 			desc: "recent loose objects don't get pruned",
@@ -575,9 +586,11 @@ func TestOptimizeRepository(t *testing.T) {
 
 				return repo
 			},
-			expectedOptimizations: map[string]float64{
-				"packed_objects_incremental": 1,
-			},
+			expectedMetrics: `# HELP gitaly_housekeeping_tasks_total Total number of housekeeping tasks performed in the repository
+# TYPE gitaly_housekeeping_tasks_total counter
+gitaly_housekeeping_tasks_total{housekeeping_task="packed_objects_incremental", status="success"} 1
+gitaly_housekeeping_tasks_total{housekeeping_task="total", status="success"} 1
+`,
 		},
 		{
 			desc: "old loose objects get pruned",
@@ -600,10 +613,12 @@ func TestOptimizeRepository(t *testing.T) {
 
 				return repo
 			},
-			expectedOptimizations: map[string]float64{
-				"packed_objects_incremental": 1,
-				"pruned_objects":             1,
-			},
+			expectedMetrics: `# HELP gitaly_housekeeping_tasks_total Total number of housekeeping tasks performed in the repository
+# TYPE gitaly_housekeeping_tasks_total counter
+gitaly_housekeeping_tasks_total{housekeeping_task="packed_objects_incremental", status="success"} 1
+gitaly_housekeeping_tasks_total{housekeeping_task="pruned_objects",status="success"} 1
+gitaly_housekeeping_tasks_total{housekeeping_task="total", status="success"} 1
+`,
 		},
 		{
 			desc: "loose refs get packed",
@@ -619,9 +634,11 @@ func TestOptimizeRepository(t *testing.T) {
 
 				return repo
 			},
-			expectedOptimizations: map[string]float64{
-				"packed_refs": 1,
-			},
+			expectedMetrics: `# HELP gitaly_housekeeping_tasks_total Total number of housekeeping tasks performed in the repository
+# TYPE gitaly_housekeeping_tasks_total counter
+gitaly_housekeeping_tasks_total{housekeeping_task="packed_refs", status="success"} 1
+gitaly_housekeeping_tasks_total{housekeeping_task="total", status="success"} 1
+`,
 		},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
@@ -635,16 +652,11 @@ func TestOptimizeRepository(t *testing.T) {
 			err := manager.OptimizeRepository(ctx, repo)
 			require.Equal(t, tc.expectedErr, err)
 
-			for _, metric := range []string{
-				"packed_objects_incremental",
-				"packed_objects_full",
-				"pruned_objects",
-				"packed_refs",
-				"written_bitmap",
-			} {
-				value := testutil.ToFloat64(manager.tasksTotal.WithLabelValues(metric))
-				require.Equal(t, tc.expectedOptimizations[metric], value, metric)
-			}
+			assert.NoError(t, testutil.CollectAndCompare(
+				manager.tasksTotal,
+				bytes.NewBufferString(tc.expectedMetrics),
+				"gitaly_housekeeping_tasks_total",
+			))
 		})
 	}
 }