Skip to content

Set quantile value to NAN if there are no samples provided for an age #305

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Sep 23, 2021
Merged

Set quantile value to NAN if there are no samples provided for an age #305

merged 4 commits into from
Sep 23, 2021

Conversation

DifferentialOrange
Copy link
Member

@DifferentialOrange DifferentialOrange commented Sep 17, 2021 •
edited by yngvar-antonsson
Loading

Closes #303

I didn't forget about

  • Tests
  • Changelog
  • Documentation (README and rst)
  • Rockspec and rpm spec

Set quantile value to NAN if there are no samples provided for an age.

Golang summary collector (link provided by Prometheus doc) uses NaN to set undefined quantile values. The decision was inpired by Golang solution.

@DifferentialOrange DifferentialOrange force-pushed the 303-quantile-for-zero-samples branch from 9883725 to 4b04408 Compare September 17, 2021 18:14
yngvar-antonsson
yngvar-antonsson reviewed Sep 19, 2021
View reviewed changes
@DifferentialOrange
Copy link
Member Author

DifferentialOrange commented Sep 20, 2021 •
edited
Loading

Seems fine
image
image

@DifferentialOrange
Copy link
Member Author

We should see how nil values are processed in Go collector quantiles since nil is not a part of Prometheus standard https://pkg.go.dev/github.com/prometheus/client_golang/prometheus#Summary

@DifferentialOrange
Copy link
Member Author

image
image

Moreover,
image
https://www.robustperception.io/get-thee-to-a-nannary

@DifferentialOrange DifferentialOrange force-pushed the 303-quantile-for-zero-samples branch 2 times, most recently from 5ba3633 to d85c203 Compare September 21, 2021 07:44
@DifferentialOrange
Copy link
Member Author

Nan version:
image
image

@DifferentialOrange
Copy link
Member Author

image
image

@DifferentialOrange DifferentialOrange force-pushed the 303-quantile-for-zero-samples branch from d85c203 to 23d8391 Compare September 21, 2021 09:36
@DifferentialOrange DifferentialOrange changed the title Set quantile value to nil if there are no samples provided for an age Set quantile value to NAN if there are no samples provided for an age Sep 21, 2021
@DifferentialOrange DifferentialOrange marked this pull request as ready for review September 21, 2021 09:40
@yngvar-antonsson
Copy link
Collaborator

@DifferentialOrange please look at my changes

@DifferentialOrange
Copy link
Member Author

DifferentialOrange commented Sep 23, 2021 •
edited
Loading

@DifferentialOrange please look at my changes

@yngvar-antonsson , thank you for your contribution. Everything seems fine except for test name/absence of comments. It's not that easy to understand what behavior is tested if you are not a developer of this quantile. I think it can be improved after merging, but I'm not insist on it.

@yngvar-antonsson yngvar-antonsson merged commit ce867b1 into master Sep 23, 2021
@yngvar-antonsson yngvar-antonsson deleted the 303-quantile-for-zero-samples branch September 23, 2021 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rosik rosik rosik approved these changes

@yngvar-antonsson yngvar-antonsson yngvar-antonsson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Summary quantiles respond +Inf if rps decreases to zero
3 participants
@DifferentialOrange @yngvar-antonsson @rosik