metrics: fix goroutine leaks in tests (#2505)

* adds Close() method to the Metrics interface * implements CodaHale metrics stats collector goroutines cleanup * fixes goroutine leaks in test Signed-off-by: Alexander Yastrebov <[email protected]>
zalando · Aug 9, 2023 · 2e836aa · 2e836aa
1 parent be73f2f
commit 2e836aa
Show file tree

Hide file tree

Showing 9 changed files with 74 additions and 4 deletions.
diff --git a/VERSION b/VERSION
@@ -1 +1 @@
-v0.16
+v0.17
diff --git a/metrics/all_kind.go b/metrics/all_kind.go
@@ -93,6 +93,12 @@ func (a *All) IncErrorsStreaming(routeId string) {
 	a.codaHale.IncErrorsStreaming(routeId)
 
 }
+
+func (a *All) Close() {
+	a.codaHale.Close()
+	a.prometheus.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
@@ -45,13 +45,16 @@ type CodaHale struct {
 	createGauge   func() metrics.GaugeFloat64
 	options       Options
 	handler       http.Handler
+	quit          chan struct{}
 }
 
 // NewCodaHale returns a new CodaHale backend of metrics.
 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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() {}