From ec1fc4992c41e1be19cdab1565e1654d5aa5689b Mon Sep 17 00:00:00 2001 From: Alexander Yastrebov Date: Mon, 7 Aug 2023 17:46:31 +0200 Subject: [PATCH] metrics: fix goroutine leaks in tests * adds Close() method to the Metrics interface * implements CodaHale metrics stats collector goroutines cleanup * fixes goroutine leaks in test Signed-off-by: Alexander Yastrebov --- VERSION | 2 +- metrics/all_kind.go | 5 +++++ metrics/codahale.go | 24 ++++++++++++++++++++++-- metrics/codahale_test.go | 8 ++++++++ metrics/main_test.go | 18 ++++++++++++++++++ metrics/metrics.go | 1 + metrics/metrics_test.go | 15 ++++++++++++++- metrics/metricstest/metricsmock.go | 2 ++ metrics/prometheus.go | 2 ++ 9 files changed, 73 insertions(+), 4 deletions(-) create mode 100644 metrics/main_test.go diff --git a/VERSION b/VERSION index 39fa1765f9..2b8bac3194 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -v0.16 +v0.17 diff --git a/metrics/all_kind.go b/metrics/all_kind.go index d4e9e0e3da..f28a981a90 100644 --- a/metrics/all_kind.go +++ b/metrics/all_kind.go @@ -93,6 +93,11 @@ func (a *All) IncErrorsStreaming(routeId string) { a.codaHale.IncErrorsStreaming(routeId) } + +func (a *All) Close() { + a.codaHale.Close() +} + func (a *All) RegisterHandler(path string, handler *http.ServeMux) { a.prometheusHandler = a.prometheus.getHandler() a.codaHaleHandler = a.codaHale.getHandler(path) diff --git a/metrics/codahale.go b/metrics/codahale.go index d5eff0aa8b..0b7fa8241d 100644 --- a/metrics/codahale.go +++ b/metrics/codahale.go @@ -45,6 +45,7 @@ type CodaHale struct { createGauge func() metrics.GaugeFloat64 options Options handler http.Handler + quit chan struct{} } // NewCodaHale returns a new CodaHale backend of metrics. @@ -52,6 +53,8 @@ func NewCodaHale(o Options) *CodaHale { o = applyCompatibilityDefaults(o) c := &CodaHale{} + + c.quit = make(chan struct{}) c.reg = metrics.NewRegistry() var createSample func() metrics.Sample @@ -68,12 +71,12 @@ func NewCodaHale(o Options) *CodaHale { if o.EnableDebugGcMetrics { metrics.RegisterDebugGCStats(c.reg) - go metrics.CaptureDebugGCStats(c.reg, statsRefreshDuration) + go c.collectStats(metrics.CaptureDebugGCStatsOnce) } if o.EnableRuntimeMetrics { metrics.RegisterRuntimeMemStats(c.reg) - go metrics.CaptureRuntimeMemStats(c.reg, statsRefreshDuration) + go c.collectStats(metrics.CaptureRuntimeMemStatsOnce) } return c @@ -234,6 +237,23 @@ func (c *CodaHale) IncErrorsStreaming(routeId string) { } } +func (c *CodaHale) Close() { + close(c.quit) +} + +func (c *CodaHale) collectStats(capture func(r metrics.Registry)) { + ticker := time.NewTicker(statsRefreshDuration) + defer ticker.Stop() + for { + select { + case <-ticker.C: + capture(c.reg) + case <-c.quit: + return + } + } +} + func (c *CodaHale) RegisterHandler(path string, handler *http.ServeMux) { h := c.getHandler(path) handler.Handle(path, h) diff --git a/metrics/codahale_test.go b/metrics/codahale_test.go index 30de34829c..7f7fd6e8a3 100644 --- a/metrics/codahale_test.go +++ b/metrics/codahale_test.go @@ -36,6 +36,7 @@ func TestUseVoidByDefaultOptions(t *testing.T) { func TestDefaultOptionsWithListener(t *testing.T) { o := Options{} c := NewCodaHale(o) + defer c.Close() if c == Void { t.Error("Options containing a listener should create a registry") @@ -53,6 +54,7 @@ func TestDefaultOptionsWithListener(t *testing.T) { func TestCodaHaleDebugGcStats(t *testing.T) { o := Options{EnableDebugGcMetrics: true} c := NewCodaHale(o) + defer c.Close() if c.reg.Get("debug.GCStats.LastGC") == nil { t.Error("Options enabled debug gc stats but failed to find the key 'debug.GCStats.LastGC'") @@ -62,6 +64,7 @@ func TestCodaHaleDebugGcStats(t *testing.T) { func TestCodaHaleRuntimeStats(t *testing.T) { o := Options{EnableRuntimeMetrics: true} c := NewCodaHale(o) + defer c.Close() if c.reg.Get("runtime.MemStats.Alloc") == nil { t.Error("Options enabled runtime stats but failed to find the key 'runtime.MemStats.Alloc'") @@ -71,6 +74,7 @@ func TestCodaHaleRuntimeStats(t *testing.T) { func TestCodaHaleMeasurement(t *testing.T) { o := Options{} c := NewCodaHale(o) + defer c.Close() g1 := c.getGauge("TestGauge") c.UpdateGauge("TestGauge", 1) @@ -172,6 +176,8 @@ func TestCodaHaleProxyMetrics(t *testing.T) { for _, pmt := range proxyMetricsTests { t.Run(pmt.metricsKey, func(t *testing.T) { m := NewCodaHale(Options{}) + defer m.Close() + pmt.measureFunc(m) time.Sleep(20 * time.Millisecond) @@ -496,6 +502,8 @@ func TestCodaHaleServeMetrics(t *testing.T) { } m := NewCodaHale(ti.options) + defer m.Close() + for _, mi := range ti.measures { m.MeasureServe(mi.route, mi.host, mi.method, mi.status, time.Now().Add(-mi.duration)) } diff --git a/metrics/main_test.go b/metrics/main_test.go new file mode 100644 index 0000000000..951ed6830c --- /dev/null +++ b/metrics/main_test.go @@ -0,0 +1,18 @@ +package metrics + +import ( + "os" + "testing" + + "github.com/AlexanderYastrebov/noleak" + codahale "github.com/rcrowley/go-metrics" +) + +func TestMain(m *testing.M) { + // codahale library has a global arbiter goroutine to update timers and there is no way to stop it, + // see https://github.com/rcrowley/go-metrics/pull/46 + // Create a dummy timer to start the arbiter goroutine before running tests such that leak detector does not report it. + _ = codahale.NewTimer() + + os.Exit(noleak.CheckMain(m)) +} diff --git a/metrics/metrics.go b/metrics/metrics.go index cb6404f408..88ebc49b1e 100644 --- a/metrics/metrics.go +++ b/metrics/metrics.go @@ -76,6 +76,7 @@ type Metrics interface { IncErrorsStreaming(routeId string) RegisterHandler(path string, handler *http.ServeMux) UpdateGauge(key string, value float64) + Close() } // Options for initializing metrics collection. diff --git a/metrics/metrics_test.go b/metrics/metrics_test.go index 8ef232c68f..55efe6f161 100644 --- a/metrics/metrics_test.go +++ b/metrics/metrics_test.go @@ -52,7 +52,10 @@ func TestHandlerCodaHaleBadRequests(t *testing.T) { Format: metrics.CodaHaleKind, EnableRuntimeMetrics: true, } - mh := metrics.NewDefaultHandler(o) + m := metrics.NewMetrics(o) + defer m.Close() + + mh := metrics.NewHandler(o, m) r1, _ := http.NewRequest("GET", "/", nil) rw1 := httptest.NewRecorder() @@ -76,6 +79,8 @@ func TestHandlerCodaHaleAllMetricsRequest(t *testing.T) { EnableRuntimeMetrics: true, } m := metrics.NewCodaHale(o) + defer m.Close() + mh := metrics.NewHandler(o, m) m.IncCounter("TestHandlerCodaHaleAllMetricsRequest") @@ -103,6 +108,8 @@ func TestHandlerCodaHaleSingleMetricsRequest(t *testing.T) { EnableRuntimeMetrics: true, } m := metrics.NewCodaHale(o) + defer m.Close() + mh := metrics.NewHandler(o, m) m.IncCounter("TestHandlerCodaHaleSingleMetricsRequest") @@ -134,6 +141,8 @@ func TestHandlerCodaHaleSingleMetricsRequestWhenUsingPrefix(t *testing.T) { EnableRuntimeMetrics: true, } m := metrics.NewCodaHale(o) + defer m.Close() + mh := metrics.NewHandler(o, m) m.IncCounter("TestHandlerCodaHaleSingleMetricsRequestWhenUsingPrefix") @@ -164,6 +173,8 @@ func TestHandlerCodaHaleMetricsRequestWithPattern(t *testing.T) { EnableRuntimeMetrics: true, } m := metrics.NewCodaHale(o) + defer m.Close() + mh := metrics.NewHandler(o, m) m.UpdateGauge("runtime.Num", 5.0) @@ -202,6 +213,8 @@ func TestHandlerCodaHaleUnknownMetricRequest(t *testing.T) { EnableRuntimeMetrics: true, } m := metrics.NewCodaHale(o) + defer m.Close() + mh := metrics.NewHandler(o, m) r, _ := http.NewRequest("GET", "/metrics/DOES-NOT-EXIST", nil) diff --git a/metrics/metricstest/metricsmock.go b/metrics/metricstest/metricsmock.go index c10188962b..a61b59b161 100644 --- a/metrics/metricstest/metricsmock.go +++ b/metrics/metricstest/metricsmock.go @@ -177,3 +177,5 @@ func (m *MockMetrics) Gauge(key string) (v float64, ok bool) { return } + +func (m *MockMetrics) Close() {} diff --git a/metrics/prometheus.go b/metrics/prometheus.go index 6fe77a3438..d2f969df92 100644 --- a/metrics/prometheus.go +++ b/metrics/prometheus.go @@ -455,3 +455,5 @@ func (p *Prometheus) MeasureBackend5xx(start time.Time) { func (p *Prometheus) IncErrorsStreaming(routeID string) { p.proxyStreamingErrorsM.WithLabelValues(routeID).Inc() } + +func (p *Prometheus) Close() {}