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

fix inclusive-exclusiveness of histogram ToString #2817

Closed
wants to merge 2 commits into from

Conversation

ajkr
Copy link
Contributor

@ajkr ajkr commented Aug 31, 2017

I spent too much time thinking about histograms lately and realized boundary values fall into the lower bucket, not the upper bucket. It's because we're using std::map::lower_bound here:

valueIndexMap_.lower_bound(value);
. Fixed histogram's ToString() to reflect this.

Test Plan:

  • unit test to verify in which bucket boundary values are added
  • verified db_bench histogram output:
[       0,       1 ]    81936   8.194%   8.194% ##
(       1,       2 ]   636865  63.687%  71.880% #############
(       2,       3 ]   237846  23.785%  95.665% #####
(       3,       4 ]    34207   3.421%  99.085% #
(       4,       6 ]     5712   0.571%  99.657% 
(       6,      10 ]      914   0.091%  99.748% 
(      10,      15 ]       50   0.005%  99.753% 
...

@facebook-github-bot
Copy link
Contributor

@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@ajkr ajkr requested review from sagar0 and siying August 31, 2017 22:48
@facebook-github-bot
Copy link
Contributor

@ajkr updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Contributor

@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

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

Successfully merging this pull request may close these issues.

None yet

3 participants