redisbp: Minor improvements on the prometheus exporter

1. Move the Desc from the exporter to global variable as they really don't change between different exporter instances. 2. Add a test to make sure that a service creating multiple redisbp clients won't cause prometheus exporter registration issues.
reddit · Mar 10, 2022 · ca159a8 · ca159a8
1 parent a6f0c8a
commit ca159a8
Show file tree

Hide file tree

Showing 3 changed files with 95 additions and 80 deletions.
diff --git a/redis/db/redisbp/monitored_client.go b/redis/db/redisbp/monitored_client.go
@@ -27,7 +27,10 @@ func NewMonitoredClient(name string, opt *redis.Options) *redis.Client {
 	client := redis.NewClient(opt)
 	client.AddHook(SpanHook{ClientName: name})
 
-	if err := prometheus.Register(newExporter(client, name)); err != nil {
+	if err := prometheus.Register(exporter{
+		client: client,
+		name:   name,
+	}); err != nil {
 		// prometheus.Register should never fail because
 		// exporter.Describe is a no-op, but just in case.
 		return nil
@@ -42,7 +45,10 @@ func NewMonitoredFailoverClient(name string, opt *redis.FailoverOptions) *redis.
 	client := redis.NewFailoverClient(opt)
 	client.AddHook(SpanHook{ClientName: name})
 
-	if err := prometheus.Register(newExporter(client, name)); err != nil {
+	if err := prometheus.Register(exporter{
+		client: client,
+		name:   name,
+	}); err != nil {
 		// prometheus.Register should never fail because
 		// exporter.Describe is a no-op, but just in case.
 		return nil
@@ -93,7 +99,10 @@ func NewMonitoredClusterClient(name string, opt *redis.ClusterOptions) *ClusterC
 	client := redis.NewClusterClient(opt)
 	client.AddHook(SpanHook{ClientName: name})
 
-	if err := prometheus.Register(newExporter(client, name)); err != nil {
+	if err := prometheus.Register(exporter{
+		client: client,
+		name:   name,
+	}); err != nil {
 		// prometheus.Register should never fail because
 		// exporter.Describe is a no-op, but just in case.
 		return nil

diff --git a/redis/db/redisbp/prometheus.go b/redis/db/redisbp/prometheus.go
@@ -11,116 +11,102 @@ const (
 	nameLabel = "pool"
 )
 
-// exporter provides an interface for Prometheus metrics.
-type exporter struct {
-	client PoolStatser
-	name   string
-
-	poolHitsCounterDesc     *prometheus.Desc
-	poolMissesCounterDesc   *prometheus.Desc
-	poolTimeoutsCounterDesc *prometheus.Desc
-	totalConnectionsDesc    *prometheus.Desc
-	idleConnectionsDesc     *prometheus.Desc
-	staleConnectionsDesc    *prometheus.Desc
-}
-
-func newExporter(client PoolStatser, name string) *exporter {
-	labels := []string{
+var (
+	promLabels = []string{
 		nameLabel,
 	}
 
-	return &exporter{
-		client: client,
-		name:   name,
-
-		// Upstream docs: https://pkg.go.dev/github.com/go-redis/redis/v8/internal/pool#Stats
+	// Counters.
+	poolHitsCounterDesc = prometheus.NewDesc(
+		prometheus.BuildFQName(promNamespace, subsystemPool, "hits_total"),
+		"Number of times free connection was found in the pool",
+		promLabels,
+		nil,
+	)
+	poolMissesCounterDesc = prometheus.NewDesc(
+		prometheus.BuildFQName(promNamespace, subsystemPool, "misses_total"),
+		"Number of times free connection was NOT found in the pool",
+		promLabels,
+		nil,
+	)
+	poolTimeoutsCounterDesc = prometheus.NewDesc(
+		prometheus.BuildFQName(promNamespace, subsystemPool, "timeouts_total"),
+		"Number of times a wait timeout occurred",
+		promLabels,
+		nil,
+	)
 
-		// Counters.
-		poolHitsCounterDesc: prometheus.NewDesc(
-			prometheus.BuildFQName(promNamespace, subsystemPool, "hits_total"),
-			"Number of times free connection was found in the pool",
-			labels,
-			nil,
-		),
-		poolMissesCounterDesc: prometheus.NewDesc(
-			prometheus.BuildFQName(promNamespace, subsystemPool, "misses_total"),
-			"Number of times free connection was NOT found in the pool",
-			labels,
-			nil,
-		),
-		poolTimeoutsCounterDesc: prometheus.NewDesc(
-			prometheus.BuildFQName(promNamespace, subsystemPool, "timeouts_total"),
-			"Number of times a wait timeout occurred",
-			labels,
-			nil,
-		),
+	// Gauges.
+	totalConnectionsDesc = prometheus.NewDesc(
+		prometheus.BuildFQName(promNamespace, subsystemPool, "connections"),
+		"Number of connections in this redisbp pool",
+		promLabels,
+		nil,
+	)
+	idleConnectionsDesc = prometheus.NewDesc(
+		prometheus.BuildFQName(promNamespace, subsystemPool, "idle_connections"),
+		"Number of idle connections in this redisbp pool",
+		promLabels,
+		nil,
+	)
+	staleConnectionsDesc = prometheus.NewDesc(
+		prometheus.BuildFQName(promNamespace, subsystemPool, "stale_connections"),
+		"Number of stale connections in this redisbp pool",
+		promLabels,
+		nil,
+	)
+)
 
-		// Gauges.
-		totalConnectionsDesc: prometheus.NewDesc(
-			prometheus.BuildFQName(promNamespace, subsystemPool, "connections"),
-			"Number of connections in this redisbp pool",
-			labels,
-			nil,
-		),
-		idleConnectionsDesc: prometheus.NewDesc(
-			prometheus.BuildFQName(promNamespace, subsystemPool, "idle_connections"),
-			"Number of idle connections in this redisbp pool",
-			labels,
-			nil,
-		),
-		staleConnectionsDesc: prometheus.NewDesc(
-			prometheus.BuildFQName(promNamespace, subsystemPool, "stale_connections"),
-			"Number of stale connections in this redisbp pool",
-			labels,
-			nil,
-		),
-	}
+// exporter provides an interface for Prometheus metrics.
+type exporter struct {
+	client PoolStatser
+	name   string
 }
 
 // Describe implements the prometheus.Collector interface.
-func (e *exporter) Describe(ch chan<- *prometheus.Desc) {
+func (e exporter) Describe(ch chan<- *prometheus.Desc) {
 	// All metrics are described dynamically.
 }
 
 // Collect implements prometheus.Collector.
-func (e *exporter) Collect(ch chan<- prometheus.Metric) {
+func (e exporter) Collect(ch chan<- prometheus.Metric) {
 	stats := e.client.PoolStats()
 
 	// Counters.
 	ch <- prometheus.MustNewConstMetric(
-		e.poolHitsCounterDesc,
+		poolHitsCounterDesc,
 		prometheus.CounterValue,
 		float64(stats.Hits),
 		e.name,
 	)
 	ch <- prometheus.MustNewConstMetric(
-		e.poolMissesCounterDesc,
+		poolMissesCounterDesc,
 		prometheus.CounterValue,
 		float64(stats.Misses),
 		e.name,
 	)
 	ch <- prometheus.MustNewConstMetric(
-		e.poolTimeoutsCounterDesc,
+		poolTimeoutsCounterDesc,
 		prometheus.CounterValue,
 		float64(stats.Timeouts),
 		e.name,
 	)
 
 	// Gauges.
 	ch <- prometheus.MustNewConstMetric(
-		e.totalConnectionsDesc,
+		totalConnectionsDesc,
 		prometheus.GaugeValue,
 		float64(stats.TotalConns),
 		e.name,
 	)
 	ch <- prometheus.MustNewConstMetric(
-		e.idleConnectionsDesc,
+		idleConnectionsDesc,
 		prometheus.GaugeValue,
 		float64(stats.IdleConns),
 		e.name,
 	)
 	ch <- prometheus.MustNewConstMetric(
-		e.staleConnectionsDesc,
+		staleConnectionsDesc,
 		prometheus.GaugeValue,
 		float64(stats.StaleConns),
 		e.name,

diff --git a/redis/db/redisbp/prometheus_test.go b/redis/db/redisbp/prometheus_test.go
@@ -23,24 +23,44 @@ func (c fakeClient) PoolStats() *redis.PoolStats {
 	}
 }
 
-func TestRedisPoolExporter(t *testing.T) {
-	client := &fakeClient{}
+func TestRedisPoolExporterRegister(t *testing.T) {
+	exporters := []exporter{
+		{
+			client: fakeClient{},
+			name:   "foo",
+		},
+		{
+			client: fakeClient{},
+			name:   "bar",
+		},
+	}
+	for i, exporter := range exporters {
+		if err := prometheus.Register(exporter); err != nil {
+			t.Errorf("Register #%d failed: %v", i, err)
+		}
+	}
+}
 
-	exporter := newExporter(
-		client,
-		"test",
-	)
-	// No real test here, we just want to make sure that Collect call will not
-	// panic, which would happen if we have a label mismatch.
+func TestRedisPoolExporterCollect(t *testing.T) {
+	exporter := exporter{
+		client: fakeClient{},
+		name:   "test",
+	}
 	ch := make(chan prometheus.Metric)
 	var wg sync.WaitGroup
 	wg.Add(1)
 	go func() {
 		defer wg.Done()
+		// Drain the channel
 		for range ch {
 		}
 	}()
+	t.Cleanup(func() {
+		close(ch)
+		wg.Wait()
+	})
+
+	// No real test here, we just want to make sure that Collect call will not
+	// panic, which would happen if we have a label mismatch.
 	exporter.Collect(ch)
-	close(ch)
-	wg.Wait()
 }