-
Notifications
You must be signed in to change notification settings - Fork 179
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
prometheus: prevent panic when incrmenting counter #146
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@lgfa29 as an alternative because this seems like such a weird case and a programmer error rather than a runtime error, what would you think about shipping a version of Nomad pinned to this branch to see if we can get reports on the error to fix the underlying cause? |
Ah good idea. I will open a PR with this dependency update on Nomad tomorrow. I will probably rebase this as branch to |
The Prometheus Counter.Add() method panics if the value being added is negative. Even if care is taken by consumers to never pass a negative value the panic could still happen due to float conversion or overflow. This change prevents go-metrics from causing consumers to crash.
9c4352b
to
d9ca9af
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for confirming this can occur in unexpected ways especially since we are working with floats here!
I think if we've had real-world pain caused, it makes sense to be more defensive here.
Update `go-metrics` to v0.5.3 to pick hashicorp/go-metrics#146.
Update `go-metrics` to v0.5.3 to pick hashicorp/go-metrics#146.
Update `go-metrics` to v0.5.3 to pick hashicorp/go-metrics#146.
Update `go-metrics` to v0.5.3 to pick hashicorp/go-metrics#146.
Update `go-metrics` to v0.5.3 to pick hashicorp/go-metrics#146.
The Prometheus Counter.Add() method panics if the value being added is negative. Even if care is taken by consumers to never pass a negative value the panic could still happen due to float conversion or overflow.
This change prevents go-metrics from causing consumers to crash.
A panic like this has been reported in hashicorp/nomad#15861, but, as far as I can tell, Nomad only calls
IncrCounter
andIncrCounterWithLabels
using positive values.