From 72339077f7d79eeb9d0612c55236aa4aa6da4084 Mon Sep 17 00:00:00 2001
From: Thong Kuah <tkuah@gitlab.com>
Date: Thu, 20 Jun 2019 14:03:21 +1200
Subject: Add failing test showing N+1

We have an N+1 problem where N is environments.
---
 spec/controllers/projects/merge_requests_controller_spec.rb | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index 0eca663a683..1120bf6dcb0 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -878,6 +878,18 @@ describe Projects::MergeRequestsController do
         expect(control_count).to be <= 137
       end
 
+      it 'has no N+1 issues for environments', :request_store do
+        # First run to insert test data from lets, which does take up some 30 queries
+        get_ci_environments_status
+
+        control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) { get_ci_environments_status }.count
+
+        environment2 = create(:environment, project: forked)
+        create(:deployment, :succeed, environment: environment2, sha: sha, ref: 'master', deployable: build)
+
+        expect { get_ci_environments_status }.not_to exceed_all_query_limit(control_count)
+      end
+
       def get_ci_environments_status(extra_params = {})
         params = {
           namespace_id: merge_request.project.namespace.to_param,
-- 
cgit v1.2.1


From 1668f40f430c656ed9c20898605db21a66cb5937 Mon Sep 17 00:00:00 2001
From: Thong Kuah <tkuah@gitlab.com>
Date: Mon, 1 Jul 2019 20:24:20 +1200
Subject: Remove fallback to project.deployment_platform

This improves query performance of
MergeRequestsController#ci_environments_status a lot.

However this means old deployments that deployed to kubernetes clusters
with prometheus installations will no longer show performance metrics as
we cannot backfill cluster_id from deployment_platform with certainty
(clusters may be edited/added/deleted, which changes the results of
deployment_platform).
---
 app/models/deployment.rb                |  7 +------
 changelogs/unreleased/63475-fix-n-1.yml |  5 +++++
 spec/models/deployment_spec.rb          | 12 ------------
 3 files changed, 6 insertions(+), 18 deletions(-)
 create mode 100644 changelogs/unreleased/63475-fix-n-1.yml

diff --git a/app/models/deployment.rb b/app/models/deployment.rb
index a8f5642f726..3b31e7b1333 100644
--- a/app/models/deployment.rb
+++ b/app/models/deployment.rb
@@ -206,13 +206,8 @@ class Deployment < ApplicationRecord
     end
   end
 
-  # TODO remove fallback case to deployment_platform_cluster.
-  # Otherwise we will continue to pay the performance penalty described in
-  # https://gitlab.com/gitlab-org/gitlab-ce/issues/63475
   def cluster_prometheus
-    cluster_with_fallback = cluster || deployment_platform_cluster
-
-    cluster_with_fallback.application_prometheus if cluster_with_fallback&.application_prometheus_available?
+    cluster.application_prometheus if cluster&.application_prometheus_available?
   end
 
   def ref_path
diff --git a/changelogs/unreleased/63475-fix-n-1.yml b/changelogs/unreleased/63475-fix-n-1.yml
new file mode 100644
index 00000000000..3ed825290fd
--- /dev/null
+++ b/changelogs/unreleased/63475-fix-n-1.yml
@@ -0,0 +1,5 @@
+---
+title: Improve performance of MergeRequestsController#ci_environment_status endpoint
+merge_request: 30224
+author:
+type: performance
diff --git a/spec/models/deployment_spec.rb b/spec/models/deployment_spec.rb
index 713fb647708..a926e1913a7 100644
--- a/spec/models/deployment_spec.rb
+++ b/spec/models/deployment_spec.rb
@@ -341,18 +341,6 @@ describe Deployment do
 
         it { is_expected.to be_truthy }
       end
-
-      context 'fallback deployment platform' do
-        let(:cluster) { create(:cluster, :provided_by_user, environment_scope: '*', projects: [deployment.project]) }
-        let!(:prometheus) { create(:clusters_applications_prometheus, :installed, cluster: cluster) }
-
-        before do
-          expect(deployment.project).to receive(:deployment_platform).and_return(cluster.platform)
-          expect(cluster.application_prometheus).to receive(:can_query?).and_return(true)
-        end
-
-        it { is_expected.to be_truthy }
-      end
     end
   end
 
-- 
cgit v1.2.1


From 1b5b0dea5228ae7fd520c8bca3f03c4799a4d31d Mon Sep 17 00:00:00 2001
From: Thong Kuah <tkuah@gitlab.com>
Date: Mon, 1 Jul 2019 20:27:58 +1200
Subject: Share project object in EnvironmentStatus

Otherwise, each EnvironmentStatus object instantiates its own project
when really they are the same. Improves query count performance
---
 app/models/environment_status.rb                   |  8 ++++----
 spec/models/environment_status_spec.rb             |  3 +--
 spec/serializers/environment_status_entity_spec.rb | 10 ++++++++--
 3 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/app/models/environment_status.rb b/app/models/environment_status.rb
index 2fb6cadc8cd..2c71520cec1 100644
--- a/app/models/environment_status.rb
+++ b/app/models/environment_status.rb
@@ -3,11 +3,10 @@
 class EnvironmentStatus
   include Gitlab::Utils::StrongMemoize
 
-  attr_reader :environment, :merge_request, :sha
+  attr_reader :project, :environment, :merge_request, :sha
 
   delegate :id, to: :environment
   delegate :name, to: :environment
-  delegate :project, to: :environment
   delegate :status, to: :deployment, allow_nil: true
   delegate :deployed_at, to: :deployment, allow_nil: true
 
@@ -21,7 +20,8 @@ class EnvironmentStatus
     build_environments_status(mr, user, mr.merge_pipeline)
   end
 
-  def initialize(environment, merge_request, sha)
+  def initialize(project, environment, merge_request, sha)
+    @project = project
     @environment = environment
     @merge_request = merge_request
     @sha = sha
@@ -67,7 +67,7 @@ class EnvironmentStatus
     pipeline.environments.available.map do |environment|
       next unless Ability.allowed?(user, :read_environment, environment)
 
-      EnvironmentStatus.new(environment, mr, pipeline.sha)
+      EnvironmentStatus.new(pipeline.project, environment, mr, pipeline.sha)
     end.compact
   end
   private_class_method :build_environments_status
diff --git a/spec/models/environment_status_spec.rb b/spec/models/environment_status_spec.rb
index c503c35305f..e2836420df9 100644
--- a/spec/models/environment_status_spec.rb
+++ b/spec/models/environment_status_spec.rb
@@ -11,11 +11,10 @@ describe EnvironmentStatus do
   let(:merge_request) { create(:merge_request, :deployed_review_app, deployment: deployment) }
   let(:sha)           { deployment.sha }
 
-  subject(:environment_status) { described_class.new(environment, merge_request, sha) }
+  subject(:environment_status) { described_class.new(project, environment, merge_request, sha) }
 
   it { is_expected.to delegate_method(:id).to(:environment) }
   it { is_expected.to delegate_method(:name).to(:environment) }
-  it { is_expected.to delegate_method(:project).to(:environment) }
   it { is_expected.to delegate_method(:deployed_at).to(:deployment) }
   it { is_expected.to delegate_method(:status).to(:deployment) }
 
diff --git a/spec/serializers/environment_status_entity_spec.rb b/spec/serializers/environment_status_entity_spec.rb
index 8a6a38fe5f8..f421432e8d6 100644
--- a/spec/serializers/environment_status_entity_spec.rb
+++ b/spec/serializers/environment_status_entity_spec.rb
@@ -9,7 +9,7 @@ describe EnvironmentStatusEntity do
   let(:project)       { deployment.project }
   let(:merge_request) { create(:merge_request, :deployed_review_app, deployment: deployment) }
 
-  let(:environment_status) { EnvironmentStatus.new(environment, merge_request, merge_request.diff_head_sha) }
+  let(:environment_status) { EnvironmentStatus.new(project, environment, merge_request, merge_request.diff_head_sha) }
   let(:entity)             { described_class.new(environment_status, request: request) }
 
   subject { entity.as_json }
@@ -55,8 +55,14 @@ describe EnvironmentStatusEntity do
     before do
       project.add_maintainer(user)
       allow(deployment).to receive(:prometheus_adapter).and_return(prometheus_adapter)
-      allow(prometheus_adapter).to receive(:query).with(:deployment, deployment).and_return(simple_metrics)
       allow(entity).to receive(:deployment).and_return(deployment)
+
+      expect_next_instance_of(DeploymentMetrics) do |deployment_metrics|
+        allow(deployment_metrics).to receive(:prometheus_adapter).and_return(prometheus_adapter)
+
+        allow(prometheus_adapter).to receive(:query)
+          .with(:deployment, deployment).and_return(simple_metrics)
+      end
     end
 
     context 'when deployment succeeded' do
-- 
cgit v1.2.1


From d2ba2951f737082edd568505f985ebf9a0808be7 Mon Sep 17 00:00:00 2001
From: Thong Kuah <tkuah@gitlab.com>
Date: Mon, 1 Jul 2019 21:40:59 +1200
Subject: Extract deployment_metrics into own object

We can now share project so that we don't have to load project twice.
Also, this extracts non-relevant logic out of Deployment.

Update DeploymentsController accordingly
---
 app/controllers/projects/deployments_controller.rb |  14 +--
 app/models/deployment.rb                           |  32 ------
 app/models/deployment_metrics.rb                   |  50 +++++++++
 app/models/environment_status.rb                   |   4 +
 app/serializers/environment_status_entity.rb       |   6 +-
 .../projects/deployments_controller_spec.rb        |  74 +++++--------
 spec/models/deployment_metrics_spec.rb             | 114 +++++++++++++++++++++
 spec/models/deployment_spec.rb                     | 107 -------------------
 8 files changed, 205 insertions(+), 196 deletions(-)
 create mode 100644 app/models/deployment_metrics.rb
 create mode 100644 spec/models/deployment_metrics_spec.rb

diff --git a/app/controllers/projects/deployments_controller.rb b/app/controllers/projects/deployments_controller.rb
index 0a009477d61..32111b07a0b 100644
--- a/app/controllers/projects/deployments_controller.rb
+++ b/app/controllers/projects/deployments_controller.rb
@@ -15,24 +15,22 @@ class Projects::DeploymentsController < Projects::ApplicationController
   # rubocop: enable CodeReuse/ActiveRecord
 
   def metrics
-    return render_404 unless deployment.has_metrics?
+    return render_404 unless deployment_metrics.has_metrics?
 
-    @metrics = deployment.metrics
+    @metrics = deployment_metrics.metrics
     if @metrics&.any?
       render json: @metrics, status: :ok
     else
       head :no_content
     end
-  rescue NotImplementedError
-    render_404
   end
 
   def additional_metrics
-    return render_404 unless deployment.has_metrics?
+    return render_404 unless deployment_metrics.has_metrics?
 
     respond_to do |format|
       format.json do
-        metrics = deployment.additional_metrics
+        metrics = deployment_metrics.additional_metrics
 
         if metrics.any?
           render json: metrics
@@ -45,6 +43,10 @@ class Projects::DeploymentsController < Projects::ApplicationController
 
   private
 
+  def deployment_metrics
+    @deployment_metrics ||= DeploymentMetrics.new(deployment.project, deployment)
+  end
+
   # rubocop: disable CodeReuse/ActiveRecord
   def deployment
     @deployment ||= environment.deployments.find_by(iid: params[:id])
diff --git a/app/models/deployment.rb b/app/models/deployment.rb
index 3b31e7b1333..ad32ec3f1d5 100644
--- a/app/models/deployment.rb
+++ b/app/models/deployment.rb
@@ -176,40 +176,8 @@ class Deployment < ApplicationRecord
     deployed_at&.to_time&.in_time_zone&.to_s(:medium)
   end
 
-  def has_metrics?
-    success? && prometheus_adapter&.can_query?
-  end
-
-  def metrics
-    return {} unless has_metrics?
-
-    metrics = prometheus_adapter.query(:deployment, self)
-    metrics&.merge(deployment_time: finished_at.to_i) || {}
-  end
-
-  def additional_metrics
-    return {} unless has_metrics?
-
-    metrics = prometheus_adapter.query(:additional_metrics_deployment, self)
-    metrics&.merge(deployment_time: finished_at.to_i) || {}
-  end
-
   private
 
-  def prometheus_adapter
-    service = project.find_or_initialize_service('prometheus')
-
-    if service.can_query?
-      service
-    else
-      cluster_prometheus
-    end
-  end
-
-  def cluster_prometheus
-    cluster.application_prometheus if cluster&.application_prometheus_available?
-  end
-
   def ref_path
     File.join(environment.ref_path, 'deployments', iid.to_s)
   end
diff --git a/app/models/deployment_metrics.rb b/app/models/deployment_metrics.rb
new file mode 100644
index 00000000000..2056c8bc59c
--- /dev/null
+++ b/app/models/deployment_metrics.rb
@@ -0,0 +1,50 @@
+# frozen_string_literal: true
+
+class DeploymentMetrics
+  include Gitlab::Utils::StrongMemoize
+
+  attr_reader :project, :deployment
+
+  delegate :cluster, to: :deployment
+
+  def initialize(project, deployment)
+    @project = project
+    @deployment = deployment
+  end
+
+  def has_metrics?
+    deployment.success? && prometheus_adapter&.can_query?
+  end
+
+  def metrics
+    return {} unless has_metrics?
+
+    metrics = prometheus_adapter.query(:deployment, deployment)
+    metrics&.merge(deployment_time: deployment.finished_at.to_i) || {}
+  end
+
+  def additional_metrics
+    return {} unless has_metrics?
+
+    metrics = prometheus_adapter.query(:additional_metrics_deployment, deployment)
+    metrics&.merge(deployment_time: deployment.finished_at.to_i) || {}
+  end
+
+  private
+
+  def prometheus_adapter
+    strong_memoize(:prometheus_adapter) do
+      service = project.find_or_initialize_service('prometheus')
+
+      if service.can_query?
+        service
+      else
+        cluster_prometheus
+      end
+    end
+  end
+
+  def cluster_prometheus
+    cluster.application_prometheus if cluster&.application_prometheus_available?
+  end
+end
diff --git a/app/models/environment_status.rb b/app/models/environment_status.rb
index 2c71520cec1..1b3c094901b 100644
--- a/app/models/environment_status.rb
+++ b/app/models/environment_status.rb
@@ -33,6 +33,10 @@ class EnvironmentStatus
     end
   end
 
+  def has_metrics?
+    DeploymentMetrics.new(project, deployment).has_metrics?
+  end
+
   def changes
     return [] if project.route_map_for(sha).nil?
 
diff --git a/app/serializers/environment_status_entity.rb b/app/serializers/environment_status_entity.rb
index f6321b9e520..811cc2ad5af 100644
--- a/app/serializers/environment_status_entity.rb
+++ b/app/serializers/environment_status_entity.rb
@@ -11,7 +11,7 @@ class EnvironmentStatusEntity < Grape::Entity
     project_environment_path(es.project, es.environment)
   end
 
-  expose :metrics_url, if: ->(*) { can_read_environment? && deployment.has_metrics? } do |es|
+  expose :metrics_url, if: ->(*) { can_read_environment? && has_metrics? } do |es|
     metrics_project_environment_deployment_path(es.project, es.environment, es.deployment)
   end
 
@@ -45,8 +45,8 @@ class EnvironmentStatusEntity < Grape::Entity
     object.environment
   end
 
-  def deployment
-    object.deployment
+  def has_metrics?
+    object.has_metrics?
   end
 
   def project
diff --git a/spec/controllers/projects/deployments_controller_spec.rb b/spec/controllers/projects/deployments_controller_spec.rb
index 95417936df4..b9ee69a617b 100644
--- a/spec/controllers/projects/deployments_controller_spec.rb
+++ b/spec/controllers/projects/deployments_controller_spec.rb
@@ -41,34 +41,26 @@ describe Projects::DeploymentsController do
   describe 'GET #metrics' do
     let(:deployment) { create(:deployment, :success, project: project, environment: environment) }
 
-    before do
-      allow(controller).to receive(:deployment).and_return(deployment)
-    end
-
     context 'when metrics are disabled' do
-      before do
-        allow(deployment).to receive(:has_metrics?).and_return false
-      end
-
       it 'responds with not found' do
-        get :metrics, params: deployment_params(id: deployment.id)
+        get :metrics, params: deployment_params(id: deployment.to_param)
 
         expect(response).to be_not_found
       end
     end
 
     context 'when metrics are enabled' do
-      before do
-        allow(deployment).to receive(:has_metrics?).and_return true
-      end
-
       context 'when environment has no metrics' do
         before do
-          expect(deployment).to receive(:metrics).and_return(nil)
+          expect_next_instance_of(DeploymentMetrics) do |deployment_metrics|
+            allow(deployment_metrics).to receive(:has_metrics?).and_return(true)
+
+            expect(deployment_metrics).to receive(:metrics).and_return(nil)
+          end
         end
 
         it 'returns a empty response 204 resposne' do
-          get :metrics, params: deployment_params(id: deployment.id)
+          get :metrics, params: deployment_params(id: deployment.to_param)
           expect(response).to have_gitlab_http_status(204)
           expect(response.body).to eq('')
         end
@@ -84,11 +76,15 @@ describe Projects::DeploymentsController do
         end
 
         before do
-          expect(deployment).to receive(:metrics).and_return(empty_metrics)
+          expect_next_instance_of(DeploymentMetrics) do |deployment_metrics|
+            allow(deployment_metrics).to receive(:has_metrics?).and_return(true)
+
+            expect(deployment_metrics).to receive(:metrics).and_return(empty_metrics)
+          end
         end
 
         it 'returns a metrics JSON document' do
-          get :metrics, params: deployment_params(id: deployment.id)
+          get :metrics, params: deployment_params(id: deployment.to_param)
 
           expect(response).to be_ok
           expect(json_response['success']).to be(true)
@@ -96,54 +92,32 @@ describe Projects::DeploymentsController do
           expect(json_response['last_update']).to eq(42)
         end
       end
-
-      context 'when metrics service does not implement deployment metrics' do
-        before do
-          allow(deployment).to receive(:metrics).and_raise(NotImplementedError)
-        end
-
-        it 'responds with not found' do
-          get :metrics, params: deployment_params(id: deployment.id)
-
-          expect(response).to be_not_found
-        end
-      end
     end
   end
 
   describe 'GET #additional_metrics' do
     let(:deployment) { create(:deployment, :success, project: project, environment: environment) }
 
-    before do
-      allow(controller).to receive(:deployment).and_return(deployment)
-    end
-
     context 'when metrics are disabled' do
-      before do
-        allow(deployment).to receive(:has_metrics?).and_return false
-      end
-
       it 'responds with not found' do
-        get :metrics, params: deployment_params(id: deployment.id)
+        get :metrics, params: deployment_params(id: deployment.to_param)
 
         expect(response).to be_not_found
       end
     end
 
     context 'when metrics are enabled' do
-      let(:prometheus_adapter) { double('prometheus_adapter', can_query?: true) }
-
-      before do
-        allow(deployment).to receive(:prometheus_adapter).and_return(prometheus_adapter)
-      end
-
       context 'when environment has no metrics' do
         before do
-          expect(deployment).to receive(:additional_metrics).and_return({})
+          expect_next_instance_of(DeploymentMetrics) do |deployment_metrics|
+            allow(deployment_metrics).to receive(:has_metrics?).and_return(true)
+
+            expect(deployment_metrics).to receive(:additional_metrics).and_return({})
+          end
         end
 
         it 'returns a empty response 204 response' do
-          get :additional_metrics, params: deployment_params(id: deployment.id, format: :json)
+          get :additional_metrics, params: deployment_params(id: deployment.to_param, format: :json)
           expect(response).to have_gitlab_http_status(204)
           expect(response.body).to eq('')
         end
@@ -159,11 +133,15 @@ describe Projects::DeploymentsController do
         end
 
         before do
-          expect(deployment).to receive(:additional_metrics).and_return(empty_metrics)
+          expect_next_instance_of(DeploymentMetrics) do |deployment_metrics|
+            allow(deployment_metrics).to receive(:has_metrics?).and_return(true)
+
+            expect(deployment_metrics).to receive(:additional_metrics).and_return(empty_metrics)
+          end
         end
 
         it 'returns a metrics JSON document' do
-          get :additional_metrics, params: deployment_params(id: deployment.id, format: :json)
+          get :additional_metrics, params: deployment_params(id: deployment.to_param, format: :json)
 
           expect(response).to be_ok
           expect(json_response['success']).to be(true)
diff --git a/spec/models/deployment_metrics_spec.rb b/spec/models/deployment_metrics_spec.rb
new file mode 100644
index 00000000000..7c574a8b6c8
--- /dev/null
+++ b/spec/models/deployment_metrics_spec.rb
@@ -0,0 +1,114 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe DeploymentMetrics do
+  describe '#has_metrics?' do
+    subject { described_class.new(deployment.project, deployment).has_metrics? }
+
+    context 'when deployment is failed' do
+      let(:deployment) { create(:deployment, :failed) }
+
+      it { is_expected.to be_falsy }
+    end
+
+    context 'when deployment is success' do
+      let(:deployment) { create(:deployment, :success) }
+
+      context 'without a monitoring service' do
+        it { is_expected.to be_falsy }
+      end
+
+      context 'with a Prometheus Service' do
+        let(:prometheus_service) { instance_double(PrometheusService, can_query?: true) }
+
+        before do
+          allow(deployment.project).to receive(:find_or_initialize_service).with('prometheus').and_return prometheus_service
+        end
+
+        it { is_expected.to be_truthy }
+      end
+
+      context 'with a Prometheus Service that cannot query' do
+        let(:prometheus_service) { instance_double(PrometheusService, can_query?: false) }
+
+        before do
+          allow(deployment.project).to receive(:find_or_initialize_service).with('prometheus').and_return prometheus_service
+        end
+
+        it { is_expected.to be_falsy }
+      end
+
+      context 'with a cluster Prometheus' do
+        let(:deployment) { create(:deployment, :success, :on_cluster) }
+        let!(:prometheus) { create(:clusters_applications_prometheus, :installed, cluster: deployment.cluster) }
+
+        before do
+          expect(deployment.cluster.application_prometheus).to receive(:can_query?).and_return(true)
+        end
+
+        it { is_expected.to be_truthy }
+      end
+    end
+  end
+
+  describe '#metrics' do
+    let(:deployment) { create(:deployment, :success) }
+    let(:prometheus_adapter) { instance_double(PrometheusService, can_query?: true) }
+    let(:deployment_metrics) { described_class.new(deployment.project, deployment) }
+
+    subject { deployment_metrics.metrics }
+
+    context 'metrics are disabled' do
+      it { is_expected.to eq({}) }
+    end
+
+    context 'metrics are enabled' do
+      let(:simple_metrics) do
+        {
+          success: true,
+          metrics: {},
+          last_update: 42
+        }
+      end
+
+      before do
+        allow(deployment_metrics).to receive(:prometheus_adapter).and_return(prometheus_adapter)
+        expect(prometheus_adapter).to receive(:query).with(:deployment, deployment).and_return(simple_metrics)
+      end
+
+      it { is_expected.to eq(simple_metrics.merge({ deployment_time: deployment.created_at.to_i })) }
+    end
+  end
+
+  describe '#additional_metrics' do
+    let(:project) { create(:project, :repository) }
+    let(:deployment) { create(:deployment, :succeed, project: project) }
+    let(:deployment_metrics) { described_class.new(deployment.project, deployment) }
+
+    subject { deployment_metrics.additional_metrics }
+
+    context 'metrics are disabled' do
+      it { is_expected.to eq({}) }
+    end
+
+    context 'metrics are enabled' do
+      let(:simple_metrics) do
+        {
+          success: true,
+          metrics: {},
+          last_update: 42
+        }
+      end
+
+      let(:prometheus_adapter) { instance_double('prometheus_adapter', can_query?: true) }
+
+      before do
+        allow(deployment_metrics).to receive(:prometheus_adapter).and_return(prometheus_adapter)
+        expect(prometheus_adapter).to receive(:query).with(:additional_metrics_deployment, deployment).and_return(simple_metrics)
+      end
+
+      it { is_expected.to eq(simple_metrics.merge({ deployment_time: deployment.created_at.to_i })) }
+    end
+  end
+end
diff --git a/spec/models/deployment_spec.rb b/spec/models/deployment_spec.rb
index a926e1913a7..79647c5719c 100644
--- a/spec/models/deployment_spec.rb
+++ b/spec/models/deployment_spec.rb
@@ -295,113 +295,6 @@ describe Deployment do
     end
   end
 
-  describe '#has_metrics?' do
-    subject { deployment.has_metrics? }
-
-    context 'when deployment is failed' do
-      let(:deployment) { create(:deployment, :failed) }
-
-      it { is_expected.to be_falsy }
-    end
-
-    context 'when deployment is success' do
-      let(:deployment) { create(:deployment, :success) }
-
-      context 'without a monitoring service' do
-        it { is_expected.to be_falsy }
-      end
-
-      context 'with a Prometheus Service' do
-        let(:prometheus_service) { double(:prometheus_service, can_query?: true) }
-
-        before do
-          allow(deployment.project).to receive(:find_or_initialize_service).with('prometheus').and_return prometheus_service
-        end
-
-        it { is_expected.to be_truthy }
-      end
-
-      context 'with a Prometheus Service that cannot query' do
-        let(:prometheus_service) { double(:prometheus_service, can_query?: false) }
-
-        before do
-          allow(deployment.project).to receive(:find_or_initialize_service).with('prometheus').and_return prometheus_service
-        end
-
-        it { is_expected.to be_falsy }
-      end
-
-      context 'with a cluster Prometheus' do
-        let(:deployment) { create(:deployment, :success, :on_cluster) }
-        let!(:prometheus) { create(:clusters_applications_prometheus, :installed, cluster: deployment.cluster) }
-
-        before do
-          expect(deployment.cluster.application_prometheus).to receive(:can_query?).and_return(true)
-        end
-
-        it { is_expected.to be_truthy }
-      end
-    end
-  end
-
-  describe '#metrics' do
-    let(:deployment) { create(:deployment, :success) }
-    let(:prometheus_adapter) { double('prometheus_adapter', can_query?: true) }
-
-    subject { deployment.metrics }
-
-    context 'metrics are disabled' do
-      it { is_expected.to eq({}) }
-    end
-
-    context 'metrics are enabled' do
-      let(:simple_metrics) do
-        {
-          success: true,
-          metrics: {},
-          last_update: 42
-        }
-      end
-
-      before do
-        allow(deployment).to receive(:prometheus_adapter).and_return(prometheus_adapter)
-        allow(prometheus_adapter).to receive(:query).with(:deployment, deployment).and_return(simple_metrics)
-      end
-
-      it { is_expected.to eq(simple_metrics.merge({ deployment_time: deployment.created_at.to_i })) }
-    end
-  end
-
-  describe '#additional_metrics' do
-    let(:project) { create(:project, :repository) }
-    let(:deployment) { create(:deployment, :succeed, project: project) }
-
-    subject { deployment.additional_metrics }
-
-    context 'metrics are disabled' do
-      it { is_expected.to eq({}) }
-    end
-
-    context 'metrics are enabled' do
-      let(:simple_metrics) do
-        {
-          success: true,
-          metrics: {},
-          last_update: 42
-        }
-      end
-
-      let(:prometheus_adapter) { double('prometheus_adapter', can_query?: true) }
-
-      before do
-        allow(deployment).to receive(:prometheus_adapter).and_return(prometheus_adapter)
-        allow(prometheus_adapter).to receive(:query).with(:additional_metrics_deployment, deployment).and_return(simple_metrics)
-      end
-
-      it { is_expected.to eq(simple_metrics.merge({ deployment_time: deployment.created_at.to_i })) }
-    end
-  end
-
   describe '#stop_action' do
     let(:build) { create(:ci_build) }
 
-- 
cgit v1.2.1


From a2cd33d1baa3de0774ebfcff2e6bbffffcb5d025 Mon Sep 17 00:00:00 2001
From: Thong Kuah <tkuah@gitlab.com>
Date: Mon, 1 Jul 2019 21:45:17 +1200
Subject: Could not address last 5 queries

Split into followup issue
---
 spec/controllers/projects/merge_requests_controller_spec.rb | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index 1120bf6dcb0..ea160a21955 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -887,7 +887,10 @@ describe Projects::MergeRequestsController do
         environment2 = create(:environment, project: forked)
         create(:deployment, :succeed, environment: environment2, sha: sha, ref: 'master', deployable: build)
 
-        expect { get_ci_environments_status }.not_to exceed_all_query_limit(control_count)
+        # TODO address the last 5 queries
+        # See https://gitlab.com/gitlab-org/gitlab-ce/issues/63952
+        leeway = 5
+        expect { get_ci_environments_status }.not_to exceed_all_query_limit(control_count + leeway)
       end
 
       def get_ci_environments_status(extra_params = {})
-- 
cgit v1.2.1


From 25ba11a2aad9571e4759cdc711199634e3392c5b Mon Sep 17 00:00:00 2001
From: Thong Kuah <tkuah@gitlab.com>
Date: Mon, 1 Jul 2019 22:10:15 +1200
Subject: Remove un-used method

We stopped calling the fallback so we can remove this now
---
 app/models/deployment.rb       |  5 -----
 spec/models/deployment_spec.rb | 28 ----------------------------
 2 files changed, 33 deletions(-)

diff --git a/app/models/deployment.rb b/app/models/deployment.rb
index ad32ec3f1d5..b69cda4f2f9 100644
--- a/app/models/deployment.rb
+++ b/app/models/deployment.rb
@@ -85,11 +85,6 @@ class Deployment < ApplicationRecord
     Commit.truncate_sha(sha)
   end
 
-  # Deprecated - will be replaced by a persisted cluster_id
-  def deployment_platform_cluster
-    environment.deployment_platform&.cluster
-  end
-
   def execute_hooks
     deployment_data = Gitlab::DataBuilder::Deployment.build(self)
     project.execute_services(deployment_data, :deployment_hooks)
diff --git a/spec/models/deployment_spec.rb b/spec/models/deployment_spec.rb
index 79647c5719c..d4e631f109b 100644
--- a/spec/models/deployment_spec.rb
+++ b/spec/models/deployment_spec.rb
@@ -322,32 +322,4 @@ describe Deployment do
       end
     end
   end
-
-  describe '#deployment_platform_cluster' do
-    let(:deployment) { create(:deployment) }
-    let(:project) { deployment.project }
-    let(:environment) { deployment.environment }
-
-    subject { deployment.deployment_platform_cluster }
-
-    before do
-      expect(project).to receive(:deployment_platform)
-        .with(environment: environment.name).and_call_original
-    end
-
-    context 'project has no deployment platform' do
-      before do
-        expect(project.clusters).to be_empty
-      end
-
-      it { is_expected.to be_nil }
-    end
-
-    context 'project has a deployment platform' do
-      let!(:cluster) { create(:cluster, projects: [project]) }
-      let!(:platform) { create(:cluster_platform_kubernetes, cluster: cluster) }
-
-      it { is_expected.to eq cluster }
-    end
-  end
 end
-- 
cgit v1.2.1


From c85d6b0c744e9a971fafdf58328f907dfa0de127 Mon Sep 17 00:00:00 2001
From: Thong Kuah <tkuah@gitlab.com>
Date: Thu, 4 Jul 2019 17:26:37 +1200
Subject: Restore fallback to deployment_platform_cluster

In 12.2 we will remove this fallback.
---
 app/models/deployment_metrics.rb                            | 13 ++++++++++++-
 spec/controllers/projects/merge_requests_controller_spec.rb |  7 ++++---
 spec/models/deployment_metrics_spec.rb                      | 12 ++++++++++++
 3 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/app/models/deployment_metrics.rb b/app/models/deployment_metrics.rb
index 2056c8bc59c..cfe762ca25e 100644
--- a/app/models/deployment_metrics.rb
+++ b/app/models/deployment_metrics.rb
@@ -44,7 +44,18 @@ class DeploymentMetrics
     end
   end
 
+  # TODO remove fallback case to deployment_platform_cluster.
+  # Otherwise we will continue to pay the performance penalty described in
+  # https://gitlab.com/gitlab-org/gitlab-ce/issues/63475
+  #
+  # Removal issue: https://gitlab.com/gitlab-org/gitlab-ce/issues/64105
   def cluster_prometheus
-    cluster.application_prometheus if cluster&.application_prometheus_available?
+    cluster_with_fallback = cluster || deployment_platform_cluster
+
+    cluster_with_fallback.application_prometheus if cluster_with_fallback&.application_prometheus_available?
+  end
+
+  def deployment_platform_cluster
+    deployment.environment.deployment_platform&.cluster
   end
 end
diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index ea160a21955..2a6736018ea 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -887,9 +887,10 @@ describe Projects::MergeRequestsController do
         environment2 = create(:environment, project: forked)
         create(:deployment, :succeed, environment: environment2, sha: sha, ref: 'master', deployable: build)
 
-        # TODO address the last 5 queries
-        # See https://gitlab.com/gitlab-org/gitlab-ce/issues/63952
-        leeway = 5
+        # TODO address the last 11 queries
+        # See https://gitlab.com/gitlab-org/gitlab-ce/issues/63952 (5 queries)
+        # And https://gitlab.com/gitlab-org/gitlab-ce/issues/64105 (6 queries)
+        leeway = 11
         expect { get_ci_environments_status }.not_to exceed_all_query_limit(control_count + leeway)
       end
 
diff --git a/spec/models/deployment_metrics_spec.rb b/spec/models/deployment_metrics_spec.rb
index 7c574a8b6c8..0aadb1f3a5e 100644
--- a/spec/models/deployment_metrics_spec.rb
+++ b/spec/models/deployment_metrics_spec.rb
@@ -49,6 +49,18 @@ describe DeploymentMetrics do
 
         it { is_expected.to be_truthy }
       end
+
+      context 'fallback deployment platform' do
+        let(:cluster) { create(:cluster, :provided_by_user, environment_scope: '*', projects: [deployment.project]) }
+        let!(:prometheus) { create(:clusters_applications_prometheus, :installed, cluster: cluster) }
+
+        before do
+          expect(deployment.project).to receive(:deployment_platform).and_return(cluster.platform)
+          expect(cluster.application_prometheus).to receive(:can_query?).and_return(true)
+        end
+
+        it { is_expected.to be_truthy }
+      end
     end
   end
 
-- 
cgit v1.2.1


From 792455450a1934181369412bc5ba4082c832ed86 Mon Sep 17 00:00:00 2001
From: Thong Kuah <tkuah@gitlab.com>
Date: Mon, 8 Jul 2019 09:19:21 +1200
Subject: BE feedback: memoize deployment_metrics

Also memoize has_metrics? as well, that might be expensive, and it
should not change for the lifetime of EnvironmentStatus
---
 app/models/environment_status.rb | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/app/models/environment_status.rb b/app/models/environment_status.rb
index 1b3c094901b..465a42759df 100644
--- a/app/models/environment_status.rb
+++ b/app/models/environment_status.rb
@@ -34,7 +34,9 @@ class EnvironmentStatus
   end
 
   def has_metrics?
-    DeploymentMetrics.new(project, deployment).has_metrics?
+    strong_memoize(:has_metrics) do
+      deployment_metrics.has_metrics?
+    end
   end
 
   def changes
@@ -52,6 +54,10 @@ class EnvironmentStatus
 
   PAGE_EXTENSIONS = /\A\.(s?html?|php|asp|cgi|pl)\z/i.freeze
 
+  def deployment_metrics
+    @deployment_metrics ||= DeploymentMetrics.new(project, deployment)
+  end
+
   def build_change(file)
     public_path = project.public_path_for_source_path(file.new_path, sha)
     return if public_path.nil?
-- 
cgit v1.2.1


From 56c129293578cb620daf1c19b1bc61cd18d9fa83 Mon Sep 17 00:00:00 2001
From: Thong Kuah <tkuah@gitlab.com>
Date: Mon, 8 Jul 2019 09:36:22 +1200
Subject: Add retry:0 for controller specs n+1

As a workaround for https://gitlab.com/gitlab-org/gitlab-ce/issues/64116
---
 spec/controllers/projects/merge_requests_controller_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index 2a6736018ea..9878f88a395 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -878,7 +878,7 @@ describe Projects::MergeRequestsController do
         expect(control_count).to be <= 137
       end
 
-      it 'has no N+1 issues for environments', :request_store do
+      it 'has no N+1 issues for environments', :request_store, retry: 0 do
         # First run to insert test data from lets, which does take up some 30 queries
         get_ci_environments_status
 
-- 
cgit v1.2.1