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.

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

Rescale samples on reads too #254

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ashrayjain
Copy link

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

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.
@buffer51
Copy link

@rcrowley @mihasya

@bmoylan
Copy link

bmoylan commented Feb 14, 2019

Friendly ping @rcrowley @mihasya

@mihasya
Copy link
Collaborator

mihasya commented Jul 4, 2019

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
Copy link

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
Copy link

any news on this ?

@ashrayjain
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

timer percentiles get stuck if update isn't called
6 participants