From d2ba2951f737082edd568505f985ebf9a0808be7 Mon Sep 17 00:00:00 2001 From: Thong Kuah 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