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

fix(prometheus): Handle +Inf/-Inf from Prometheus #707

Merged
merged 4 commits into from
Apr 29, 2020

Conversation

igcherkaev
Copy link
Contributor

  • fix(prometheus): Prometheus can return "+Inf" and "-Inf" as values for metrics. This fixes java.lang.NumberFormatException in such cases.

Prometheus can return not only "NaN", but also "+Inf" and "-Inf" as values for metrics. Handle it properly and not crash if received such.
@spinnakerbot
Copy link
Contributor

The following commits need their title changed:

  • 68be830: Handle +Inf/-Inf from prometheus

Please format your commit title into the form:

<type>(<scope>): <subject>, e.g. fix(kubernetes): address NPE in status check

This allows us to easily generate changelogs & determine semantic version numbers when cutting releases. You can read more about commit conventions here.

Prometheus can return not only "NaN", but also "+Inf" and "-Inf" as values for metrics. Handle it properly and not crash if received such.
@plumpy plumpy requested a review from fieldju April 17, 2020 18:45
Copy link
Member

@plumpy plumpy left a comment

Choose a reason for hiding this comment

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

LGTM, but I'll leave it up to Justin to merge...

Comment on lines +85 to +87
case "NaN":
dataValues.add(Double.NaN);
break;
Copy link
Member

Choose a reason for hiding this comment

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

It looks like Double.valueOf should handle this case already, although perhaps it's more clear to spell it out here anyway? It wants "Infinity" instead of "Inf", unfortunately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I thought about it, but decided to add it in case if they change it in Java in the future for any reason. It's like, you either track how NaN is spelled by Prometheus and at the same time ensure that Java parser expects the same, or you just map it explicitly to the constant of the Double class.

@igcherkaev
Copy link
Contributor Author

If Justin @fieldju finds it good, could you also cherry-pick it to the existing supported releases?

@emptywee
Copy link

@fieldju Justin, please, review? :) Thank you!

@fieldju
Copy link
Collaborator

fieldju commented Apr 29, 2020

Justin got back from paternity leave checking this out now.

@fieldju fieldju self-assigned this Apr 29, 2020
@fieldju fieldju merged commit e8737fa into spinnaker:master Apr 29, 2020
@fieldju
Copy link
Collaborator

fieldju commented Apr 29, 2020

If Justin @fieldju finds it good, could you also cherry-pick it to the existing supported releases?

@igcherkaev, You can do that yourself with the bot: https://www.spinnaker.io/community/contributing/releasing/#cherry-pick-using-mergify

@emptywee
Copy link

Justin got back from paternity leave checking this out now.

Congratulations! And welcome back! :)

@igcherkaev
Copy link
Contributor Author

@Mergifyio backport release-1.18.x

@igcherkaev
Copy link
Contributor Author

@Mergifyio backport release-1.19.x

@mergify
Copy link
Contributor

mergify bot commented Apr 29, 2020

@igcherkaev is not allowed to run commands

@fieldju
Copy link
Collaborator

fieldju commented Apr 29, 2020

@Mergifyio backport release-1.18.x

@fieldju
Copy link
Collaborator

fieldju commented Apr 29, 2020

@Mergifyio backport release-1.19.x

mergify bot pushed a commit that referenced this pull request Apr 29, 2020
* Handle +Inf/-Inf from prometheus

Prometheus can return not only "NaN", but also "+Inf" and "-Inf" as values for metrics. Handle it properly and not crash if received such.

* fix(prometheus): Handle +Inf/-Inf as values from prometheus

Prometheus can return not only "NaN", but also "+Inf" and "-Inf" as values for metrics. Handle it properly and not crash if received such.

Co-authored-by: Justin Field <justin.field@armory.io>
(cherry picked from commit e8737fa)
@mergify
Copy link
Contributor

mergify bot commented Apr 29, 2020

Command backport release-1.18.x: success

Backports have been created

mergify bot pushed a commit that referenced this pull request Apr 29, 2020
* Handle +Inf/-Inf from prometheus

Prometheus can return not only "NaN", but also "+Inf" and "-Inf" as values for metrics. Handle it properly and not crash if received such.

* fix(prometheus): Handle +Inf/-Inf as values from prometheus

Prometheus can return not only "NaN", but also "+Inf" and "-Inf" as values for metrics. Handle it properly and not crash if received such.

Co-authored-by: Justin Field <justin.field@armory.io>
(cherry picked from commit e8737fa)
@mergify
Copy link
Contributor

mergify bot commented Apr 29, 2020

Command backport release-1.19.x: success

Backports have been created

@emptywee
Copy link

WoOT WOoT! Way to go, bot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants