Migration: Allow prometheus alerts per environment

Prior this commit only one alert was allowed per project and metric.

db/schema.rb

@@ -2472,7 +2472,7 @@ ActiveRecord::Schema.define(version: 20190222110418) do
     t.integer "project_id", null: false
     t.integer "prometheus_metric_id", null: false
     t.index ["environment_id"], name: "index_prometheus_alerts_on_environment_id", using: :btree
-    t.index ["project_id", "prometheus_metric_id"], name: "index_prometheus_alerts_on_project_id_and_prometheus_metric_id", unique: true, using: :btree
+    t.index ["project_id", "prometheus_metric_id", "environment_id"], name: "index_prometheus_alerts_metric_environment", unique: true, using: :btree
     t.index ["prometheus_metric_id"], name: "index_prometheus_alerts_on_prometheus_metric_id", using: :btree
   end
 

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

@@ -15,13 +15,11 @@ module Projects
       before_action :authorize_admin_project!, except: [:notify]
       before_action :alert, only: [:update, :show, :destroy]
 
-      # rubocop: disable CodeReuse/ActiveRecord
       def index
-        alerts = project.prometheus_alerts.reorder(id: :asc)
+        alerts = prometheus_alerts.order_by('id_asc')
 
         render json: serialize_as_json(alerts)
       end
-      # rubocop: enable CodeReuse/ActiveRecord
 
       def show
         render json: serialize_as_json(alert)
@@ -40,7 +38,7 @@ module Projects
       end
 
       def create
-        @alert = project.prometheus_alerts.create(alerts_params)
+        @alert = prometheus_alerts.create(alerts_params)
 
         if @alert.persisted?
           schedule_prometheus_update!
@@ -96,7 +94,7 @@ module Projects
       end
 
       def alert
-        @alert ||= project.prometheus_alerts.for_metric(params[:id]).first || render_404
+        @alert ||= prometheus_alerts.for_metric(params[:id]).first || render_404
       end
 
       def application
@@ -115,6 +113,10 @@ module Projects
 
         @project = Project.find_by_full_path("#{namespace}/#{id}")
       end
+
+      def prometheus_alerts
+        project.prometheus_alerts.for_environment(params[:environment_id])
+      end
     end
   end
 end

ee/app/models/prometheus_alert.rb

 # frozen_string_literal: true
 
 class PrometheusAlert < ActiveRecord::Base
+  include Sortable
+
   OPERATORS_MAP = {
     lt: "<",
     eq: "=",
@@ -24,6 +26,7 @@ class PrometheusAlert < ActiveRecord::Base
   delegate :title, :query, to: :prometheus_metric
 
   scope :for_metric, -> (metric) { where(prometheus_metric: metric) }
+  scope :for_environment, -> (environment) { where(environment_id: environment) }
 
   def self.distinct_projects
     sub_query = self.group(:project_id).select(1)

ee/changelogs/unreleased/7565-prometheus-alerts-for-multiple-environments.yml

+---
+title: Allow assigning Prometheus alerts to multiple environments
+merge_request: 7361
+author:
+type: fixed

ee/db/migrate/20180912113336_allow_prometheus_alerts_per_environment.rb

+# frozen_string_literal: true
+
+class AllowPrometheusAlertsPerEnvironment < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  INDEX_METRIC_ENVIRONMENT_NAME = 'index_prometheus_alerts_metric_environment'
+
+  disable_ddl_transaction!
+
+  def up
+    # Before we create the new index we need to remove it to deal with possible
+    # failures from previous migration.
+    #
+    # See also https://gitlab.com/gitlab-org/gitlab-ce/issues/58164
+    remove_concurrent_index :prometheus_alerts, INDEX_METRIC_ENVIRONMENT_NAME
+
+    add_concurrent_index :prometheus_alerts, new_columns,
+      name: INDEX_METRIC_ENVIRONMENT_NAME, unique: true
+
+    remove_concurrent_index :prometheus_alerts, old_columns
+  end
+
+  def down
+    delete_duplicate_alerts!
+
+    # Before we create the new index we need to remove it to deal with possible
+    # failures from previous migration.
+    #
+    # See also https://gitlab.com/gitlab-org/gitlab-ce/issues/58164
+    remove_concurrent_index :prometheus_alerts, old_columns
+
+    add_concurrent_index :prometheus_alerts, old_columns, unique: true
+
+    remove_concurrent_index :prometheus_alerts, new_columns,
+      name: INDEX_METRIC_ENVIRONMENT_NAME
+  end
+
+  private
+
+  class PrometheusAlert < ActiveRecord::Base
+    include ::EachBatch
+  end
+
+  # Before adding a more narrow index again we need to make sure to delete
+  # newest "duplicate" alerts and keep only the oldest alert per project and metric.
+  def delete_duplicate_alerts!
+    duplicate_alerts = PrometheusAlert
+      .select('MIN(id) AS min, COUNT(id), project_id')
+      .group(:project_id)
+      .having('COUNT(id) > 1')
+
+    duplicate_alerts.each do |alert|
+      PrometheusAlert
+        .where(project_id: alert['project_id'])
+        .where('id <> ?', alert['min'])
+        .each_batch { |batch| batch.delete_all }
+    end
+  end
+
+  def new_columns
+    [:project_id, :prometheus_metric_id, :environment_id]
+  end
+
+  def old_columns
+    [:project_id, :prometheus_metric_id]
+  end
+end

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

@@ -13,7 +13,7 @@ module EE
 
           def query_with_alert(project, environment)
             alerts_map =
-              project.prometheus_alerts.each_with_object({}) do |alert, hsh|
+              environment.prometheus_alerts.each_with_object({}) do |alert, hsh|
                 hsh[alert[:prometheus_metric_id]] = alert.prometheus_metric_id
               end
 

ee/spec/controllers/projects/prometheus/alerts_controller_spec.rb

@@ -3,10 +3,10 @@
 require 'spec_helper'
 
 describe Projects::Prometheus::AlertsController do
-  let(:user) { create(:user) }
-  let(:project) { create(:project) }
-  let(:environment) { create(:environment, project: project) }
-  let(:metric) { create(:prometheus_metric, project: project) }
+  set(:user) { create(:user) }
+  set(:project) { create(:project) }
+  set(:environment) { create(:environment, project: project) }
+  set(:metric) { create(:prometheus_metric, project: project) }
 
   before do
     stub_licensed_features(prometheus_alerts: true)
@@ -14,73 +14,147 @@ describe Projects::Prometheus::AlertsController do
     sign_in(user)
   end
 
-  describe 'GET #index' do
-    context 'when project has no prometheus alert' do
-      it 'renders forbidden when unlicensed' do
-        stub_licensed_features(prometheus_alerts: false)
+  shared_examples 'unlicensed' do
+    before do
+      stub_licensed_features(prometheus_alerts: false)
+    end
 
-        get :index, params: project_params
+    it 'returns not_found' do
+      make_request
 
-        expect(response).to have_gitlab_http_status(:not_found)
+      expect(response).to have_gitlab_http_status(:not_found)
+    end
+  end
+
+  shared_examples 'project non-specific environment' do |status|
+    let(:other) { create(:environment) }
+
+    it "returns #{status}" do
+      make_request(environment_id: other)
+
+      expect(response).to have_gitlab_http_status(status)
+    end
+
+    if status == :ok
+      it 'returns no prometheus alerts' do
+        make_request(environment_id: other)
+
+        expect(json_response).to be_empty
       end
+    end
+  end
+
+  shared_examples 'project non-specific metric' do |status|
+    let(:other) { create(:prometheus_alert) }
+
+    it "returns #{status}" do
+      make_request(id: other.prometheus_metric_id)
+
+      expect(response).to have_gitlab_http_status(status)
+    end
+  end
 
+  describe 'GET #index' do
+    def make_request(opts = {})
+      get :index, params: request_params(opts, environment_id: environment)
+    end
+
+    context 'when project has no prometheus alert' do
       it 'returns an empty response' do
-        get :index, params: project_params
+        make_request
 
-        expect(response).to have_gitlab_http_status(200)
-        expect(JSON.parse(response.body)).to be_empty
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(json_response).to be_empty
       end
     end
 
     context 'when project has prometheus alerts' do
-      before do
-        create_list(:prometheus_alert, 3, project: project, environment: environment)
+      let(:production) { create(:environment, project: project) }
+      let(:staging) { create(:environment, project: project) }
+      let(:json_alert_ids) { json_response.map { |alert| alert['id'] } }
+
+      let!(:production_alerts) do
+        create_list(:prometheus_alert, 2, project: project, environment: production)
       end
 
-      it 'contains prometheus alerts' do
-        get :index, params: project_params
+      let!(:staging_alerts) do
+        create_list(:prometheus_alert, 1, project: project, environment: staging)
+      end
 
-        expect(response).to have_gitlab_http_status(200)
-        expect(JSON.parse(response.body).count).to eq(3)
+      it 'contains prometheus alerts only for the production environment' do
+        make_request(environment_id: production)
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(json_response.count).to eq(2)
+        expect(json_alert_ids).to eq(production_alerts.map(&:id))
       end
-    end
-  end
 
-  describe 'GET #show' do
-    context 'when alert does not exist' do
-      it 'renders 404' do
-        get :show, params: project_params(id: PrometheusAlert.all.maximum(:prometheus_metric_id).to_i + 1)
+      it 'contains prometheus alerts only for the staging environment' do
+        make_request(environment_id: staging)
 
-        expect(response).to have_gitlab_http_status(404)
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(json_response.count).to eq(1)
+        expect(json_alert_ids).to eq(staging_alerts.map(&:id))
+      end
+
+      it 'does not return prometheus alerts without environment' do
+        make_request(environment_id: nil)
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(json_response).to be_empty
       end
     end
 
-    context 'when alert exists' do
-      let(:alert) { create(:prometheus_alert, project: project, environment: environment, prometheus_metric: metric) }
+    it_behaves_like 'unlicensed'
+    it_behaves_like 'project non-specific environment', :ok
+  end
 
-      it 'renders forbidden when unlicensed' do
-        stub_licensed_features(prometheus_alerts: false)
+  describe 'GET #show' do
+    let(:alert) do
+      create(:prometheus_alert,
+             project: project,
+             environment: environment,
+             prometheus_metric: metric)
+    end
 
-        get :show, params: project_params(id: alert.prometheus_metric_id)
+    def make_request(opts = {})
+      get :show, params: request_params(
+        opts,
+        id: alert.prometheus_metric_id,
+        environment_id: environment
+      )
+    end
+
+    context 'when alert does not exist' do
+      it 'returns not_found' do
+        make_request(id: 0)
 
         expect(response).to have_gitlab_http_status(:not_found)
       end
+    end
 
-      it 'renders the alert' do
-        alert_params = {
-          "id" => alert.id,
-          "title" => alert.title,
-          "query" => alert.query,
-          "operator" => alert.computed_operator,
-          "threshold" => alert.threshold,
-          "alert_path" => Gitlab::Routing.url_helpers.project_prometheus_alert_path(project, alert.prometheus_metric_id, environment_id: alert.environment.id, format: :json)
+    context 'when alert exists' do
+      let(:alert_params) do
+        {
+          'id' => alert.id,
+          'title' => alert.title,
+          'query' => alert.query,
+          'operator' => alert.computed_operator,
+          'threshold' => alert.threshold,
+          'alert_path' => alert_path(alert)
         }
+      end
 
-        get :show, params: project_params(id: alert.prometheus_metric_id)
+      it 'renders the alert' do
+        make_request
 
-        expect(response).to have_gitlab_http_status(200)
-        expect(JSON.parse(response.body)).to include(alert_params)
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(json_response).to include(alert_params)
       end
+
+      it_behaves_like 'unlicensed'
+      it_behaves_like 'project non-specific environment', :not_found
+      it_behaves_like 'project non-specific metric', :not_found
     end
   end
 
@@ -96,20 +170,20 @@ describe Projects::Prometheus::AlertsController do
         .and_return(notify_service)
     end
 
-    it 'renders ok if notification succeeds' do
+    it 'returns ok if notification succeeds' do
       expect(notify_service).to receive(:execute).and_return(true)
 
       post :notify, params: project_params, session: { as: :json }
 
-      expect(response).to have_gitlab_http_status(200)
+      expect(response).to have_gitlab_http_status(:ok)
     end
 
-    it 'renders unprocessable entity if notification fails' do
+    it 'returns unprocessable entity if notification fails' do
       expect(notify_service).to receive(:execute).and_return(false)
 
       post :notify, params: project_params, session: { as: :json }
 
-      expect(response).to have_gitlab_http_status(422)
+      expect(response).to have_gitlab_http_status(:unprocessable_entity)
     end
 
     context 'bearer token' do
@@ -133,7 +207,7 @@ describe Projects::Prometheus::AlertsController do
 
       context 'when missing' do
         it 'passes nil' do
-          expect(notify_service) .to receive(:execute).with(nil)
+          expect(notify_service).to receive(:execute).with(nil)
 
           post :notify, params: project_params, as: :json
         end
@@ -142,120 +216,142 @@ describe Projects::Prometheus::AlertsController do
   end
 
   describe 'POST #create' do
-    it 'renders forbidden when unlicensed' do
-      stub_licensed_features(prometheus_alerts: false)
+    let(:schedule_update_service) { spy }
 
-      post :create, params: project_params(
-        operator: ">",
-        threshold: "1",
-        environment_id: environment.id,
-        prometheus_metric_id: metric.id
-      )
+    let(:alert_params) do
+      {
+        'title' => metric.title,
+        'query' => metric.query,
+        'operator' => '>',
+        'threshold' => 1.0
+      }
+    end
 
-      expect(response).to have_gitlab_http_status(:not_found)
+    def make_request(opts = {})
+      post :create, params: request_params(
+        opts,
+        operator: '>',
+        threshold: '1',
+        environment_id: environment,
+        prometheus_metric_id: metric
+      )
     end
 
     it 'creates a new prometheus alert' do
-      schedule_update_service = spy
-      alert_params = {
-        "title" => metric.title,
-        "query" => metric.query,
-        "operator" => ">",
-        "threshold" => 1.0
-      }
-
-      allow(::Clusters::Applications::ScheduleUpdateService).to receive(:new).and_return(schedule_update_service)
+      allow(::Clusters::Applications::ScheduleUpdateService)
+        .to receive(:new).and_return(schedule_update_service)
 
-      post :create, params: project_params(
-        operator: ">",
-        threshold: "1",
-        environment_id: environment.id,
-        prometheus_metric_id: metric.id
-      )
+      make_request
 
       expect(schedule_update_service).to have_received(:execute)
-      expect(response).to have_gitlab_http_status(200)
-      expect(JSON.parse(response.body)).to include(alert_params)
+      expect(response).to have_gitlab_http_status(:ok)
+      expect(json_response).to include(alert_params)
     end
 
-    context 'with a project non-specific environment' do
-      let(:environment) { create(:environment) }
+    it 'returns no_content for an invalid metric' do
+      make_request(prometheus_metric_id: 'invalid')
 
-      it 'returns 204 status' do
-        post :create, params: project_params(
-          operator: ">",
-          threshold: "1",
-          environment_id: environment.id,
-          prometheus_metric_id: metric.id
-        )
-
-        expect(response).to have_gitlab_http_status(:no_content)
-      end
+      expect(response).to have_gitlab_http_status(:no_content)
     end
+
+    it_behaves_like 'unlicensed'
+    it_behaves_like 'project non-specific environment', :no_content
   end
 
-  describe 'POST #update' do
+  describe 'PUT #update' do
     let(:schedule_update_service) { spy }
-    let(:alert) { create(:prometheus_alert, project: project, environment: environment, prometheus_metric: metric) }
 
-    before do
-      allow(::Clusters::Applications::ScheduleUpdateService).to receive(:new).and_return(schedule_update_service)
+    let(:alert) do
+      create(:prometheus_alert,
+             project: project,
+             environment: environment,
+             prometheus_metric: metric)
     end
 
-    it 'renders forbidden when unlicensed' do
-      stub_licensed_features(prometheus_alerts: false)
+    let(:alert_params) do
+      {
+        'id' => alert.id,
+        'title' => alert.title,
+        'query' => alert.query,
+        'operator' => '<',
+        'threshold' => alert.threshold,
+        'alert_path' => alert_path(alert)
+      }
+    end
 
-      put :update, params: project_params(id: alert.prometheus_metric_id, operator: "<")
+    before do
+      allow(::Clusters::Applications::ScheduleUpdateService)
+        .to receive(:new).and_return(schedule_update_service)
+    end
 
-      expect(response).to have_gitlab_http_status(:not_found)
+    def make_request(opts = {})
+      put :update, params: request_params(
+        opts,
+        id: alert.prometheus_metric_id,
+        operator: '<',
+        environment_id: alert.environment
+      )
     end
 
     it 'updates an already existing prometheus alert' do
-      alert_params = {
-        "id" => alert.id,
-        "title" => alert.title,
-        "query" => alert.query,
-        "operator" => "<",
-        "threshold" => alert.threshold,
-        "alert_path" => Gitlab::Routing.url_helpers.project_prometheus_alert_path(project, alert.prometheus_metric_id, environment_id: alert.environment.id, format: :json)
-      }
-
-      expect do
-        put :update, params: project_params(id: alert.prometheus_metric_id, operator: "<")
-      end.to change { alert.reload.operator }.to("lt")
+      expect { make_request(operator: '<') }
+        .to change { alert.reload.operator }.to('lt')
 
       expect(schedule_update_service).to have_received(:execute)
-      expect(response).to have_gitlab_http_status(200)
-      expect(JSON.parse(response.body)).to include(alert_params)
+      expect(response).to have_gitlab_http_status(:ok)
+      expect(json_response).to include(alert_params)
     end
+
+    it_behaves_like 'unlicensed'
+    it_behaves_like 'project non-specific environment', :not_found
+    it_behaves_like 'project non-specific metric', :not_found
   end
 
   describe 'DELETE #destroy' do
     let(:schedule_update_service) { spy }
-    let!(:alert) { create(:prometheus_alert, project: project, prometheus_metric: metric) }
 
-    before do
-      allow(::Clusters::Applications::ScheduleUpdateService).to receive(:new).and_return(schedule_update_service)
+    let!(:alert) do
+      create(:prometheus_alert, project: project, prometheus_metric: metric)
     end
 
-    it 'renders forbidden when unlicensed' do
-      stub_licensed_features(prometheus_alerts: false)
-
-      delete :destroy, params: project_params(id: alert.prometheus_metric_id)
+    before do
+      allow(::Clusters::Applications::ScheduleUpdateService)
+        .to receive(:new).and_return(schedule_update_service)
+    end
 
-      expect(response).to have_gitlab_http_status(:not_found)
+    def make_request(opts = {})
+      delete :destroy, params: request_params(
+        opts,
+        id: alert.prometheus_metric_id,
+        environment_id: alert.environment
+      )
     end
 
     it 'destroys the specified prometheus alert' do
-      expect do
-        delete :destroy, params: project_params(id: alert.prometheus_metric_id)
-      end.to change { PrometheusAlert.count }.from(1).to(0)
+      expect { make_request }.to change { PrometheusAlert.count }.by(-1)
 
       expect(schedule_update_service).to have_received(:execute)
     end
+
+    it_behaves_like 'unlicensed'
+    it_behaves_like 'project non-specific environment', :not_found
+    it_behaves_like 'project non-specific metric', :not_found
   end
 
   def project_params(opts = {})
     opts.reverse_merge(namespace_id: project.namespace, project_id: project)
   end
+
+  def request_params(opts = {}, defaults = {})
+    project_params(opts.reverse_merge(defaults))
+  end
+
+  def alert_path(alert)
+    project_prometheus_alert_path(
+      project,
+      alert.prometheus_metric_id,
+      environment_id: alert.environment,
+      format: :json
+    )
+  end
 end

ee/spec/migrations/allow_prometheus_alerts_per_environment_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('ee', 'db', 'migrate', '20180912113336_allow_prometheus_alerts_per_environment.rb')
+
+describe AllowPrometheusAlertsPerEnvironment, :migration do
+  let(:migration) { described_class.new }
+  let(:namespaces) { table(:namespaces) }
+  let(:projects) { table(:projects) }
+  let(:environments) { table(:environments) }
+  let(:prometheus_metrics) { table(:prometheus_metrics) }
+  let(:prometheus_alerts) { table(:prometheus_alerts) }
+  let(:now) { Time.now }
+
+  let(:old_index_columns) { %i[project_id prometheus_metric_id] }
+  let(:new_index_columns) { %i[project_id prometheus_metric_id environment_id] }
+  let(:new_index_name) { described_class::INDEX_METRIC_ENVIRONMENT_NAME }
+
+  describe '#up' do
+    it 'creates a wider index dropping the narrower one' do
+      migration.up
+
+      expect(unique_index?(new_index_columns, name: new_index_name))
+        .to eq(true)
+
+      expect(unique_index?(old_index_columns)).to eq(false)
+    end
+  end
+
+  describe '#down' do
+    let(:ns) { namespace(id: 1, name: 'ns') }
+    let(:a) { project(id: 1, name: 'a') }
+    let(:b) { project(id: 2, name: 'b') }
+    let(:c) { project(id: 3, name: 'c') }
+    let(:a_prd) { environment(id: 11, name: 'a_prd', project: a) }
+    let(:a_stg) { environment(id: 12, name: 'a_stg', project: a) }
+    let(:a_can) { environment(id: 13, name: 'a_can', project: a) }
+    let(:b_prd) { environment(id: 14, name: 'b_prd', project: b) }
+    let(:b_stg) { environment(id: 15, name: 'b_stg', project: b) }
+    let(:c_prd) { environment(id: 16, name: 'c_prd', project: c) }
+    let(:metric_a) { metric(id: 21, project: a) }
+    let(:metric_b) { metric(id: 22, project: b) }
+    let(:metric_c) { metric(id: 23, project: c) }
+
+    let(:alert_a_prd) { alert(id: 31, metric: metric_a, environment: a_prd) }
+    let(:alert_a_stg) { alert(id: 32, metric: metric_a, environment: a_stg) }
+    let(:alert_a_can) { alert(id: 33, metric: metric_a, environment: a_can) }
+    let(:alert_b_stg) { alert(id: 34, metric: metric_b, environment: b_stg) }
+    let(:alert_b_prd) { alert(id: 35, metric: metric_b, environment: b_prd) }
+    let(:alert_c_prd) { alert(id: 36, metric: metric_c, environment: c_prd) }
+
+    before do
+      # Migration up to allow multiple alerts per environment
+      schema_migrate_up!
+    end
+
+    it 'deletes duplicate alerts before narrowing the index' do
+      # create
+      alert_a_prd
+      alert_a_stg
+      alert_a_can
+      alert_b_prd
+      alert_b_stg
+      alert_c_prd
+
+      migration.down
+
+      expect(unique_index?(old_index_columns, unique: true)).to eq(true)
+
+      expect(unique_index?(new_index_columns, name: new_index_name))
+        .to eq(false)
+
+      expect(prometheus_alerts.all.to_a)
+        .to contain_exactly(alert_a_prd, alert_b_stg, alert_c_prd)
+    end
+  end
+
+  private
+
+  def unique_index?(columns, opts = {})
+    migration.index_exists?(:prometheus_alerts,
+                            columns, opts.merge(unique: true))
+  end
+
+  def namespace(id:, name:)
+    namespaces.create!(id: id, name: name, path: name)
+  end
+
+  def project(id:, name:)
+    projects.create!(id: id, name: name, path: name, namespace_id: ns.id)
+  end
+
+  def environment(id:, name:, project:)
+    environments.create!(id: id, name: name, slug: name,
+                         project_id: project.id)
+  end
+
+  def metric(id:, project:)
+    prometheus_metrics.create!(
+      id: id,
+      project_id: project.id,
+      title: 'title',
+      query: 'query',
+      group: 1,
+      created_at: now,
+      updated_at: now,
+      common: false
+    )
+  end
+
+  def alert(id:, metric:, environment:)
+    prometheus_alerts.create!(
+      id: id,
+      project_id: metric.project_id,
+      environment_id: environment.id,
+      prometheus_metric_id: metric.id,
+      threshold: 1.0,
+      operator: '=',
+      created_at: now,
+      updated_at: now
+    )
+  end
+end