Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add Stop() to Meter #197

Merged
merged 1 commit into from
Nov 28, 2017
Merged

add Stop() to Meter #197

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 31 additions & 3 deletions meter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ type Meter interface {
Rate15() float64
RateMean() float64
Snapshot() Meter
Stop()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

}

// GetOrRegisterMeter returns an existing Meter or constructs and registers a
Expand All @@ -34,7 +35,8 @@ func NewMeter() Meter {
m := newStandardMeter()
arbiter.Lock()
defer arbiter.Unlock()
arbiter.meters = append(arbiter.meters, m)
m.id = arbiter.newID()
arbiter.meters[m.id] = m
if !arbiter.started {
arbiter.started = true
go arbiter.tick()
Expand Down Expand Up @@ -86,6 +88,9 @@ func (m *MeterSnapshot) RateMean() float64 { return m.rateMean }
// Snapshot returns the snapshot.
func (m *MeterSnapshot) Snapshot() Meter { return m }

// Stop is a no-op.
func (m *MeterSnapshot) Stop() {}

// NilMeter is a no-op Meter.
type NilMeter struct{}

Expand All @@ -110,12 +115,17 @@ func (NilMeter) RateMean() float64 { return 0.0 }
// Snapshot is a no-op.
func (NilMeter) Snapshot() Meter { return NilMeter{} }

// Stop is a no-op.
func (NilMeter) Stop() {}

// StandardMeter is the standard implementation of a Meter.
type StandardMeter struct {
lock sync.RWMutex
snapshot *MeterSnapshot
a1, a5, a15 EWMA
startTime time.Time
stopped bool
id int64
}

func newStandardMeter() *StandardMeter {
Expand All @@ -128,6 +138,14 @@ func newStandardMeter() *StandardMeter {
}
}

// Stop stops the meter, Mark() will panic if you use it after being stopped.
func (m *StandardMeter) Stop() {
arbiter.Lock()
defer arbiter.Unlock()
m.stopped = true
delete(arbiter.meters, m.id)
}

// Count returns the number of events recorded.
func (m *StandardMeter) Count() int64 {
m.lock.RLock()
Expand All @@ -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")
Copy link

@jpudney jpudney Apr 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should really return error instead of panicking?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and change the interface for that? That's like the Mark() on a snapshot which also panics

Copy link

@imkira imkira May 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
m.snapshot.count += n
m.a1.Update(n)
m.a5.Update(n)
Expand Down Expand Up @@ -208,11 +229,12 @@ func (m *StandardMeter) tick() {
type meterArbiter struct {
sync.RWMutex
started bool
meters []*StandardMeter
meters map[int64]*StandardMeter
Copy link
Contributor

@slaunay slaunay Jun 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about using a set of *StandardMeter aka map[*StandardMeter]struct{} to avoid id collision/overflow yet benefit from fast and easy deletion?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think that would be a better solution than adding the ID generation

ticker *time.Ticker
id int64
}

var arbiter = meterArbiter{ticker: time.NewTicker(5e9)}
var arbiter = meterArbiter{ticker: time.NewTicker(5e9), meters: make(map[int64]*StandardMeter)}

// Ticks meters on the scheduled interval
func (ma *meterArbiter) tick() {
Expand All @@ -224,6 +246,12 @@ func (ma *meterArbiter) tick() {
}
}

// should only be called with Lock() held
func (ma *meterArbiter) newID() int64 {
ma.id++
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Someone, somewhere will overflow this

Copy link

@imkira imkira Apr 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return ma.id
}

func (ma *meterArbiter) tickMeters() {
ma.RLock()
defer ma.RUnlock()
Expand Down
4 changes: 3 additions & 1 deletion meter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,11 @@ func TestGetOrRegisterMeter(t *testing.T) {
func TestMeterDecay(t *testing.T) {
ma := meterArbiter{
ticker: time.NewTicker(time.Millisecond),
meters: make(map[int64]*StandardMeter),
}
m := newStandardMeter()
ma.meters = append(ma.meters, m)
m.id = ma.newID()
ma.meters[m.id] = m
go ma.tick()
m.Mark(1)
rateMean := m.RateMean()
Expand Down