Recover gracefully when issuable counts are too expensive

On the MR list page, we like to display how many issues were found from
the filtered search query *in total*. However, especially when the
filter includes conditions on the MR title or description, this can be
very expensive to calculate, and involve reading gigabytes of text data
from the database.

As long as the data is already in the page cache, this usually finishes
within the 15-second timeout on GitLab.com, but if the database cache
is cold, a statement timeout is the usual occurrence.

More generally, it's not very clever to spend so much time calculating
a piece of information with marginal value.

This MR applies a shorter limit to the counting statements and provides
for graceful fallback to a '?' value, with a nice tooltip, if the query
times out. This means we're able to view the results in a reasonable
time, rather than the page taking a long time to load, or not loading
at all.

app/controllers/concerns/issuable_collections.rb

@@ -66,6 +66,7 @@ module IssuableCollections
     limit = relation.limit_value.to_f
 
     return 1 if limit == 0
+    return (params[:page] || 1).to_i + 1 if row_count == -1
 
     (row_count.to_f / limit).ceil
   end

app/finders/issuable_finder.rb

@@ -153,7 +153,9 @@ class IssuableFinder
   end
 
   def row_count
-    Gitlab::IssuablesCountForState.new(self).for_state_or_opened(params[:state])
+    Gitlab::IssuablesCountForState
+      .new(self, nil, fast_fail: Feature.enabled?(:soft_fail_count_by_state, parent))
+      .for_state_or_opened(params[:state])
   end
 
   # We often get counts for each state by running a query per state, and

app/helpers/issuables_helper.rb

@@ -250,7 +250,22 @@ module IssuablesHelper
 
     if display_count
       count = issuables_count_for_state(issuable_type, state)
-      html << " " << content_tag(:span, number_with_delimiter(count), class: 'badge badge-pill')
+      tag =
+        if count == -1
+          tooltip = _("Couldn't calculate number of %{issuables}.") % { issuables: issuable_type.to_s.humanize(capitalize: false) }
+
+          content_tag(
+            :span,
+            '?',
+            class: 'badge badge-pill has-tooltip',
+            aria: { label: tooltip },
+            title: tooltip
+          )
+        else
+          content_tag(:span, number_with_delimiter(count), class: 'badge badge-pill')
+        end
+
+      html << " " << tag
     end
 
     html.html_safe

app/models/application_record.rb

@@ -52,6 +52,16 @@ class ApplicationRecord < ActiveRecord::Base
     end
   end
 
+  # Start a new transaction with a shorter-than-usual statement timeout. This is
+  # currently one third of the default 15-second timeout
+  def self.with_fast_statement_timeout
+    transaction(requires_new: true) do
+      connection.exec_query("SET LOCAL statement_timeout = 5000")
+
+      yield
+    end
+  end
+
   def self.safe_find_or_create_by(*args, &block)
     safe_ensure_unique(retries: 1) do
       find_or_create_by(*args, &block)

app/views/projects/issues/export_csv/_modal.html.haml

@@ -10,11 +10,12 @@
           %a.close{ href: '#', 'data-dismiss' => 'modal' }
             = sprite_icon('close', css_class: 'gl-icon')
         .modal-body
-          .modal-subheader
-            = icon('check', { class: 'checkmark' })
-            %strong.gl-ml-3
-              - issues_count = issuables_count_for_state(:issues, params[:state])
-              = n_('%d issue selected', '%d issues selected', issues_count) % issues_count
+          - issues_count = issuables_count_for_state(:issues, params[:state])
+          - unless issues_count == -1 # The count timed out
+            .modal-subheader
+              = icon('check', { class: 'checkmark' })
+              %strong.gl-ml-3
+                = n_('%d issue selected', '%d issues selected', issues_count) % issues_count
           .modal-text
             = html_escape(_('The CSV export will be created in the background. Once finished, it will be sent to %{strong_open}%{email}%{strong_close} in an attachment.')) % { email: @current_user.notification_email, strong_open: '<strong>'.html_safe, strong_close: '</strong>'.html_safe }
         .modal-footer

changelogs/unreleased/249180-add-time-limit-on-counts.yml

+---
+title: Recover gracefully when issuable counts are too expensive
+merge_request: 44184
+author:
+type: fixed

config/feature_flags/development/soft_fail_count_by_state.yml

+---
+name: soft_fail_count_by_state
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/44184
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/263222
+type: development
+group: group::source code
+default_enabled: false

doc/administration/monitoring/prometheus/gitlab_metrics.md

@@ -113,6 +113,8 @@ The following metrics are available:
 | `action_cable_pool_largest_size`                               | Gauge     |                   13.4 | Largest number of worker threads observed so far in ActionCable thread pool | `server_mode` |
 | `action_cable_pool_pending_tasks`                              | Gauge     |                   13.4 | Number of tasks waiting to be executed in ActionCable thread pool | `server_mode` |
 | `action_cable_pool_tasks_total`                                | Gauge     |                   13.4 | Total number of tasks executed in ActionCable thread pool | `server_mode` |
+| `gitlab_issuable_fast_count_by_state_total`                    | Counter   |                   13.5 | Total number of row count operations on issue/merge request list pages | |
+| `gitlab_issuable_fast_count_by_state_failures_total`           | Counter   |                   13.5 | Number of soft-failed row count operations on issue/merge request list pages | |
 
 ## Metrics controlled by a feature flag
 
@@ -122,6 +124,8 @@ The following metrics can be controlled by feature flags:
 |:---------------------------------------------------------------|:-------------------------------------------------------------------|
 | `gitlab_method_call_duration_seconds`                          | `prometheus_metrics_method_instrumentation`                        |
 | `gitlab_view_rendering_duration_seconds`                       | `prometheus_metrics_view_instrumentation`                          |
+| `gitlab_issuable_fast_count_by_state_total`                    | `soft_fail_count_by_state` |
+| `gitlab_issuable_fast_count_by_state_failures_total`           | `soft_fail_count_by_state` |
 
 ## Sidekiq metrics
 

ee/spec/finders/epics_finder_spec.rb

@@ -664,6 +664,16 @@ RSpec.describe EpicsFinder do
       expect(results).to eq('opened' => 2, 'closed' => 1, 'all' => 3)
     end
 
+    it 'returns -1 if the query times out' do
+      finder = described_class.new(search_user, group_id: group.id)
+
+      expect_next_instance_of(described_class) do |subfinder|
+        expect(subfinder).to receive(:execute).and_raise(ActiveRecord::QueryCanceled)
+      end
+
+      expect(finder.row_count).to eq(-1)
+    end
+
     context 'when using group cte for search' do
       before do
         stub_feature_flags(use_subquery_for_group_issues_search: false)

lib/gitlab/issuables_count_for_state.rb

@@ -16,9 +16,12 @@ module Gitlab
     end
 
     # finder - The finder class to use for retrieving the issuables.
-    def initialize(finder, project = nil)
+    # fast_fail - restrict counting to a shorter period, degrading gracefully on
+    # failure
+    def initialize(finder, project = nil, fast_fail: false)
       @finder = finder
       @project = project
+      @fast_fail = fast_fail
       @cache = Gitlab::SafeRequestStore[CACHE_KEY] ||= initialize_cache
     end
 
@@ -26,6 +29,10 @@ module Gitlab
       self[state || :opened]
     end
 
+    def fast_fail?
+      !!@fast_fail
+    end
+
     # Define method for each state
     STATES.each do |state|
       define_method(state) { self[state] }
@@ -53,7 +60,53 @@ module Gitlab
     end
 
     def initialize_cache
-      Hash.new { |hash, finder| hash[finder] = finder.count_by_state }
+      Hash.new { |hash, finder| hash[finder] = perform_count(finder) }
+    end
+
+    def perform_count(finder)
+      return finder.count_by_state unless fast_fail?
+
+      fast_count_by_state_attempt!
+
+      # Determining counts when referring to issuable titles or descriptions can
+      # be very expensive, and involve the database reading gigabytes of data
+      # for a relatively minor piece of functionality. This may slow index pages
+      # by seconds in the best case, or lead to a statement timeout in the worst
+      # case.
+      #
+      # In time, we may be able to use elasticsearch or postgresql tsv columns
+      # to perform the calculation more efficiently. Until then, use a shorter
+      # timeout and return -1 as a sentinel value if it is triggered
+      begin
+        ApplicationRecord.with_fast_statement_timeout do
+          finder.count_by_state
+        end
+      rescue ActiveRecord::QueryCanceled => err
+        fast_count_by_state_failure!
+
+        Gitlab::ErrorTracking.track_exception(
+          err,
+          params: finder.params,
+          current_user_id: finder.current_user&.id,
+          issue_url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/249180'
+        )
+
+        Hash.new(-1)
+      end
+    end
+
+    def fast_count_by_state_attempt!
+      Gitlab::Metrics.counter(
+        :gitlab_issuable_fast_count_by_state_total,
+        "Count of total calls to IssuableFinder#count_by_state with fast failure"
+      ).increment
+    end
+
+    def fast_count_by_state_failure!
+      Gitlab::Metrics.counter(
+        :gitlab_issuable_fast_count_by_state_failures_total,
+        "Count of failed calls to IssuableFinder#count_by_state with fast failure"
+      ).increment
     end
   end
 end

locale/gitlab.pot

@@ -7448,6 +7448,9 @@ msgstr ""
 msgid "Could not upload your designs as one or more files uploaded are not supported."
 msgstr ""
 
+msgid "Couldn't calculate number of %{issuables}."
+msgstr ""
+
 msgid "Country"
 msgstr ""
 

spec/controllers/concerns/issuable_collections_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe IssuableCollections do
+  using RSpec::Parameterized::TableSyntax
+
   let(:user) { create(:user) }
 
   let(:controller) do
@@ -25,13 +27,35 @@ RSpec.describe IssuableCollections do
   end
 
   describe '#page_count_for_relation' do
-    let(:params) { { state: 'opened' } }
+    let(:relation) { double(:relation, limit_value: 20) }
+
+    context 'row count is known' do
+      let(:params) { { state: 'opened' } }
+
+      it 'returns the number of pages' do
+        pages = controller.send(:page_count_for_relation, relation, 28)
+
+        expect(pages).to eq(2)
+      end
+    end
+
+    context 'row_count is unknown' do
+      where(:page_param, :expected) do
+        nil | 2
+        1   | 2
+        '1' | 2
+        2   | 3
+      end
 
-    it 'returns the number of pages' do
-      relation = double(:relation, limit_value: 20)
-      pages = controller.send(:page_count_for_relation, relation, 28)
+      with_them do
+        let(:params) { { state: 'opened', page: page_param } }
 
-      expect(pages).to eq(2)
+        it 'returns current page + 1 if the row count is unknown' do
+          pages = controller.send(:page_count_for_relation, relation, -1)
+
+          expect(pages).to eq(expected)
+        end
+      end
     end
   end
 

spec/features/issuables/issuable_list_spec.rb

@@ -48,6 +48,14 @@ RSpec.describe 'issuable list', :js do
     end
   end
 
+  it 'displays a warning if counting the number of issues times out' do
+    allow_any_instance_of(IssuesFinder).to receive(:count_by_state).and_raise(ActiveRecord::QueryCanceled)
+
+    visit_issuable_list(:issue)
+
+    expect(page).to have_text('Open ? Closed ? All ?')
+  end
+
   it "counts merge requests closing issues icons for each issue" do
     visit_issuable_list(:issue)
 

spec/finders/issues_finder_spec.rb

@@ -843,6 +843,16 @@ RSpec.describe IssuesFinder do
 
       expect(finder.row_count).to be_zero
     end
+
+    it 'returns -1 if the query times out' do
+      finder = described_class.new(admin)
+
+      expect_next_instance_of(described_class) do |subfinder|
+        expect(subfinder).to receive(:execute).and_raise(ActiveRecord::QueryCanceled)
+      end
+
+      expect(finder.row_count).to eq(-1)
+    end
   end
 
   describe '#with_confidentiality_access_check' do

spec/finders/merge_requests_finder_spec.rb

@@ -500,6 +500,16 @@ RSpec.describe MergeRequestsFinder do
 
         expect(finder.row_count).to eq(1)
       end
+
+      it 'returns -1 if the query times out' do
+        finder = described_class.new(user)
+
+        expect_next_instance_of(described_class) do |subfinder|
+          expect(subfinder).to receive(:execute).and_raise(ActiveRecord::QueryCanceled)
+        end
+
+        expect(finder.row_count).to eq(-1)
+      end
     end
 
     context 'external authorization' do

spec/lib/gitlab/issuables_count_for_state_spec.rb

@@ -4,14 +4,15 @@ require 'spec_helper'
 
 RSpec.describe Gitlab::IssuablesCountForState do
   let(:finder) do
-    double(:finder, count_by_state: { opened: 2, closed: 1 })
+    double(:finder, current_user: nil, params: {}, count_by_state: { opened: 2, closed: 1 })
   end
 
-  let(:counter) { described_class.new(finder) }
+  let(:project) { nil }
+  let(:fast_fail) { nil }
+  let(:counter) { described_class.new(finder, project, fast_fail: fast_fail) }
 
   describe 'project given' do
     let(:project) { build(:project) }
-    let(:counter) { described_class.new(finder, project) }
 
     it 'provides the project' do
       expect(counter.project).to eq(project)
@@ -50,5 +51,19 @@ RSpec.describe Gitlab::IssuablesCountForState do
     it 'returns 0 when using an invalid state name as a String' do
       expect(counter['kittens']).to be_zero
     end
+
+    context 'fast_fail enabled' do
+      let(:fast_fail) { true }
+
+      it 'returns the expected value' do
+        expect(counter[:closed]).to eq(1)
+      end
+
+      it 'returns -1 when the database times out' do
+        expect(finder).to receive(:count_by_state).and_raise(ActiveRecord::QueryCanceled)
+
+        expect(counter[:closed]).to eq(-1)
+      end
+    end
   end
 end