Skip to content

add default values for sums of apiserver_request:availability30d #1038

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

Merged
merged 3 commits into from
Mar 24, 2025

Conversation

Mx7ca
Copy link
Contributor

@Mx7ca Mx7ca commented Mar 13, 2025

This PR should fix #1037 and in the end also fix prometheus-community/helm-charts#5407

More information can be found in prometheus-community/helm-charts#5415 where the change has been explained in more detail

@Mx7ca Mx7ca requested review from povilasv and skl as code owners March 13, 2025 18:54
@lorenzofelletti
Copy link
Contributor

Hi! I see what you're trying to achieve with this PR, and I agree with you that fallbacks value vector(0) should be provided in case there are no slow writes, etc.

However, sometimes no data is the actually correct answer, when the value for the expression is actually impossible to calculate.

Thus, I would advise for some default values you added in this PR to be removed.

@Mx7ca
Copy link
Contributor Author

Mx7ca commented Mar 22, 2025

@lorenzofelletti I've removed some of the defaults again. Especially when something was subtracted, I've made sure that the subtrahend will not be calculated before the minuend (see subtraction-terms) because otherwise it could result in

0 (default) - X

and therefore result in a misleading difference.

Could you please check it out again?

Copy link
Contributor

@lorenzofelletti lorenzofelletti left a comment

Choose a reason for hiding this comment

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

Thank you @Mx7ca ! I think this is a very valuable contribution!

Copy link
Collaborator

@skl skl left a comment

Choose a reason for hiding this comment

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

Thanks all for review and discussion

@skl skl merged commit 5894150 into kubernetes-monitoring:master Mar 24, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants