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
(cherry picked from commit 90cb7fb7b9f8703547fa62719650394478653c62)

2 files changed, 57 insertions, 17 deletions

diff --git a/internal/praefect/datastore/collector.go b/internal/praefect/datastore/collector.go
index 71f145e67..40390b47a 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 descUnavailableRepositories = prometheus.NewDesc(
-	"gitaly_praefect_unavailable_repositories",
-	"Number of repositories that have no healthy, up to date replicas.",
-	[]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,
+	)
+
+	descriptions = []*prometheus.Desc{descReadOnlyRepositories, descUnavailableRepositories}
 )
 
 // RepositoryStoreCollector collects metrics from the RepositoryStore.
@@ -47,7 +51,9 @@ func NewRepositoryStoreCollector(log logrus.FieldLogger, virtualStorages []strin
 }
 
 func (c *RepositoryStoreCollector) Describe(ch chan<- *prometheus.Desc) {
-	prometheus.DescribeByCollect(c, ch)
+	for _, desc := range descriptions {
+		ch <- desc
+	}
 }
 
 func (c *RepositoryStoreCollector) Collect(ch chan<- prometheus.Metric) {
@@ -61,7 +67,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 3514b1239..3cc972e36 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"
@@ -211,3 +215,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)
+}