Make metric registration idempotent

Signed-off-by: João Vilaça <jvilaca@redhat.com>

pkg/operatormetrics/collector.go

@@ -2,6 +2,7 @@ package operatormetrics
 
 import (
 	"fmt"
+	"strings"
 
 	"github.com/prometheus/client_golang/prometheus"
 )
@@ -24,6 +25,16 @@ type CollectorResult struct {
 	Value  float64
 }
 
+func (c Collector) hash() string {
+	var sb strings.Builder
+
+	for _, cm := range c.Metrics {
+		sb.WriteString(cm.GetOpts().Name)
+	}
+
+	return sb.String()
+}
+
 func (c Collector) Describe(ch chan<- *prometheus.Desc) {
 	for _, cm := range c.Metrics {
 		cm.getCollector().Describe(ch)

pkg/operatormetrics/collector_test.go

@@ -25,7 +25,8 @@ var _ = Describe("Collector", func() {
 
 	Describe("Collect", func() {
 		BeforeEach(func() {
-			CleanRegistry()
+			err := CleanRegistry()
+			Expect(err).NotTo(HaveOccurred())
 		})
 
 		It("should collect metrics from registered collectors", func() {

pkg/operatormetrics/wrapper_registry.go

@@ -1,15 +1,20 @@
 package operatormetrics
 
+import "fmt"
+
 var operatorRegistry = newRegistry()
 
 type operatorRegisterer struct {
-	registeredMetrics          map[string]Metric
+	registeredMetrics map[string]Metric
+
+	registeredCollectors       map[string]Collector
 	registeredCollectorMetrics map[string]Metric
 }
 
 func newRegistry() operatorRegisterer {
 	return operatorRegisterer{
 		registeredMetrics:          map[string]Metric{},
+		registeredCollectors:       map[string]Collector{},
 		registeredCollectorMetrics: map[string]Metric{},
 	}
 }
@@ -18,11 +23,17 @@ func newRegistry() operatorRegisterer {
 func RegisterMetrics(allMetrics ...[]Metric) error {
 	for _, metricList := range allMetrics {
 		for _, metric := range metricList {
-			err := Register(metric.getCollector())
+			if metricExists(metric) {
+				err := unregisterMetric(metric)
+				if err != nil {
+					return err
+				}
+			}
+
+			err := registerMetric(metric)
 			if err != nil {
 				return err
 			}
-			operatorRegistry.registeredMetrics[metric.GetOpts().Name] = metric
 		}
 	}
 
@@ -32,13 +43,16 @@ func RegisterMetrics(allMetrics ...[]Metric) error {
 // RegisterCollector registers the collector with the Prometheus registry.
 func RegisterCollector(collectors ...Collector) error {
 	for _, collector := range collectors {
-		err := Register(collector)
-		if err != nil {
-			return err
+		if collectorExists(collector) {
+			err := unregisterCollector(collector)
+			if err != nil {
+				return err
+			}
 		}
 
-		for _, cm := range collector.Metrics {
-			operatorRegistry.registeredCollectorMetrics[cm.GetOpts().Name] = cm
+		err := registerCollector(collector)
+		if err != nil {
+			return err
 		}
 	}
 
@@ -61,16 +75,84 @@ func ListMetrics() []Metric {
 }
 
 // CleanRegistry removes all registered metrics.
-func CleanRegistry() {
+func CleanRegistry() error {
 	for _, metric := range operatorRegistry.registeredMetrics {
-		if succeeded := Unregister(metric.getCollector()); succeeded {
-			delete(operatorRegistry.registeredMetrics, metric.GetOpts().Name)
+		err := unregisterMetric(metric)
+		if err != nil {
+			return err
+		}
+	}
+
+	for _, collector := range operatorRegistry.registeredCollectors {
+		err := unregisterCollector(collector)
+		if err != nil {
+			return err
 		}
 	}
 
-	for _, metric := range operatorRegistry.registeredCollectorMetrics {
-		if succeeded := Unregister(metric.getCollector()); succeeded {
+	return nil
+}
+
+func metricExists(metric Metric) bool {
+	_, ok := operatorRegistry.registeredMetrics[metric.GetOpts().Name]
+	return ok
+}
+
+func unregisterMetric(metric Metric) error {
+	if succeeded := Unregister(metric.getCollector()); succeeded {
+		delete(operatorRegistry.registeredMetrics, metric.GetOpts().Name)
+		return nil
+	}
+
+	return fmt.Errorf("failed to unregister from Prometheus client metric %s", metric.GetOpts().Name)
+}
+
+func registerMetric(metric Metric) error {
+	err := Register(metric.getCollector())
+	if err != nil {
+		return err
+	}
+	operatorRegistry.registeredMetrics[metric.GetOpts().Name] = metric
+
+	return nil
+}
+
+func collectorExists(collector Collector) bool {
+	_, ok := operatorRegistry.registeredCollectors[collector.hash()]
+	return ok
+}
+
+func unregisterCollector(collector Collector) error {
+	if succeeded := Unregister(collector); succeeded {
+		delete(operatorRegistry.registeredCollectors, collector.hash())
+		for _, metric := range collector.Metrics {
 			delete(operatorRegistry.registeredCollectorMetrics, metric.GetOpts().Name)
 		}
+		return nil
+	}
+
+	return fmt.Errorf("failed to unregister from Prometheus client collector with metrics: %s", buildCollectorMetricListString(collector))
+}
+
+func registerCollector(collector Collector) error {
+	err := Register(collector)
+	if err != nil {
+		return err
+	}
+
+	operatorRegistry.registeredCollectors[collector.hash()] = collector
+	for _, cm := range collector.Metrics {
+		operatorRegistry.registeredCollectorMetrics[cm.GetOpts().Name] = cm
+	}
+
+	return nil
+}
+
+func buildCollectorMetricListString(collector Collector) string {
+	metricsList := ""
+	for _, metric := range collector.Metrics {
+		metricsList += metric.GetOpts().Name + ", "
 	}
+	metricsList = metricsList[:len(metricsList)-2]
+	return metricsList
 }

pkg/operatormetrics/wrapper_registry_test.go

@@ -19,7 +19,8 @@ var _ = Describe("Registry", func() {
 
 	Describe("RegisterMetrics", func() {
 		BeforeEach(func() {
-			CleanRegistry()
+			err := CleanRegistry()
+			Expect(err).NotTo(HaveOccurred())
 		})
 
 		It("should register metrics without error", func() {
@@ -34,22 +35,24 @@ var _ = Describe("Registry", func() {
 			Expect(operatorRegistry.registeredMetrics).To(HaveKey(testGaugeOpts.Name))
 		})
 
-		It("should return an error when registering a duplicate metric", func() {
+		It("should replace metrics with the same name in different RegisterMetrics call", func() {
 			counter := NewCounter(testCounterOpts)
 
 			err := RegisterMetrics([]Metric{counter})
 			Expect(err).NotTo(HaveOccurred())
 
 			err = RegisterMetrics([]Metric{counter})
-			Expect(err).To(HaveOccurred())
+			Expect(err).NotTo(HaveOccurred())
 
 			Expect(operatorRegistry.registeredMetrics).To(HaveLen(1))
+			Expect(operatorRegistry.registeredMetrics).To(HaveKey(testCounterOpts.Name))
 		})
 	})
 
 	Describe("ListMetrics", func() {
 		BeforeEach(func() {
-			CleanRegistry()
+			err := CleanRegistry()
+			Expect(err).NotTo(HaveOccurred())
 		})
 
 		It("should return a list of all registered metrics", func() {
@@ -65,4 +68,88 @@ var _ = Describe("Registry", func() {
 			Expect(metrics).To(ContainElement(gauge))
 		})
 	})
+
+	Describe("RegisterCollector", func() {
+		BeforeEach(func() {
+			err := CleanRegistry()
+			Expect(err).NotTo(HaveOccurred())
+		})
+
+		var customResourceCollectorCallback = func() []CollectorResult {
+			return []CollectorResult{}
+		}
+
+		It("should register collectors without error", func() {
+			collector := Collector{
+				Metrics: []Metric{
+					NewCounter(testCounterOpts),
+				},
+				CollectCallback: customResourceCollectorCallback,
+			}
+
+			err := RegisterCollector(collector)
+			Expect(err).NotTo(HaveOccurred())
+
+			Expect(operatorRegistry.registeredCollectorMetrics).To(HaveLen(1))
+			Expect(operatorRegistry.registeredCollectorMetrics).To(HaveKey(testCounterOpts.Name))
+		})
+
+		It("should replace metrics with the same name in different RegisterCollector call", func() {
+			collector := Collector{
+				Metrics: []Metric{
+					NewCounter(testCounterOpts),
+				},
+				CollectCallback: customResourceCollectorCallback,
+			}
+
+			err := RegisterCollector(collector)
+			Expect(err).NotTo(HaveOccurred())
+
+			err = RegisterCollector(collector)
+			Expect(err).NotTo(HaveOccurred())
+
+			Expect(operatorRegistry.registeredCollectorMetrics).To(HaveLen(1))
+			Expect(operatorRegistry.registeredCollectorMetrics).To(HaveKey(testCounterOpts.Name))
+		})
+	})
+
+	Describe("CleanRegistry", func() {
+		BeforeEach(func() {
+			err := CleanRegistry()
+			Expect(err).NotTo(HaveOccurred())
+		})
+
+		It("should remove all metrics from the registry", func() {
+			counter := NewCounter(testCounterOpts)
+			gauge := NewGauge(testGaugeOpts)
+
+			err := RegisterMetrics([]Metric{counter, gauge})
+			Expect(err).NotTo(HaveOccurred())
+
+			err = CleanRegistry()
+			Expect(err).NotTo(HaveOccurred())
+
+			Expect(operatorRegistry.registeredMetrics).To(HaveLen(0))
+		})
+
+		It("should remove all collectors from the registry", func() {
+			collector := Collector{
+				Metrics: []Metric{
+					NewCounter(testCounterOpts),
+				},
+				CollectCallback: func() []CollectorResult {
+					return []CollectorResult{}
+				},
+			}
+
+			err := RegisterCollector(collector)
+			Expect(err).NotTo(HaveOccurred())
+
+			err = CleanRegistry()
+			Expect(err).NotTo(HaveOccurred())
+
+			Expect(operatorRegistry.registeredCollectors).To(HaveLen(0))
+			Expect(operatorRegistry.registeredCollectorMetrics).To(HaveLen(0))
+		})
+	})
 })

pkg/operatorrules/registry.go

@@ -59,6 +59,7 @@ func ListAlerts() []promv1.Rule {
 }
 
 // CleanRegistry removes all registered rules and alerts.
-func CleanRegistry() {
+func CleanRegistry() error {
 	operatorRegistry = newRegistry()
+	return nil
 }