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: Added support for disabling recovery notifications #8430

Merged
merged 3 commits into from Mar 22, 2018

Conversation

Projects
None yet
3 participants
@laf
Member

laf commented Mar 20, 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

Added support for disabling sending recovery notifications.

Also:

  • blacklisted two more tables from being used in the query builder. No reason people should be alerting on those.
  • Also clear out the 'count' value for an alert so that alerts will be raised if matched again.

I've tested this with two of the exact same rules, one however had recovery alerts disabled. Max 5 alerts, repeated every 1 second.

I received 5 alerts for each, renamed the device so the alerts would clear and received just one recovery email. Then renamed the device back and alerts triggered again through the same sequence.

@laf

This comment has been minimized.

Member

laf commented Mar 20, 2018

One question will be, call it disable Recovery or Recovery notification and change the no_recovery to just recovery and invert the checks? The only issue with that though is backwards compatibility, recovery: true would have to be set which it currently isn't so I think I prefer it how it is.

@murrant

This comment has been minimized.

Member

murrant commented Mar 20, 2018

Think we should add tooltips to all the fields. It can help a lot to have just a bit of extra context.

Issue Recovery [[ on ] ] is my vote :) Or just "Recovery"

You can check if recovery is not set to default to old behavior.

@laf

This comment has been minimized.

Member

laf commented Mar 20, 2018

Issue Recovery [[ on ] ] is my vote :) Or just "Recovery"

You can check if recovery is not set to default to old behavior.

You mean just default to if recovery doesn't exist then default to true? I can switch it around if needs be.

@laf

This comment has been minimized.

Member

laf commented Mar 20, 2018

Updated

@scrutinizer-notifier

This comment has been minimized.

scrutinizer-notifier commented Mar 20, 2018

The inspection completed: No new issues

@murrant murrant merged commit 544b363 into librenms:master Mar 22, 2018

2 checks passed

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

@laf laf deleted the laf:alerts/issue-8353 branch Mar 23, 2018

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

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