From 207a8a10ec6e1ebe4c6141ee8019fb8061a30120 Mon Sep 17 00:00:00 2001 From: devopsbo3 <69951731+devopsbo3@users.noreply.github.com> Date: Fri, 10 Nov 2023 12:27:53 -0600 Subject: [PATCH] Revert "metrics: use sync.map in registry (#27159)" This reverts commit dd2f5b3bae43b4cc61212254025e3472a5c00c36. --- metrics/registry.go | 99 ++++++++++++++++++++++------------------ metrics/registry_test.go | 25 ---------- 2 files changed, 55 insertions(+), 69 deletions(-) diff --git a/metrics/registry.go b/metrics/registry.go index ec6e37c54f..4c62248351 100644 --- a/metrics/registry.go +++ b/metrics/registry.go @@ -45,17 +45,21 @@ type Registry interface { // Unregister the metric with the given name. Unregister(string) + + // Unregister all metrics. (Mostly for testing.) + UnregisterAll() } -// The standard implementation of a Registry uses sync.map +// The standard implementation of a Registry is a mutex-protected map // of names to metrics. type StandardRegistry struct { - metrics sync.Map + metrics map[string]interface{} + mutex sync.Mutex } // Create a new registry. func NewRegistry() Registry { - return &StandardRegistry{} + return &StandardRegistry{metrics: make(map[string]interface{})} } // Call the given function for each registered metric. @@ -67,8 +71,9 @@ func (r *StandardRegistry) Each(f func(string, interface{})) { // Get the metric by the given name or nil if none is registered. func (r *StandardRegistry) Get(name string) interface{} { - item, _ := r.metrics.Load(name) - return item + r.mutex.Lock() + defer r.mutex.Unlock() + return r.metrics[name] } // Gets an existing metric or creates and registers a new one. Threadsafe @@ -76,48 +81,35 @@ func (r *StandardRegistry) Get(name string) interface{} { // 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{} { - // fast path - cached, ok := r.metrics.Load(name) - if ok { - return cached + r.mutex.Lock() + defer r.mutex.Unlock() + if metric, ok := r.metrics[name]; ok { + return metric } 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 - } - return item + r.register(name, i) + return i } // Register the given metric under the given name. Returns a DuplicateMetric // if a metric by the given name is already registered. func (r *StandardRegistry) Register(name string, i interface{}) error { - // fast path - _, ok := r.metrics.Load(name) - if ok { - return DuplicateMetric(name) - } - - if v := reflect.ValueOf(i); v.Kind() == reflect.Func { - i = v.Call(nil)[0].Interface() - } - _, loaded, _ := r.loadOrRegister(name, i) - if loaded { - return DuplicateMetric(name) - } - return nil + r.mutex.Lock() + defer r.mutex.Unlock() + return r.register(name, i) } // Run all registered healthchecks. func (r *StandardRegistry) RunHealthchecks() { - r.metrics.Range(func(key, value any) bool { - if h, ok := value.(Healthcheck); ok { + r.mutex.Lock() + defer r.mutex.Unlock() + for _, i := range r.metrics { + if h, ok := i.(Healthcheck); ok { h.Check() } - return true - }) + } } // GetAll metrics in the Registry @@ -185,31 +177,45 @@ func (r *StandardRegistry) GetAll() map[string]map[string]interface{} { // Unregister the metric with the given name. func (r *StandardRegistry) Unregister(name string) { + r.mutex.Lock() + defer r.mutex.Unlock() r.stop(name) - r.metrics.LoadAndDelete(name) + delete(r.metrics, name) } -func (r *StandardRegistry) loadOrRegister(name string, i interface{}) (interface{}, bool, bool) { +// Unregister all metrics. (Mostly for testing.) +func (r *StandardRegistry) UnregisterAll() { + r.mutex.Lock() + defer r.mutex.Unlock() + for name := range r.metrics { + r.stop(name) + delete(r.metrics, name) + } +} + +func (r *StandardRegistry) register(name string, i interface{}) error { + if _, ok := r.metrics[name]; ok { + return DuplicateMetric(name) + } switch i.(type) { case Counter, CounterFloat64, Gauge, GaugeFloat64, Healthcheck, Histogram, Meter, Timer, ResettingTimer: - default: - return nil, false, false + r.metrics[name] = i } - item, loaded := r.metrics.LoadOrStore(name, i) - return item, loaded, true + return nil } func (r *StandardRegistry) registered() map[string]interface{} { - metrics := make(map[string]interface{}) - r.metrics.Range(func(key, value any) bool { - metrics[key.(string)] = value - return true - }) + r.mutex.Lock() + defer r.mutex.Unlock() + metrics := make(map[string]interface{}, len(r.metrics)) + for name, i := range r.metrics { + metrics[name] = i + } return metrics } func (r *StandardRegistry) stop(name string) { - if i, ok := r.metrics.Load(name); ok { + if i, ok := r.metrics[name]; ok { if s, ok := i.(Stoppable); ok { s.Stop() } @@ -302,6 +308,11 @@ func (r *PrefixedRegistry) Unregister(name string) { r.underlying.Unregister(realName) } +// Unregister all metrics. (Mostly for testing.) +func (r *PrefixedRegistry) UnregisterAll() { + r.underlying.UnregisterAll() +} + var ( DefaultRegistry = NewRegistry() EphemeralRegistry = NewRegistry() diff --git a/metrics/registry_test.go b/metrics/registry_test.go index 7cc5cf14fe..d277ae5c3e 100644 --- a/metrics/registry_test.go +++ b/metrics/registry_test.go @@ -1,7 +1,6 @@ package metrics import ( - "sync" "testing" ) @@ -14,30 +13,6 @@ func BenchmarkRegistry(b *testing.B) { } } -func BenchmarkRegistryGetOrRegisterParallel_8(b *testing.B) { - benchmarkRegistryGetOrRegisterParallel(b, 8) -} - -func BenchmarkRegistryGetOrRegisterParallel_32(b *testing.B) { - benchmarkRegistryGetOrRegisterParallel(b, 32) -} - -func benchmarkRegistryGetOrRegisterParallel(b *testing.B, amount int) { - r := NewRegistry() - b.ResetTimer() - var wg sync.WaitGroup - for i := 0; i < amount; i++ { - wg.Add(1) - go func() { - for i := 0; i < b.N; i++ { - r.GetOrRegister("foo", NewMeter) - } - wg.Done() - }() - } - wg.Wait() -} - func TestRegistry(t *testing.T) { r := NewRegistry() r.Register("foo", NewCounter())