rcrowley / go-metrics

Go port of Coda Hale's Metrics library
Other
3.46k stars 494 forks source link

Allow SampleRescaleThreshold to be configured from userspace #142

Open narqo opened 8 years ago

narqo commented 8 years ago

Port fixes from #72 as it looks abandoned.

p.s. Still not sure it's the best possible solution, so ideas are welcome.

mihasya commented 8 years ago

Narqo, thanks for the updated PR. After re-reading the last PR and reading @rcrowley I think the main concern here is concurrent access. Using a mutex as in the original patch is the "obvious" solution. Richard was concerned about adding a mutex, presumably for performance and complexity reasons. However, looking at the code, it appears that the interval is cached on a per-sample basis in t1, so it's only ever called *once per sample per SampleRescaleThreshold. What do you think is a realistically low value for that? Maybe we should just go back to using the mutex since in computer terms it's not going to happen all that frequently at all.

Alternatively, we could just add a member on each Sample that caches the value and limit the need to sync to only initialization time at the cost of an extra time.Duration (int64).

One other point: this seems like the sort of thing that would cause "unexpected" behavior if changed during the runtime of an application, correct? Wondering if we should add a little code to protect from people doing this more than once.

narqo commented 8 years ago

Alternatively, we could just add a member on each Sample that caches the value and limit the need to sync to only initialization time at the cost of an extra time.Duration (int64)

I think this might be the simplest and quite safe solution we can do without compatibility break. Something like:

var SampleRescalThreshold = ...

type ExpDecaySample struct {
    ···
    rescaleThreshold time.Duration,
}

func NewExpDecaySample(reservoirSize int, alpha float64) Sample {
    ···
    s := &ExpDecaySample{
        rescaleThreshold: SampleRescalThreshold,
        ···
    }
}

I think to attach rescaleThreshold to the Registry instance would be a more proper solution. But currently, I don't think it's possible without breaking the API.

Wondering if we should add a little code to protect from people doing this more than once.

For sure.