praefect: Do not collect repository store metrics on startup

Our current code path will trigger the RepositoryStoreCollector to query
the database on startup, even if the prometheus listener is not
listening. This is because we call DescribeByCollect in the Describe
method. The Prometheus client will call Describe on Register, which ends
up triggering the Collect method and hence runs the queries. Instead, we
can just provide the decriptions separately from the Collect method.

Changelog: fixed

2 files changed, 56 insertions, 16 deletions

diff --git a/internal/praefect/datastore/collector.go b/internal/praefect/datastore/collector.go
index be3b6f646..a99f6514d 100644
--- a/internal/praefect/datastore/collector.go
+++ b/internal/praefect/datastore/collector.go
@@ -11,21 +11,25 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/praefect/datastore/glsql"
 )
 
-// This is kept for backwards compatibility as some alerting rules depend on this.
-// The unavailable repositories is a more accurate description for the metric and
-// is exported below so we can migrate to it.
-var descReadOnlyRepositories = prometheus.NewDesc(
-	"gitaly_praefect_read_only_repositories",
-	"Number of repositories in read-only mode within a virtual storage.",
-	[]string{"virtual_storage"},
-	nil,
-)
+var (
+	// This is kept for backwards compatibility as some alerting rules depend on this.
+	// The unavailable repositories is a more accurate description for the metric and
+	// is exported below so we can migrate to it.
+	descReadOnlyRepositories = prometheus.NewDesc(
+		"gitaly_praefect_read_only_repositories",
+		"Number of repositories in read-only mode within a virtual storage.",
+		[]string{"virtual_storage"},
+		nil,
+	)
+
+	descUnavailableRepositories = prometheus.NewDesc(
+		"gitaly_praefect_unavailable_repositories",
+		"Number of repositories that have no healthy, up to date replicas.",
+		[]string{"virtual_storage"},
+		nil,
+	)
 
-var descUnavailableRepositories = prometheus.NewDesc(
-	"gitaly_praefect_unavailable_repositories",
-	"Number of repositories that have no healthy, up to date replicas.",
-	[]string{"virtual_storage"},
-	nil,
+	descriptions = []*prometheus.Desc{descReadOnlyRepositories, descUnavailableRepositories}
 )
 
 // RepositoryStoreCollector collects metrics from the RepositoryStore.
@@ -48,7 +52,9 @@ func NewRepositoryStoreCollector(log logrus.FieldLogger, virtualStorages []strin
 
 //nolint: revive,stylecheck // This is unintentionally missing documentation.
 func (c *RepositoryStoreCollector) Describe(ch chan<- *prometheus.Desc) {
-	prometheus.DescribeByCollect(c, ch)
+	for _, desc := range descriptions {
+		ch <- desc
+	}
 }
 
 //nolint: revive,stylecheck // This is unintentionally missing documentation.
@@ -63,7 +69,7 @@ func (c *RepositoryStoreCollector) Collect(ch chan<- prometheus.Metric) {
 	}
 
 	for _, vs := range c.virtualStorages {
-		for _, desc := range []*prometheus.Desc{descReadOnlyRepositories, descUnavailableRepositories} {
+		for _, desc := range descriptions {
 			ch <- prometheus.MustNewConstMetric(desc, prometheus.GaugeValue, float64(unavailableCounts[vs]), vs)
 		}
 	}
diff --git a/internal/praefect/datastore/collector_test.go b/internal/praefect/datastore/collector_test.go
index 3fd1cfe41..861fe3998 100644
--- a/internal/praefect/datastore/collector_test.go
+++ b/internal/praefect/datastore/collector_test.go
@@ -2,14 +2,18 @@ package datastore
 
 import (
 	"context"
+	"database/sql"
+	"errors"
 	"fmt"
 	"strings"
 	"testing"
 	"time"
 
+	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/testutil"
 	"github.com/sirupsen/logrus"
 	"github.com/sirupsen/logrus/hooks/test"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/praefect/datastore/glsql"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
@@ -220,3 +224,33 @@ gitaly_praefect_unavailable_repositories{virtual_storage="virtual-storage-2"} 0
 		})
 	}
 }
+
+type checkIfQueriedDB struct {
+	queried bool
+}
+
+func (c *checkIfQueriedDB) QueryContext(ctx context.Context, query string, args ...interface{}) (*sql.Rows, error) {
+	c.queried = true
+	return nil, errors.New("QueryContext should not be called")
+}
+
+func (c *checkIfQueriedDB) QueryRowContext(ctx context.Context, query string, args ...interface{}) *sql.Row {
+	c.queried = true
+	return &sql.Row{}
+}
+
+func (c *checkIfQueriedDB) ExecContext(ctx context.Context, query string, args ...interface{}) (sql.Result, error) {
+	c.queried = true
+	return nil, errors.New("ExecContext should not be called")
+}
+
+func TestRepositoryStoreCollector_CollectNotCalledOnRegister(t *testing.T) {
+	logger, _ := test.NewNullLogger()
+
+	var db checkIfQueriedDB
+	c := NewRepositoryStoreCollector(logger, []string{"virtual-storage-1", "virtual-storage-2"}, &db, 2*time.Second)
+	registry := prometheus.NewRegistry()
+	registry.MustRegister(c)
+
+	assert.False(t, db.queried)
+}