Merge branch '243753-slow-count-queries-in-group-page-sidebar' into 'master'

Cached sidebar issues count in group pages

See merge request gitlab-org/gitlab!49739

app/helpers/groups_helper.rb

@@ -62,6 +62,14 @@ module GroupsHelper
     can?(current_user, :set_emails_disabled, group) && !group.parent&.emails_disabled?
   end
 
+  def group_open_issues_count(group)
+    if Feature.enabled?(:cached_sidebar_open_issues_count, group)
+      cached_open_group_issues_count(group)
+    else
+      number_with_delimiter(group_issues_count(state: 'opened'))
+    end
+  end
+
   def group_issues_count(state:)
     IssuesFinder
       .new(current_user, group_id: @group.id, state: state, non_archived: true, include_subgroups: true)
@@ -69,6 +77,21 @@ module GroupsHelper
       .count
   end
 
+  def cached_open_group_issues_count(group)
+    count_service = Groups::OpenIssuesCountService
+    issues_count = count_service.new(group, current_user).count
+
+    if issues_count > count_service::CACHED_COUNT_THRESHOLD
+      ActiveSupport::NumberHelper
+        .number_to_human(
+          issues_count,
+          units: { thousand: 'k', million: 'm' }, precision: 1, significant: false, format: '%n%u'
+        )
+    else
+      number_with_delimiter(issues_count)
+    end
+  end
+
   def group_merge_requests_count(state:)
     MergeRequestsFinder
       .new(current_user, group_id: @group.id, state: state, non_archived: true, include_subgroups: true)

app/services/groups/open_issues_count_service.rb

+# frozen_string_literal: true
+
+module Groups
+  # Service class for counting and caching the number of open issues of a group.
+  class OpenIssuesCountService < BaseCountService
+    include Gitlab::Utils::StrongMemoize
+
+    VERSION = 1
+    PUBLIC_COUNT_KEY = 'group_public_open_issues_count'
+    TOTAL_COUNT_KEY = 'group_total_open_issues_count'
+    CACHED_COUNT_THRESHOLD = 1000
+    EXPIRATION_TIME = 24.hours
+
+    attr_reader :group, :user
+
+    def initialize(group, user = nil)
+      @group = group
+      @user = user
+    end
+
+    # Reads count value from cache and return it if present.
+    # If empty or expired, #uncached_count will calculate the issues count for the group and
+    # compare it with the threshold. If it is greater, it will be written to the cache and returned.
+    # If below, it will be returned without being cached.
+    # This results in only caching large counts and calculating the rest with every call to maintain
+    # accuracy.
+    def count
+      cached_count = Rails.cache.read(cache_key)
+      return cached_count unless cached_count.blank?
+
+      refreshed_count = uncached_count
+      update_cache_for_key(cache_key) { refreshed_count } if refreshed_count > CACHED_COUNT_THRESHOLD
+      refreshed_count
+    end
+
+    def cache_key(key = nil)
+      ['groups', 'open_issues_count_service', VERSION, group.id, cache_key_name]
+    end
+
+    private
+
+    def cache_options
+      super.merge({ expires_in: EXPIRATION_TIME })
+    end
+
+    def cache_key_name
+      public_only? ? PUBLIC_COUNT_KEY : TOTAL_COUNT_KEY
+    end
+
+    def public_only?
+      !user_is_at_least_reporter?
+    end
+
+    def user_is_at_least_reporter?
+      strong_memoize(:user_is_at_least_reporter) do
+        group.member?(user, Gitlab::Access::REPORTER)
+      end
+    end
+
+    def relation_for_count
+      IssuesFinder.new(user, group_id: group.id, state: 'opened', non_archived: true, include_subgroups: true, public_only: public_only?).execute
+    end
+  end
+end

app/views/layouts/nav/sidebar/_group.html.haml

-- issues_count = group_issues_count(state: 'opened')
+- issues_count = group_open_issues_count(@group)
 - merge_requests_count = group_merge_requests_count(state: 'opened')
 
 .nav-sidebar{ class: ("sidebar-collapsed-desktop" if collapsed_sidebar?), **tracking_attrs('groups_side_navigation', 'render', 'groups_side_navigation') }
@@ -54,14 +54,14 @@
               = sprite_icon('issues')
             %span.nav-item-name
               = _('Issues')
-            %span.badge.badge-pill.count= number_with_delimiter(issues_count)
+            %span.badge.badge-pill.count= issues_count
 
           %ul.sidebar-sub-level-items{ data: { qa_selector: 'group_issues_sidebar_submenu'} }
             = nav_link(path: ['groups#issues', 'labels#index', 'milestones#index', 'iterations#index'], html_options: { class: "fly-out-top-item" } ) do
               = link_to issues_group_path(@group) do
                 %strong.fly-out-top-item-name
                   = _('Issues')
-                %span.badge.badge-pill.count.issue_counter.fly-out-badge= number_with_delimiter(issues_count)
+                %span.badge.badge-pill.count.issue_counter.fly-out-badge= issues_count
 
             %li.divider.fly-out-top-item
             = nav_link(path: 'groups#issues', html_options: { class: 'home' }) do

config/feature_flags/development/cached_sidebar_open_issues_count.yml

+---
+name: cached_sidebar_open_issues_count
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/49739
+rollout_issue_url:
+milestone: '13.8'
+type: development
+group: group::product planning
+default_enabled: false

doc/user/project/issues/index.md

@@ -119,7 +119,7 @@ and modify them if you have the necessary [permissions](../../permissions.md).
 > - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/17589) in GitLab 13.3.
 
 Assignees in the sidebar are updated in real time. This feature is **disabled by default**.
-To enable, you need to enable [ActionCable in-app mode](https://docs.gitlab.com/omnibus/settings/actioncable.html).
+To enable it, you need to enable [ActionCable in-app mode](https://docs.gitlab.com/omnibus/settings/actioncable.html).
 
 ### Issues List
 
@@ -137,7 +137,22 @@ view, you can also make certain changes [in bulk](../bulk_editing.md) to the dis
 For more information, see the [Issue Data and Actions](issue_data_and_actions.md) page
 for a rundown of all the fields and information in an issue.
 
-You can sort a list of issues in several ways, for example by issue creation date, milestone due date. For more information, see the [Sorting and Ordering Issue Lists](sorting_issue_lists.md) page.
+You can sort a list of issues in several ways, for example by issue creation date, milestone due date.
+For more information, see the [Sorting and ordering issue lists](sorting_issue_lists.md) page.
+
+#### Cached issue count
+
+> - [Introduced]([link-to-issue](https://gitlab.com/gitlab-org/gitlab/-/issues/243753)) in GitLab 13.9.
+> - It's [deployed behind a feature flag](../../feature_flags.md), disabled by default.
+> - It's disabled on GitLab.com.
+> - It's not recommended for production use.
+> - To use this feature in GitLab self-managed instances, ask a GitLab administrator to [enable it](#enable-or-disable-cached-issue-count) **(CORE ONLY)**
+
+WARNING:
+This feature might not be available to you. Check the **version history** note above for details.
+
+In a group, the sidebar displays the total count of open issues and this value is cached if higher
+than 1000. The cached value is rounded to thousands (or millions) and updated every 24 hours.
 
 ### Issue boards
 
@@ -226,3 +241,22 @@ You can then see issue statuses in the [issue list](#issues-list) and the
 - [Issues API](../../../api/issues.md)
 - Configure an [external issue tracker](../../../integration/external-issue-tracker.md)
   such as Jira, Redmine, Bugzilla, or EWM.
+
+## Enable or disable cached issue count **(CORE ONLY)**
+
+Cached issue count in the left sidebar is under development and not ready for production use. It is
+deployed behind a feature flag that is **disabled by default**.
+[GitLab administrators with access to the GitLab Rails console](../../../administration/feature_flags.md)
+can enable it.
+
+To enable it:
+
+```ruby
+Feature.enable(:cached_sidebar_open_issues_count)
+```
+
+To disable it:
+
+```ruby
+Feature.disable(:cached_sidebar_open_issues_count)
+```

spec/helpers/groups_helper_spec.rb

@@ -444,4 +444,82 @@ RSpec.describe GroupsHelper do
       end
     end
   end
+
+  describe '#group_open_issues_count' do
+    let_it_be(:current_user) { create(:user) }
+    let_it_be(:group) { create(:group, :public) }
+    let_it_be(:count_service) { Groups::OpenIssuesCountService }
+
+    before do
+      allow(helper).to receive(:current_user) { current_user }
+    end
+
+    context 'when cached_sidebar_open_issues_count feature flag is enabled' do
+      before do
+        stub_feature_flags(cached_sidebar_open_issues_count: true)
+      end
+
+      it 'returns count value from cache' do
+        allow_next_instance_of(count_service) do |service|
+          allow(service).to receive(:count).and_return(2500)
+        end
+
+        expect(helper.group_open_issues_count(group)).to eq('2.5k')
+      end
+    end
+
+    context 'when cached_sidebar_open_issues_count feature flag is disabled' do
+      before do
+        stub_feature_flags(cached_sidebar_open_issues_count: false)
+      end
+
+      it 'returns not cached issues count' do
+        allow(helper).to receive(:group_issues_count).and_return(2500)
+
+        expect(helper.group_open_issues_count(group)).to eq('2,500')
+      end
+    end
+  end
+
+  describe '#cached_open_group_issues_count' do
+    let_it_be(:current_user) { create(:user) }
+    let_it_be(:group) { create(:group, name: 'group') }
+    let_it_be(:count_service) { Groups::OpenIssuesCountService }
+
+    before do
+      allow(helper).to receive(:current_user) { current_user }
+    end
+
+    it 'returns all digits for count value under 1000' do
+      allow_next_instance_of(count_service) do |service|
+        allow(service).to receive(:count).and_return(999)
+      end
+
+      expect(helper.cached_open_group_issues_count(group)).to eq('999')
+    end
+
+    it 'returns truncated digits for count value over 1000' do
+      allow_next_instance_of(count_service) do |service|
+        allow(service).to receive(:count).and_return(2300)
+      end
+
+      expect(helper.cached_open_group_issues_count(group)).to eq('2.3k')
+    end
+
+    it 'returns truncated digits for count value over 10000' do
+      allow_next_instance_of(count_service) do |service|
+        allow(service).to receive(:count).and_return(12560)
+      end
+
+      expect(helper.cached_open_group_issues_count(group)).to eq('12.6k')
+    end
+
+    it 'returns truncated digits for count value over 100000' do
+      allow_next_instance_of(count_service) do |service|
+        allow(service).to receive(:count).and_return(112560)
+      end
+
+      expect(helper.cached_open_group_issues_count(group)).to eq('112.6k')
+    end
+  end
 end

spec/services/groups/open_issues_count_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Groups::OpenIssuesCountService, :use_clean_rails_memory_store_caching do
+  let_it_be(:group) { create(:group, :public)}
+  let_it_be(:project) { create(:project, :public, namespace: group) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:issue) { create(:issue, :opened, project: project) }
+  let_it_be(:confidential) { create(:issue, :opened, confidential: true, project: project) }
+  let_it_be(:closed) { create(:issue, :closed, project: project) }
+
+  subject { described_class.new(group, user) }
+
+  describe '#relation_for_count' do
+    before do
+      allow(IssuesFinder).to receive(:new).and_call_original
+    end
+
+    it 'uses the IssuesFinder to scope issues' do
+      expect(IssuesFinder)
+        .to receive(:new)
+        .with(user, group_id: group.id, state: 'opened', non_archived: true, include_subgroups: true, public_only: true)
+
+      subject.count
+    end
+  end
+
+  describe '#count' do
+    context 'when user is nil' do
+      it 'does not include confidential issues in the issue count' do
+        expect(described_class.new(group).count).to eq(1)
+      end
+    end
+
+    context 'when user is provided' do
+      context 'when user can read confidential issues' do
+        before do
+          group.add_reporter(user)
+        end
+
+        it 'returns the right count with confidential issues' do
+          expect(subject.count).to eq(2)
+        end
+      end
+
+      context 'when user cannot read confidential issues' do
+        before do
+          group.add_guest(user)
+        end
+
+        it 'does not include confidential issues' do
+          expect(subject.count).to eq(1)
+        end
+      end
+
+      context 'with different cache values' do
+        let(:public_count_key) { subject.cache_key(described_class::PUBLIC_COUNT_KEY) }
+        let(:under_threshold) { described_class::CACHED_COUNT_THRESHOLD - 1 }
+        let(:over_threshold) { described_class::CACHED_COUNT_THRESHOLD + 1 }
+
+        context 'when cache is empty' do
+          before do
+            Rails.cache.delete(public_count_key)
+          end
+
+          it 'refreshes cache if value over threshold' do
+            allow(subject).to receive(:uncached_count).and_return(over_threshold)
+
+            expect(subject.count).to eq(over_threshold)
+            expect(Rails.cache.read(public_count_key)).to eq(over_threshold)
+          end
+
+          it 'does not refresh cache if value under threshold' do
+            allow(subject).to receive(:uncached_count).and_return(under_threshold)
+
+            expect(subject.count).to eq(under_threshold)
+            expect(Rails.cache.read(public_count_key)).to be_nil
+          end
+        end
+
+        context 'when cached count is under the threshold value' do
+          before do
+            Rails.cache.write(public_count_key, under_threshold)
+          end
+
+          it 'does not refresh cache' do
+            expect(Rails.cache).not_to receive(:write)
+            expect(subject.count).to eq(under_threshold)
+          end
+        end
+
+        context 'when cached count is over the threshold value' do
+          before do
+            Rails.cache.write(public_count_key, over_threshold)
+          end
+
+          it 'does not refresh cache' do
+            expect(Rails.cache).not_to receive(:write)
+            expect(subject.count).to eq(over_threshold)
+          end
+        end
+      end
+    end
+  end
+end