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

@@ -15,24 +15,22 @@ class Projects::DeploymentsController < Projects::ApplicationController
   # rubocop: enable CodeReuse/ActiveRecord
   def metrics
-    return render_404 unless deployment.has_metrics?
-    @metrics = deployment.metrics
+    return render_404 unless deployment_metrics.has_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])

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

app/models/deployment_metrics.rb

+# 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

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?

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

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)

spec/models/deployment_metrics_spec.rb

+# 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

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) }