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

Give an option to repeat alert notifications #3536

Merged
merged 10 commits into from Feb 24, 2017
Merged

Give an option to repeat alert notifications #3536

merged 10 commits into from Feb 24, 2017

Conversation

edmundoa
Copy link
Contributor

This PR fixes the issue described in #3511 by adding an option to the alert conditions parameters to repeat notifications.

The repeat_notifications option is disabled by default, so all alerts will only send a notification when a new alert is triggered, the behaviour introduced in 2.2.0.

When users rely on repeated notifications on their alerting, they can enable repeat_notifications and Graylog will send a notification after each positive evaluation of the alert condition, just as we did in previous versions.

Alerts will continue having state in the UI, and users can see all sent notifications in the alert details page.

Edmundo Alvarez added 7 commits February 22, 2017 11:37
When user sets the `repeat_notifications` flag in the alert condition,
resend the notifications regardless of the alert state. The alert will
continue to have a state anyway, and all sent notifications will be
visible in the web interface on the alerts detail page.

Refs #3511
- Sort histories by creation datetime
- Add the time were the notification was sent, to help seeing the order
and also that the attempts sending the notification are different
Copy link
Member

@dennisoelkers dennisoelkers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good in general, minor comments added.

*/
package org.graylog2.alerts;

import com.google.inject.Inject;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nitpick, but please use javax.inject.Inject instead. com.google.inject.Inject is guice-specific, we should try to avoid that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Damn auto imports... ✅

@@ -30,7 +30,9 @@ const AlarmCallbackHistoryOverview = React.createClass({
return <Spinner />;
}

const histories = this.state.histories.map(this._formatHistory);
const histories = this.state.histories
.sort((h1, h2) => h1.created_at.localeCompare(h2.created_at))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comparing two timestamps lexicographically might not lead to a correct result. We should use moment().isBefore()/isAfter() for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dennisoelkers dennisoelkers merged commit 003bea9 into master Feb 24, 2017
@dennisoelkers dennisoelkers deleted the issue-3511 branch February 24, 2017 16:20
dennisoelkers pushed a commit that referenced this pull request Feb 24, 2017
* Add option to repeat notifications

Refs #3511

* Resend notifications when user sets the flag

When user sets the `repeat_notifications` flag in the alert condition,
resend the notifications regardless of the alert state. The alert will
continue to have a state anyway, and all sent notifications will be
visible in the web interface on the alerts detail page.

Refs #3511

* Add repetition setting to alert summaries

* Add repetition setting to alert timeline

* Add date to notification histories

- Sort histories by creation datetime
- Add the time were the notification was sent, to help seeing the order
and also that the attempts sending the notification are different

* Do not include ms in history datetime

* Add license header

* Rename variable

* Fix import

* Use moment to sort notifications by date

(cherry picked from commit 003bea9)
joschi pushed a commit that referenced this pull request Mar 2, 2017
* Add option to repeat notifications

Refs #3511

* Resend notifications when user sets the flag

When user sets the `repeat_notifications` flag in the alert condition,
resend the notifications regardless of the alert state. The alert will
continue to have a state anyway, and all sent notifications will be
visible in the web interface on the alerts detail page.

Refs #3511

* Add repetition setting to alert summaries

* Add repetition setting to alert timeline

* Add date to notification histories

- Sort histories by creation datetime
- Add the time were the notification was sent, to help seeing the order
and also that the attempts sending the notification are different

* Do not include ms in history datetime

* Add license header

* Rename variable

* Fix import

* Use moment to sort notifications by date

(cherry picked from commit 003bea9)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants