Skip to content

metrics: hardcode lazy metric construction codepath #31962

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions metrics/counter.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,7 @@ import (
// GetOrRegisterCounter returns an existing Counter or constructs and registers
// a new Counter.
func GetOrRegisterCounter(name string, r Registry) *Counter {
if r == nil {
r = DefaultRegistry
}
return r.GetOrRegister(name, NewCounter).(*Counter)
return GetOrRegister(name, NewCounter, r).(*Counter)
}

// NewCounter constructs a new Counter.
Expand Down
5 changes: 1 addition & 4 deletions metrics/counter_float64.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,7 @@ import (
// GetOrRegisterCounterFloat64 returns an existing *CounterFloat64 or constructs and registers
// a new CounterFloat64.
func GetOrRegisterCounterFloat64(name string, r Registry) *CounterFloat64 {
if nil == r {
r = DefaultRegistry
}
return r.GetOrRegister(name, NewCounterFloat64).(*CounterFloat64)
return GetOrRegister(name, NewCounterFloat64, r).(*CounterFloat64)
}

// NewCounterFloat64 constructs a new CounterFloat64.
Expand Down
5 changes: 1 addition & 4 deletions metrics/gauge.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,7 @@ func (g GaugeSnapshot) Value() int64 { return int64(g) }
// GetOrRegisterGauge returns an existing Gauge or constructs and registers a
// new Gauge.
func GetOrRegisterGauge(name string, r Registry) *Gauge {
if r == nil {
r = DefaultRegistry
}
return r.GetOrRegister(name, NewGauge).(*Gauge)
return GetOrRegister(name, NewGauge, r).(*Gauge)
}

// NewGauge constructs a new Gauge.
Expand Down
5 changes: 1 addition & 4 deletions metrics/gauge_float64.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,7 @@ import (
// GetOrRegisterGaugeFloat64 returns an existing GaugeFloat64 or constructs and registers a
// new GaugeFloat64.
func GetOrRegisterGaugeFloat64(name string, r Registry) *GaugeFloat64 {
if nil == r {
r = DefaultRegistry
}
return r.GetOrRegister(name, NewGaugeFloat64()).(*GaugeFloat64)
return GetOrRegister(name, NewGaugeFloat64, r).(*GaugeFloat64)
}

// GaugeFloat64Snapshot is a read-only copy of a GaugeFloat64.
Expand Down
5 changes: 1 addition & 4 deletions metrics/gauge_info.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,7 @@ func (val GaugeInfoValue) String() string {
// GetOrRegisterGaugeInfo returns an existing GaugeInfo or constructs and registers a
// new GaugeInfo.
func GetOrRegisterGaugeInfo(name string, r Registry) *GaugeInfo {
if nil == r {
r = DefaultRegistry
}
return r.GetOrRegister(name, NewGaugeInfo()).(*GaugeInfo)
return GetOrRegister(name, NewGaugeInfo, r).(*GaugeInfo)
}

// NewGaugeInfo constructs a new GaugeInfo.
Expand Down
10 changes: 2 additions & 8 deletions metrics/histogram.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,19 +23,13 @@ type Histogram interface {
// GetOrRegisterHistogram returns an existing Histogram or constructs and
// registers a new StandardHistogram.
func GetOrRegisterHistogram(name string, r Registry, s Sample) Histogram {
if nil == r {
r = DefaultRegistry
}
return r.GetOrRegister(name, func() Histogram { return NewHistogram(s) }).(Histogram)
return GetOrRegister(name, func() Histogram { return NewHistogram(s) }, r).(Histogram)
}

// GetOrRegisterHistogramLazy returns an existing Histogram or constructs and
// registers a new StandardHistogram.
func GetOrRegisterHistogramLazy(name string, r Registry, s func() Sample) Histogram {
if nil == r {
r = DefaultRegistry
}
return r.GetOrRegister(name, func() Histogram { return NewHistogram(s()) }).(Histogram)
return GetOrRegister(name, func() Histogram { return NewHistogram(s()) }, r).(Histogram)
}

// NewHistogram constructs a new StandardHistogram from a Sample.
Expand Down
5 changes: 1 addition & 4 deletions metrics/meter.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,7 @@ import (
// Be sure to unregister the meter from the registry once it is of no use to
// allow for garbage collection.
func GetOrRegisterMeter(name string, r Registry) *Meter {
if r == nil {
r = DefaultRegistry
}
return r.GetOrRegister(name, NewMeter).(*Meter)
return GetOrRegister(name, NewMeter, r).(*Meter)
}

// NewMeter constructs a new Meter and launches a goroutine.
Expand Down
32 changes: 12 additions & 20 deletions metrics/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package metrics
import (
"errors"
"fmt"
"reflect"
"sort"
"strings"
"sync"
Expand All @@ -30,10 +29,9 @@ type Registry interface {
// GetAll metrics in the Registry.
GetAll() map[string]map[string]interface{}

// GetOrRegister gets an existing metric or registers the given one.
// The interface can be the metric to register if not found in registry,
// or a function returning the metric for lazy instantiation.
GetOrRegister(string, interface{}) interface{}
// GetOrRegister returns an existing metric or registers the one returned
// by the given constructor.
GetOrRegister(string, func() interface{}) interface{}

// Register the given metric under the given name.
Register(string, interface{}) error
Expand Down Expand Up @@ -95,19 +93,13 @@ func (r *StandardRegistry) Get(name string) interface{} {
// alternative to calling Get and Register on failure.
// The interface can be the metric to register if not found in registry,
// or a function returning the metric for lazy instantiation.
func (r *StandardRegistry) GetOrRegister(name string, i interface{}) interface{} {
func (r *StandardRegistry) GetOrRegister(name string, ctor func() interface{}) interface{} {
// fast path
cached, ok := r.metrics.Load(name)
if ok {
return cached
}
if v := reflect.ValueOf(i); v.Kind() == reflect.Func {
i = v.Call(nil)[0].Interface()
}
item, _, ok := r.loadOrRegister(name, i)
if !ok {
return i
}
item, _, _ := r.loadOrRegister(name, ctor())
return item
}

Expand All @@ -120,9 +112,6 @@ func (r *StandardRegistry) Register(name string, i interface{}) error {
return fmt.Errorf("%w: %v", ErrDuplicateMetric, name)
}

if v := reflect.ValueOf(i); v.Kind() == reflect.Func {
i = v.Call(nil)[0].Interface()
}
_, loaded, _ := r.loadOrRegister(name, i)
if loaded {
return fmt.Errorf("%w: %v", ErrDuplicateMetric, name)
Expand Down Expand Up @@ -295,9 +284,9 @@ func (r *PrefixedRegistry) Get(name string) interface{} {
// GetOrRegister gets an existing metric or registers the given one.
// The interface can be the metric to register if not found in registry,
// or a function returning the metric for lazy instantiation.
func (r *PrefixedRegistry) GetOrRegister(name string, metric interface{}) interface{} {
func (r *PrefixedRegistry) GetOrRegister(name string, ctor func() interface{}) interface{} {
realName := r.prefix + name
return r.underlying.GetOrRegister(realName, metric)
return r.underlying.GetOrRegister(realName, ctor)
}

// Register the given metric under the given name. The name will be prefixed.
Expand Down Expand Up @@ -338,8 +327,11 @@ func Get(name string) interface{} {

// GetOrRegister gets an existing metric or creates and registers a new one. Threadsafe
// alternative to calling Get and Register on failure.
func GetOrRegister(name string, i interface{}) interface{} {
return DefaultRegistry.GetOrRegister(name, i)
func GetOrRegister[T any](name string, ctor func() T, r Registry) interface{} {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to have Registry as the first argument here

Copy link
Contributor Author

@omerfirmak omerfirmak Jun 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was my first choice too, but then I decided to follow the existing signature of metric specific counterparts like GetOrRegisterCounter et al. Which might makes sense because r is an "optional" argument which you can just pass a nil in. It looks slightly better for a nil to be at the end rather than at the beginning of args list imo. I don't mind changing it tho.

Copy link
Contributor

@fjl fjl Jun 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetOrRegister should do the type assertion internally and return T instead of interface{}.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this does not need to be exported.

Copy link
Contributor Author

@omerfirmak omerfirmak Jun 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetOrRegister should do the type assertion internally and return T instead of interface{}.

Would've been nice but that is not possible, because the already registered metric can be of a different type. TestRegistryGetOrRegister relies on that behaviour. We could do that if you think we don't need to support that case.

Also this does not need to be exported.

It was already exported, do you want to make it private?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could do that if you think we don't need to support that case.

I feel like it doesn't make sense to support that, went ahead and done the change.

Copy link
Contributor

@fjl fjl Jun 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, when I wrote "this does not need to be exported", I meant, please make the new function GetOrRegister private, since it is an internal function to be called by the GetOrRegisterXXX functions.

Copy link
Contributor Author

@omerfirmak omerfirmak Jun 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know what you meant and I don't mind making it private. But this is not a new function, I am just modifying an existing one which was already public. It doesn't make sense to make it private because this is part of a family of functions that are all public. see https://github.com/ethereum/go-ethereum/pull/31962/files#diff-6bb88bbbfabec07942822873c99fb049bb38511750afd4179d6b99f0ec0fb955R338-R359

Copy link
Contributor Author

@omerfirmak omerfirmak Jun 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

restored the old public function to keep the interface intact and made this one private in 83c5e78

if r == nil {
r = DefaultRegistry
}
return r.GetOrRegister(name, func() any { return ctor() })
}

// Register the given metric under the given name. Returns a ErrDuplicateMetric
Expand Down
14 changes: 7 additions & 7 deletions metrics/registry_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ func benchmarkRegistryGetOrRegisterParallel(b *testing.B, amount int) {
wg.Add(1)
go func() {
for i := 0; i < b.N; i++ {
r.GetOrRegister("foo", NewMeter)
GetOrRegister("foo", NewMeter, r)
}
wg.Done()
}()
Expand Down Expand Up @@ -98,8 +98,8 @@ func TestRegistryGetOrRegister(t *testing.T) {
r := NewRegistry()

// First metric wins with GetOrRegister
_ = r.GetOrRegister("foo", NewCounter())
m := r.GetOrRegister("foo", NewGauge())
_ = GetOrRegister("foo", NewCounter, r)
m := GetOrRegister("foo", NewGauge, r)
if _, ok := m.(*Counter); !ok {
t.Fatal(m)
}
Expand All @@ -123,8 +123,8 @@ func TestRegistryGetOrRegisterWithLazyInstantiation(t *testing.T) {
r := NewRegistry()

// First metric wins with GetOrRegister
_ = r.GetOrRegister("foo", NewCounter)
m := r.GetOrRegister("foo", NewGauge)
_ = GetOrRegister("foo", NewCounter, r)
m := GetOrRegister("foo", NewGauge, r)
if _, ok := m.(*Counter); !ok {
t.Fatal(m)
}
Expand Down Expand Up @@ -165,7 +165,7 @@ func TestPrefixedChildRegistryGetOrRegister(t *testing.T) {
r := NewRegistry()
pr := NewPrefixedChildRegistry(r, "prefix.")

_ = pr.GetOrRegister("foo", NewCounter())
_ = GetOrRegister("foo", NewCounter, pr)

i := 0
r.Each(func(name string, m interface{}) {
Expand All @@ -182,7 +182,7 @@ func TestPrefixedChildRegistryGetOrRegister(t *testing.T) {
func TestPrefixedRegistryGetOrRegister(t *testing.T) {
r := NewPrefixedRegistry("prefix.")

_ = r.GetOrRegister("foo", NewCounter())
_ = GetOrRegister("foo", NewCounter, r)

i := 0
r.Each(func(name string, m interface{}) {
Expand Down
5 changes: 1 addition & 4 deletions metrics/resetting_timer.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,7 @@ import (
// GetOrRegisterResettingTimer returns an existing ResettingTimer or constructs and registers a
// new ResettingTimer.
func GetOrRegisterResettingTimer(name string, r Registry) *ResettingTimer {
if nil == r {
r = DefaultRegistry
}
return r.GetOrRegister(name, NewResettingTimer).(*ResettingTimer)
return GetOrRegister(name, NewResettingTimer, r).(*ResettingTimer)
}

// NewRegisteredResettingTimer constructs and registers a new ResettingTimer.
Expand Down
5 changes: 1 addition & 4 deletions metrics/runtimehistogram.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,8 @@ import (
)

func getOrRegisterRuntimeHistogram(name string, scale float64, r Registry) *runtimeHistogram {
if r == nil {
r = DefaultRegistry
}
constructor := func() Histogram { return newRuntimeHistogram(scale) }
return r.GetOrRegister(name, constructor).(*runtimeHistogram)
return GetOrRegister(name, constructor, r).(*runtimeHistogram)
}

// runtimeHistogram wraps a runtime/metrics histogram.
Expand Down
5 changes: 1 addition & 4 deletions metrics/timer.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,7 @@ import (
// Be sure to unregister the meter from the registry once it is of no use to
// allow for garbage collection.
func GetOrRegisterTimer(name string, r Registry) *Timer {
if nil == r {
r = DefaultRegistry
}
return r.GetOrRegister(name, NewTimer).(*Timer)
return GetOrRegister(name, NewTimer, r).(*Timer)
}

// NewCustomTimer constructs a new Timer from a Histogram and a Meter.
Expand Down