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

Replace percentile with quantile #171

Merged
merged 3 commits into from
Jul 20, 2020

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Jul 18, 2020

The use of percentiles to convey summary statistics is likely to lead to errors, since readers and writers must multiply or divide by 100.0 to compute a population ratio. Percentiles are good for display. Quantiles are good for math. This proposal just replaces percentile with quantile and replaces [0, 100] with [0, 1].

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

This is just personal preference I feel. I would support this if more people feel the same no strong opinion pro/cons.

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

LGTM

@bogdandrutu
Copy link
Member

@open-telemetry/specs-metrics-approvers please review. This is not very controversial

@bogdandrutu
Copy link
Member

@jmacd please rebase and make CI happy and I will merge

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

+1 to terminology change

However, I am surprised we have these values in the protocol in the first place. Anything that captures quantiles during ingestion is a recipe for someone doing bad math later at query time, like re-aggregating those quantiles.

@jmacd
Copy link
Contributor Author

jmacd commented Jul 20, 2020

However, I am surprised we have these values in the protocol in the first place.

These values are needed in the protocol where the server scrapes a Prometheus Summary metric, for example, but this is another reason that Min and Max should be their own field: the extreme quantiles can be re-aggregated correctly.

@bogdandrutu
Copy link
Member

They can only be merged and not subtracted (from cumulative to delta) but it is still good. Also probably we should always send deltas and let the backend deal with this problem.

@bogdandrutu
Copy link
Member

@jmacd do you mind making this PR ready to merge?

@jmacd
Copy link
Contributor Author

jmacd commented Jul 20, 2020

I've been having trouble getting the build to pass. I believe we can stop checking-in generated code and can remove the call to verify-go.sh. #173

@jmacd jmacd requested a review from a team July 20, 2020 19:20
@bogdandrutu bogdandrutu merged commit c44400d into open-telemetry:master Jul 20, 2020
@lubingfeng
Copy link

lubingfeng commented Aug 6, 2020

@jmacd When can we get #159 and #171 closed, which are blockers for getting stats proposal finalized, based on discussion two weeks ago.

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.

7 participants