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

Allow num_oid to use OCTET STRING indexes #10410

Merged
merged 3 commits into from Aug 21, 2019

Conversation

@PipoCanaja
Copy link
Contributor

commented Jul 4, 2019

Hi,

This PR creates a new variable $index_string that contains the OID converted as OCTET_STRING. basically, it converts a string index into an OID, so it can be used for snmpgets.

Exemple for an efficient_ip appliance, which exposes DNS/DHCP statistics indexed by name :

.1.3.6.1.4.1.2440.1.4.2.3.1.3.3.116.99.112 = 0
.1.3.6.1.4.1.2440.1.4.2.3.1.3.3.117.100.112 = 72

it represents :

$ snmptranslate '-M' '/opt/librenms/mibs:/opt/librenms/mibs/efficientip' -m all .1.3.6.1.4.1.2440.1.4.2.3.1.3.3.116.99.112
EIP-STATS-MIB::eipDnsStatValue."tcp"
$ snmptranslate '-M' '/opt/librenms/mibs:/opt/librenms/mibs/efficientip' -m all .1.3.6.1.4.1.2440.1.4.2.3.1.3.3.117.100.112
EIP-STATS-MIB::eipDnsStatValue."udp"

In the YAML file, it is now possible to do the following for DNS stats :

                    oid: eipDnsStatTable
                    value: eipDnsStatValue
                    num_oid: '.1.3.6.1.4.1.2440.1.4.2.3.1.3.{{ $index_string }}'
                    descr: '{{ $eipDnsStatName }}'
                    group: 'DNS'

#10413 is also necessary in order to load the COUNTER values and graph them correctly. Nobody cares of the absolut number of DNS requests (RRD GAUGE), but prefers the RRD COUNTER which derives it.
Capture d’écran 2019-07-04 à 17 03 31

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
After you are done testing, you can remove the changes with ./scripts/github-remove. If there are schema changes, you can ask on discord how to revert.

@PipoCanaja PipoCanaja added the Discovery label Jul 4, 2019

@PipoCanaja PipoCanaja added this to the 1.54 milestone Jul 4, 2019

@PipoCanaja

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

PR for extended support of efficient IP devices will come after a 2nd PR adding a DERIVE support for the 'count' sensor.

@murrant

This comment has been minimized.

Copy link
Member

commented Jul 7, 2019

Should we be forcing it to use numeric instead?

@PipoCanaja

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2019

I can think of one corner case at least: Let's imagine some device would use a string index, but containing only digits ;) That would be bad and will hopefully never occur, but ...

@PipoCanaja

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2019

That beeing said, it would be far easier for the user who defines the YAML file if he does not have to think about which index to use, so I will be happy to implement it.

@murrant

This comment has been minimized.

Copy link
Member

commented Jul 7, 2019

I'm saying there is an output option on snmp commands to just show string indexes as numeric. There is no reason to use them as strings. It could be handy to have the information available, but not sure that is worth it.

@PipoCanaja

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2019

Got you. Issue would be the other way around. We don't know when to convert the num_index to string (unless we parse the mib or add an option to the YAML file to tell YamlDiscovery to do the conversion.
And some devices are not providing any OID to list all the string keys. (I had the issue already).
So I think we would be on the safe side if we keep the same default behaviour we have now.

@murrant murrant modified the milestones: 1.54, 1.55 Jul 28, 2019

@murrant

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

This pull request has been mentioned on LibreNMS Community. There might be relevant details there:

https://community.librenms.org/t/existing-device-with-custom-oid/9032/6

@PipoCanaja

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

Hi @murrant
Re-checked this, and still think this PR is a safe way to do it :

  • No change for any existing config.
  • Possible to use string-based index if needed.
    If it is OK with you like this, I would love to see it merged so I can start upgrading again one of my prod instances :D
    Thnx

@PipoCanaja PipoCanaja self-assigned this Aug 20, 2019

@laf
laf approved these changes Aug 20, 2019
Copy link
Member

left a comment

If this does indeed work then I'm fine with merging it in.

@PipoCanaja

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

I installed this patch a month ago on an instance that monitors efficientIP DNS/DHCP appliances, and it did well so far. No impact in any existing devices.

@PipoCanaja PipoCanaja merged commit e7a86ad into librenms:master Aug 21, 2019

6 checks passed

Inspection Summary
Details
Node: analysis
Details
Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
codeclimate All good!
Details
license/cla Contributor License Agreement is signed.
Details
@murrant

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

This pull request has been mentioned on LibreNMS Community. There might be relevant details there:

https://community.librenms.org/t/v1-55-release-changelog-august-2019/9428/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.