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

Switch sparse histograms to base-2 buckets #886

Merged
merged 3 commits into from
Jun 24, 2021
Merged

Conversation

beorn7
Copy link
Member

@beorn7 beorn7 commented Jun 23, 2021

This seem what OTel is converging towards, see
open-telemetry/oteps#149 .

I see pros and cons with base-10 vs base-2. They are discussed in
detail in that OTel PR, and the gist of the discussion is pretty much
in line with my design doc. Since the balance is easy to tip here, I
think we should go with base-2 if OTel picks base-2. This also seems
to be in agreement with several proprietary solution (see again the
discussion on that OTel PR.)

The idea to make the number of buckets per power of 2 (or formerly 10)
a power of 2 itself was also sketched out in the design doc
already. It guarantees mergeability of different resolutions. I was
undecided between making it a recommendation or mandatory. Now I think
it should be mandatory as it has the additional benefit of playing
well with OTel's plans.

This commit also addresses a number of outstanding TODOs.

This depends on prometheus/client_model#47 (go.mod currently pinned to that commit).

@codesome note that this is not going to master, but just to the highly experimental sparsehistogram branch. I still wanted to create a PR so that you can chime in.

This seem what OTel is converging towards, see
open-telemetry/oteps#149 .

I see pros and cons with base-10 vs base-2. They are discussed in
detail in that OTel PR, and the gist of the discussion is pretty much
in line with my design doc. Since the balance is easy to tip here, I
think we should go with base-2 if OTel picks base-2. This also seems
to be in agreement with several proprietary solution (see again the
discussion on that OTel PR.)

The idea to make the number of buckets per power of 2 (or formerly 10)
a power of 2 itself was also sketched out in the design doc
already. It guarantees mergeability of different resolutions. I was
undecided between making it a recommendation or mandatory. Now I think
it should be mandatory as it has the additional benefit of playing
well with OTel's plans.

This commit also addresses a number of outstanding TODOs.

Signed-off-by: beorn7 <beorn@grafana.com>
Signed-off-by: beorn7 <beorn@grafana.com>
@beorn7 beorn7 requested a review from codesome June 23, 2021 20:04
Copy link
Member

@codesome codesome left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Signed-off-by: beorn7 <beorn@grafana.com>
@beorn7 beorn7 merged commit 43f31c2 into sparsehistogram Jun 24, 2021
@beorn7 beorn7 deleted the beorn7/histogram branch June 24, 2021 20:22
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.

2 participants