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

feature: Added support for sensors to be discovered from yaml #6859

Merged
merged 12 commits into from Jun 26, 2017

Conversation

Projects
None yet
8 participants
@laf
Member

laf commented Jun 17, 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.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926

Just pushing, nothing more right now.

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Jun 17, 2017

Thank you for submitting a PR @laf! We have found the following @Rosiak based on the history of these files to review this PR.

mention-bot commented Jun 17, 2017

Thank you for submitting a PR @laf! We have found the following @Rosiak based on the history of these files to review this PR.

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented Jun 17, 2017

Auto-Deploy finished, Test PR at http://6859.ci.librenms.org or https://6859.ci.librenms.org

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented Jun 17, 2017

Auto-Deploy finished, Test PR at http://6859.ci.librenms.org or https://6859.ci.librenms.org

laf added some commits Jun 18, 2017

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented Jun 18, 2017

Auto-Deploy finished, Test PR at http://6859.ci.librenms.org or https://6859.ci.librenms.org

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jun 18, 2017

Member

Ok this is open for feedback now.

@murrant I've removed the oid_name section in the last commit that you don't like :P

Member

laf commented Jun 18, 2017

Ok this is open for feedback now.

@murrant I've removed the oid_name section in the last commit that you don't like :P

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented Jun 18, 2017

Auto-Deploy finished, Test PR at http://6859.ci.librenms.org or https://6859.ci.librenms.org

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Jun 19, 2017

Member

Looks amazing laf :)

Member

murrant commented Jun 19, 2017

Looks amazing laf :)

@FTBZ

This comment has been minimized.

Show comment
Hide comment
@FTBZ

FTBZ Jun 19, 2017

Contributor

YAML type is working only for OIDs in table or can be used for all kinds?

Contributor

FTBZ commented Jun 19, 2017

YAML type is working only for OIDs in table or can be used for all kinds?

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jun 19, 2017

Member

How do you mean @FTBZ?

Member

laf commented Jun 19, 2017

How do you mean @FTBZ?

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jun 19, 2017

Member

Do you mean can it be used for OIDs that aren't in a table? No not at the moment. If you have an example device that this doesn't work on then I can look at seeing what we can do.

Member

laf commented Jun 19, 2017

Do you mean can it be used for OIDs that aren't in a table? No not at the moment. If you have an example device that this doesn't work on then I can look at seeing what we can do.

@FTBZ

This comment has been minimized.

Show comment
Hide comment
@FTBZ

FTBZ Jun 19, 2017

Contributor

Exactly, I have already had cases where the OIDs of sensors are not in tables but I do not remember which. Good to know for further integration.

Contributor

FTBZ commented Jun 19, 2017

Exactly, I have already had cases where the OIDs of sensors are not in tables but I do not remember which. Good to know for further integration.

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented Jun 19, 2017

Auto-Deploy finished, Test PR at http://6859.ci.librenms.org or https://6859.ci.librenms.org

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Jun 19, 2017

Member

If you snmpwalk a single item, doesn't basically return a single getnext?

Member

murrant commented Jun 19, 2017

If you snmpwalk a single item, doesn't basically return a single getnext?

Show outdated Hide outdated includes/discovery/sensors.inc.php
foreach ($data_array as $data) {
foreach ((array)$data['oid'] as $oid) {
$tmp_name = $data['oid'];
$pre_cache[$tmp_name] = snmpwalk_cache_oid($device, $oid, $pre_cache[$tmp_name], $device['dynamic_discovery']['mib'], null, '-OeQUs');

This comment has been minimized.

@murrant

murrant Jun 19, 2017

Member

You should check if the oid has already been fetched here.

@murrant

murrant Jun 19, 2017

Member

You should check if the oid has already been fetched here.

This comment has been minimized.

@murrant

murrant Jun 19, 2017

Member

And send an empty array to the walk function.

@murrant

murrant Jun 19, 2017

Member

And send an empty array to the walk function.

This comment has been minimized.

@laf

laf Jun 19, 2017

Member

Done.

Walking a single entry is ok but I'm not sure how well the code will work against that yet. I've added support for passing a description as a string as well as saying which key in the array contains the description so it's possibly ok with single OIDs.

@laf

laf Jun 19, 2017

Member

Done.

Walking a single entry is ok but I'm not sure how well the code will work against that yet. I've added support for passing a description as a string as well as saying which key in the array contains the description so it's possibly ok with single OIDs.

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented Jun 19, 2017

Auto-Deploy finished, Test PR at http://6859.ci.librenms.org or https://6859.ci.librenms.org

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jun 19, 2017

Member

Rebased. Should pass travis now.

Member

laf commented Jun 19, 2017

Rebased. Should pass travis now.

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented Jun 19, 2017

Auto-Deploy finished, Test PR at http://6859.ci.librenms.org or https://6859.ci.librenms.org

@maciejkaczkowski

This comment has been minimized.

Show comment
Hide comment
@maciejkaczkowski

maciejkaczkowski Jun 20, 2017

Contributor
  1. Is possible to add {{ $index }} to descr? I have device without descr of ports:
    .1.3.6.1.4.1.11195.2.1.10.2.1.12.1 = INTEGER: -68
    .1.3.6.1.4.1.11195.2.1.10.2.1.12.2 = INTEGER: -57
    .1.3.6.1.4.1.11195.2.1.11.2.1.27.1 = INTEGER: 100
    .1.3.6.1.4.1.11195.2.1.11.2.1.27.2 = INTEGER: 90

Tuner Input Level 1 = -68
Tuner Input Level 2 = -57
Modulator Output Level 1 = 100
Modulator Output Level 2 = 90

  1. Is possible to add option to skip if empty or skip if value = xxx?
Contributor

maciejkaczkowski commented Jun 20, 2017

  1. Is possible to add {{ $index }} to descr? I have device without descr of ports:
    .1.3.6.1.4.1.11195.2.1.10.2.1.12.1 = INTEGER: -68
    .1.3.6.1.4.1.11195.2.1.10.2.1.12.2 = INTEGER: -57
    .1.3.6.1.4.1.11195.2.1.11.2.1.27.1 = INTEGER: 100
    .1.3.6.1.4.1.11195.2.1.11.2.1.27.2 = INTEGER: 90

Tuner Input Level 1 = -68
Tuner Input Level 2 = -57
Modulator Output Level 1 = 100
Modulator Output Level 2 = 90

  1. Is possible to add option to skip if empty or skip if value = xxx?
@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jun 20, 2017

Member

I've added support for using index in descr.

Can you give me an example on what you'd want to skip?

Member

laf commented Jun 20, 2017

I've added support for using index in descr.

Can you give me an example on what you'd want to skip?

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented Jun 20, 2017

Auto-Deploy finished, Test PR at http://6859.ci.librenms.org or https://6859.ci.librenms.org

@maciejkaczkowski

This comment has been minimized.

Show comment
Hide comment
@maciejkaczkowski

maciejkaczkowski Jun 21, 2017

Contributor

Thank you.
For example Arris C4, if card is not inserted to chassis then value is 999
https://github.com/librenms/librenms/blob/master/includes/discovery/sensors/temperature/cmts.inc.php

Contributor

maciejkaczkowski commented Jun 21, 2017

Thank you.
For example Arris C4, if card is not inserted to chassis then value is 999
https://github.com/librenms/librenms/blob/master/includes/discovery/sensors/temperature/cmts.inc.php

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jun 22, 2017

Member

@maciejkaczkowski I've added that support:

skip_values:

Member

laf commented Jun 22, 2017

@maciejkaczkowski I've added that support:

skip_values:

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jun 26, 2017

Member

This should be good for a merge now imho.

Member

laf commented Jun 26, 2017

This should be good for a merge now imho.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Jun 26, 2017

Member

Looks good to me laf, I plan to try to convert one of the devices I have, but we don't have to wait on me to merge.

Member

murrant commented Jun 26, 2017

Looks good to me laf, I plan to try to convert one of the devices I have, but we don't have to wait on me to merge.

@Rosiak

This comment has been minimized.

Show comment
Hide comment
@Rosiak

Rosiak Jun 26, 2017

Contributor

Almost there!
However it only adds one of the multiple sensors under otherStateSensorTable:
Working
screen shot 2017-06-26 at 20 00 14
This PR:
screen shot 2017-06-26 at 20 00 20

Output from SNMP from this table:

otherStateSensorId.513106929 = nbAlinkEnc_0_3_SPLK
otherStateSensorId.2637721517 = nbAlinkEnc_0_4_SMOK
otherStateSensorId.3502248167 = nbLinkStatus_eth0
otherStateSensorId.3823829717 = nbAlinkPowerStatus_can0
otherStateSensorValue.513106929 = 0
otherStateSensorValue.2637721517 = 0
otherStateSensorValue.3502248167 = 1
otherStateSensorValue.3823829717 = 0
otherStateSensorErrorStatus.513106929 = 0
otherStateSensorErrorStatus.2637721517 = 0
otherStateSensorErrorStatus.3502248167 = 0
otherStateSensorErrorStatus.3823829717 = 0
otherStateSensorLabel.513106929 = Spot leak  (3)
otherStateSensorLabel.2637721517 = Smoke  (4)
otherStateSensorLabel.3502248167 = Ethernet Link Status
otherStateSensorLabel.3823829717 = A-Link Bus Power
otherStateSensorEncId.513106929 = nbAlinkEnc_0
otherStateSensorEncId.2637721517 = nbAlinkEnc_0
otherStateSensorEncId.3502248167 = nbBaseEnclosure
otherStateSensorEncId.3823829717 = nbBaseEnclosure
otherStateSensorPortId.513106929 =
otherStateSensorPortId.2637721517 =
otherStateSensorPortId.3502248167 =
otherStateSensorPortId.3823829717 =
otherStateSensorValueStr.513106929 = No Leak
otherStateSensorValueStr.2637721517 = No Smoke
otherStateSensorValueStr.3502248167 = Up
otherStateSensorValueStr.3823829717 = OK
otherStateSensorEntry.8.513106929 = 513106929
otherStateSensorEntry.8.2637721517 = 2637721517
otherStateSensorEntry.8.3502248167 = 3502248167
otherStateSensorEntry.8.3823829717 = 3823829717
Contributor

Rosiak commented Jun 26, 2017

Almost there!
However it only adds one of the multiple sensors under otherStateSensorTable:
Working
screen shot 2017-06-26 at 20 00 14
This PR:
screen shot 2017-06-26 at 20 00 20

Output from SNMP from this table:

otherStateSensorId.513106929 = nbAlinkEnc_0_3_SPLK
otherStateSensorId.2637721517 = nbAlinkEnc_0_4_SMOK
otherStateSensorId.3502248167 = nbLinkStatus_eth0
otherStateSensorId.3823829717 = nbAlinkPowerStatus_can0
otherStateSensorValue.513106929 = 0
otherStateSensorValue.2637721517 = 0
otherStateSensorValue.3502248167 = 1
otherStateSensorValue.3823829717 = 0
otherStateSensorErrorStatus.513106929 = 0
otherStateSensorErrorStatus.2637721517 = 0
otherStateSensorErrorStatus.3502248167 = 0
otherStateSensorErrorStatus.3823829717 = 0
otherStateSensorLabel.513106929 = Spot leak  (3)
otherStateSensorLabel.2637721517 = Smoke  (4)
otherStateSensorLabel.3502248167 = Ethernet Link Status
otherStateSensorLabel.3823829717 = A-Link Bus Power
otherStateSensorEncId.513106929 = nbAlinkEnc_0
otherStateSensorEncId.2637721517 = nbAlinkEnc_0
otherStateSensorEncId.3502248167 = nbBaseEnclosure
otherStateSensorEncId.3823829717 = nbBaseEnclosure
otherStateSensorPortId.513106929 =
otherStateSensorPortId.2637721517 =
otherStateSensorPortId.3502248167 =
otherStateSensorPortId.3823829717 =
otherStateSensorValueStr.513106929 = No Leak
otherStateSensorValueStr.2637721517 = No Smoke
otherStateSensorValueStr.3502248167 = Up
otherStateSensorValueStr.3823829717 = OK
otherStateSensorEntry.8.513106929 = 513106929
otherStateSensorEntry.8.2637721517 = 2637721517
otherStateSensorEntry.8.3502248167 = 3502248167
otherStateSensorEntry.8.3823829717 = 3823829717
@Rosiak

Just a tiny comment, then I think we're good to go!

Show outdated Hide outdated doc/Developing/Sensor-State-Support.md
- { descr: motionDetected, graph: 0, value: 1, generic: 2 }
-
oid: otherStateSensorTable
value: otherStateSensorValue

This comment has been minimized.

@Rosiak

Rosiak Jun 26, 2017

Contributor

I believe the correct name is:
otherStateSensorErrorStatus

@Rosiak

Rosiak Jun 26, 2017

Contributor

I believe the correct name is:
otherStateSensorErrorStatus

This comment has been minimized.

@laf

laf Jun 26, 2017

Member

Updated, don't think it matters though.

@laf

laf Jun 26, 2017

Member

Updated, don't think it matters though.

Show outdated Hide outdated doc/Developing/Sensor-State-Support.md
num_oid: .1.3.6.1.4.1.5528.100.4.2.10.1.3.
descr: otherStateSensorLabel
index: '{{ $index }}'
state_name: otherStateSensorErrorStatus

This comment has been minimized.

@Rosiak

Rosiak Jun 26, 2017

Contributor

How about otherStateSensor?

@Rosiak

Rosiak Jun 26, 2017

Contributor

How about otherStateSensor?

This comment has been minimized.

@laf

laf Jun 26, 2017

Member

It's how it is in the old code so replicating that.

@laf

laf Jun 26, 2017

Member

It's how it is in the old code so replicating that.

Show outdated Hide outdated includes/definitions/discovery/netbotz.yaml
- { descr: motionDetected, graph: 0, value: 1, generic: 2 }
-
oid: otherStateSensorTable
value: otherStateSensorValue

This comment has been minimized.

@Rosiak

Rosiak Jun 26, 2017

Contributor

I believe the correct name is:
otherStateSensorErrorStatus

@Rosiak

Rosiak Jun 26, 2017

Contributor

I believe the correct name is:
otherStateSensorErrorStatus

Show outdated Hide outdated includes/definitions/discovery/netbotz.yaml
num_oid: .1.3.6.1.4.1.5528.100.4.2.10.1.3.
descr: otherStateSensorLabel
index: '{{ $index }}'
state_name: otherStateSensorErrorStatus

This comment has been minimized.

@Rosiak

Rosiak Jun 26, 2017

Contributor

How about otherStateSensor?

@Rosiak

Rosiak Jun 26, 2017

Contributor

How about otherStateSensor?

@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Jun 26, 2017

The inspection completed: No new issues

scrutinizer-notifier commented Jun 26, 2017

The inspection completed: No new issues

@laf laf merged commit 5e32474 into librenms:master Jun 26, 2017

2 of 3 checks passed

Auto-Deploy Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@laf laf deleted the laf:feature/yaml-for-discovery branch Jun 26, 2017

@lock

This comment has been minimized.

Show comment
Hide comment
@lock

lock bot May 17, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

lock bot commented May 17, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

@lock lock bot locked as resolved and limited conversation to collaborators May 17, 2018

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