add Stop() to Meter #197
add Stop() to Meter #197
Conversation
| @@ -140,6 +158,9 @@ func (m *StandardMeter) Count() int64 { | |||
| func (m *StandardMeter) Mark(n int64) { | |||
| m.lock.Lock() | |||
| defer m.lock.Unlock() | |||
| if m.stopped { | |||
| panic("Mark called on a stopped Meter") | |||
There was a problem hiding this comment.
Should really return error instead of panicking?
There was a problem hiding this comment.
and change the interface for that? That's like the Mark() on a snapshot which also panics
There was a problem hiding this comment.
Up until now there is no panic on anything else except snapshots and the like, which are unconditional panics. If we do this here, extreme caution must be taken while doing unregister+Mark concurrently. Being able to safely ignore (no-op) this would be better IMHO
| @@ -224,6 +246,12 @@ func (ma *meterArbiter) tick() { | |||
| } | |||
| } | |||
|
|
|||
| // should only be called with Lock() held | |||
| func (ma *meterArbiter) newID() int64 { | |||
| ma.id++ | |||
There was a problem hiding this comment.
Someone, somewhere will overflow this
There was a problem hiding this comment.
That also made me a bit concerned.
The only reason why we want a map is to have constant-time deletion, so we don't actually need the concept of "IDs" for metrics.
Couldn't we register the meter in the arbiter's map having both key and value as the pointer to itself (avoid ID generation)? We could also add a check to stopped to make sure it gets done only once.
|
Is there an ETA for merging this PR? |
| @@ -208,11 +229,12 @@ func (m *StandardMeter) tick() { | |||
| type meterArbiter struct { | |||
| sync.RWMutex | |||
| started bool | |||
| meters []*StandardMeter | |||
| meters map[int64]*StandardMeter | |||
There was a problem hiding this comment.
What about using a set of *StandardMeter aka map[*StandardMeter]struct{} to avoid id collision/overflow yet benefit from fast and easy deletion?
There was a problem hiding this comment.
Yes, I think that would be a better solution than adding the ID generation
| @@ -15,6 +15,7 @@ type Meter interface { | |||
| Rate15() float64 | |||
| RateMean() float64 | |||
| Snapshot() Meter | |||
| Stop() | |||
There was a problem hiding this comment.
I would add the same method on Timer as they rely on a Meter under the hood.
I am even wondering if having an interface Stoppable implemented by Timer and Meter and invoking Stop in Registry.Unregister(string) when the metric implements such an interface is the proper way to clean those metrics that can leak (potentially supporting similar behavior for custom metrics).
What do you think?
There was a problem hiding this comment.
Rephrasing just so I understand: you're suggesting we add the interface and implementations, then leave it up to the user to call Unregister when a metric has outlived its usefulness, correct? I suppose there's not really a magical way we can know when it's safe to do so internally.
There was a problem hiding this comment.
@mihasya Absolutely.
Unregister (and UnregisterAll by extension) would check if the underlying interface{} metric implements Stoppable and call Stop() as like you said there is no way to know when those metrics are not used anymore.
|
Hope to fix this leak problem |
|
Sorry, I've been a horrendous maintainer, but this bug definitely needs a fix. I've made some comments on this that also apply to #206. I am in favor of both of @slaunay's comments - if we can use the pointer as the hash key and call |
|
@mihasya Any chance you can take a look at those changes as it is impacting quite a few downstream projects? |
this also removes the meter from the tickers.