diff --git a/pkg/metrics/metrics.go b/pkg/metrics/metrics.go index 952c4ac..56be83d 100644 --- a/pkg/metrics/metrics.go +++ b/pkg/metrics/metrics.go @@ -133,17 +133,26 @@ func Register() { ) } -func RegisterCollector(collector prometheus.Collector) { +// RegisterCollector registers a collector and returns the collector to record +// observations on. If an equal collector is already registered, that one is returned +// instead; observations on the given collector would otherwise never be scraped. +// Registration errors are logged, as they would leave the collector silently absent +// from the metrics endpoint. +func RegisterCollector[T prometheus.Collector](collector T) T { err := prometheus.DefaultRegisterer.Register(collector) if err == nil { - return + return collector } - if _, ok := errors.AsType[prometheus.AlreadyRegisteredError](err); ok { - return + if alreadyRegistered, ok := errors.AsType[prometheus.AlreadyRegisteredError](err); ok { + if existing, ok := alreadyRegistered.ExistingCollector.(T); ok { + return existing + } + return collector } log.Warnf("metrics: registering collector: %+v", err) + return collector } func ObserveRedisLatency(operation string, fun func() error) error { diff --git a/pkg/metrics/metrics_test.go b/pkg/metrics/metrics_test.go new file mode 100644 index 0000000..ea850ff --- /dev/null +++ b/pkg/metrics/metrics_test.go @@ -0,0 +1,28 @@ +package metrics_test + +import ( + "testing" + + "github.com/prometheus/client_golang/prometheus" + "github.com/stretchr/testify/assert" + + "github.com/nais/wonderwall/pkg/metrics" +) + +func TestRegisterCollector(t *testing.T) { + newCounter := func() *prometheus.CounterVec { + return prometheus.NewCounterVec(prometheus.CounterOpts{ + Name: "wonderwall_test_register_collector_total", + Help: "counter used to assert registration behaviour", + }, []string{"label"}) + } + + first := newCounter() + assert.Same(t, first, metrics.RegisterCollector(first), "the first registration is used as-is") + + // an equal collector is already registered, so observations on the second one would + // never be scraped; the registered one must be returned instead + second := newCounter() + assert.Same(t, first, metrics.RegisterCollector(second)) + assert.NotSame(t, second, metrics.RegisterCollector(second)) +} diff --git a/pkg/middleware/prometheus.go b/pkg/middleware/prometheus.go index 1ba2c4f..a812e14 100644 --- a/pkg/middleware/prometheus.go +++ b/pkg/middleware/prometheus.go @@ -56,8 +56,8 @@ func Prometheus(provider string, buckets ...float64) *PrometheusMiddleware { []string{"code", "method", "path", "host"}, ) - metrics.RegisterCollector(m.reqs) - metrics.RegisterCollector(m.latency) + m.reqs = metrics.RegisterCollector(m.reqs) + m.latency = metrics.RegisterCollector(m.latency) return &m }