Merge pull request #1 from slaunay/bugfix/meter-memory-leak

Use set of meters and update documentation

README.md

@@ -42,12 +42,22 @@ t.Update(47)
 Register() is not threadsafe. For threadsafe metric registration use
 GetOrRegister:
 
-```
+```go
 t := metrics.GetOrRegisterTimer("account.create.latency", nil)
 t.Time(func() {})
 t.Update(47)
 ```
 
+**NOTE:** Be sure to either unregister meters and timers otherwise and they will
+leak memory:
+
+```go
+// Will call Stop() on the Meter to allow for garbage collection
+metrics.Unregister("quux")
+// Or similarly for a Timer that embeds a Meter
+metrics.Unregister("bang")
+```
+
 Periodically log every metric in human-readable form to standard error:
 
 ```go

meter.go

@@ -20,6 +20,8 @@ type Meter interface {
 
 // GetOrRegisterMeter returns an existing Meter or constructs and registers a
 // new StandardMeter.
+// 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 nil == r {
 		r = DefaultRegistry
@@ -28,15 +30,15 @@ func GetOrRegisterMeter(name string, r Registry) Meter {
 }
 
 // NewMeter constructs a new StandardMeter and launches a goroutine.
+// Be sure to call Stop() once the meter is of no use to allow for garbage collection.
 func NewMeter() Meter {
 	if UseNilMetrics {
 		return NilMeter{}
 	}
 	m := newStandardMeter()
 	arbiter.Lock()
 	defer arbiter.Unlock()
-	m.id = arbiter.newID()
-	arbiter.meters[m.id] = m
+	arbiter.meters[m] = struct{}{}
 	if !arbiter.started {
 		arbiter.started = true
 		go arbiter.tick()
@@ -46,6 +48,8 @@ func NewMeter() Meter {
 
 // NewMeter constructs and registers a new StandardMeter and launches a
 // goroutine.
+// Be sure to unregister the meter from the registry once it is of no use to
+// allow for garbage collection.
 func NewRegisteredMeter(name string, r Registry) Meter {
 	c := NewMeter()
 	if nil == r {
@@ -125,7 +129,6 @@ type StandardMeter struct {
 	a1, a5, a15 EWMA
 	startTime   time.Time
 	stopped     bool
-	id          int64
 }
 
 func newStandardMeter() *StandardMeter {
@@ -138,15 +141,15 @@ func newStandardMeter() *StandardMeter {
 	}
 }
 
-// Stop stops the meter, Mark() will panic if you use it after being stopped.
+// Stop stops the meter, Mark() will be a no-op if you use it after being stopped.
 func (m *StandardMeter) Stop() {
 	m.lock.Lock()
 	stopped := m.stopped
 	m.stopped = true
 	m.lock.Unlock()
 	if !stopped {
 		arbiter.Lock()
-		delete(arbiter.meters, m.id)
+		delete(arbiter.meters, m)
 		arbiter.Unlock()
 	}
 }
@@ -231,15 +234,16 @@ func (m *StandardMeter) tick() {
 	m.updateSnapshot()
 }
 
+// meterArbiter ticks meters every 5s from a single goroutine.
+// meters are references in a set for future stopping.
 type meterArbiter struct {
 	sync.RWMutex
 	started bool
-	meters  map[int64]*StandardMeter
+	meters  map[*StandardMeter]struct{}
 	ticker  *time.Ticker
-	id      int64
 }
 
-var arbiter = meterArbiter{ticker: time.NewTicker(5e9), meters: make(map[int64]*StandardMeter)}
+var arbiter = meterArbiter{ticker: time.NewTicker(5e9), meters: make(map[*StandardMeter]struct{})}
 
 // Ticks meters on the scheduled interval
 func (ma *meterArbiter) tick() {
@@ -251,16 +255,10 @@ func (ma *meterArbiter) tick() {
 	}
 }
 
-// should only be called with Lock() held
-func (ma *meterArbiter) newID() int64 {
-	ma.id++
-	return ma.id
-}
-
 func (ma *meterArbiter) tickMeters() {
 	ma.RLock()
 	defer ma.RUnlock()
-	for _, meter := range ma.meters {
+	for meter := range ma.meters {
 		meter.tick()
 	}
 }

meter_test.go

@@ -24,11 +24,10 @@ func TestGetOrRegisterMeter(t *testing.T) {
 func TestMeterDecay(t *testing.T) {
 	ma := meterArbiter{
 		ticker: time.NewTicker(time.Millisecond),
-		meters: make(map[int64]*StandardMeter),
+		meters: make(map[*StandardMeter]struct{}),
 	}
 	m := newStandardMeter()
-	m.id = ma.newID()
-	ma.meters[m.id] = m
+	ma.meters[m] = struct{}{}
 	go ma.tick()
 	m.Mark(1)
 	rateMean := m.RateMean()

timer.go

@@ -29,6 +29,8 @@ type Timer interface {
 
 // GetOrRegisterTimer returns an existing Timer or constructs and registers a
 // new StandardTimer.
+// 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
@@ -37,6 +39,7 @@ func GetOrRegisterTimer(name string, r Registry) Timer {
 }
 
 // NewCustomTimer constructs a new StandardTimer from a Histogram and a Meter.
+// Be sure to call Stop() once the timer is of no use to allow for garbage collection.
 func NewCustomTimer(h Histogram, m Meter) Timer {
 	if UseNilMetrics {
 		return NilTimer{}
@@ -48,6 +51,8 @@ func NewCustomTimer(h Histogram, m Meter) Timer {
 }
 
 // NewRegisteredTimer constructs and registers a new StandardTimer.
+// Be sure to unregister the meter from the registry once it is of no use to
+// allow for garbage collection.
 func NewRegisteredTimer(name string, r Registry) Timer {
 	c := NewTimer()
 	if nil == r {
@@ -59,6 +64,7 @@ func NewRegisteredTimer(name string, r Registry) Timer {
 
 // NewTimer constructs a new StandardTimer using an exponentially-decaying
 // sample with the same reservoir size and alpha as UNIX load averages.
+// Be sure to call Stop() once the timer is of no use to allow for garbage collection.
 func NewTimer() Timer {
 	if UseNilMetrics {
 		return NilTimer{}