-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
add max to histogram stats #1968
Conversation
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
@@ -411,6 +411,9 @@ struct HistogramData { | |||
double percentile99; | |||
double average; | |||
double standard_deviation; | |||
// zero-initialize new members since old Statistics::histogramData() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I don't understand this comment. What is the problem with old implementations ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if a user has an old subclass of Statistics, their implementation of histogramData() won't write anything to max until they update it. I updated ours via the change in util/histogram.cc, which is used by StatisticsImpl::histogramData()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense !
@ajkr updated the pull request - view changes - changes since last import |
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
@ajkr updated the pull request - view changes - changes since last import |
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
@ajkr updated the pull request - view changes - changes since last import |
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Domas enlightened me about p100 (i.e., max) stats. Let's add them to our histograms.
Test Plan: