-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Adds Histogram Aggregator #433
Adds Histogram Aggregator #433
Conversation
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.
Looks good from an overall structure. I think with the suggestions/fixes mentioned it should be good to go.
Change to less-than buckets. Add offset checks. Unexport fields that don't need to be exported. Fix tests when running on profile with int64 number kind.
I've addressed all the discussions, thanks everyone :) |
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.
Looks good!
This adds a new Aggregator implementation
Histogram
.Fixes #317
It returns a
Buckets
which consists of the bucket's boundaries and the counts for each bucket. These counts are not cumulative, the reason for this is to make the update simpler and faster. Exporters may accumulate them if needed.