Merge branch...

Merge branch '11212-improve-default-title-and-description-of-issues-opened-from-managed-prometheus-alerts' into 'master'

Resolve "Improve default title and description of issues opened from managed Prometheus alerts"

See merge request gitlab-org/gitlab-ee!14614

ee/app/presenters/projects/prometheus/alert_presenter.rb

@@ -27,6 +27,19 @@ module Projects
         end
       end
 
+      def starts_at
+        super&.rfc3339
+      end
+
+      def issue_summary_markdown
+        <<~MARKDOWN.chomp
+          ## Summary
+
+          #{metadata_list}
+          #{annotation_list}
+        MARKDOWN
+      end
+
       private
 
       def alert_title
@@ -38,6 +51,31 @@ module Projects
 
         "#{gitlab_alert.title} #{gitlab_alert.computed_operator} #{gitlab_alert.threshold} for 5 minutes"
       end
+
+      def metadata_list
+        metadata = []
+
+        metadata << bullet('starts_at', starts_at) if starts_at
+        metadata << bullet('full_query', backtick(full_query)) if full_query
+
+        metadata.join("\n")
+      end
+
+      def annotation_list
+        strong_memoize(:annotation_list) do
+          annotations
+            .map { |annotation| bullet(annotation.label, annotation.value) }
+            .join("\n")
+        end
+      end
+
+      def bullet(key, value)
+        "* #{key}: #{value}"
+      end
+
+      def backtick(value)
+        "`#{value}`"
+      end
     end
   end
 end

ee/app/services/incident_management/create_issue_service.rb

@@ -41,24 +41,12 @@ module IncidentManagement
     end
 
     def alert_summary
-      <<~MARKDOWN
-        ## Summary
-
-        #{annotation_list}
-      MARKDOWN
-    end
-
-    def annotation_list
-      strong_memoize(:annotation_list) do
-        alert.annotations
-          .map { |annotation| "* #{annotation.label}: #{annotation.value}" }
-          .join("\n")
-      end
+      alert.issue_summary_markdown
     end
 
     def alert
       strong_memoize(:alert) do
-        Gitlab::Alerting::Alert.new(project: project, payload: params)
+        Gitlab::Alerting::Alert.new(project: project, payload: params).present
       end
     end
 

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

@@ -60,7 +60,7 @@ module Projects
         def validate_date(date)
           return unless date
 
-          Time.parse(date)
+          Time.rfc3339(date)
           date
         rescue ArgumentError
         end

ee/changelogs/unreleased/11212-improve-default-title-and-description-of-issues-opened-from-managed-prometheus-alerts.yml

+---
+title: Improve default title and description of issues opened from managed Prometheus alerts
+merge_request: 14614
+author:
+type: changed

ee/lib/gitlab/alerting/alert.rb

@@ -17,7 +17,7 @@ module Gitlab
 
       def title
         strong_memoize(:title) do
-          parse_title_from_payload
+          gitlab_alert&.title || parse_title_from_payload
         end
       end
 
@@ -37,8 +37,20 @@ module Gitlab
         end
       end
 
+      def starts_at
+        strong_memoize(:starts_at) do
+          parse_datetime_from_payload('startsAt')
+        end
+      end
+
+      def full_query
+        strong_memoize(:full_query) do
+          gitlab_alert&.full_query || parse_expr_from_payload
+        end
+      end
+
       def valid?
-        project && title
+        project && title && starts_at
       end
 
       def present
@@ -58,9 +70,9 @@ module Gitlab
       end
 
       def parse_title_from_payload
-        gitlab_alert&.title ||
-          payload&.dig('annotations', 'title') ||
-          payload&.dig('annotations', 'summary')
+        payload&.dig('annotations', 'title') ||
+          payload&.dig('annotations', 'summary') ||
+          payload&.dig('labels', 'alertname')
       end
 
       def parse_description_from_payload
@@ -72,6 +84,27 @@ module Gitlab
           Alerting::AlertAnnotation.new(label: label, value: value)
         end
       end
+
+      def parse_datetime_from_payload(field)
+        value = payload&.dig(field)
+        return unless value
+
+        Time.rfc3339(value)
+      rescue ArgumentError
+      end
+
+      # Parses `g0.expr` from `generatorURL`.
+      #
+      # Example: http://localhost:9090/graph?g0.expr=vector%281%29&g0.tab=1
+      def parse_expr_from_payload
+        url = payload&.dig('generatorURL')
+        return unless url
+
+        uri = URI(url)
+
+        Rack::Utils.parse_query(uri.query).fetch('g0.expr')
+      rescue URI::InvalidURIError, KeyError
+      end
     end
   end
 end

ee/spec/factories/alerting/alert.rb

@@ -9,6 +9,10 @@ FactoryBot.define do
       metric_id nil
 
       after(:build) do |alert, evaluator|
+        unless alert.payload.key?('startsAt')
+          alert.payload['startsAt'] = Time.now.rfc3339
+        end
+
         if metric_id = evaluator.metric_id
           alert.payload['labels'] ||= {}
           alert.payload['labels']['gitlab_alert_id'] = metric_id.to_s

ee/spec/lib/gitlab/alerting/alert_spec.rb

@@ -8,74 +8,213 @@ describe Gitlab::Alerting::Alert do
   let(:alert) { build(:alerting_alert, project: project, payload: payload) }
   let(:payload) { {} }
 
-  context 'with gitlab alert' do
+  shared_context 'gitlab alert' do
+    let(:gitlab_alert_id) { gitlab_alert.prometheus_metric_id.to_s }
     let!(:gitlab_alert) { create(:prometheus_alert, project: project) }
 
     before do
-      payload['labels'] = {
-        'gitlab_alert_id' => gitlab_alert_id
-      }
+      payload['labels'] = { 'gitlab_alert_id' => gitlab_alert_id }
     end
+  end
 
-    context 'with matching gitlab_alert_id' do
-      let(:gitlab_alert_id) { gitlab_alert.prometheus_metric_id.to_s }
+  shared_examples 'invalid alert' do
+    it 'is invalid' do
+      expect(alert).not_to be_valid
+    end
+  end
 
-      it 'loads gitlab_alert' do
-        expect(alert.gitlab_alert).to eq(gitlab_alert)
-      end
+  shared_examples 'parse payload' do |*pairs|
+    context 'without payload' do
+      it { is_expected.to be_nil }
+    end
 
-      it 'delegates environment to gitlab_alert' do
-        expect(alert.environment).to eq(gitlab_alert.environment)
+    pairs.each do |pair|
+      context "with #{pair}" do
+        let(:value) { 'some value' }
+
+        before do
+          section, name = pair.split('/')
+          payload[section] = { name => value }
+        end
+
+        it { is_expected.to eq(value) }
       end
+    end
+  end
 
-      it 'prefers gitlab_alert\'s title over annotated title' do
-        payload['annontations'] = { 'title' => 'other title' }
+  describe '#gitlab_alert' do
+    subject { alert.gitlab_alert }
 
-        expect(alert.title).to eq(gitlab_alert.title)
+    context 'without payload' do
+      it { is_expected.to be_nil }
+    end
+
+    context 'with gitlab alert' do
+      include_context 'gitlab alert'
+
+      it { is_expected.to eq(gitlab_alert) }
+    end
+
+    context 'with unknown gitlab alert' do
+      include_context 'gitlab alert' do
+        let(:gitlab_alert_id) { 'unknown' }
       end
 
-      it 'is valid' do
-        expect(alert).to be_valid
+      it { is_expected.to be_nil }
+    end
+  end
+
+  describe '#title' do
+    subject { alert.title }
+
+    it_behaves_like 'parse payload',
+      'annotations/title',
+      'annotations/summary',
+      'labels/alertname'
+
+    context 'with gitlab alert' do
+      include_context 'gitlab alert'
+
+      context 'with annotations/title' do
+        let(:value) { 'annotation title' }
+
+        before do
+          payload['annotations'] = { 'title' => value }
+        end
+
+        it { is_expected.to eq(gitlab_alert.title) }
       end
     end
+  end
+
+  describe '#description' do
+    subject { alert.description }
+
+    it_behaves_like 'parse payload', 'annotations/description'
+  end
 
-    context 'with unknown gitlab_alert_id' do
-      let(:gitlab_alert_id) { 'unknown' }
+  describe '#annotations' do
+    subject { alert.annotations }
 
-      it 'cannot load gitlab_alert' do
-        expect(alert.gitlab_alert).to be_nil
+    context 'without payload' do
+      it { is_expected.to eq([]) }
+    end
+
+    context 'with payload' do
+      before do
+        payload['annotations'] = { 'foo' => 'value1', 'bar' => 'value2' }
       end
 
-      it 'is invalid' do
-        expect(alert).not_to be_valid
+      it 'parses annotations' do
+        expect(subject.size).to eq(2)
+        expect(subject.map(&:label)).to eq(%w[foo bar])
+        expect(subject.map(&:value)).to eq(%w[value1 value2])
       end
     end
   end
 
-  context 'with annotations' do
-    before do
-      payload['annotations'] = {
-        'label' => 'value',
-        'another' => 'value2'
-      }
+  describe '#environment' do
+    subject { alert.environment }
+
+    context 'without gitlab_alert' do
+      it { is_expected.to be_nil }
     end
 
-    it 'parses annotations' do
-      expect(alert.annotations.size).to eq(2)
-      expect(alert.annotations.map(&:label)).to eq(%w(label another))
-      expect(alert.annotations.map(&:value)).to eq(%w(value value2))
+    context 'with gitlab alert' do
+      include_context 'gitlab alert'
+
+      it { is_expected.to eq(gitlab_alert.environment) }
     end
   end
 
-  context 'without annotations' do
-    it 'has no annotations' do
-      expect(alert.annotations).to be_empty
+  describe '#starts_at' do
+    subject { alert.starts_at }
+
+    context 'with empty startsAt' do
+      before do
+        payload['startsAt'] = nil
+      end
+
+      it { is_expected.to be_nil }
+    end
+
+    context 'with invalid startsAt' do
+      before do
+        payload['startsAt'] = 'invalid'
+      end
+
+      it { is_expected.to be_nil }
+    end
+
+    context 'with payload' do
+      let(:time) { Time.now.change(usec: 0) }
+
+      before do
+        payload['startsAt'] = time.rfc3339
+      end
+
+      it { is_expected.to eq(time) }
     end
   end
 
-  context 'with empty payload' do
-    it 'cannot load gitlab_alert' do
-      expect(alert.gitlab_alert).to be_nil
+  describe '#full_query' do
+    using RSpec::Parameterized::TableSyntax
+
+    subject { alert.full_query }
+
+    where(:generator_url, :expected_query) do
+      nil | nil
+      'http://localhost' | nil
+      'invalid url' | nil
+      'http://localhost:9090/graph?g1.expr=vector%281%29' | nil
+      'http://localhost:9090/graph?g0.expr=vector%281%29' | 'vector(1)'
+    end
+
+    with_them do
+      before do
+        payload['generatorURL'] = generator_url
+      end
+
+      it { is_expected.to eq(expected_query) }
+    end
+
+    context 'with gitlab alert' do
+      include_context 'gitlab alert'
+
+      before do
+        payload['generatorURL'] = 'http://localhost:9090/graph?g0.expr=vector%281%29'
+      end
+
+      it { is_expected.to eq(gitlab_alert.full_query) }
+    end
+  end
+
+  describe '#valid?' do
+    before do
+      payload.update(
+        'annotations' => { 'title' => 'some title' },
+        'startsAt' => Time.now.rfc3339
+      )
+    end
+
+    subject { alert }
+
+    it { is_expected.to be_valid }
+
+    context 'without project' do
+      # Redefine to prevent:
+      # project is a NilClass - rspec-set works with ActiveRecord models only
+      let(:alert) { build(:alerting_alert, project: nil, payload: payload) }
+
+      it { is_expected.not_to be_valid }
+    end
+
+    context 'without starts_at' do
+      before do
+        payload['startsAt'] = nil
+      end
+
+      it { is_expected.not_to be_valid }
     end
   end
 end

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

@@ -20,13 +20,13 @@ describe EE::Emails::Projects do
       Notify.prometheus_alert_fired_email(project.id, user.id, alert_params)
     end
 
-    context 'with an alert' do
-      let(:alert_params) do
-        {
-          'labels' => {
-            'gitlab_alert_id' => alert.prometheus_metric_id.to_s
-          }
-        }
+    let(:alert_params) do
+      { 'startsAt' => Time.now.rfc3339 }
+    end
+
+    context 'with a gitlab alert' do
+      before do
+        alert_params['labels'] = { 'gitlab_alert_id' => alert.prometheus_metric_id.to_s }
       end
 
       let(:title) do
@@ -60,19 +60,15 @@ describe EE::Emails::Projects do
       end
     end
 
-    context 'without an alert' do
+    context 'with no payload' do
       let(:alert_params) { {} }
 
       it_behaves_like 'no email'
     end
 
     context 'with an unknown alert' do
-      let(:alert_params) do
-        {
-          'labels' => {
-            'gitlab_alert_id' => 'unknown'
-          }
-        }
+      before do
+        alert_params['labels'] = { 'gitlab_alert_id' => 'unknown' }
       end
 
       it_behaves_like 'no email'
@@ -85,12 +81,8 @@ describe EE::Emails::Projects do
         metrics_project_environments_url(project)
       end
 
-      let(:alert_params) do
-        {
-          'annotations' => {
-            'title' => title
-          }
-        }
+      before do
+        alert_params['annotations'] = { 'title' => title }
       end
 
       it_behaves_like 'an email sent from GitLab'

ee/spec/presenters/projects/prometheus/alert_presenter_spec.rb

@@ -6,7 +6,8 @@ describe Projects::Prometheus::AlertPresenter do
   set(:project) { create(:project) }
 
   let(:presenter) { described_class.new(alert) }
-  let(:alert) { create(:alerting_alert, project: project) }
+  let(:payload) { {} }
+  let(:alert) { create(:alerting_alert, project: project, payload: payload) }
 
   describe '#project_full_path' do
     subject { presenter.project_full_path }
@@ -14,6 +15,58 @@ describe Projects::Prometheus::AlertPresenter do
     it { is_expected.to eq(project.full_path) }
   end
 
+  describe '#starts_at' do
+    subject { presenter.starts_at }
+
+    before do
+      payload['startsAt'] = starts_at
+    end
+
+    context 'with valid datetime' do
+      let(:datetime) { Time.now }
+      let(:starts_at) { datetime.rfc3339 }
+
+      it { is_expected.to eq(datetime.rfc3339) }
+    end
+
+    context 'with invalid datetime' do
+      let(:starts_at) { 'invalid' }
+
+      it { is_expected.to be_nil }
+    end
+  end
+
+  describe '#issue_summary_markdown' do
+    subject { presenter.issue_summary_markdown }
+
+    context 'without default payload' do
+      it do
+        is_expected.to include('## Summary')
+        is_expected.to include('* starts_at:')
+        is_expected.not_to include('* full_query:')
+      end
+    end
+
+    context 'with annotations' do
+      before do
+        payload['annotations'] = { 'foo' => 'value1', 'bar' => 'value2' }
+      end
+
+      it do
+        is_expected.to include('* foo: value1')
+        is_expected.to include('* bar: value2')
+      end
+    end
+
+    context 'with full query' do
+      before do
+        payload['generatorURL'] = 'http://host?g0.expr=query'
+      end
+
+      it { is_expected.to include('* full_query: `query`') }
+    end
+  end
+
   context 'with gitlab alert' do
     let(:gitlab_alert) { create(:prometheus_alert, project: project) }
     let(:metric_id) { gitlab_alert.prometheus_metric_id }
@@ -49,10 +102,6 @@ describe Projects::Prometheus::AlertPresenter do
     end
 
     describe '#performance_dashboard_link' do
-      before do
-        gitlab_alert.save!
-      end
-
       let(:expected_link) do
         Gitlab::Routing.url_helpers
           .metrics_project_environment_url(project, alert.environment)

ee/spec/services/incident_management/create_issue_service_spec.rb

@@ -5,9 +5,19 @@ require 'spec_helper'
 describe IncidentManagement::CreateIssueService do
   let(:project) { create(:project, :repository) }
   let(:service) { described_class.new(project, nil, alert_payload) }
+  let(:alert_starts_at) { Time.now }
   let(:alert_title) { 'TITLE' }
+  let(:alert_annotations) { { title: alert_title } }
+
   let(:alert_payload) do
-    build_alert_payload(annotations: { title: alert_title })
+    build_alert_payload(
+      annotations: alert_annotations,
+      starts_at: alert_starts_at
+    )
+  end
+
+  let(:alert_presenter) do
+    Gitlab::Alerting::Alert.new(project: project, payload: alert_payload).present
   end
 
   let!(:setting) do
@@ -30,8 +40,7 @@ describe IncidentManagement::CreateIssueService do
 
         expect(issue.author).to eq(User.alert_bot)
         expect(issue.title).to eq(alert_title)
-        expect(issue.description).to include('Summary')
-        expect(issue.description).to include(alert_title)
+        expect(issue.description).to include(alert_presenter.issue_summary_markdown)
         expect(issue.description).not_to include(summary_separator)
       end
     end
@@ -48,8 +57,7 @@ describe IncidentManagement::CreateIssueService do
         it 'creates an issue appending issue template' do
           expect(subject).to include(status: :success)
 
-          expect(issue.description).to include('Summary')
-          expect(issue.description).to include(alert_title)
+          expect(issue.description).to include(alert_presenter.issue_summary_markdown)
           expect(issue.description).to include(summary_separator)
           expect(issue.description).to include(issue_template_content)
         end
@@ -93,15 +101,27 @@ describe IncidentManagement::CreateIssueService do
       end
     end
 
-    context 'with an invalid alert payload' do
-      let(:alert_payload) { build_alert_payload(annotations: {}) }
+    describe 'with invalid alert payload' do
+      shared_examples 'invalid alert' do
+        it 'does not create an issue' do
+          expect(service)
+            .to receive(:log_error)
+            .with(error_message('invalid alert'))
+
+          expect(subject).to eq(status: :error, message: 'invalid alert')
+        end
+      end
+
+      context 'without title' do
+        let(:alert_annotations) { {} }
 
-      it 'does not create an issue' do
-        expect(service)
-          .to receive(:log_error)
-          .with(error_message('invalid alert'))
+        it_behaves_like 'invalid alert'
+      end
 
-        expect(subject).to eq(status: :error, message: 'invalid alert')
+      context 'without startsAt' do
+        let(:alert_starts_at) { nil }
+
+        it_behaves_like 'invalid alert'
       end
     end
   end
@@ -122,8 +142,12 @@ describe IncidentManagement::CreateIssueService do
 
   private
 
-  def build_alert_payload(annotations: {})
-    { 'annotations' => annotations.stringify_keys }
+  def build_alert_payload(annotations: {}, starts_at: Time.now)
+    {
+      'annotations' => annotations.stringify_keys
+    }.tap do |payload|
+      payload['startsAt'] = starts_at.rfc3339 if starts_at
+    end
   end
 
   def error_message(message)

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

@@ -267,7 +267,7 @@ describe Projects::Prometheus::Alerts::CreateEventsService do
 
   # Example: 2018-09-27T18:25:31.079079416Z
   def utc_rfc3339(date)
-    date.utc.strftime("%FT%T.%9NZ")
+    date.utc.rfc3339
   rescue
     date
   end