fix: apc load, runtime and current sensors #4780

Merged
merged 3 commits into from Oct 13, 2016

Projects

None yet

5 participants

@crcro
Contributor
crcro commented Oct 12, 2016 edited

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

  • Have you signed the Contributors agreement
  • Have you followed our code guidelines?
  • fix in functions for ignoring poller.php values
  • added default 80 limit for load sensors
  • fixes discovery for apc ups sensors: load, runtime, current (and default alerts for them)
includes/polling/functions.inc.php
@@ -45,10 +45,10 @@ function poll_sensor($device, $class, $unit)
if (file_exists('includes/polling/sensors/'. $class .'/'. $device['os'] .'.inc.php')) {
require_once 'includes/polling/sensors/'. $class .'/'. $device['os'] .'.inc.php';
+ } else {
+ $sensor_value = trim(str_replace('"', '', $snmp_data[$sensor['sensor_oid']]));
@laf
laf Oct 12, 2016 Member

We shouldn't need to move that to here, it can be left generically below as no sensor value should have a " in it.

@crcro
crcro Oct 12, 2016 Contributor

that line was just put in the else statement, it was like that originally :)

+ * the source code distribution for details.
+ */
+
+$sensor_value = snmp_get($device, $sensor['sensor_oid'], '-OUqnvt');
@laf
laf Oct 12, 2016 Member

Any reason we need to call snmpget here when we will have already done so (with different flags).

@crcro
crcro Oct 12, 2016 Contributor

that "t" flag does the trick, without it the sensor return time in format: 0:0:13:00, with that one we got time-ticks, which are used on discovery process (and also all the divisors/multipliers are based on time-ticks)

@murrant
murrant Oct 13, 2016 Contributor

I'd suggest changing the generic code and adding -Ot there is no reason we would want a human readable format for any time based sensor.

@laf
Member
laf commented Oct 12, 2016

In general it seems ok although people have reported the charge file is not working and no change was done here.

@@ -239,14 +238,15 @@
echo $item['type'].' '.$item['mib'].' UPS';
}
- if (stristr($current_oid, 'HighPrec')) {
- $current = ($oids / $item['divisor']);
+ if ($is_highprec === true) {
@murrant
murrant Oct 12, 2016 Contributor

I think it is a bit silly to set a bool then if else later.
Just move the code to set $current up to the if/else where is_highprec is set.

Actually, probably the default divisor should be 1 and set to 10 and divide the current value for high precision

@murrant
Contributor
murrant commented Oct 13, 2016

I just pushed a change to pull this back a bit. Testing would be appreciated.

@scrutinizer-notifier

The inspection completed: 1 new issues, 1 updated code elements

@murrant murrant referenced this pull request Oct 13, 2016
Merged

refactor: Improve sensors polling for performance increase #4725

2 of 2 tasks complete
@Rosiak Rosiak merged commit b867d3d into librenms:master Oct 13, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@crcro crcro deleted the crcro:fix-apc-sensors branch Oct 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment