This repository has been archived by the owner on Jul 27, 2023. It is now read-only.
Prevent uint underflows when calculating rates and percentages #201
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
leeavital
reviewed
Oct 30, 2018
| @@ -159,9 +159,14 @@ func calculateCtrPct(cur, prev, sys2, sys1 uint64, numCPU int, before time.Time) | |||
| return 0 | |||
| } | |||
|
|
|||
| // Prevent uint underflows | |||
| if prev > cur || sys1 > sys2 { | |||
There was a problem hiding this comment.
Should we be returning a negative percent here instead of zero?
There was a problem hiding this comment.
Spoke with Kevin offline, and I misunderstood what this was supposed to catch.
Could this be more clearly written as:
if cur == 0 || sys2 == 0 {
return 0
}
Since zero is a value we're treating specially?
There was a problem hiding this comment.
For this case yes. Given that the agent is more difficult to upgrade in the field, I'd prefer to be a bit defensive here though and guard against underflows in the case that the current values are non-zero and smaller than the previous ones which could occur in some future unforeseen circumstance.
sunhay
approved these changes
Oct 31, 2018
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR addresses the scenario where some metrics were being reported as impossibly large values.
The cause here was unsigned integer underflow as when we calculate the "rate" for a given metric, we calculate
(current - previous) / elapsed time in seconds. In our case,currentandpreviousare usually unsigned integers so ifprevious > current, the subtraction operation will overflow.This usually only occurs in corner cases where the container metrics were not able to be collected for some reason, possibly because the container is shutting down.