Commit 16df6b5a authored by syasonik's avatar syasonik

Only email when triggered or resolved

parent f6e9e634
......@@ -32,23 +32,6 @@ module AlertManagement
super
end
override :process_firing_alert
def process_firing_alert
super
reset_alert_status
end
def reset_alert_status
return if alert.trigger
logger.warn(
message: 'Unable to update AlertManagement::Alert status to triggered',
project_id: project.id,
alert_id: alert.id
)
end
override :incoming_payload
def incoming_payload
strong_memoize(:incoming_payload) do
......
......@@ -29,7 +29,7 @@ module AlertManagement
# Creates or closes issue for alert and notifies stakeholders
def complete_post_processing_tasks
process_incident_issues if process_issues?
send_alert_email if send_email?
send_alert_email if send_email? && notifying_alert?
end
def process_existing_alert
......@@ -116,6 +116,10 @@ module AlertManagement
incoming_payload.ends_at.present?
end
def notifying_alert?
alert.triggered? || alert.resolved?
end
def alert_source
alert.monitoring_tool
end
......
---
title: Stop notifying users of acknowledged alerts and stop changing the status of
acknowledged Prometheus alerts to Triggered
merge_request: 53330
author:
type: changed
......@@ -11,7 +11,7 @@ module EE
def complete_post_processing_tasks
super
notify_oncall if oncall_notification_recipients.present?
notify_oncall if oncall_notification_recipients.present? && notifying_alert?
end
def notify_oncall
......
......@@ -17,12 +17,14 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do
context 'when alert payload is valid' do
let(:parsed_payload) { Gitlab::AlertManagement::Payload.parse(project, payload, monitoring_tool: 'Prometheus') }
let(:fingerprint) { parsed_payload.gitlab_fingerprint }
let(:payload) do
let(:payload) { raw_payload }
let(:raw_payload) do
{
'status' => 'firing',
'labels' => { 'alertname' => 'GitalyFileServerDown' },
'annotations' => { 'title' => 'Alert title' },
'startsAt' => '2020-04-27T10:10:22.265949279Z',
'endsAt' => '2020-04-27T10:20:22.265949279Z',
'generatorURL' => 'http://8d467bd4607a:9090/graph?g0.expr=vector%281%29&g0.tab=1'
}
end
......@@ -31,14 +33,10 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do
let_it_be(:schedule) { create(:incident_management_oncall_schedule, project: project) }
let_it_be(:rotation) { create(:incident_management_oncall_rotation, schedule: schedule) }
let_it_be(:participant) { create(:incident_management_oncall_participant, :with_developer_access, rotation: rotation) }
let(:notification_args) do
[
[participant.user],
having_attributes(class: AlertManagement::Alert, fingerprint: fingerprint)
]
end
it_behaves_like 'Alert Notification Service sends notification email to on-call users'
let(:resolving_payload) { raw_payload.merge('status' => 'resolved') }
let(:users) { [participant.user] }
it_behaves_like 'oncall users are correctly notified'
end
end
end
......
......@@ -68,14 +68,12 @@ RSpec.describe Projects::Alerting::NotifyService do
let_it_be(:schedule) { create(:incident_management_oncall_schedule, project: project) }
let_it_be(:rotation) { create(:incident_management_oncall_rotation, schedule: schedule) }
let_it_be(:participant) { create(:incident_management_oncall_participant, :with_developer_access, rotation: rotation) }
let(:notification_args) do
[
[participant.user],
having_attributes(class: AlertManagement::Alert, title: payload['title'])
]
end
let(:payload) { { 'fingerprint' => 'fingerprint' } }
let(:resolving_payload) { { 'fingerprint' => 'fingerprint', "end_time": Time.current.iso8601 } }
let(:users) { [participant.user] }
let(:fingerprint) { Digest::SHA1.hexdigest('fingerprint') }
it_behaves_like 'Alert Notification Service sends notification email to on-call users'
it_behaves_like 'oncall users are correctly notified'
end
end
end
# frozen_string_literal: true
# Requires `users` and `fingerprint` to be defined
RSpec.shared_examples 'Alert Notification Service sends notification email to on-call users' do
let(:notification_service) { instance_double(NotificationService) }
context 'with oncall schedules enabled' do
before do
stub_licensed_features(oncall_schedules: project)
end
it 'sends a notification' do
expect(NotificationService).to receive(:new).and_return(notification_service)
it 'sends a notification' do
expect(NotificationService).to receive(:new).and_return(notification_service)
expect(notification_service)
.to receive_message_chain(:async, :notify_oncall_users_of_alert)
.with(
users,
having_attributes(class: AlertManagement::Alert, fingerprint: fingerprint)
)
expect(notification_service)
.to receive_message_chain(:async, :notify_oncall_users_of_alert)
.with(*notification_args)
expect(subject).to be_success
end
end
context 'with oncall schedules disabled' do
it 'does not notify the on-call users' do
expect(NotificationService).not_to receive(:new)
expect(subject).to be_success
end
expect(subject).to be_success
end
end
# frozen_string_literal: true
# Requires `project`, `users`, `fingerprint`, and `resolving_payload`
RSpec.shared_examples 'oncall users are correctly notified' do
context 'with feature enabled' do
before do
stub_licensed_features(oncall_schedules: project)
end
it_behaves_like 'Alert Notification Service sends notification email to on-call users'
context 'when alert with the same fingerprint already exists' do
let!(:alert) { create(:alert_management_alert, status, fingerprint: fingerprint, project: project) }
it_behaves_like 'Alert Notification Service sends notification email to on-call users' do
let(:status) { :triggered }
end
it_behaves_like 'Alert Notification Service sends no notifications' do
let(:status) { :acknowledged }
end
it_behaves_like 'Alert Notification Service sends notification email to on-call users' do
let(:status) { :resolved }
end
it_behaves_like 'Alert Notification Service sends no notifications' do
let(:status) { :ignored }
end
context 'with resolving payload' do
let(:status) { :triggered }
let(:payload) { resolving_payload }
it_behaves_like 'Alert Notification Service sends notification email to on-call users'
end
end
end
context 'with feature disabled' do
it_behaves_like 'Alert Notification Service sends no notifications'
end
end
......@@ -68,36 +68,29 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do
let!(:alert) { create(:alert_management_alert, :resolved, project: project, fingerprint: fingerprint) }
it_behaves_like 'creates an alert management alert'
it_behaves_like 'Alert Notification Service sends notification email'
end
context 'existing alert is ignored' do
let!(:alert) { create(:alert_management_alert, :ignored, project: project, fingerprint: fingerprint) }
it_behaves_like 'adds an alert management alert event'
it_behaves_like 'Alert Notification Service sends no notifications'
end
context 'two existing alerts, one resolved one open' do
let!(:resolved_alert) { create(:alert_management_alert, :resolved, project: project, fingerprint: fingerprint) }
let!(:alert) { create(:alert_management_alert, project: project, fingerprint: fingerprint) }
context 'existing alert is acknowledged' do
let!(:alert) { create(:alert_management_alert, :acknowledged, project: project, fingerprint: fingerprint) }
it_behaves_like 'adds an alert management alert event'
it_behaves_like 'Alert Notification Service sends no notifications'
end
context 'when status change did not succeed' do
before do
allow(AlertManagement::Alert).to receive(:for_fingerprint).and_return([alert])
allow(alert).to receive(:trigger).and_return(false)
end
it 'writes a warning to the log' do
expect(Gitlab::AppLogger).to receive(:warn).with(
message: 'Unable to update AlertManagement::Alert status to triggered',
project_id: project.id,
alert_id: alert.id
)
context 'two existing alerts, one resolved one open' do
let!(:resolved_alert) { create(:alert_management_alert, :resolved, project: project, fingerprint: fingerprint) }
let!(:alert) { create(:alert_management_alert, project: project, fingerprint: fingerprint) }
execute
end
it_behaves_like 'adds an alert management alert event'
it_behaves_like 'Alert Notification Service sends notification email'
end
context 'when auto-creation of issues is disabled' do
......@@ -109,11 +102,7 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do
context 'when emails are disabled' do
let(:send_email) { false }
it 'does not send notification' do
expect(NotificationService).not_to receive(:new)
expect(subject).to be_success
end
it_behaves_like 'Alert Notification Service sends no notifications'
end
end
......@@ -136,11 +125,7 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do
context 'when emails are disabled' do
let(:send_email) { false }
it 'does not send notification' do
expect(NotificationService).not_to receive(:new)
expect(subject).to be_success
end
it_behaves_like 'Alert Notification Service sends no notifications'
end
end
......@@ -235,11 +220,7 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do
context 'when emails are disabled' do
let(:send_email) { false }
it 'does not send notification' do
expect(NotificationService).not_to receive(:new)
expect(subject).to be_success
end
it_behaves_like 'Alert Notification Service sends no notifications'
end
end
......
......@@ -3,7 +3,7 @@
RSpec.shared_examples 'Alert Notification Service sends notification email' do
let(:notification_service) { spy }
it 'sends a notification for firing alerts only' do
it 'sends a notification' do
expect(NotificationService)
.to receive(:new)
.and_return(notification_service)
......@@ -15,15 +15,15 @@ RSpec.shared_examples 'Alert Notification Service sends notification email' do
end
end
RSpec.shared_examples 'Alert Notification Service sends no notifications' do |http_status:|
let(:notification_service) { spy }
let(:create_events_service) { spy }
RSpec.shared_examples 'Alert Notification Service sends no notifications' do |http_status: nil|
it 'does not notify' do
expect(notification_service).not_to receive(:async)
expect(create_events_service).not_to receive(:execute)
expect(NotificationService).not_to receive(:new)
expect(subject).to be_error
expect(subject.http_status).to eq(http_status)
if http_status.present?
expect(subject).to be_error
expect(subject.http_status).to eq(http_status)
else
expect(subject).to be_success
end
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment