search

rcrowley / go-metrics

Go port of Coda Hale's Metrics library
Other
3.43k stars 493 forks source link

Rescale samples on reads too #254

Open ashrayjain opened 5 years ago

ashrayjain commented 5 years ago

Previously, we were only rescaling on updates. This meant that histograms would latch on to last updated values and stay at those values until the next update for a reader who reads the histogram periodically instead of decaying with time.

Fixes #215

buffer51 commented 5 years ago

@rcrowley @mihasya

bmoylan commented 5 years ago

Friendly ping @rcrowley @mihasya

mihasya commented 5 years ago

This looks like a very good change, but it makes me wonder if folks have come to rely on the old behavior, and if changing this is a "backwards incompatible" change 🤔 Also trying to think of a case where this might cause a performance issue.. In a set up where you have lots of rarely updated counters that are read regularly, you could suddenly find yourself recomputing a bunch of EWMAs for things that were previously "free." Hmmm.

I do see a bunch of votes for this, and have definitely been annoyed by it in the past.

p-kozlowski commented 4 years ago

I'm worried the fix is not complete - Snapshot() is a read, so it needs rescaling, too. Otherwise, the reads performed on a Snapshoted metric will not trigger the rescaling.

If it seems out of scope of this MR, I could create another one. @ashrayjain, @mihasya what do you folks think about it?

schmurfy commented 4 years ago

any news on this ?

ashrayjain commented 4 years ago

@p-kozlowski you are correct. I have updated this to also rescale on Snapshot()