Merge branch '199400-send-email-notifications-for-generic-alerts' into 'master'

Send email notifications for Generic Alerts

Closes #199400

See merge request gitlab-org/gitlab!24414

app/presenters/projects/prometheus/alert_presenter.rb

@@ -6,6 +6,7 @@ module Projects
       RESERVED_ANNOTATIONS = %w(gitlab_incident_markdown title).freeze
       GENERIC_ALERT_SUMMARY_ANNOTATIONS = %w(monitoring_tool service hosts).freeze
       MARKDOWN_LINE_BREAK = "  \n".freeze
+      INCIDENT_LABEL_NAME = IncidentManagement::CreateIssueService::INCIDENT_LABEL[:title].freeze
 
       def full_title
         [environment_name, alert_title].compact.join(': ')
@@ -31,6 +32,18 @@ module Projects
         end
       end
 
+      def show_performance_dashboard_link?
+        gitlab_alert.present?
+      end
+
+      def show_incident_issues_link?
+        project.incident_management_setting&.create_issue?
+      end
+
+      def incident_issues_link
+        project_issues_url(project, label_name: INCIDENT_LABEL_NAME)
+      end
+
       def starts_at
         super&.rfc3339
       end

app/services/concerns/incident_management/settings.rb

+# frozen_string_literal: true
+module IncidentManagement
+  module Settings
+    def incident_management_setting
+      strong_memoize(:incident_management_setting) do
+        project.incident_management_setting ||
+          project.build_incident_management_setting
+      end
+    end
+
+    def process_issues?
+      incident_management_setting.create_issue?
+    end
+  end
+end

app/services/projects/alerting/notify_service.rb

@@ -4,12 +4,14 @@ module Projects
   module Alerting
     class NotifyService < BaseService
       include Gitlab::Utils::StrongMemoize
+      include IncidentManagement::Settings
 
       def execute(token)
         return forbidden unless alerts_service_activated?
         return unauthorized unless valid_token?(token)
 
-        process_incident_issues
+        process_incident_issues if process_issues?
+        send_alert_email if send_email?
 
         ServiceResponse.success
       rescue Gitlab::Alerting::NotificationPayloadParser::BadPayloadError
@@ -20,11 +22,21 @@ module Projects
 
       delegate :alerts_service, :alerts_service_activated?, to: :project
 
+      def send_email?
+        incident_management_setting.send_email?
+      end
+
       def process_incident_issues
         IncidentManagement::ProcessAlertWorker
           .perform_async(project.id, parsed_payload)
       end
 
+      def send_alert_email
+        notification_service
+          .async
+          .prometheus_alerts_fired(project, [parsed_payload])
+      end
+
       def parsed_payload
         Gitlab::Alerting::NotificationPayloadParser.call(params.to_h)
       end

changelogs/unreleased/199400-send-email-notifications-for-generic-alerts.yml

+---
+title: Send alert emails for generic incident alerts
+merge_request: 24414
+author:
+type: added

ee/app/services/ee/notification_service.rb

@@ -70,19 +70,23 @@ module EE
     def prometheus_alerts_fired(project, alerts)
       return if project.emails_disabled?
 
+      owners_and_maintainers_without_invites(project).to_a.product(alerts).each do |recipient, alert|
+        mailer.prometheus_alert_fired_email(project.id, recipient.user.id, alert).deliver_later
+      end
+    end
+
+    private
+
+    def owners_and_maintainers_without_invites(project)
       recipients = project.members.active_without_invites_and_requests.owners_and_masters
 
       if recipients.empty? && project.group
         recipients = project.group.members.active_without_invites_and_requests.owners_and_masters
       end
 
-      recipients.to_a.product(alerts).each do |recipient, alert|
-        mailer.prometheus_alert_fired_email(project.id, recipient.user.id, alert).deliver_later
-      end
+      recipients
     end
 
-    private
-
     def send_new_review_notification(review)
       recipients = ::NotificationRecipientService.build_new_review_recipients(review)
 

ee/app/services/projects/prometheus/alerts/notify_service.rb

@@ -5,6 +5,7 @@ module Projects
     module Alerts
       class NotifyService < BaseService
         include Gitlab::Utils::StrongMemoize
+        include IncidentManagement::Settings
 
         def execute(token)
           return false unless valid_payload_size?
@@ -20,38 +21,20 @@ module Projects
 
         private
 
-        def valid_payload_size?
-          Gitlab::Utils::DeepSize.new(params).valid?
-        end
-
         def incident_management_available?
           project.feature_available?(:incident_management)
         end
 
-        def incident_management_setting
-          strong_memoize(:incident_management_setting) do
-            project.incident_management_setting ||
-              project.build_incident_management_setting
-          end
+        def valid_payload_size?
+          Gitlab::Utils::DeepSize.new(params).valid?
         end
 
         def send_email?
-          # Send email if the `incident_management` feature flag is disabled.
-          # This is done in order to keep the old behavior of sending emails for
-          # any project which does not have the new `incident_management` feature.
-          # See point 3 in
-          # https://gitlab.com/gitlab-org/gitlab/merge_requests/9830#what-does-this-mr-do
           return firings.any? unless incident_management_available?
 
           incident_management_setting.send_email && firings.any?
         end
 
-        def process_issues?
-          return unless incident_management_available?
-
-          incident_management_setting.create_issue?
-        end
-
         def firings
           @firings ||= alerts_by_status('firing')
         end

ee/app/views/notify/prometheus_alert_fired_email.html.haml

 %p
-  An alert has been triggered in #{@alert.project_full_path}.
+  = _('An alert has been triggered in %{project_path}.') % { project_path: @alert.project_full_path }
 
 - if description = @alert.description
   %p
-    Description: #{description}
+    = _('Description:')
+    = description
 
 - if env_name = @alert.environment_name
   %p
-    Environment: #{env_name}
+    = _('Environment:')
+    = env_name
 
 - if metric_query = @alert.metric_query
   %p
-    Metric:
+    = _('Metric:')
 
     %pre
       = metric_query
 
-%p
-  = link_to('View performance dashboard.', @alert.performance_dashboard_link)
+- if @alert.show_incident_issues_link?
+  %p
+    = link_to(_('View incident issues.'), @alert.incident_issues_link)
+
+- if @alert.show_performance_dashboard_link?
+  %p
+    = link_to(_('View performance dashboard.'), @alert.performance_dashboard_link)
+

ee/app/views/notify/prometheus_alert_fired_email.text.erb

-An alert has been triggered in <%= @alert.project_full_path %>.
+<%= _('An alert has been triggered in %{project_path}.') % { project_path: @alert.project_full_path } %>.
 
 <% if description = @alert.description %>
-Description: <%= description %>
+<%= _('Description:') %> <%= description %>
 <% end %>
 
 <% if env_name = @alert.environment_name %>
-Environment: <%= env_name %>
+<%= _('Environment:') %> <%= env_name %>
 <% end %>
 
 <% if metric_query = @alert.metric_query %>
-Metric: <%= metric_query %>
+<%= _('Metric:') %> <%= metric_query %>
 <% end %>
 
-View the performance dashboard at <%= @alert.performance_dashboard_link %>
+<% if @alert.show_incident_issues_link? %>
+<%= _('View incident issues.') %> <%= @alert.incident_issues_link %>
+<% end %>
+
+<% if @alert.show_performance_dashboard_link? %>
+<%= _('View the performance dashboard at') %> <%= @alert.performance_dashboard_link %>
+<% end %>

ee/spec/mailers/ee/emails/projects_spec.rb

@@ -13,6 +13,20 @@ describe EE::Emails::Projects do
     end
   end
 
+  shared_examples 'shows the incident issues url' do
+    context 'create issue setting enabled' do
+      before do
+        create(:project_incident_management_setting, project: project, create_issue: true)
+      end
+
+      let(:incident_issues_url) do
+        project_issues_url(project, label_name: 'incident')
+      end
+
+      it { is_expected.to have_body_text(incident_issues_url) }
+    end
+  end
+
   let_it_be(:user) { create(:user) }
 
   describe '#prometheus_alert_fired_email' do
@@ -58,6 +72,8 @@ describe EE::Emails::Projects do
         is_expected.to have_body_text(alert.full_query)
         is_expected.to have_body_text(metrics_url)
       end
+
+      it_behaves_like 'shows the incident issues url'
     end
 
     context 'with no payload' do
@@ -83,6 +99,7 @@ describe EE::Emails::Projects do
 
       before do
         alert_params['annotations'] = { 'title' => title }
+        alert_params['generatorURL'] = 'http://localhost:9090/graph?g0.expr=vector%281%29&g0.tab=1'
       end
 
       it_behaves_like 'an email sent from GitLab'
@@ -98,8 +115,6 @@ describe EE::Emails::Projects do
         is_expected.to have_body_text(project.full_path)
         is_expected.not_to have_body_text('Description:')
         is_expected.not_to have_body_text('Environment:')
-        is_expected.not_to have_body_text('Metric:')
-        is_expected.to have_body_text(metrics_url)
       end
 
       context 'with annotated description' do
@@ -114,6 +129,8 @@ describe EE::Emails::Projects do
           is_expected.to have_body_text(description)
         end
       end
+
+      it_behaves_like 'shows the incident issues url'
     end
   end
 end

ee/spec/services/projects/prometheus/alerts/notify_service_spec.rb

@@ -309,14 +309,6 @@ describe Projects::Prometheus::Alerts::NotifyService do
           it_behaves_like 'does not process incident issues'
         end
       end
-
-      context 'without license' do
-        before do
-          stub_licensed_features(incident_management: false)
-        end
-
-        it_behaves_like 'does not process incident issues'
-      end
     end
   end
 

locale/gitlab.pot

@@ -1674,6 +1674,9 @@ msgstr ""
 msgid "Amount of time (in hours) that users are allowed to skip forced configuration of two-factor authentication"
 msgstr ""
 
+msgid "An alert has been triggered in %{project_path}."
+msgstr ""
+
 msgid "An application called %{link_to_client} is requesting access to your GitLab account."
 msgstr ""
 
@@ -12164,6 +12167,9 @@ msgstr ""
 msgid "Metric was successfully updated."
 msgstr ""
 
+msgid "Metric:"
+msgstr ""
+
 msgid "MetricChart|Please select a metric"
 msgstr ""
 
@@ -21508,6 +21514,9 @@ msgstr ""
 msgid "View group labels"
 msgstr ""
 
+msgid "View incident issues."
+msgstr ""
+
 msgid "View issue"
 msgstr ""
 
@@ -21532,6 +21541,9 @@ msgstr ""
 msgid "View open merge request"
 msgstr ""
 
+msgid "View performance dashboard."
+msgstr ""
+
 msgid "View project"
 msgstr ""
 
@@ -21550,6 +21562,9 @@ msgstr ""
 msgid "View the latest successful deployment to this environment"
 msgstr ""
 
+msgid "View the performance dashboard at"
+msgstr ""
+
 msgid "Viewing commit"
 msgstr ""
 

spec/lib/gitlab/alerting/alert_spec.rb

@@ -17,6 +17,12 @@ describe Gitlab::Alerting::Alert do
     end
   end
 
+  shared_context 'full query' do
+    before do
+      payload['generatorURL'] = 'http://localhost:9090/graph?g0.expr=vector%281%29'
+    end
+  end
+
   shared_examples 'invalid alert' do
     it 'is invalid' do
       expect(alert).not_to be_valid
@@ -180,10 +186,7 @@ describe Gitlab::Alerting::Alert do
 
     context 'with gitlab alert' do
       include_context 'gitlab alert'
-
-      before do
-        payload['generatorURL'] = 'http://localhost:9090/graph?g0.expr=vector%281%29'
-      end
+      include_context 'full query'
 
       it { is_expected.to eq(gitlab_alert.full_query) }
     end

spec/presenters/projects/prometheus/alert_presenter_spec.rb

@@ -9,6 +9,15 @@ describe Projects::Prometheus::AlertPresenter do
   let(:payload) { {} }
   let(:alert) { create(:alerting_alert, project: project, payload: payload) }
 
+  shared_context 'gitlab alert' do
+    let(:gitlab_alert) { create(:prometheus_alert, project: project) }
+    let(:metric_id) { gitlab_alert.prometheus_metric_id }
+
+    let(:alert) do
+      create(:alerting_alert, project: project, metric_id: metric_id)
+    end
+  end
+
   describe '#project_full_path' do
     subject { presenter.project_full_path }
 
@@ -145,14 +154,36 @@ describe Projects::Prometheus::AlertPresenter do
     end
   end
 
+  describe '#show_performance_dashboard_link?' do
+    subject { presenter.show_performance_dashboard_link? }
+
+    it { is_expected.to be_falsey }
+
     context 'with gitlab alert' do
-    let(:gitlab_alert) { create(:prometheus_alert, project: project) }
-    let(:metric_id) { gitlab_alert.prometheus_metric_id }
+      include_context 'gitlab alert'
 
-    let(:alert) do
-      create(:alerting_alert, project: project, metric_id: metric_id)
+      it { is_expected.to eq(true) }
+    end
   end
 
+  describe '#show_incident_issues_link?' do
+    subject { presenter.show_incident_issues_link? }
+
+    it { is_expected.to be_falsey }
+
+    context 'create issue setting enabled' do
+      before do
+        create(:project_incident_management_setting, project: project, create_issue: true)
+        project.reload
+      end
+
+      it { is_expected.to eq(true) }
+    end
+  end
+
+  context 'with gitlab alert' do
+    include_context 'gitlab alert'
+
     describe '#full_title' do
       let(:query_title) do
         "#{gitlab_alert.title} #{gitlab_alert.computed_operator} #{gitlab_alert.threshold} for 5 minutes"
@@ -189,6 +220,17 @@ describe Projects::Prometheus::AlertPresenter do
 
       it { is_expected.to eq(expected_link) }
     end
+
+    describe '#incident_issues_link' do
+      let(:expected_link) do
+        Gitlab::Routing.url_helpers
+          .project_issues_url(project, label_name: described_class::INCIDENT_LABEL_NAME)
+      end
+
+      subject { presenter.incident_issues_link }
+
+      it { is_expected.to eq(expected_link) }
+    end
   end
 
   context 'without gitlab alert' do

spec/services/projects/alerting/notify_service_spec.rb

@@ -25,7 +25,31 @@ describe Projects::Alerting::NotifyService do
     end
   end
 
-  shared_examples 'does not process incident issues' do |http_status:|
+  shared_examples 'sends notification email' do
+    let(:notification_service) { spy }
+
+    it 'sends a notification for firing alerts only' do
+      expect(NotificationService)
+        .to receive(:new)
+        .and_return(notification_service)
+
+      expect(notification_service)
+        .to receive_message_chain(:async, :prometheus_alerts_fired)
+
+      expect(subject.status).to eq(:success)
+    end
+  end
+
+  shared_examples 'does not process incident issues' do
+    it 'does not process issues' do
+      expect(IncidentManagement::ProcessAlertWorker)
+        .not_to receive(:perform_async)
+
+      expect(subject.status).to eq(:success)
+    end
+  end
+
+  shared_examples 'does not process incident issues due to error' do |http_status:|
     it 'does not process issues' do
       expect(IncidentManagement::ProcessAlertWorker)
         .not_to receive(:perform_async)
@@ -54,11 +78,23 @@ describe Projects::Alerting::NotifyService do
 
       context 'with valid token' do
         let(:token) { alerts_service.token }
+        let(:incident_management_setting) { double(send_email?: email_enabled, create_issue?: issue_enabled) }
+        let(:email_enabled) { false }
+        let(:issue_enabled) { false }
 
-        context 'with a valid payload' do
-          it_behaves_like 'processes incident issues', 1
+        before do
+          allow(service)
+            .to receive(:incident_management_setting)
+            .and_return(incident_management_setting)
         end
 
+        it_behaves_like 'does not process incident issues'
+
+        context 'issue enabled' do
+          let(:issue_enabled) { true }
+
+          it_behaves_like 'processes incident issues', 1
+
           context 'with an invalid payload' do
             before do
               allow(Gitlab::Alerting::NotificationPayloadParser)
@@ -66,19 +102,26 @@ describe Projects::Alerting::NotifyService do
                 .and_raise(Gitlab::Alerting::NotificationPayloadParser::BadPayloadError)
             end
 
-          it_behaves_like 'does not process incident issues', http_status: 400
+            it_behaves_like 'does not process incident issues due to error', http_status: 400
           end
         end
 
-      context 'with invalid token' do
-        it_behaves_like 'does not process incident issues', http_status: 401
+        context 'with emails turned on' do
+          let(:email_enabled) { true }
+
+          it_behaves_like 'sends notification email'
+        end
       end
+
+      context 'with invalid token' do
+        it_behaves_like 'does not process incident issues due to error', http_status: 401
       end
 
       context 'with deactivated Alerts Service' do
         let!(:alerts_service) { create(:alerts_service, :inactive, project: project) }
 
-      it_behaves_like 'does not process incident issues', http_status: 403
+        it_behaves_like 'does not process incident issues due to error', http_status: 403
+      end
     end
   end
 end