refactor: Tidy up sensor discovery #5550

Merged
merged 17 commits into from Feb 3, 2017

Conversation

Projects
None yet
6 participants
@laf
Member

laf commented Jan 21, 2017

DO NOT DELETE THIS TEXT

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 - please do NOT submit a pull request unless you have (signing the agreement in the same pull request is fine). Your commit message for signing the agreement must appear as per the docs.
  • Have you followed our code guidelines?

Initial start at cleaning up sensor discovery.

We now no longer cycle through all include files and instead only process files that exist for the device group, device os or rfc1628.

Some files have been moved around so they are named correctly after the OS, removed because the code was moved to another file or duplicated code because it needed to run on more than one OS but no shared group.

It may give a slight performance increase because it's not looping through all the files but that wasn't the aim of the PR. It's done so that the code is simplified and not including lots of files.

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Jan 21, 2017

Thank you for submitting a PR @laf! We have found the following @murrant, @tuxis-ie and @crcro based on the history of these files to review this PR.

Thank you for submitting a PR @laf! We have found the following @murrant, @tuxis-ie and @crcro based on the history of these files to review this PR.

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
includes/discovery/functions.inc.php
+function sensors($types, $device, $pre_cache = array())
+{
+ global $config;
+ if (is_array($types)) {

This comment has been minimized.

@murrant

murrant Jan 23, 2017

Member

You can avoid the is_array check by casting $types to array foreach ((array)$types as $type)

@murrant

murrant Jan 23, 2017

Member

You can avoid the is_array check by casting $types to array foreach ((array)$types as $type)

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jan 23, 2017

Member

Updated it

Member

laf commented Jan 23, 2017

Updated it

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
+ include_once $dir . $device['os'] . '.inc.php';
+ }
+ if (isset($config['modules_compat']['rfc1628'][$device['os']]) && $config['modules_compat']['rfc1628'][$device['os']]) {
+ if (is_file($dir . '/rfc1628.inc.php')) {

This comment has been minimized.

@murrant

murrant Jan 24, 2017

Member

This check might not be needed.

Could also avoid the previous is_file() checks if you use @include_once, but I'm not sure which would be better/preferred.

@murrant

murrant Jan 24, 2017

Member

This check might not be needed.

Could also avoid the previous is_file() checks if you use @include_once, but I'm not sure which would be better/preferred.

This comment has been minimized.

@laf

laf Jan 24, 2017

Member

Why can we ignore the checks? the rfc1628 aren't named after groups or os so need calling.

Not sure what you mean by avoid the is_file checks, do you mean just call include_once and not worry about no file existing?

@laf

laf Jan 24, 2017

Member

Why can we ignore the checks? the rfc1628 aren't named after groups or os so need calling.

Not sure what you mean by avoid the is_file checks, do you mean just call include_once and not worry about no file existing?

This comment has been minimized.

@murrant

murrant Jan 24, 2017

Member

I mean is_file(rfc1628.inc.php) will always be true.

And yes ignore when the file doesn't exist, it won't be included then. Not sure which way I prefer.

@murrant

murrant Jan 24, 2017

Member

I mean is_file(rfc1628.inc.php) will always be true.

And yes ignore when the file doesn't exist, it won't be included then. Not sure which way I prefer.

This comment has been minimized.

@laf

laf Jan 24, 2017

Member

Ah, well rfc1628 doesn't exist in all sensor types hence the checks still.

@laf

laf Jan 24, 2017

Member

Ah, well rfc1628 doesn't exist in all sensor types hence the checks still.

@@ -0,0 +1,37 @@
+<?php
+
+$oids = snmp_walk($device, '.1.3.6.1.4.1.10876.2.1.1.1.1.3', '-OsqnU', 'SUPERMICRO-HEALTH-MIB');

This comment has been minimized.

@murrant

murrant Jan 24, 2017

Member

Supermicro is a bit of an oddball because it should be supported on windows too, but it probably shouldn't be it's on os. Perhaps move to supermicro.inc.php and include in windows and linux?

@murrant

murrant Jan 24, 2017

Member

Supermicro is a bit of an oddball because it should be supported on windows too, but it probably shouldn't be it's on os. Perhaps move to supermicro.inc.php and include in windows and linux?

@@ -15,3 +15,59 @@
discover_sensor($valid['sensor'], 'temperature', $device, $sensor_oid, 1, $sensor_type, $descr, 1, 1, null, null, null, null, $value);
}
}
+
+echo 'HP_ILO ';

This comment has been minimized.

@murrant

murrant Jan 24, 2017

Member

It might be a good idea to retain the if (startswith($sysObjectId, '.1.3.6.1.4.1.232.') ) { check here.

@murrant

murrant Jan 24, 2017

Member

It might be a good idea to retain the if (startswith($sysObjectId, '.1.3.6.1.4.1.232.') ) { check here.

+}
+
+// Supermicro sensors
+$oids = snmp_walk($device, '.1.3.6.1.4.1.10876.2.1.1.1.1.3', '-Osqn', 'SUPERMICRO-HEALTH-MIB');

This comment has been minimized.

@murrant

murrant Jan 24, 2017

Member

I know it is probably out of scope for this PR, but this should probably be in pre-cache.

@murrant

murrant Jan 24, 2017

Member

I know it is probably out of scope for this PR, but this should probably be in pre-cache.

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jan 24, 2017

Member

Updated

Member

laf commented Jan 24, 2017

Updated

+ discover_sensor($valid['sensor'], 'temperature', $device, $temperature_oid, $oid, 'hpilo', $descr, '1', '1', null, null, null, $threshold, $temperature);
+ }
+ }
+ }

This comment has been minimized.

@murrant

murrant Jan 24, 2017

Member

I think you are missing the include supermicro.inc.php in this file.

@murrant

murrant Jan 24, 2017

Member

I think you are missing the include supermicro.inc.php in this file.

This comment has been minimized.

@laf

laf Jan 24, 2017

Member

Yeah put it in the supermicro one instead lol :(

@laf

laf Jan 24, 2017

Member

Yeah put it in the supermicro one instead lol :(

@@ -15,3 +15,30 @@
discover_sensor($valid['sensor'], 'temperature', $device, $sensor_oid, 1, $sensor_type, $descr, 1, 1, null, null, null, null, $value);
}
}
+
+if (startswith($sysObjectId, '.1.3.6.1.4.1.232.')) {

This comment has been minimized.

@murrant

murrant Jan 25, 2017

Member

Should be starts_with(

@murrant

murrant Jan 25, 2017

Member

Should be starts_with(

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jan 25, 2017

Member

Updated

Member

laf commented Jan 25, 2017

Updated

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jan 25, 2017

Member

Rebasing and pushing update now.

Member

laf commented Jan 25, 2017

Rebasing and pushing update now.

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jan 26, 2017

Member

Whilst it's not big deal, rfc1628.inc.php doesn't exist for all sensor modules so for devices which support it they will try and include non-existent files.

Member

laf commented Jan 26, 2017

Whilst it's not big deal, rfc1628.inc.php doesn't exist for all sensor modules so for devices which support it they will try and include non-existent files.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Jan 26, 2017

Member

@laf oh, I had a slight misunderstanding on rfc1628 stuff. Give me a bit to sort it out.

Member

murrant commented Jan 26, 2017

@laf oh, I had a slight misunderstanding on rfc1628 stuff. Give me a bit to sort it out.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Jan 26, 2017

Member

Ok, let me do one more testing pass and this should be good to merge.

Member

murrant commented Jan 26, 2017

Ok, let me do one more testing pass and this should be good to merge.

@Rosiak

This comment has been minimized.

Show comment
Hide comment
@Rosiak

Rosiak Jan 26, 2017

Contributor

Lemme test this one.

Contributor

Rosiak commented Jan 26, 2017

Lemme test this one.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jan 26, 2017

Member

I'm rebasing this now, we ok to merge?

Member

laf commented Jan 26, 2017

I'm rebasing this now, we ok to merge?

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jan 30, 2017

Member

I've rebased this.

Member

laf commented Jan 30, 2017

I've rebased this.

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jan 30, 2017

Member

Thanks to @Rosiak for spotting the last issue, fix now pushed so should be good to go @librenms/reviewers

Member

laf commented Jan 30, 2017

Thanks to @Rosiak for spotting the last issue, fix now pushed so should be good to go @librenms/reviewers

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Jan 30, 2017

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

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

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Feb 2, 2017

Member

Anything further on this one?

Member

laf commented Feb 2, 2017

Anything further on this one?

@murrant murrant merged commit d4e8c54 into librenms:master Feb 3, 2017

2 checks passed

Auto-Deploy Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@laf laf deleted the laf:sensor-discovery-tidyup branch Feb 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment