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

[dev.icinga.com #10497] check_memory and check_swap plugins do unit conversion and rounding before percentage calculations resulting in imprecise percentages #3571

Closed
icinga-migration opened this issue Oct 30, 2015 · 6 comments
Labels
bug
Milestone

Comments

@icinga-migration
Copy link
Member

@icinga-migration icinga-migration commented Oct 30, 2015

This issue has been migrated from Redmine: https://dev.icinga.com/issues/10497

Created by pv2b on 2015-10-30 12:59:57 +00:00

Assignee: jflach
Status: Resolved (closed on 2016-02-19 10:30:49 +00:00)
Target Version: 2.4.2
Last Update: 2016-02-23 09:58:17 +00:00 (in Redmine)

Icinga Version: 2.3.11
Backport?: Already backported
Include in Changelog: 1

Several check plugins, including check_memory on windows, and check_swap have a programming error in them, causing them to have imprecise percentage calculations, when large units are used.

For example, a server with 8 GB or memory and 1.452 GB free memory should properly be reporting 18.15% free memory, but is actually reporting 1/8 = 12.5% free memory, if the GB unit is used.

Looking at the source file from here as an example: https://github.com/Icinga/icinga2/blob/f7b5aa33ce2672f220ef285b42eaedfa077a71a7/plugins/check\_memory.cpp

On line 206 and 207, printInfo.tRam and printInfo.aRam (which are 64-bit unsigned integers) are populated with total and available RAM measurements, converted into the desired unit, rounded to the nearest whole number, and then stored as an integer.

Later, on line 166, the percentage is then calculated by dividing printInfo.aRam by printInfo.tRam, resulting in the imprecise calculation mentioned earlier.

As a workaround, a unit should be large enough to provide a sufficient amount of digits for a reasonably precise percentage calculation. MB should be fine, GB is not unless you have servers with very large amounts (>100) of RAM. Even then, users should be aware of the limited precision.

As a solution, the code should be updated to perform a percentage calculation prior to rounding of the values in question.

Attachments

Changesets

2016-02-19 10:30:12 +00:00 by pv2b 5c18c1e

Fixed precision for percentage calculations with large units

The check_memory and check_swap plugins on Windows were incorrectly
rounding the memory/swap measurements to the nearest unit prior to
calculating a percentage. This was causing imprecise percentage
values when the unit selected meant that the values in question had
few significant figures.

fixes #10497

Signed-off-by: Jean Flach <jean-marcel.flach@netways.de>

2016-02-23 08:47:19 +00:00 by pv2b 30f9a1e

Fixed precision for percentage calculations with large units

The check_memory and check_swap plugins on Windows were incorrectly
rounding the memory/swap measurements to the nearest unit prior to
calculating a percentage. This was causing imprecise percentage
values when the unit selected meant that the values in question had
few significant figures.

fixes #10497

Signed-off-by: Jean Flach <jean-marcel.flach@netways.de>
@icinga-migration
Copy link
Member Author

@icinga-migration icinga-migration commented Oct 31, 2015

Updated by pv2b on 2015-10-31 13:38:56 +00:00

I've written up a patch for this and I'll submit it as soon as I figure out the source control...

@icinga-migration
Copy link
Member Author

@icinga-migration icinga-migration commented Oct 31, 2015

Updated by pv2b on 2015-10-31 13:55:56 +00:00

  • File added 0001-Fixed-precision-for-percentage-calculations-with-lar.patch

Here's a patch for the issue in question as promised. While making the patch though, I have realised that it's probably a configuration error to have such a large unit configured in the first place, because it means that your performance data is just as truncated, not just the percentage in the check output.

Ah well, at least I figured out how to set up an Icinga2 development environment for next time. :-) I hope the patch fulfills the specs of the project.

@icinga-migration
Copy link
Member Author

@icinga-migration icinga-migration commented Nov 4, 2015

Updated by jflach on 2015-11-04 10:29:52 +00:00

  • Status changed from New to Assigned
  • Assigned to set to jflach

Hey, thanks for your patch!
I will review it later this or next week, currently we have our hands full with 2.4

@icinga-migration
Copy link
Member Author

@icinga-migration icinga-migration commented Feb 19, 2016

Updated by pv2b on 2016-02-19 10:30:49 +00:00

  • Status changed from Assigned to Resolved
  • Done % changed from 0 to 100

Applied in changeset 5c18c1e.

@icinga-migration
Copy link
Member Author

@icinga-migration icinga-migration commented Feb 19, 2016

Updated by jflach on 2016-02-19 10:32:33 +00:00

  • Target Version set to 2.4.2

I will review it later this or next week
... Or next year :D

@icinga-migration
Copy link
Member Author

@icinga-migration icinga-migration commented Feb 23, 2016

Updated by gbeutner on 2016-02-23 09:58:17 +00:00

  • Backport? changed from Not yet backported to Already backported
@icinga-migration icinga-migration added this to the 2.4.2 milestone Jan 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.