Merge branch '9835-introduce-finder-to-lookup-prometheusalerts' into 'master'

Add AlertsFinder and extract services to manage alerts

Closes #9835

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

ee/app/controllers/projects/prometheus/alerts_controller.rb

@@ -16,8 +16,6 @@ module Projects
       before_action :alert, only: [:update, :show, :destroy]
 
       def index
-        alerts = prometheus_alerts.order_by('id_asc')
-
         render json: serialize_as_json(alerts)
       end
 
@@ -27,10 +25,8 @@ module Projects
 
       def notify
         token = extract_alert_manager_token(request)
-        notify = Projects::Prometheus::Alerts::NotifyService
-          .new(project, current_user, params.permit!)
 
-        if notify.execute(token)
+        if notify_service.execute(token)
           head :ok
         else
           head :unprocessable_entity
@@ -38,7 +34,7 @@ module Projects
       end
 
       def create
-        @alert = prometheus_alerts.create(alerts_params)
+        @alert = create_service.execute
 
         if @alert.persisted?
           schedule_prometheus_update!
@@ -50,7 +46,7 @@ module Projects
       end
 
       def update
-        if alert.update(alerts_params)
+        if update_service.execute(alert)
           schedule_prometheus_update!
 
           render json: serialize_as_json(alert)
@@ -60,7 +56,7 @@ module Projects
       end
 
       def destroy
-        if alert.destroy
+        if destroy_service.execute(alert)
           schedule_prometheus_update!
 
           head :ok
@@ -72,13 +68,27 @@ module Projects
       private
 
       def alerts_params
-        alerts_params = params.permit(:operator, :threshold, :environment_id, :prometheus_metric_id)
+        params.permit(:operator, :threshold, :environment_id, :prometheus_metric_id)
+      end
+
+      def notify_service
+        Projects::Prometheus::Alerts::NotifyService
+          .new(project, current_user, params.permit!)
+      end
 
-        if alerts_params[:operator].present?
-          alerts_params[:operator] = PrometheusAlert.operator_to_enum(alerts_params[:operator])
+      def create_service
+        Projects::Prometheus::Alerts::CreateService
+          .new(project, current_user, alerts_params)
       end
 
-        alerts_params
+      def update_service
+        Projects::Prometheus::Alerts::UpdateService
+          .new(project, current_user, alerts_params)
+      end
+
+      def destroy_service
+        Projects::Prometheus::Alerts::DestroyService
+          .new(project, current_user, nil)
       end
 
       def schedule_prometheus_update!
@@ -90,11 +100,23 @@ module Projects
       end
 
       def serializer
-        PrometheusAlertSerializer.new(project: project, current_user: current_user)
+        PrometheusAlertSerializer
+          .new(project: project, current_user: current_user)
+      end
+
+      def alerts
+        alerts_finder.execute
       end
 
       def alert
-        @alert ||= prometheus_alerts.for_metric(params[:id]).first || render_404
+        @alert ||= alerts_finder(metric: params[:id]).execute.first || render_404
+      end
+
+      def alerts_finder(opts = {})
+        Projects::Prometheus::AlertsFinder.new({
+          project: project,
+          environment: params[:environment_id]
+        }.reverse_merge(opts))
       end
 
       def application

ee/app/finders/projects/prometheus/alerts_finder.rb

+# frozen_string_literal: true
+
+module Projects
+  module Prometheus
+    # Find Prometheus alerts by +project+, by +environment+, or both.
+    #
+    # Optionally filter by +metric+.
+    #
+    # Arguments:
+    #   params:
+    #     project: Project | integer
+    #     environment: Environment | integer
+    #     metric: PrometheusMetric | integer
+    class AlertsFinder
+      def initialize(params = {})
+        unless params[:project] || params[:environment]
+          raise ArgumentError,
+            'Please provide either :project or :environment, or both'
+        end
+
+        @params = params
+      end
+
+      # Find all matching alerts
+      #
+      # @return [ActiveRecord::Relation<PrometheusAlert>]
+      def execute
+        relation = by_project(PrometheusAlert)
+        relation = by_environment(relation)
+        relation = by_metric(relation)
+        relation = ordered(relation)
+
+        relation
+      end
+
+      private
+
+      attr_reader :params
+
+      def by_project(relation)
+        return relation unless params[:project]
+
+        relation.for_project(params[:project])
+      end
+
+      def by_environment(relation)
+        return relation unless params[:environment]
+
+        relation.for_environment(params[:environment])
+      end
+
+      def by_metric(relation)
+        return relation unless params[:metric]
+
+        relation.for_metric(params[:metric])
+      end
+
+      def ordered(relation)
+        relation.order_by('id_asc')
+      end
+    end
+  end
+end

ee/app/models/prometheus_alert.rb

@@ -26,6 +26,7 @@ class PrometheusAlert < ActiveRecord::Base
   delegate :title, :query, to: :prometheus_metric
 
   scope :for_metric, -> (metric) { where(prometheus_metric: metric) }
+  scope :for_project, -> (project) { where(project_id: project) }
   scope :for_environment, -> (environment) { where(environment_id: environment) }
 
   def self.distinct_projects

ee/app/services/clusters/applications/prometheus_config_service.rb

@@ -129,8 +129,11 @@ module Clusters
 
       def alerts(environment)
         variables = Gitlab::Prometheus::QueryVariables.call(environment)
+        alerts = Projects::Prometheus::AlertsFinder
+          .new(environment: environment)
+          .execute
 
-        environment.prometheus_alerts.map do |alert|
+        alerts.map do |alert|
           substitute_query_variables(alert.to_param, variables)
         end
       end

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

+# frozen_string_literal: true
+
+module Projects
+  module Prometheus
+    module Alerts
+      module AlertParams
+        def alert_params
+          return params if params[:operator].blank?
+
+          params.merge(
+            operator: PrometheusAlert.operator_to_enum(params[:operator])
+          )
+        end
+      end
+    end
+  end
+end

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

@@ -30,7 +30,7 @@ module Projects
           gitlab_alert_id = payload.dig('labels', 'gitlab_alert_id')
           return unless gitlab_alert_id
 
-          alert = project.prometheus_alerts.for_metric(gitlab_alert_id).first
+          alert = find_alert(gitlab_alert_id)
           return unless alert
 
           payload_key = PrometheusAlertEvent.payload_key_for(gitlab_alert_id, started_at)
@@ -50,6 +50,13 @@ module Projects
           params['alerts']
         end
 
+        def find_alert(metric)
+          Projects::Prometheus::AlertsFinder
+            .new(project: project, metric: metric)
+            .execute
+            .first
+        end
+
         def validate_date(date)
           return unless date
 

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

+# frozen_string_literal: true
+
+module Projects
+  module Prometheus
+    module Alerts
+      class CreateService < BaseService
+        include AlertParams
+
+        def execute
+          project.prometheus_alerts.create(alert_params)
+        end
+      end
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+module Projects
+  module Prometheus
+    module Alerts
+      class DestroyService < BaseService
+        def execute(alert)
+          alert.destroy
+        end
+      end
+    end
+  end
+end

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

@@ -62,7 +62,7 @@ module Projects
           alert_id = gitlab_alert_id
           return unless alert_id
 
-          alert = project.prometheus_alerts.for_metric(alert_id).first
+          alert = find_alert(project, alert_id)
           return unless alert
 
           cluster = alert.environment.deployment_platform&.cluster
@@ -72,6 +72,13 @@ module Projects
           cluster.application_prometheus
         end
 
+        def find_alert(project, metric)
+          Projects::Prometheus::AlertsFinder
+            .new(project: project, metric: metric)
+            .execute
+            .first
+        end
+
         def gitlab_alert_id
           alerts&.first&.dig('labels', 'gitlab_alert_id')
         end

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

+# frozen_string_literal: true
+
+module Projects
+  module Prometheus
+    module Alerts
+      class UpdateService < BaseService
+        include AlertParams
+
+        def execute(alert)
+          alert.update(alert_params)
+        end
+      end
+    end
+  end
+end

ee/app/services/projects/prometheus/metrics/base_service.rb

@@ -29,9 +29,14 @@ module Projects
         end
 
         def alert
-          strong_memoize(:alert) do
-            metric.prometheus_alerts.find_by(project: project) # rubocop: disable CodeReuse/ActiveRecord
+          strong_memoize(:alert) { find_alert(metric) }
         end
+
+        def find_alert(metric)
+          Projects::Prometheus::AlertsFinder
+            .new(project: project, metric: metric)
+            .execute
+            .first
         end
 
         def has_alert?

ee/lib/ee/gitlab/prometheus/queries/query_additional_metrics.rb

@@ -13,7 +13,7 @@ module EE
 
           def query_with_alert(project, environment)
             alerts_map =
-              environment.prometheus_alerts.each_with_object({}) do |alert, hsh|
+              alerts(project, environment).each_with_object({}) do |alert, hsh|
                 hsh[alert[:prometheus_metric_id]] = alert.prometheus_metric_id
               end
 
@@ -38,6 +38,12 @@ module EE
 
           private
 
+          def alerts(project, environment)
+            ::Projects::Prometheus::AlertsFinder
+              .new(project: project, environment: environment)
+              .execute
+          end
+
           def alert_path(alerts_map, key, project, environment)
             ::Gitlab::Routing.url_helpers.project_prometheus_alert_path(project, alerts_map[key], environment_id: environment.id, format: :json)
           end

ee/lib/gitlab/alerting/alert.rb

@@ -45,7 +45,10 @@ module Gitlab
         metric_id = payload&.dig('labels', 'gitlab_alert_id')
         return unless metric_id
 
-        project.prometheus_alerts.for_metric(metric_id).first
+        Projects::Prometheus::AlertsFinder
+          .new(project: project, metric: metric_id)
+          .execute
+          .first
       end
 
       def parse_title_from_payload

ee/spec/finders/projects/prometheus/alerts_finder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Projects::Prometheus::AlertsFinder do
+  let(:finder) { described_class.new(params) }
+  let(:params) { {} }
+
+  describe 'with params' do
+    set(:project) { create(:project) }
+    set(:other_project) { create(:project) }
+    set(:other_env) { create(:environment, project: other_project) }
+    set(:production) { create(:environment, project: project) }
+    set(:staging) { create(:environment, project: project) }
+    set(:alert) { create_alert(project, production) }
+    set(:alert2) { create_alert(project, production) }
+    set(:stg_alert) { create_alert(project, staging) }
+    set(:other_alert) { create_alert(other_project, other_env) }
+
+    describe '#execute' do
+      subject { finder.execute }
+
+      context 'with project' do
+        before do
+          params[:project] = project
+        end
+
+        it { is_expected.to eq([alert, alert2, stg_alert]) }
+
+        context 'with matching metric' do
+          before do
+            params[:metric] = alert.prometheus_metric
+          end
+
+          it { is_expected.to eq([alert]) }
+        end
+
+        context 'with matching metric id' do
+          before do
+            params[:metric] = alert.prometheus_metric_id
+          end
+
+          it { is_expected.to eq([alert]) }
+        end
+
+        context 'with project non-specific metric' do
+          before do
+            params[:metric] = other_alert.prometheus_metric
+          end
+
+          it { is_expected.to be_empty }
+        end
+      end
+
+      context 'with environment' do
+        before do
+          params[:environment] = production
+        end
+
+        it { is_expected.to eq([alert, alert2]) }
+
+        context 'with matching metric' do
+          before do
+            params[:metric] = alert.prometheus_metric
+          end
+
+          it { is_expected.to eq([alert]) }
+        end
+
+        context 'with environment non-specific metric' do
+          before do
+            params[:metric] = stg_alert.prometheus_metric
+          end
+
+          it { is_expected.to be_empty }
+        end
+      end
+
+      context 'with matching project and environment' do
+        before do
+          params[:project] = project
+          params[:environment] = production
+        end
+
+        it { is_expected.to eq([alert, alert2]) }
+
+        context 'with matching metric' do
+          before do
+            params[:metric] = alert.prometheus_metric
+          end
+
+          it { is_expected.to eq([alert]) }
+        end
+
+        context 'with environment non-specific metric' do
+          before do
+            params[:metric] = stg_alert.prometheus_metric
+          end
+
+          it { is_expected.to be_empty }
+        end
+      end
+
+      context 'with non-matching project-environment pair' do
+        before do
+          params[:project] = project
+          params[:environment] = other_env
+        end
+
+        it { is_expected.to be_empty }
+      end
+    end
+
+    private
+
+    def create_alert(project, environment)
+      create(:prometheus_alert, project: project, environment: environment)
+    end
+  end
+
+  describe 'without params' do
+    subject { finder }
+
+    it 'raises an error' do
+      expect { subject }
+        .to raise_error(ArgumentError, 'Please provide either :project or :environment, or both')
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Projects::Prometheus::Alerts::CreateService do
+  set(:project) { create(:project) }
+  set(:user) { create(:user) }
+
+  let(:service) { described_class.new(project, user, params) }
+
+  subject { service.execute }
+
+  describe '#execute' do
+    context 'with params' do
+      set(:environment) { create(:environment, project: project) }
+
+      set(:metric) do
+        create(:prometheus_metric, project: project)
+      end
+
+      let(:params) do
+        {
+          environment_id: environment.id,
+          prometheus_metric_id: metric.id,
+          operator: '<',
+          threshold: 1.0
+        }
+      end
+
+      it 'creates an alert' do
+        expect(subject).to be_persisted
+
+        expect(subject).to have_attributes(
+          project: project,
+          environment: environment,
+          prometheus_metric: metric,
+          operator: 'lt',
+          threshold: 1.0
+        )
+      end
+    end
+
+    context 'without params' do
+      let(:params) { {} }
+
+      it 'fails to create' do
+        expect(subject).to be_new_record
+        expect(subject).to be_invalid
+      end
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Projects::Prometheus::Alerts::DestroyService do
+  set(:project) { create(:project) }
+  set(:user) { create(:user) }
+  set(:alert) { create(:prometheus_alert, project: project) }
+
+  let(:service) { described_class.new(project, user, nil) }
+
+  describe '#execute' do
+    subject { service.execute(alert) }
+
+    it 'deletes the alert' do
+      expect(subject).to be_truthy
+
+      expect(alert).to be_destroyed
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Projects::Prometheus::Alerts::UpdateService do
+  set(:project) { create(:project) }
+  set(:user) { create(:user) }
+  set(:environment) { create(:environment, project: project) }
+
+  set(:alert) do
+    create(:prometheus_alert, project: project, environment: environment)
+  end
+
+  let(:service) { described_class.new(project, user, params) }
+
+  let(:params) do
+    {
+      environment_id: alert.environment_id,
+      prometheus_metric_id: alert.prometheus_metric_id,
+      operator: '=',
+      threshold: 2.0
+    }
+  end
+
+  describe '#execute' do
+    subject { service.execute(alert) }
+
+    context 'with valid params' do
+      it 'updates the alert' do
+        expect(subject).to be_truthy
+
+        expect(alert.reload).to have_attributes(
+          operator: 'eq',
+          threshold: 2.0
+        )
+      end
+    end
+
+    context 'with invalid params' do
+      let(:other_environment) { create(:environment) }
+
+      before do
+        params[:environment_id] = other_environment.id
+      end
+
+      it 'fails to update' do
+        expect(subject).to be_falsey
+
+        expect(alert).to be_invalid
+      end
+    end
+  end
+end