Merge branch '325656-cache-state-counts-for-issuables-list-no-params-3' into 'master'

Cache group issues count for state

See merge request gitlab-org/gitlab!67418

app/helpers/issuables_helper.rb

@@ -3,6 +3,7 @@
 module IssuablesHelper
   include GitlabRoutingHelper
   include IssuablesDescriptionTemplatesHelper
+  include ::Sidebars::Concerns::HasPill
 
   def sidebar_gutter_toggle_icon
     content_tag(:span, class: 'js-sidebar-toggle-container', data: { is_expanded: !sidebar_gutter_collapsed? }) do
@@ -187,19 +188,18 @@ module IssuablesHelper
   end
 
   def issuables_state_counter_text(issuable_type, state, display_count)
-    titles = {
-      opened: "Open"
-    }
-
+    titles = { opened: "Open" }
     state_title = titles[state] || state.to_s.humanize
     html = content_tag(:span, state_title)
 
     return html.html_safe unless display_count
 
     count = issuables_count_for_state(issuable_type, state)
-
     if count != -1
-      html << " " << content_tag(:span, number_with_delimiter(count), class: 'badge badge-muted badge-pill gl-badge gl-tab-counter-badge sm')
+      html << " " << content_tag(:span,
+        format_count(issuable_type, count, Gitlab::IssuablesCountForState::THRESHOLD),
+        class: 'badge badge-muted badge-pill gl-badge gl-tab-counter-badge sm'
+      )
     end
 
     html.html_safe
@@ -284,7 +284,9 @@ module IssuablesHelper
   end
 
   def issuables_count_for_state(issuable_type, state)
-    Gitlab::IssuablesCountForState.new(finder)[state]
+    store_in_cache = parent.is_a?(Group) ? parent.cached_issues_state_count_enabled? : false
+
+    Gitlab::IssuablesCountForState.new(finder, store_in_redis_cache: store_in_cache)[state]
   end
 
   def close_issuable_path(issuable)
@@ -438,6 +440,14 @@ module IssuablesHelper
   def parent
     @project || @group
   end
+
+  def format_count(issuable_type, count, threshold)
+    if issuable_type == :issues && parent.is_a?(Group) && parent.cached_issues_state_count_enabled?
+      format_cached_count(threshold, count)
+    else
+      number_with_delimiter(count)
+    end
+  end
 end
 
 IssuablesHelper.prepend_mod_with('IssuablesHelper')

app/models/group.rb

@@ -735,6 +735,10 @@ class Group < Namespace
     Timelog.in_group(self)
   end
 
+  def cached_issues_state_count_enabled?
+    Feature.enabled?(:cached_issues_state_count, self, default_enabled: :yaml)
+  end
+
   private
 
   def max_member_access(user_ids)

config/feature_flags/development/cached_issues_state_count.yml

+---
+name: cached_issues_state_count
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/67418
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/333089
+milestone: '14.3'
+type: development
+group: group::product planning
+default_enabled: false

ee/lib/sidebars/groups/menus/epics_menu.rb

@@ -43,7 +43,7 @@ module Sidebars
             count_service = ::Groups::EpicsCountService
             count = count_service.new(context.group, context.current_user).count
 
-            format_cached_count(count_service, count)
+            format_cached_count(count_service::CACHED_COUNT_THRESHOLD, count)
           end
         end
 

lib/gitlab/issuables_count_for_state.rb

@@ -5,11 +5,14 @@ module Gitlab
   class IssuablesCountForState
     # The name of the Gitlab::SafeRequestStore cache key.
     CACHE_KEY = :issuables_count_for_state
+    # The expiration time for the Rails cache.
+    CACHE_EXPIRES_IN = 10.minutes
+    THRESHOLD = 1000
 
     # The state values that can be safely casted to a Symbol.
     STATES = %w[opened closed merged all].freeze
 
-    attr_reader :project
+    attr_reader :project, :finder
 
     def self.declarative_policy_class
       'IssuablePolicy'
@@ -18,11 +21,12 @@ module Gitlab
     # finder - The finder class to use for retrieving the issuables.
     # fast_fail - restrict counting to a shorter period, degrading gracefully on
     # failure
-    def initialize(finder, project = nil, fast_fail: false)
+    def initialize(finder, project = nil, fast_fail: false, store_in_redis_cache: false)
       @finder = finder
       @project = project
       @fast_fail = fast_fail
       @cache = Gitlab::SafeRequestStore[CACHE_KEY] ||= initialize_cache
+      @store_in_redis_cache = store_in_redis_cache
     end
 
     def for_state_or_opened(state = nil)
@@ -52,7 +56,16 @@ module Gitlab
     private
 
     def cache_for_finder
-      @cache[@finder]
+      cached_counts = Rails.cache.read(redis_cache_key, cache_options) if cache_issues_count?
+
+      cached_counts ||= @cache[finder]
+      return cached_counts if cached_counts.empty?
+
+      if cache_issues_count? && cached_counts.values.all? { |count| count >= THRESHOLD }
+        Rails.cache.write(redis_cache_key, cached_counts, cache_options)
+      end
+
+      cached_counts
     end
 
     def cast_state_to_symbol?(state)
@@ -108,5 +121,33 @@ module Gitlab
         "Count of failed calls to IssuableFinder#count_by_state with fast failure"
       ).increment
     end
+
+    def cache_issues_count?
+      @store_in_redis_cache &&
+        finder.instance_of?(IssuesFinder) &&
+        parent_group.present? &&
+        !params_include_filters?
+    end
+
+    def parent_group
+      finder.params.group
+    end
+
+    def redis_cache_key
+      ['group', parent_group&.id, 'issues']
+    end
+
+    def cache_options
+      { expires_in: CACHE_EXPIRES_IN }
+    end
+
+    def params_include_filters?
+      non_filtering_params = %i[
+        scope state sort group_id include_subgroups
+        attempt_group_search_optimizations non_archived issue_types
+      ]
+
+      finder.params.except(*non_filtering_params).values.any?
+    end
   end
 end

lib/sidebars/concerns/has_pill.rb

@@ -21,8 +21,8 @@ module Sidebars
         {}
       end
 
-      def format_cached_count(count_service, count)
-        if count > count_service::CACHED_COUNT_THRESHOLD
+      def format_cached_count(threshold, count)
+        if count > threshold
           number_to_human(
             count,
             units: { thousand: 'k', million: 'm' }, precision: 1, significant: false, format: '%n%u'

lib/sidebars/groups/menus/issues_menu.rb

@@ -38,7 +38,7 @@ module Sidebars
             count_service = ::Groups::OpenIssuesCountService
             count = count_service.new(context.group, context.current_user).count
 
-            format_cached_count(count_service, count)
+            format_cached_count(count_service::CACHED_COUNT_THRESHOLD, count)
           end
         end
 

lib/sidebars/groups/menus/merge_requests_menu.rb

@@ -37,7 +37,7 @@ module Sidebars
             count_service = ::Groups::MergeRequestsCountService
             count = count_service.new(context.group, context.current_user).count
 
-            format_cached_count(count_service, count)
+            format_cached_count(count_service::CACHED_COUNT_THRESHOLD, count)
           end
         end
 

spec/features/groups/issues_spec.rb

@@ -94,6 +94,41 @@ RSpec.describe 'Group issues page' do
         expect(page).not_to have_content issue.title[0..80]
       end
     end
+
+    context 'when cached issues state count is enabled', :clean_gitlab_redis_cache do
+      before do
+        stub_feature_flags(cached_issues_state_count: true)
+      end
+
+      it 'truncates issue counts if over the threshold' do
+        allow(Rails.cache).to receive(:read).and_call_original
+        allow(Rails.cache).to receive(:read).with(
+          ['group', group.id, 'issues'],
+          { expires_in: Gitlab::IssuablesCountForState::CACHE_EXPIRES_IN }
+        ).and_return({ opened: 1050, closed: 500, all: 1550 })
+
+        visit issues_group_path(group)
+
+        expect(page).to have_text('Open 1.1k Closed 500 All 1.6k')
+      end
+    end
+
+    context 'when cached issues state count is disabled', :clean_gitlab_redis_cache do
+      before do
+        stub_feature_flags(cached_issues_state_count: false)
+      end
+
+      it 'does not truncate counts if they are over the threshold' do
+        allow_next_instance_of(IssuesFinder) do |finder|
+          allow(finder).to receive(:count_by_state).and_return(true)
+            .and_return({ opened: 1050, closed: 500, all: 1550 })
+        end
+
+        visit issues_group_path(group)
+
+        expect(page).to have_text('Open 1,050 Closed 500 All 1,550')
+      end
+    end
   end
 
   context 'manual ordering', :js do

spec/helpers/issuables_helper_spec.rb

@@ -123,7 +123,7 @@ RSpec.describe IssuablesHelper do
   end
 
   describe '#issuables_state_counter_text' do
-    let(:user) { create(:user) }
+    let_it_be(:user) { create(:user) }
 
     describe 'state text' do
       context 'when number of issuables can be generated' do
@@ -159,6 +159,38 @@ RSpec.describe IssuablesHelper do
             .to eq('<span>All</span>')
         end
       end
+
+      context 'when count is over the threshold' do
+        let_it_be(:group) { create(:group) }
+
+        before do
+          allow(helper).to receive(:issuables_count_for_state).and_return(1100)
+          allow(helper).to receive(:parent).and_return(group)
+          stub_const("Gitlab::IssuablesCountForState::THRESHOLD", 1000)
+        end
+
+        context 'when feature flag cached_issues_state_count is disabled' do
+          before do
+            stub_feature_flags(cached_issues_state_count: false)
+          end
+
+          it 'returns complete count' do
+            expect(helper.issuables_state_counter_text(:issues, :opened, true))
+              .to eq('<span>Open</span> <span class="badge badge-muted badge-pill gl-badge gl-tab-counter-badge sm">1,100</span>')
+          end
+        end
+
+        context 'when feature flag cached_issues_state_count is enabled' do
+          before do
+            stub_feature_flags(cached_issues_state_count: true)
+          end
+
+          it 'returns truncated count' do
+            expect(helper.issuables_state_counter_text(:issues, :opened, true))
+              .to eq('<span>Open</span> <span class="badge badge-muted badge-pill gl-badge gl-tab-counter-badge sm">1.1k</span>')
+          end
+        end
+      end
     end
   end
 

spec/lib/gitlab/issuables_count_for_state_spec.rb

@@ -66,4 +66,106 @@ RSpec.describe Gitlab::IssuablesCountForState do
       end
     end
   end
+
+  context 'when store_in_redis_cache is `true`', :clean_gitlab_redis_cache do
+    let_it_be(:user) { create(:user) }
+    let_it_be(:group) { create(:group) }
+
+    let(:cache_options) { { expires_in: 10.minutes } }
+    let(:cache_key) { ['group', group.id, 'issues'] }
+    let(:threshold) { described_class::THRESHOLD }
+    let(:states_count) { { opened: 1, closed: 1, all: 2 } }
+    let(:params) { {} }
+
+    subject { described_class.new(finder, fast_fail: true, store_in_redis_cache: true ) }
+
+    before do
+      allow(finder).to receive(:count_by_state).and_return(states_count)
+      allow_next_instance_of(described_class) do |counter|
+        allow(counter).to receive(:parent_group).and_return(group)
+      end
+    end
+
+    shared_examples 'calculating counts without caching' do
+      it 'does not store in redis store' do
+        expect(Rails.cache).not_to receive(:read)
+        expect(finder).to receive(:count_by_state)
+        expect(Rails.cache).not_to receive(:write)
+        expect(subject[:all]).to eq(states_count[:all])
+      end
+    end
+
+    context 'with Issues' do
+      let(:finder) { IssuesFinder.new(user, params) }
+
+      it 'returns -1 for the requested state' do
+        allow(finder).to receive(:count_by_state).and_raise(ActiveRecord::QueryCanceled)
+        expect(Rails.cache).not_to receive(:write)
+
+        expect(subject[:all]).to eq(-1)
+      end
+
+      context 'when parent group is not present' do
+        let(:group) { nil }
+
+        it_behaves_like 'calculating counts without caching'
+      end
+
+      context 'when params include search filters' do
+        let(:parent) { group }
+
+        before do
+          finder.params[:assignee_username] = [user.username, 'root']
+        end
+
+        it_behaves_like 'calculating counts without caching'
+      end
+
+      context 'when counts are stored in cache' do
+        before do
+          allow(Rails.cache).to receive(:read).with(cache_key, cache_options)
+            .and_return({ opened: 1000, closed: 1000, all: 2000 })
+        end
+
+        it 'does not call finder count_by_state' do
+          expect(finder).not_to receive(:count_by_state)
+
+          expect(subject[:all]).to eq(2000)
+        end
+      end
+
+      context 'when cache is empty' do
+        context 'when state counts are under threshold' do
+          let(:states_count) { { opened: 1, closed: 1, all: 2 } }
+
+          it 'does not store state counts in cache' do
+            expect(Rails.cache).to receive(:read).with(cache_key, cache_options)
+            expect(finder).to receive(:count_by_state)
+            expect(Rails.cache).not_to receive(:write)
+            expect(subject[:all]).to eq(states_count[:all])
+          end
+        end
+
+        context 'when state counts are over threshold' do
+          let(:states_count) do
+            { opened: threshold + 1, closed: threshold + 1, all: (threshold + 1) * 2 }
+          end
+
+          it 'stores state counts in cache' do
+            expect(Rails.cache).to receive(:read).with(cache_key, cache_options)
+            expect(finder).to receive(:count_by_state)
+            expect(Rails.cache).to receive(:write).with(cache_key, states_count, cache_options)
+
+            expect(subject[:all]).to eq((threshold + 1) * 2)
+          end
+        end
+      end
+    end
+
+    context 'with Merge Requests' do
+      let(:finder) { MergeRequestsFinder.new(user, params) }
+
+      it_behaves_like 'calculating counts without caching'
+    end
+  end
 end