Merge branch 'sy-remove-alert-retriggering' into 'master'

Only email users about alerts when status is triggered or resolving

See merge request gitlab-org/gitlab!53330

app/services/alert_management/process_prometheus_alert_service.rb

@@ -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

app/services/concerns/alert_management/alert_processing.rb

@@ -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

changelogs/unreleased/sy-remove-alert-retriggering.yml

+---
+title: Stop notifying users of acknowledged alerts and stop changing the status of
+  acknowledged Prometheus alerts to Triggered
+merge_request: 53330
+author:
+type: changed

ee/app/services/concerns/ee/alert_management/alert_processing.rb

@@ -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

ee/spec/services/alert_management/process_prometheus_alert_service_spec.rb

@@ -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
+        let(:resolving_payload) { raw_payload.merge('status' => 'resolved') }
+        let(:users) { [participant.user] }
 
-        it_behaves_like 'Alert Notification Service sends notification email to on-call users'
+        it_behaves_like 'oncall users are correctly notified'
       end
     end
   end

ee/spec/services/projects/alerting/notify_service_spec.rb

@@ -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

ee/spec/support/shared_examples/alert_notification_service_shared_examples.rb

 # 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)
 
     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)
+      .with(
+        users,
+        having_attributes(class: AlertManagement::Alert, fingerprint: fingerprint)
+      )
 
     expect(subject).to be_success
   end
-  end
 end

ee/spec/support/shared_examples/incident_management/oncall_notifications_shared_examples.rb

+# 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

spec/services/alert_management/process_prometheus_alert_service_spec.rb

@@ -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
 

spec/support/shared_examples/alert_notification_service_shared_examples.rb

@@ -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)
 
+    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