From 921306cc6e9801e913f5c200941fe36394e4ade0 Mon Sep 17 00:00:00 2001 From: yperbasis Date: Thu, 2 Jul 2026 21:17:35 +0200 Subject: [PATCH 1/3] diagnostics/metrics: characterization tests for Set.GetOrCreate* behavior Pin the get-or-create semantics of the Set registry before refactoring: same-instance return on repeated calls, wrong-type errors for the scalar families, invalid-name errors, GaugeVec identity across differing label sets, single Describe emission per vec, and double-checked-locking behavior under concurrency. All pass against the existing code. --- diagnostics/metrics/set_test.go | 127 ++++++++++++++++++++++++++++++++ 1 file changed, 127 insertions(+) create mode 100644 diagnostics/metrics/set_test.go diff --git a/diagnostics/metrics/set_test.go b/diagnostics/metrics/set_test.go new file mode 100644 index 00000000000..b7e200ae7c0 --- /dev/null +++ b/diagnostics/metrics/set_test.go @@ -0,0 +1,127 @@ +// 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 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]) + } +} From d7eb04f97b748d9ffdddc50a3f22369388778292 Mon Sep 17 00:00:00 2001 From: yperbasis Date: Thu, 2 Jul 2026 21:32:47 +0200 Subject: [PATCH 2/3] diagnostics/metrics: collapse GetOrCreate* onto generic helpers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The five GetOrCreate{Histogram,Counter,Gauge,SummaryExt,GaugeVec} methods repeated the same double-checked-locking block. Replace them with getOrCreate[T]/getOrCreateVec[T]. The GaugeVec copy carried a type check comparing a reflect.Type to itself — dead, and unreachable while namedMetricVec.metric was concretely typed; the generic widens the field to prometheus.Collector and asserts for real (now tested). registerMetricVec's double-insert guard is subsumed by the inline double-checked insert. --- diagnostics/metrics/set.go | 212 ++++++++++---------------------- diagnostics/metrics/set_test.go | 14 +++ 2 files changed, 79 insertions(+), 147 deletions(-) diff --git a/diagnostics/metrics/set.go b/diagnostics/metrics/set.go index a9dabe6de18..fd8682d1b04 100644 --- a/diagnostics/metrics/set.go +++ b/diagnostics/metrics/set.go @@ -36,8 +36,7 @@ type namedMetric struct { type namedMetricVec struct { name string - metric *prometheus.GaugeVec - isAux bool + metric prometheus.Collector } // Set is a set of metrics. @@ -157,36 +156,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 +208,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 +262,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 +281,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,14 +393,20 @@ 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) { + 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) { + var zero T s.mu.Lock() nm := s.m[name] s.mu.Unlock() 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{ @@ -531,33 +423,59 @@ func (s *Set) GetOrCreateSummaryExt(name string, window time.Duration, quantiles s.mu.Unlock() } - 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) { +func getOrCreateVec[T prometheus.Collector](s *Set, name string, create func() (T, error)) (T, error) { + var zero T s.mu.Lock() - defer s.mu.Unlock() - s.mustRegisterLocked(name, m) -} - -func (s *Set) registerMetricVec(name string, mv *prometheus.GaugeVec, isAux bool) { - s.mu.Lock() - defer s.mu.Unlock() + nm := s.vecs[name] + s.mu.Unlock() + if nm == nil { + metric, err := create() + if err != nil { + return zero, fmt.Errorf("invalid metric name %q: %w", name, err) + } - if _, exists := s.vecs[name]; !exists { - nmv := &namedMetricVec{ + nmNew := &namedMetricVec{ name: name, - metric: mv, - isAux: isAux, + metric: metric, } - s.vecs[name] = nmv - s.av = append(s.av, nmv) + 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() } + + m, ok := any(nm.metric).(T) + if !ok { + return zero, fmt.Errorf("metric %q isn't a %s. It is %T", name, metricTypeName[T](), nm.metric) + } + + return m, nil +} + +func metricTypeName[T any]() string { + t := reflect.TypeFor[T]() + if t.Kind() == reflect.Pointer { + t = t.Elem() + } + return t.Name() +} + +func (s *Set) registerMetric(name string, m prometheus.Metric) { + s.mu.Lock() + defer s.mu.Unlock() + s.mustRegisterLocked(name, m) } // mustRegisterLocked registers given metric with the given name. diff --git a/diagnostics/metrics/set_test.go b/diagnostics/metrics/set_test.go index b7e200ae7c0..c02c4b941d5 100644 --- a/diagnostics/metrics/set_test.go +++ b/diagnostics/metrics/set_test.go @@ -88,6 +88,20 @@ func TestSetGetOrCreateGaugeVecReturnsExisting(t *testing.T) { 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() From fdfa90f4d761b9c081da656b51e15dd72e857549 Mon Sep 17 00:00:00 2001 From: yperbasis Date: Thu, 2 Jul 2026 22:32:37 +0200 Subject: [PATCH 3/3] diagnostics/metrics: unify get-or-create helpers under a single lock getOrCreate and getOrCreateVec were line-for-line clones differing only in which registry (m/a vs vecs/av) they touched. Introduce namedEntry[M] (namedMetric and namedMetricVec become aliases) and collapse both onto one getOrCreateIn over an explicitly passed map+slice pair; the wrappers widen T to the entry type via an adapter closure, keeping the store compile-time safe. Replace the double-checked locking with a single critical section: the fast path took the same mutex anyway, and the create funcs are pure parseMetric + constructor calls, so creating under the lock eliminates the create-twice-drop-loser race. The unlock is deferred because prometheus constructors can panic on reachable inputs (a quantile/le const label, a negative summary window); a non-deferred unlock would strand the set's mutex. Also drop namedMetric.isAux: nothing ever set it (its vec-side twin was removed in the previous commit), so the ListMetricNames filter was unreachable. --- diagnostics/metrics/set.go | 68 +++++++++----------------------------- 1 file changed, 16 insertions(+), 52 deletions(-) diff --git a/diagnostics/metrics/set.go b/diagnostics/metrics/set.go index fd8682d1b04..fbd9ac8d739 100644 --- a/diagnostics/metrics/set.go +++ b/diagnostics/metrics/set.go @@ -28,16 +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.Collector -} +type namedMetric = namedEntry[prometheus.Metric] + +type namedMetricVec = namedEntry[prometheus.Collector] // Set is a set of metrics. // @@ -399,61 +397,30 @@ func (s *Set) GetOrCreateSummaryExt(name string, window time.Duration, quantiles } func getOrCreate[T prometheus.Metric](s *Set, name string, create func() (T, error)) (T, error) { - var zero T - s.mu.Lock() - nm := s.m[name] - s.mu.Unlock() - if nm == nil { - metric, err := create() - if err != nil { - return zero, 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() - } - - m, ok := any(nm.metric).(T) - if !ok { - return zero, fmt.Errorf("metric %q isn't a %s. It is %T", name, metricTypeName[T](), nm.metric) - } - - return m, nil + 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 - s.mu.Lock() - nm := s.vecs[name] - s.mu.Unlock() + mu.Lock() + defer mu.Unlock() + nm := entries[name] if nm == nil { metric, err := create() if err != nil { return zero, fmt.Errorf("invalid metric name %q: %w", name, err) } - nmNew := &namedMetricVec{ + nm = &namedEntry[M]{ 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() + entries[name] = nm + *list = append(*list, nm) } m, ok := any(nm.metric).(T) @@ -545,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)