Merge branch '276386-fast-counts-for-mr-analytics' into 'master'

Improve the performance of MR analytics page (charts)

See merge request gitlab-org/gitlab!52113

app/finders/merge_request/metrics_finder.rb

+# frozen_string_literal: true
+
+class MergeRequest::MetricsFinder
+  include Gitlab::Allowable
+
+  def initialize(current_user, params = {})
+    @current_user = current_user
+    @params = params
+  end
+
+  def execute
+    return klass.none if target_project.blank? || user_not_authorized?
+
+    items = init_collection
+    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)
+    return items unless merged_after
+
+    items.merged_after(merged_after)
+  end
+
+  def by_merged_before(items)
+    return items unless merged_before
+
+    items.merged_before(merged_before)
+  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
+
+  def merged_after
+    params[:merged_after]
+  end
+
+  def merged_before
+    params[:merged_before]
+  end
+end

app/graphql/resolvers/project_merge_requests_resolver.rb

@@ -6,5 +6,38 @@ 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.except(:lookahead, :sort, :merged_before, :merged_after).keys
+
+      # no extra filtering arguments are provided
+      return unless argument_names.empty?
+      return unless args[:merged_after] || args[:merged_before]
+
+      # Detecting a specific query pattern:
+      # mergeRequests(mergedAfter: "X", mergedBefore: "Y") {
+      #   count
+      #   totalTimeToMerge
+      # }
+      allowed_selected_fields = [:count, :total_time_to_merge]
+      selected_fields = lookahead.selections.map(&:field).map(&:original_name)
+
+      # only the allowed_selected_fields are present
+      (selected_fields - allowed_selected_fields).empty?
+    end
   end
 end

app/models/merge_request/metrics.rb

@@ -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', inverse_of: :merge_requests
 
   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)))')
@@ -21,6 +23,12 @@ class MergeRequest::Metrics < ApplicationRecord
   def ensure_target_project_id
     self.target_project_id ||= merge_request.target_project_id
   end
+
+  def self.total_time_to_merge
+    with_valid_time_to_merge
+      .pluck(time_to_merge_expression)
+      .first
+  end
 end
 
 MergeRequest::Metrics.prepend_if_ee('EE::MergeRequest::Metrics')

app/models/project.rb

@@ -218,6 +218,7 @@ class Project < ApplicationRecord
 
   # Merge Requests for target project should be removed with it
   has_many :merge_requests, foreign_key: 'target_project_id', inverse_of: :target_project
+  has_many :merge_request_metrics, foreign_key: 'target_project', class_name: 'MergeRequest::Metrics', inverse_of: :target_project
   has_many :source_of_merge_requests, foreign_key: 'source_project_id', class_name: 'MergeRequest'
   has_many :issues
   has_many :labels, class_name: 'ProjectLabel'

config/feature_flags/development/optimized_merge_request_count_with_merged_at_filter.yml

+---
+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.9'
+type: development
+group: group::optimize
+default_enabled: false

spec/finders/merge_request/metrics_finder_spec.rb

+# 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(:merged_at) { Time.new(2020, 5, 1) }
+  let_it_be(:merge_request_merged) do
+    create(:merge_request, :unique_branches, :merged, source_project: project).tap do |mr|
+      mr.metrics.update!(merged_at: merged_at)
+    end
+  end
+
+  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 { is_expected.to be_empty }
+  end
+
+  context 'when the user is not part of the project' do
+    it { is_expected.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
+      is_expected.to eq([merge_request_merged.metrics])
+    end
+
+    it 'excludes not merged records' do
+      is_expected.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 { is_expected.to eq([merge_request_merged.metrics]) }
+    end
+
+    context 'when only merged_after is given' do
+      before do
+        params.delete(:merged_before)
+      end
+
+      it { is_expected.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 { is_expected.to be_empty }
+    end
+  end
+end

spec/lib/gitlab/import_export/all_models.yml

@@ -561,6 +561,7 @@ project:
 - exported_protected_branches
 - incident_management_oncall_schedules
 - debian_distributions
+- merge_request_metrics
 award_emoji:
 - awardable
 - user
@@ -589,6 +590,7 @@ lfs_file_locks:
 project_badges:
 - project
 metrics:
+- target_project
 - merge_request
 - latest_closed_by
 - merged_by

spec/models/merge_request/metrics_spec.rb

@@ -5,6 +5,7 @@ require 'spec_helper'
 RSpec.describe MergeRequest::Metrics do
   describe 'associations' do
     it { is_expected.to belong_to(:merge_request) }
+    it { is_expected.to belong_to(:target_project).class_name('Project') }
     it { is_expected.to belong_to(:latest_closed_by).class_name('User') }
     it { is_expected.to belong_to(:merged_by).class_name('User') }
   end
@@ -36,5 +37,15 @@ RSpec.describe MergeRequest::Metrics do
         is_expected.not_to include([metrics_2])
       end
     end
+
+    describe '.by_target_project' do
+      let(:target_project) { metrics_1.target_project }
+
+      subject { described_class.by_target_project(target_project) }
+
+      it 'finds metrics record with the associated target project' do
+        is_expected.to eq([metrics_1])
+      end
+    end
   end
 end

spec/models/project_spec.rb

@@ -21,6 +21,7 @@ RSpec.describe Project, factory_default: :keep do
     it { is_expected.to have_many(:services) }
     it { is_expected.to have_many(:events) }
     it { is_expected.to have_many(:merge_requests) }
+    it { is_expected.to have_many(:merge_request_metrics).class_name('MergeRequest::Metrics') }
     it { is_expected.to have_many(:issues) }
     it { is_expected.to have_many(:milestones) }
     it { is_expected.to have_many(:iterations) }

spec/requests/api/graphql/project/merge_requests_spec.rb

@@ -396,4 +396,85 @@ 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
+
+      shared_examples 'count examples' do
+        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
+
+      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
+
+        context 'when total_time_to_merge and count is queried' do
+          let(:query) do
+            graphql_query_for(:project, { full_path: project.full_path },
+            <<~QUERY
+            mergeRequests(mergedAfter: "2020-01-01", mergedBefore: "2020-01-05", first: 0) {
+              totalTimeToMerge
+              count
+            }
+            QUERY
+            )
+          end
+
+          it 'does not query the merge requests table for the total_time_to_merge' 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.+SUM.+FROM "merge_request_metrics" WHERE/))
+          end
+        end
+
+        it_behaves_like 'count examples'
+
+        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 'queries 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_behaves_like 'count examples'
+        end
+      end
+    end
+  end
 end