diff --git a/diagnostics/metrics/set.go b/diagnostics/metrics/set.go index a9dabe6de18..fbd9ac8d739 100644 --- a/diagnostics/metrics/set.go +++ b/diagnostics/metrics/set.go @@ -28,17 +28,14 @@ import ( "github.com/prometheus/client_golang/prometheus" ) -type namedMetric struct { +type namedEntry[M any] struct { name string - metric prometheus.Metric - isAux bool + metric M } -type namedMetricVec struct { - name string - metric *prometheus.GaugeVec - isAux bool -} +type namedMetric = namedEntry[prometheus.Metric] + +type namedMetricVec = namedEntry[prometheus.Collector] // Set is a set of metrics. // @@ -157,36 +154,9 @@ func newHistogram(name string, buckets []float64, help ...string) (prometheus.Hi // // Performance tip: prefer NewHistogram instead of GetOrCreateHistogram. func (s *Set) GetOrCreateHistogram(name string, help ...string) (prometheus.Histogram, error) { - s.mu.Lock() - nm := s.m[name] - s.mu.Unlock() - if nm == nil { - metric, err := newHistogram(name, nil, help...) - if err != nil { - return nil, fmt.Errorf("invalid metric name %q: %w", name, err) - } - - nmNew := &namedMetric{ - name: name, - metric: metric, - } - - s.mu.Lock() - nm = s.m[name] - if nm == nil { - nm = nmNew - s.m[name] = nm - s.a = append(s.a, nm) - } - s.mu.Unlock() - } - - h, ok := nm.metric.(prometheus.Histogram) - if !ok { - return nil, fmt.Errorf("metric %q isn't a Histogram. It is %T", name, nm.metric) - } - - return h, nil + return getOrCreate(s, name, func() (prometheus.Histogram, error) { + return newHistogram(name, nil, help...) + }) } // NewCounter registers and returns new counter with the given name in the s. @@ -236,36 +206,9 @@ func newCounter(name string, help ...string) (prometheus.Counter, error) { // // Performance tip: prefer NewCounter instead of GetOrCreateCounter. func (s *Set) GetOrCreateCounter(name string, help ...string) (prometheus.Counter, error) { - s.mu.Lock() - nm := s.m[name] - s.mu.Unlock() - if nm == nil { - // Slow path - create and register missing counter. - metric, err := newCounter(name, help...) - if err != nil { - return nil, fmt.Errorf("invalid metric name %q: %w", name, err) - } - - nmNew := &namedMetric{ - name: name, - metric: metric, - } - s.mu.Lock() - nm = s.m[name] - if nm == nil { - nm = nmNew - s.m[name] = nm - s.a = append(s.a, nm) - } - s.mu.Unlock() - } - - c, ok := nm.metric.(prometheus.Counter) - if !ok { - return nil, fmt.Errorf("metric %q isn't a Counter. It is %T", name, nm.metric) - } - - return c, nil + return getOrCreate(s, name, func() (prometheus.Counter, error) { + return newCounter(name, help...) + }) } // NewGauge registers and returns gauge with the given name. @@ -317,36 +260,9 @@ func newGauge(name string, help ...string) (prometheus.Gauge, error) { // // Performance tip: prefer NewGauge instead of GetOrCreateGauge. func (s *Set) GetOrCreateGauge(name string, help ...string) (prometheus.Gauge, error) { - s.mu.Lock() - nm := s.m[name] - s.mu.Unlock() - if nm == nil { - // Slow path - create and register missing gauge. - metric, err := newGauge(name, help...) - if err != nil { - return nil, fmt.Errorf("invalid metric name %q: %w", name, err) - } - - nmNew := &namedMetric{ - name: name, - metric: metric, - } - s.mu.Lock() - nm = s.m[name] - if nm == nil { - nm = nmNew - s.m[name] = nm - s.a = append(s.a, nm) - } - s.mu.Unlock() - } - - g, ok := nm.metric.(prometheus.Gauge) - if !ok { - return nil, fmt.Errorf("metric %q isn't a Gauge. It is %T", name, nm.metric) - } - - return g, nil + return getOrCreate(s, name, func() (prometheus.Gauge, error) { + return newGauge(name, help...) + }) } // GetOrCreateGaugeVec returns registered GaugeVec in s with the given name @@ -363,41 +279,9 @@ func (s *Set) GetOrCreateGauge(name string, help ...string) (prometheus.Gauge, e // // The returned GaugeVec is safe to use from concurrent goroutines. func (s *Set) GetOrCreateGaugeVec(name string, labels []string, help ...string) (*prometheus.GaugeVec, error) { - s.mu.Lock() - nm := s.vecs[name] - s.mu.Unlock() - if nm == nil { - metric, err := newGaugeVec(name, labels, help...) - if err != nil { - return nil, fmt.Errorf("invalid metric name %q: %w", name, err) - } - - nmNew := &namedMetricVec{ - name: name, - metric: metric, - } - - s.mu.Lock() - nm = s.vecs[name] - if nm == nil { - nm = nmNew - s.vecs[name] = nm - s.av = append(s.av, nm) - } - s.mu.Unlock() - s.registerMetricVec(name, metric, false) - } - - if nm.metric == nil { - return nil, fmt.Errorf("metric %q is nil", name) - } - - metricType := reflect.TypeFor[*prometheus.GaugeVec]() - if metricType != reflect.TypeFor[*prometheus.GaugeVec]() { - return nil, fmt.Errorf("metric %q isn't a GaugeVec. It is %s", name, metricType) - } - - return nm.metric, nil + return getOrCreateVec(s, name, func() (*prometheus.GaugeVec, error) { + return newGaugeVec(name, labels, help...) + }) } // newGaugeVec creates a new Prometheus GaugeVec. @@ -507,57 +391,58 @@ func (s *Set) NewSummaryExt(name string, window time.Duration, quantiles map[flo // // Performance tip: prefer NewSummaryExt instead of GetOrCreateSummaryExt. func (s *Set) GetOrCreateSummaryExt(name string, window time.Duration, quantiles map[float64]float64, help ...string) (prometheus.Summary, error) { - s.mu.Lock() - nm := s.m[name] - s.mu.Unlock() + return getOrCreate(s, name, func() (prometheus.Summary, error) { + return newSummary(name, window, quantiles, help...) + }) +} + +func getOrCreate[T prometheus.Metric](s *Set, name string, create func() (T, error)) (T, error) { + return getOrCreateIn[T](&s.mu, s.m, &s.a, name, func() (prometheus.Metric, error) { return create() }) +} + +func getOrCreateVec[T prometheus.Collector](s *Set, name string, create func() (T, error)) (T, error) { + return getOrCreateIn[T](&s.mu, s.vecs, &s.av, name, func() (prometheus.Collector, error) { return create() }) +} + +func getOrCreateIn[T, M any](mu *sync.Mutex, entries map[string]*namedEntry[M], list *[]*namedEntry[M], name string, create func() (M, error)) (T, error) { + var zero T + mu.Lock() + defer mu.Unlock() + nm := entries[name] if nm == nil { - // Slow path - create and register missing summary. - metric, err := newSummary(name, window, quantiles, help...) + metric, err := create() if err != nil { - return nil, fmt.Errorf("invalid metric name %q: %w", name, err) + return zero, fmt.Errorf("invalid metric name %q: %w", name, err) } - nmNew := &namedMetric{ + nm = &namedEntry[M]{ name: name, metric: metric, } - s.mu.Lock() - nm = s.m[name] - if nm == nil { - nm = nmNew - s.m[name] = nm - s.a = append(s.a, nm) - } - s.mu.Unlock() + entries[name] = nm + *list = append(*list, nm) } - sm, ok := nm.metric.(prometheus.Summary) + m, ok := any(nm.metric).(T) if !ok { - return nil, fmt.Errorf("metric %q isn't a Summary. It is %T", name, nm.metric) + return zero, fmt.Errorf("metric %q isn't a %s. It is %T", name, metricTypeName[T](), nm.metric) } - return sm, nil + return m, nil } -func (s *Set) registerMetric(name string, m prometheus.Metric) { - s.mu.Lock() - defer s.mu.Unlock() - s.mustRegisterLocked(name, m) +func metricTypeName[T any]() string { + t := reflect.TypeFor[T]() + if t.Kind() == reflect.Pointer { + t = t.Elem() + } + return t.Name() } -func (s *Set) registerMetricVec(name string, mv *prometheus.GaugeVec, isAux bool) { +func (s *Set) registerMetric(name string, m prometheus.Metric) { s.mu.Lock() defer s.mu.Unlock() - - if _, exists := s.vecs[name]; !exists { - nmv := &namedMetricVec{ - name: name, - metric: mv, - isAux: isAux, - } - s.vecs[name] = nmv - s.av = append(s.av, nmv) - } + s.mustRegisterLocked(name, m) } // mustRegisterLocked registers given metric with the given name. @@ -627,9 +512,6 @@ func (s *Set) ListMetricNames() []string { defer s.mu.Unlock() metricNames := make([]string, 0, len(s.m)) for _, nm := range s.m { - if nm.isAux { - continue - } metricNames = append(metricNames, nm.name) } slices.Sort(metricNames) diff --git a/diagnostics/metrics/set_test.go b/diagnostics/metrics/set_test.go new file mode 100644 index 00000000000..c02c4b941d5 --- /dev/null +++ b/diagnostics/metrics/set_test.go @@ -0,0 +1,141 @@ +// Copyright 2026 The Erigon Authors +// This file is part of Erigon. +// +// Erigon is free software: you can redistribute it and/or modify +// it under the terms of the GNU Lesser General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// Erigon is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Lesser General Public License for more details. +// +// You should have received a copy of the GNU Lesser General Public License +// along with Erigon. If not, see . + +package metrics + +import ( + "sync" + "testing" + + "github.com/prometheus/client_golang/prometheus" + "github.com/stretchr/testify/require" +) + +func TestSetGetOrCreateReturnsExisting(t *testing.T) { + s := NewSet() + + h, err := s.GetOrCreateHistogram("h") + require.NoError(t, err) + h2, err := s.GetOrCreateHistogram("h") + require.NoError(t, err) + require.Same(t, h, h2) + + c, err := s.GetOrCreateCounter("c") + require.NoError(t, err) + c2, err := s.GetOrCreateCounter("c") + require.NoError(t, err) + require.Same(t, c, c2) + + g, err := s.GetOrCreateGauge("g") + require.NoError(t, err) + g2, err := s.GetOrCreateGauge("g") + require.NoError(t, err) + require.Same(t, g, g2) + + sm, err := s.GetOrCreateSummary("sm") + require.NoError(t, err) + sm2, err := s.GetOrCreateSummary("sm") + require.NoError(t, err) + require.Same(t, sm, sm2) +} + +func TestSetGetOrCreateWrongType(t *testing.T) { + s := NewSet() + + _, err := s.GetOrCreateCounter("m") + require.NoError(t, err) + + _, err = s.GetOrCreateGauge("m") + require.ErrorContains(t, err, `metric "m" isn't a Gauge. It is`) + + _, err = s.GetOrCreateHistogram("m") + require.ErrorContains(t, err, `metric "m" isn't a Histogram. It is`) + + _, err = s.GetOrCreateSummary("m") + require.ErrorContains(t, err, `metric "m" isn't a Summary. It is`) +} + +func TestSetGetOrCreateInvalidName(t *testing.T) { + s := NewSet() + + _, err := s.GetOrCreateCounter("m{unclosed") + require.ErrorContains(t, err, `invalid metric name "m{unclosed"`) + + _, err = s.GetOrCreateGaugeVec("m{unclosed", []string{"l"}) + require.ErrorContains(t, err, `invalid metric name "m{unclosed"`) +} + +func TestSetGetOrCreateGaugeVecReturnsExisting(t *testing.T) { + s := NewSet() + + gv, err := s.GetOrCreateGaugeVec("gv", []string{"a"}) + require.NoError(t, err) + gv2, err := s.GetOrCreateGaugeVec("gv", []string{"b"}) + require.NoError(t, err) + require.Same(t, gv, gv2) +} + +func TestSetGetOrCreateGaugeVecWrongType(t *testing.T) { + s := NewSet() + + cv := prometheus.NewCounterVec(prometheus.CounterOpts{Name: "v"}, []string{"l"}) + s.mu.Lock() + nmv := &namedMetricVec{name: "v", metric: cv} + s.vecs["v"] = nmv + s.av = append(s.av, nmv) + s.mu.Unlock() + + _, err := s.GetOrCreateGaugeVec("v", []string{"l"}) + require.ErrorContains(t, err, `metric "v" isn't a GaugeVec. It is *prometheus.CounterVec`) +} + +func TestSetGaugeVecDescribedOnce(t *testing.T) { + s := NewSet() + + _, err := s.GetOrCreateGaugeVec("gv", []string{"a"}) + require.NoError(t, err) + + ch := make(chan *prometheus.Desc, 16) + s.Describe(ch) + close(ch) + count := 0 + for range ch { + count++ + } + require.Equal(t, 1, count) +} + +func TestSetGetOrCreateConcurrent(t *testing.T) { + s := NewSet() + + const workers = 16 + counters := make([]prometheus.Counter, workers) + errs := make([]error, workers) + var wg sync.WaitGroup + for i := range workers { + wg.Add(1) + go func() { + defer wg.Done() + counters[i], errs[i] = s.GetOrCreateCounter("c") + }() + } + wg.Wait() + + for i := range workers { + require.NoError(t, errs[i]) + require.Same(t, counters[0], counters[i]) + } +}