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

Alert Subsys to OOP and SNMPTraps trigger Alert Rules #9765

Open
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@h-barnhart
Copy link
Contributor

h-barnhart commented Jan 31, 2019

DO NOT DELETE THIS TEXT

Added alerts init module to snmptrap.php so the RunRules() function would become available to trap handlers. Added RunRules() to production trap handlers except for the fallback handler.

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.

@murrant

This comment has been minimized.

Copy link
Member

murrant commented Feb 7, 2019

RunRules should be run for every trap. So you don't have to specify it in everyone. Probably here: LibreNMS/Snmptrap/Dispatcher.php

@h-barnhart

This comment has been minimized.

Copy link
Contributor Author

h-barnhart commented Feb 7, 2019

@murrant

This comment has been minimized.

Copy link
Member

murrant commented Feb 7, 2019

People may even want to alert on just basic alerts adding logging to the eventlog.

@murrant

This comment has been minimized.

Copy link
Member

murrant commented Feb 7, 2019

Tests don't use init.php (for whatever reason)

@h-barnhart

This comment has been minimized.

Copy link
Contributor Author

h-barnhart commented Feb 8, 2019

Moved RunRules() to Dispatcher.php so it will run rules for any handled SNMP trap. I added it as an else to the fallback logic, so if no handler is found RunRules() isn't called. As far as I can tell it works, but doesn't pass unit tests because the test env doesn't have access to the database.

Included the function files functions.php and alerts.inc.php to SnmpTrapTests.php so it will have access to RunRules(). If pre-commit.php is run with the --db option it will check the database.

@h-barnhart

This comment has been minimized.

Copy link
Contributor Author

h-barnhart commented Feb 12, 2019

Not sure what to do at this point. Running pre-commit -u --db on my local server with the changes I've made passes.

@murrant

This comment has been minimized.

Copy link
Member

murrant commented Feb 13, 2019

What I usually do is port the code to object oriented style and redirect the original function. Examples: display() and is_valid_hostname() or OSDefinition here #9803

Then in Laravel code only use OO function call. Sometimes I just remove the old function too (but that is harder if you don't have an IDE to do it for you).

@h-barnhart

This comment has been minimized.

Copy link
Contributor Author

h-barnhart commented Feb 13, 2019

Ok, I think understand what you are doing. I'll give it a shot. Unless you say otherwise I'm going to drop it in LibreNMS/Alerts.

Side note, not sure if its GitHub or Gmail, but most of your last comment got cut off in email update to the PR.

@murrant

This comment has been minimized.

Copy link
Member

murrant commented Feb 13, 2019

@h-barnhart It's me. I type stuff to quickly then realize I forgot something, then edit the comment. I should probably just create additional comments in those situations...

@murrant

This comment has been minimized.

Copy link
Member

murrant commented Mar 13, 2019

I have updated the trap testing, so maybe we can re-visit this.

@h-barnhart

This comment has been minimized.

Copy link
Contributor Author

h-barnhart commented Mar 13, 2019

@h-barnhart

This comment has been minimized.

Copy link
Contributor Author

h-barnhart commented Apr 1, 2019

I saw the change to how logging is done, did that allow SNMP Trap logs to trigger alerts?

@laf laf added the Alerting 🔔 label Apr 9, 2019

@laf laf changed the title SNMPTraps Trigger Alert Rules SNMPTraps will now trigger Alert Rules Apr 9, 2019

@laf

laf approved these changes Apr 9, 2019

Copy link
Member

laf left a comment

LGTM

{
//Check to see if under maintenance
if (IsMaintenance($device_id) > 0) {

This comment has been minimized.

Copy link
@laf

laf Apr 9, 2019

Member

This is failing tests: https://travis-ci.com/librenms/librenms/jobs/189340179

$this->IsMaintenance()?

This comment has been minimized.

Copy link
@h-barnhart

h-barnhart Apr 9, 2019

Author Contributor

I've got a few things that still need to be cleared up.

This comment has been minimized.

Copy link
@laf

laf Apr 11, 2019

Member

No worries, I've edited the title to WIP so we know. Remove it when you're ready

@laf laf changed the title SNMPTraps will now trigger Alert Rules WIP: SNMPTraps will now trigger Alert Rule Apr 11, 2019

@laf laf added the User-Pending label Apr 11, 2019

@h-barnhart h-barnhart changed the title WIP: SNMPTraps will now trigger Alert Rule WIP: Alert Subsys to OOP and SNMPTraps trigger Alert Rules Apr 17, 2019

@h-barnhart

This comment has been minimized.

Copy link
Contributor Author

h-barnhart commented Apr 17, 2019

A little scope creep, in addition to getting traps to trigger ruleRun() I converted the rest of the alert subsytem to object oriented.

@h-barnhart h-barnhart changed the title WIP: Alert Subsys to OOP and SNMPTraps trigger Alert Rules Alert Subsys to OOP and SNMPTraps trigger Alert Rules Apr 17, 2019

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