Merge branch 'ag-mr-count-query-improvement' into 'master'

Improve large MR count query: avoid primary sticking and add caching

See merge request gitlab-org/gitlab!57341

ee/app/services/groups/recent_merge_requests_count_service.rb

+# frozen_string_literal: true
+
+# Service class for counting and caching the number of open merge requests of
+# a project.
+module Groups
+  class RecentMergeRequestsCountService < BaseCountService
+    VERSION = 1
+    DURATION = 90.days
+    EXPIRES_IN = 24.hours
+
+    def initialize(group, current_user, params)
+      @group = group
+      @current_user = current_user
+      @params = params
+    end
+
+    private
+
+    def relation_for_count
+      MergeRequestsFinder.new(@current_user, @params).execute
+    end
+
+    def cache_key(key = nil)
+      ['groups', 'recent_merge_requests_count_service', VERSION, @group.id, @current_user.id, cache_key_name]
+    end
+
+    def cache_key_name
+      'recent_merge_requests_count'
+    end
+
+    def cache_options
+      super.merge(expires_in: EXPIRES_IN)
+    end
+  end
+end

ee/lib/analytics/group_activity_calculator.rb

@@ -15,8 +15,14 @@ def issues_count
     end
 
     def merge_requests_count
+      # Clear db load balancing session so that primary sticking
+      # does not apply in case a write has happened recently.
+      # We want to make sure the load of the following query
+      # does not land on the primary db.
+      ::Gitlab::Database::LoadBalancing::Session.clear_session
+
       @merge_requests_count ||=
-        MergeRequestsFinder.new(@current_user, params).execute.count
+        Groups::RecentMergeRequestsCountService.new(@group, @current_user, params).count
     end
 
     def new_members_count

ee/spec/lib/analytics/group_activity_calculator_spec.rb

@@ -39,29 +39,12 @@
   end
 
   context 'with merge requests' do
-    let!(:recent_mr) do
-      create(:merge_request,
-        source_project: project,
-        source_branch: "my-personal-branch-1")
-    end
-
-    let!(:old_mr) do
-      create(:merge_request,
-        source_project: project,
-        source_branch: "my-personal-branch-2",
-        created_at: 100.days.ago)
-    end
-
-    it 'only returns the count of recent MRs' do
-      expect(subject.merge_requests_count).to eq 1
-    end
-
-    context 'when user does not have access to some MRs' do
-      let(:secret_mr) { create(:merge_request, source_project: secret_project) }
-
-      it 'does not include those MRs' do
-        expect { secret_mr }.not_to change { subject.merge_requests_count}
+    it 'calls RecentMergeRequestsCountService#count' do
+      expect_next_instance_of(Groups::RecentMergeRequestsCountService) do |count_service|
+        expect(count_service).to receive(:count)
       end
+
+      subject.merge_requests_count
     end
   end
 

ee/spec/services/groups/recent_merge_requests_count_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Groups::RecentMergeRequestsCountService, :use_clean_rails_memory_store_caching do
+  let_it_be(:group) { create(:group) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:project) { create(:project, group: group) }
+  let_it_be(:secret_subgroup) { create(:group, parent: group) }
+  let_it_be(:secret_project) { create(:project, group: secret_subgroup) }
+  let_it_be(:secret_user) { create(:user) }
+  let_it_be(:params) do
+    { group_id: group.id,
+      state: 'all',
+      created_after: 90.days.ago,
+      include_subgroups: true,
+      attempt_group_search_optimizations: true,
+      attempt_project_search_optimizations: true }
+  end
+
+  subject { described_class.new(group, user, params) }
+
+  before_all do
+    project.add_developer(user)
+    project.add_developer(secret_user)
+    secret_project.add_developer(secret_user)
+  end
+
+  describe '#relation_for_count' do
+    before do
+      allow(MergeRequestsFinder).to receive(:new).and_call_original
+    end
+
+    it 'uses the MergeRequestsFinder to scope issues' do
+      expect(MergeRequestsFinder).to receive(:new).with(user, params)
+
+      subject.count
+    end
+  end
+
+  describe '#count' do
+    it 'only returns the count of recent MRs' do
+      create(:merge_request,
+        source_project: project,
+        source_branch: "my-personal-branch-1")
+
+      create(:merge_request,
+        source_project: project,
+        source_branch: "my-personal-branch-2",
+        created_at: 100.days.ago)
+
+      expect(subject.count).to eq 1
+    end
+
+    context 'when user does not have access to some MRs' do
+      it 'caches per-user so users will see the correct value' do
+        regular_user_count = subject.count
+
+        expect do
+          create(:merge_request, source_project: secret_project)
+          # Delete both caches to ensure that the new count gets the updated values
+          described_class.new(group, secret_user, params).delete_cache
+          subject.delete_cache
+        end.to change { described_class.new(group, secret_user, params).count }
+
+        expect(subject.count).to eq(regular_user_count)
+      end
+
+      it 'does not include those MRs' do
+        expect do
+          create(:merge_request, source_project: secret_project)
+          subject.delete_cache
+        end.not_to change { subject.count }
+      end
+    end
+  end
+end