Commit 886521c7 authored by Lin Jen-Shin's avatar Lin Jen-Shin

Merge branch '63475-fix-n-1' into 'master'

Reduce N+1 queries in MergeRequestsController#ci_environment_status

See merge request gitlab-org/gitlab-ce!30224
parents d14d1340 56c12929
......@@ -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])
......
......@@ -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)
......@@ -176,45 +171,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
# 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?
end
def ref_path
File.join(environment.ref_path, 'deployments', iid.to_s)
end
......
# 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
# 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_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
......@@ -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
......@@ -33,6 +33,12 @@ class EnvironmentStatus
end
end
def has_metrics?
strong_memoize(:has_metrics) do
deployment_metrics.has_metrics?
end
end
def changes
return [] if project.route_map_for(sha).nil?
......@@ -48,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?
......@@ -67,7 +77,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
......
......@@ -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
......
---
title: Improve performance of MergeRequestsController#ci_environment_status endpoint
merge_request: 30224
author:
type: performance
......@@ -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)
......
......@@ -878,6 +878,22 @@ describe Projects::MergeRequestsController do
expect(control_count).to be <= 137
end
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
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)
# 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
def get_ci_environments_status(extra_params = {})
params = {
namespace_id: merge_request.project.namespace.to_param,
......
# 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
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
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
......@@ -295,125 +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
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
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) }
......@@ -441,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
......@@ -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) }
......
......@@ -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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment