Commit 51097771 authored by Adam Hegyi's avatar Adam Hegyi

Improve the performance of MR analytics page

This change adds a feature flag which alters the COUNT query to only
access the merge_request_metrics table. This happens when loading the
data in the project level merge request analytics page.
parent a6080c29
# frozen_string_literal: true
class MergeRequest::MetricsFinder
include Gitlab::Allowable
def initialize(current_user, params = {})
@current_user = current_user
@params = params
end
def execute
items = init_collection
return items.none if target_project_missing? || user_not_authorized?
items = by_target_project(items)
items = by_merged_after(items)
items = by_merged_before(items)
items
end
private
attr_reader :current_user, :params
def by_target_project(items)
items.by_target_project(target_project)
end
def by_merged_after(items)
items = items.merged_after(params[:merged_after]) if params[:merged_after]
items
end
def by_merged_before(items)
items = items.merged_before(params[:merged_before]) if params[:merged_before]
items
end
def target_project_missing?
params[:target_project].blank?
end
def user_not_authorized?
!can?(current_user, :read_merge_request, target_project)
end
def init_collection
klass.all
end
def klass
MergeRequest::Metrics
end
def target_project
params[:target_project]
end
end
......@@ -6,5 +6,36 @@ module Resolvers
accept_assignee
accept_author
accept_reviewer
def resolve(**args)
scope = super
if only_count_is_selected_with_merged_at_filter?(args) && Feature.enabled?(:optimized_merge_request_count_with_merged_at_filter)
MergeRequest::MetricsFinder
.new(current_user, args.merge(target_project: project))
.execute
else
scope
end
end
def only_count_is_selected_with_merged_at_filter?(args)
return unless lookahead
argument_names = args.keys
argument_names.delete(:lookahead)
argument_names.delete(:sort)
argument_names.delete(:merged_before)
argument_names.delete(:merged_after)
# Detecting a specific query pattern:
# mergeRequests(mergedAfter: "X", mergedBefore: "Y") {
# count
# }
lookahead.selects?(:count) &&
lookahead.selections.size == 1 && # no other nodes are selected
(args[:merged_after] || args[:merged_before]) &&
argument_names.empty? # no extra filtering arguments are provided
end
end
end
......@@ -5,12 +5,14 @@ class MergeRequest::Metrics < ApplicationRecord
belongs_to :pipeline, class_name: 'Ci::Pipeline', foreign_key: :pipeline_id
belongs_to :latest_closed_by, class_name: 'User'
belongs_to :merged_by, class_name: 'User'
belongs_to :target_project, class_name: 'Project'
before_save :ensure_target_project_id
scope :merged_after, ->(date) { where(arel_table[:merged_at].gteq(date)) }
scope :merged_before, ->(date) { where(arel_table[:merged_at].lteq(date)) }
scope :with_valid_time_to_merge, -> { where(arel_table[:merged_at].gt(arel_table[:created_at])) }
scope :by_target_project, ->(project) { where(target_project_id: project) }
def self.time_to_merge_expression
Arel.sql('EXTRACT(epoch FROM SUM(AGE(merge_request_metrics.merged_at, merge_request_metrics.created_at)))')
......
---
name: optimized_merge_request_count_with_merged_at_filter
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/52113
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/299347
milestone: '13.8'
type: development
group: group::optimize
default_enabled: false
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe MergeRequest::MetricsFinder do
let_it_be(:current_user) { create(:user) }
let_it_be(:project) { create(:project, :repository) }
let_it_be(:merge_request_not_merged) { create(:merge_request, :unique_branches, source_project: project) }
let_it_be(:merge_request_merged) do
create(:merge_request, :unique_branches, :merged, source_project: project).tap do |mr|
mr.metrics.update!(merged_at: Time.new(2020, 5, 1))
end
end
let(:merged_at) { merge_request_merged.metrics.merged_at }
let(:params) do
{
target_project: project,
merged_after: merged_at - 10.days,
merged_before: merged_at + 10.days
}
end
subject { described_class.new(current_user, params).execute.to_a }
context 'when target project is missing' do
before do
params.delete(:target_project)
end
it { expect(subject).to be_empty }
end
context 'when the user is not part of the project' do
it { expect(subject).to be_empty }
end
context 'when user is part of the project' do
before do
project.add_developer(current_user)
end
it 'returns merge request records' do
expect(subject).to eq([merge_request_merged.metrics])
end
it 'excludes not merged records' do
expect(subject).not_to eq([merge_request_not_merged.metrics])
end
context 'when only merged_before is given' do
before do
params.delete(:merged_after)
end
it { expect(subject).to eq([merge_request_merged.metrics]) }
end
context 'when only merged_after is given' do
before do
params.delete(:merged_before)
end
it { expect(subject).to eq([merge_request_merged.metrics]) }
end
context 'when no records matching the date range' do
before do
params[:merged_before] = merged_at - 1.year
params[:merged_after] = merged_at - 2.years
end
it { expect(subject).to be_empty }
end
end
end
......@@ -396,4 +396,66 @@ RSpec.describe 'getting merge request listings nested in a project' do
end
end
end
context 'when only the count is requested' do
context 'when merged at filter is present' do
let_it_be(:merge_request) do
create(:merge_request, :unique_branches, source_project: project).tap do |mr|
mr.metrics.update!(merged_at: Time.new(2020, 1, 3))
end
end
let(:query) do
graphql_query_for(:project, { full_path: project.full_path },
<<~QUERY
mergeRequests(mergedAfter: "2020-01-01", mergedBefore: "2020-01-05", first: 0) {
count
}
QUERY
)
end
context 'when "optimized_merge_request_count_with_merged_at_filter" feature flag is enabled' do
before do
stub_feature_flags(optimized_merge_request_count_with_merged_at_filter: true)
end
it 'does not query the merge requests table for the count' do
query_recorder = ActiveRecord::QueryRecorder.new { post_graphql(query, current_user: current_user) }
queries = query_recorder.data.each_value.first[:occurrences]
expect(queries).not_to include(match(/SELECT COUNT\(\*\) FROM "merge_requests"/))
expect(queries).to include(match(/SELECT COUNT\(\*\) FROM "merge_request_metrics"/))
end
it 'returns the correct count' do
post_graphql(query, current_user: current_user)
count = graphql_data.dig('project', 'mergeRequests', 'count')
expect(count).to eq(1)
end
context 'when "optimized_merge_request_count_with_merged_at_filter" feature flag is disabled' do
before do
stub_feature_flags(optimized_merge_request_count_with_merged_at_filter: false)
end
it 'does not query the merge requests table for the count' do
query_recorder = ActiveRecord::QueryRecorder.new { post_graphql(query, current_user: current_user) }
queries = query_recorder.data.each_value.first[:occurrences]
expect(queries).to include(match(/SELECT COUNT\(\*\) FROM "merge_requests"/))
expect(queries).not_to include(match(/SELECT COUNT\(\*\) FROM "merge_request_metrics"/))
end
it 'returns the correct count' do
post_graphql(query, current_user: current_user)
count = graphql_data.dig('project', 'mergeRequests', 'count')
expect(count).to eq(1)
end
end
end
end
end
end
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