Merge branch 'sy-consistently-auto-resolve-alerts' into 'master'

Consistently handle recovery alerts

See merge request gitlab-org/gitlab!57302

app/services/alert_management/process_prometheus_alert_service.rb

@@ -25,13 +25,6 @@ module AlertManagement
 
     attr_reader :project, :payload
 
-    override :process_new_alert
-    def process_new_alert
-      return if resolving_alert?
-
-      super
-    end
-
     override :incoming_payload
     def incoming_payload
       strong_memoize(:incoming_payload) do

app/services/concerns/alert_management/alert_processing.rb

@@ -19,11 +19,7 @@ module AlertManagement
     # Updates or creates alert from payload for project
     # including system notes
     def process_alert
-      if alert.persisted?
-        process_existing_alert
-      else
-        process_new_alert
-      end
+      alert.persisted? ? process_existing_alert : process_new_alert
     end
 
     # Creates or closes issue for alert and notifies stakeholders
@@ -33,22 +29,16 @@ module AlertManagement
     end
 
     def process_existing_alert
-      if resolving_alert?
-        process_resolved_alert
-      else
-        process_firing_alert
-      end
+      resolving_alert? ? process_resolved_alert : process_firing_alert
     end
 
     def process_resolved_alert
       SystemNoteService.log_resolving_alert(alert, alert_source)
 
-      return unless auto_close_incident?
-
       if alert.resolve(incoming_payload.ends_at)
         SystemNoteService.change_alert_status(alert, User.alert_bot)
 
-        close_issue(alert.issue)
+        close_issue(alert.issue) if auto_close_incident?
       else
         logger.warn(
           message: 'Unable to update AlertManagement::Alert status to resolved',
@@ -76,6 +66,8 @@ module AlertManagement
       if alert.save
         alert.execute_services
         SystemNoteService.create_new_alert(alert, alert_source)
+
+        process_resolved_alert if resolving_alert?
       else
         logger.warn(
           message: "Unable to create AlertManagement::Alert from #{alert_source}",
@@ -128,7 +120,7 @@ module AlertManagement
     end
 
     def alert_source
-      alert.monitoring_tool
+      incoming_payload.monitoring_tool
     end
 
     def logger

changelogs/unreleased/sy-consistently-auto-resolve-alerts.yml

+---
+title: Always resolve GitLab alerts when recovery alert payload is received
+merge_request: 57302
+author:
+type: changed

ee/spec/services/alert_management/process_prometheus_alert_service_spec.rb

@@ -42,7 +42,7 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do
         context 'with resolving payload' do
           let(:payload) { raw_payload.merge('status' => 'resolved') }
 
-          include_examples 'oncall users are correctly notified of Prometheus recovery alert'
+          include_examples 'oncall users are correctly notified of recovery alert'
         end
       end
     end

ee/spec/support/shared_examples/services/alert_management/alert_processing/alert_recovery_shared_examples.rb

@@ -33,33 +33,3 @@ RSpec.shared_examples 'oncall users are correctly notified of recovery alert' do
     end
   end
 end
-
-RSpec.shared_examples 'oncall users are correctly notified of Prometheus recovery alert' do
-  it_behaves_like 'does not send on-call notification'
-
-  context 'when alert with the same fingerprint already exists' do
-    context 'which is triggered' do
-      let_it_be(:alert) { create(:alert_management_alert, :triggered, fingerprint: gitlab_fingerprint, project: project) }
-
-      it_behaves_like 'sends on-call notification if enabled'
-    end
-
-    context 'which is acknowledged' do
-      let_it_be(:alert) { create(:alert_management_alert, :acknowledged, fingerprint: gitlab_fingerprint, project: project) }
-
-      it_behaves_like 'sends on-call notification if enabled'
-    end
-
-    context 'which is resolved' do
-      let_it_be(:alert) { create(:alert_management_alert, :resolved, fingerprint: gitlab_fingerprint, project: project) }
-
-      it_behaves_like 'does not send on-call notification'
-    end
-
-    context 'which is ignored' do
-      let_it_be(:alert) { create(:alert_management_alert, :ignored, fingerprint: gitlab_fingerprint, project: project) }
-
-      it_behaves_like 'sends on-call notification if enabled'
-    end
-  end
-end

spec/services/alert_management/process_prometheus_alert_service_spec.rb

@@ -48,7 +48,7 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do
       context 'with resolving payload' do
         let(:prometheus_status) { 'resolved' }
 
-        it_behaves_like 'processes prometheus recovery alert'
+        it_behaves_like 'processes recovery alert'
       end
 
       context 'environment given' do

spec/support/shared_examples/services/alert_management/alert_processing/alert_recovery_shared_examples.rb

@@ -111,67 +111,3 @@ RSpec.shared_examples 'processes recovery alert' do
     end
   end
 end
-
-RSpec.shared_examples 'processes prometheus recovery alert' do
-  context 'seen for the first time' do
-    it_behaves_like 'does not create an alert management alert'
-    it_behaves_like 'does not send alert notification emails'
-    it_behaves_like 'does not process incident issues'
-  end
-
-  context 'for an existing alert with the same fingerprint' do
-    let_it_be(:gitlab_fingerprint) { Digest::SHA1.hexdigest(fingerprint) }
-
-    context 'which is triggered' do
-      let_it_be(:alert) { create(:alert_management_alert, :triggered, project: project, fingerprint: gitlab_fingerprint, monitoring_tool: source) }
-
-      it_behaves_like 'resolves an existing alert management alert'
-      it_behaves_like 'creates expected system notes for alert', :recovery_alert, :resolve_alert
-      it_behaves_like 'sends alert notification emails if enabled'
-      it_behaves_like 'closes related incident if enabled'
-      it_behaves_like 'writes a warning to the log for a failed alert status update'
-
-      it_behaves_like 'does not create an alert management alert'
-      it_behaves_like 'does not process incident issues'
-      it_behaves_like 'does not add an alert management alert event'
-    end
-
-    context 'which is ignored' do
-      let_it_be(:alert) { create(:alert_management_alert, :ignored, project: project, fingerprint: gitlab_fingerprint, monitoring_tool: source) }
-
-      it_behaves_like 'resolves an existing alert management alert'
-      it_behaves_like 'creates expected system notes for alert', :recovery_alert, :resolve_alert
-      it_behaves_like 'sends alert notification emails if enabled'
-      it_behaves_like 'closes related incident if enabled'
-      it_behaves_like 'writes a warning to the log for a failed alert status update'
-
-      it_behaves_like 'does not create an alert management alert'
-      it_behaves_like 'does not process incident issues'
-      it_behaves_like 'does not add an alert management alert event'
-    end
-
-    context 'which is acknowledged' do
-      let_it_be(:alert) { create(:alert_management_alert, :acknowledged, project: project, fingerprint: gitlab_fingerprint, monitoring_tool: source) }
-
-      it_behaves_like 'resolves an existing alert management alert'
-      it_behaves_like 'creates expected system notes for alert', :recovery_alert, :resolve_alert
-      it_behaves_like 'sends alert notification emails if enabled'
-      it_behaves_like 'closes related incident if enabled'
-      it_behaves_like 'writes a warning to the log for a failed alert status update'
-
-      it_behaves_like 'does not create an alert management alert'
-      it_behaves_like 'does not process incident issues'
-      it_behaves_like 'does not add an alert management alert event'
-    end
-
-    context 'which is resolved' do
-      let_it_be(:alert) { create(:alert_management_alert, :resolved, project: project, fingerprint: gitlab_fingerprint, monitoring_tool: source) }
-
-      it_behaves_like 'does not create an alert management alert'
-      it_behaves_like 'does not send alert notification emails'
-      it_behaves_like 'does not change the alert end time'
-      it_behaves_like 'does not process incident issues'
-      it_behaves_like 'does not add an alert management alert event'
-    end
-  end
-end

spec/support/shared_examples/services/alert_management_shared_examples.rb

@@ -48,9 +48,19 @@ end
 
 RSpec.shared_examples 'processes never-before-seen recovery alert' do
   it_behaves_like 'creates an alert management alert or errors'
-  it_behaves_like 'creates expected system notes for alert', :new_alert
+  it_behaves_like 'creates expected system notes for alert', :new_alert, :recovery_alert, :resolve_alert
   it_behaves_like 'sends alert notification emails if enabled'
-  it_behaves_like 'processes incident issues if enabled'
+  it_behaves_like 'does not process incident issues'
+  it_behaves_like 'writes a warning to the log for a failed alert status update' do
+    let(:alert) { nil } # Ensure the next alert id is used
+  end
+
+  it 'resolves the alert' do
+    subject
+
+    expect(AlertManagement::Alert.last.ended_at).to be_present
+    expect(AlertManagement::Alert.last.resolved?).to be(true)
+  end
 end
 
 RSpec.shared_examples 'processes one firing and one resolved prometheus alerts' do
@@ -58,10 +68,10 @@ RSpec.shared_examples 'processes one firing and one resolved prometheus alerts'
     expect(Gitlab::AppLogger).not_to receive(:warn)
 
     expect { subject }
-      .to change(AlertManagement::Alert, :count).by(1)
-      .and change(Note, :count).by(1)
+      .to change(AlertManagement::Alert, :count).by(2)
+      .and change(Note, :count).by(4)
   end
 
   it_behaves_like 'processes incident issues'
-  it_behaves_like 'sends alert notification emails', count: 1
+  it_behaves_like 'sends alert notification emails', count: 2
 end