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

Added support for graphing pf related stats for pfsense devices #8643

Merged
merged 41 commits into from Jun 26, 2018

Conversation

Projects
None yet
6 participants
@utelisysadmin
Contributor

utelisysadmin commented May 1, 2018

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

In response to feature request:
https://community.librenms.org/t/add-support-for-begemot-pf-mib-for-pfsense-devices/3628

utelisysadmin added some commits May 1, 2018

Added extra graph types needed for firewall stats
pf_matches, pf_badoffset, pf_fragmented, pf_short, pf_normalized, pf_memdropped
@CLAassistant

This comment has been minimized.

CLAassistant commented May 1, 2018

CLA assistant check
All committers have signed the CLA.

@@ -9,3 +9,5 @@ over:
discovery:
- sysDescr:
- pfSense
mib_dir:

This comment has been minimized.

@murrant

murrant May 5, 2018

Member

This is redundant, we always append the directory with the same name as the OS.

@murrant

This comment has been minimized.

Member

murrant commented May 5, 2018

Other than the one small change, LGTM.

@scrutinizer-notifier

This comment has been minimized.

scrutinizer-notifier commented May 8, 2018

The inspection completed: No new issues

@utelisysadmin

This comment has been minimized.

Contributor

utelisysadmin commented May 8, 2018

Done.

@laf

laf requested changes May 8, 2018 edited

Few changes from me.

Could you also rename the MIBs that can stay to be named after what you see on the DEFINITIONS line. I.e mibs/pfsense/BEGEMOT-ATM-FREEBSD-MIB.txt to mibs/pfsense/BEGEMOT-ATM-FREEBSD-MIB

Also can you add test data please: https://docs.librenms.org/#Developing/os/Test-Units/#example-workflow

@@ -0,0 +1,325 @@
--

This comment has been minimized.

@laf

laf May 8, 2018

Member

This mib already exists: mibs/RSTP-MIB so you can delete this one.

@@ -0,0 +1,1483 @@
--

This comment has been minimized.

@laf

laf May 8, 2018

Member

This mib already exists in mibs/BRIDGE-MIB so you can delete this one.

$memdropped = $oids[0]['pfCounterMemDrop'];
if (is_numeric($states)) {

This comment has been minimized.

@laf

laf May 8, 2018

Member

Not really sure if this will be a problem or not but as you check if each of these values are numeric then it could mean that in the event they aren't then from one poll to another you could end up with data out of order or the wrong number of DS' in the RRD if you have transient issues with the snmp_multi_get() call.

This comment has been minimized.

@murrant

murrant Jun 19, 2018

Member

@laf I don't think that is an issue here since the data is fetched by oid name.

This comment has been minimized.

@laf

laf Jun 20, 2018

Member

That's not what I'm getting at. I mean if the response from snmp for whatever reason produces non-numeric output even for just some values then the order of the rrdtool update (+ other data stores) will be out of order and effect the data we store.

This comment has been minimized.

@utelisysadmin

utelisysadmin Jun 21, 2018

Contributor

Well, would it be better to skip the check and just attempt to shove whatever value I get into the RRD?

This comment has been minimized.

@laf

laf Jun 21, 2018

Member

Actually I've just realised that these are all creating separate files anyway so this isn't an issue.

This comment has been minimized.

@laf

laf Jun 21, 2018

Member

If you can fix the mib issues from my comments I think we're good to go.

@kkrumm1 kkrumm1 added the Enhancement label May 21, 2018

@laf

This comment has been minimized.

Member

laf commented Jun 13, 2018

@utelisysadmin Any update on the comments we've left?

@utelisysadmin

This comment has been minimized.

Contributor

utelisysadmin commented Jun 19, 2018

Hi.
Apologies for the delay, been busy with other things. I'll remove the MIBs which are duplicate.

I do understand the comment about the number check, but I don't understand what do You want me to do about it. After all, all I did was copy the existing codebase for OpenBSD. All the OID used here are of type NUMERIC anyway.

utelisysadmin added some commits Jun 22, 2018

Delete RSTP-MIB.txt
MIB already exists
Delete BRIDGE-MIB.txt
MIB already exists
Rename BEGEMOT-ATM.txt to BEGEMOT-ATM
removed .txt from filename
Rename BEGEMOT-HAST-MIB.txt to BEGEMOT-HAST-MIB
removed .txt from filename
Rename BEGEMOT-IP-MIB.txt to BEGEMOT-IP-MIB
removed .txt from filename
Rename BEGEMOT-MIB.txt to BEGEMOT-MIB
removed .txt from filename
Rename BEGEMOT-MIB2-MIB.txt to BEGEMOT-MIB2-MIB
removed .txt from filename
Rename BEGEMOT-NETGRAPH.txt to BEGEMOT-NETGRAPH
removed .txt from filename
Rename BEGEMOT-PF-MIB.txt to BEGEMOT-PF-MIB
removed .txt from filename
Rename BEGEMOT-SNMPD.txt to BEGEMOT-SNMPD
removed .txt from filename
@utelisysadmin

This comment has been minimized.

Contributor

utelisysadmin commented Jun 22, 2018

Renamed all MIBs as requested.

@laf

Just the last few mibs with .txt at the end that need fixing.

utelisysadmin added some commits Jun 25, 2018

Rename FOKUS-MIB.txt to FOKUS-MIB
removed .txt from name
Rename FREEBSD-MIB.txt to FREEBSD-MIB
removed .txt from name
@utelisysadmin

This comment has been minimized.

Contributor

utelisysadmin commented Jun 25, 2018

Yep, I forgot two. Renamed now.

@laf laf added Device 🖥 and removed Enhancement labels Jun 26, 2018

@laf

laf approved these changes Jun 26, 2018

LGTM

@laf laf merged commit 1e47f57 into librenms:master Jun 26, 2018

3 checks passed

WIP ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

mattie47 added a commit to mattie47/librenms that referenced this pull request Jul 2, 2018

Added support for graphing pf related stats for pfsense devices (libr…
…enms#8643)

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 followed our [code guidelines?](http://docs.librenms.org/Developing/Code-Guidelines/)

#### Testers

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

In response to feature request:
https://community.librenms.org/t/add-support-for-begemot-pf-mib-for-pfsense-devices/3628

@lock lock bot locked as resolved and limited conversation to collaborators Aug 25, 2018

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