Automatic merge of gitlab-org/gitlab master

Gemfile

@@ -355,7 +355,7 @@ group :development, :test do
   gem 'bullet', '~> 6.1.3'
   gem 'pry-byebug'
   gem 'pry-rails', '~> 0.3.9'
-  gem 'pry-shell', '~> 0.4.0'
+  gem 'pry-shell', '~> 0.5.0'
 
   gem 'awesome_print', require: false
 

Gemfile.lock

@@ -933,7 +933,7 @@ GEM
       pry (~> 0.13.0)
     pry-rails (0.3.9)
       pry (>= 0.10.4)
-    pry-shell (0.4.1)
+    pry-shell (0.5.0)
       pry (~> 0.13.0)
       tty-markdown
       tty-prompt
@@ -1567,7 +1567,7 @@ DEPENDENCIES
   prometheus-client-mmap (~> 0.15.0)
   pry-byebug
   pry-rails (~> 0.3.9)
-  pry-shell (~> 0.4.0)
+  pry-shell (~> 0.5.0)
   puma (~> 5.3.1)
   puma_worker_killer (~> 0.3.1)
   rack (~> 2.2.3)

app/finders/error_tracking/errors_finder.rb

@@ -15,8 +15,7 @@ module ErrorTracking
       collection = by_status(collection)
       collection = sort(collection)
 
-      # Limit collection until pagination implemented.
-      limit(collection)
+      collection.keyset_paginate(cursor: params[:cursor], per_page: limit)
     end
 
     private
@@ -39,9 +38,9 @@ module ErrorTracking
       params[:sort] ? collection.sort_by_attribute(params[:sort]) : collection.order_id_desc
     end
 
-    def limit(collection)
+    def limit
       # Restrict the maximum limit at 100 records.
-      collection.limit([(params[:limit] || 20).to_i, 100].min)
+      [(params[:limit] || 20).to_i, 100].min
     end
   end
 end

app/services/error_tracking/list_issues_service.rb

@@ -76,16 +76,21 @@ module ErrorTracking
         filter_opts = {
           status: opts[:issue_status],
           sort: opts[:sort],
-          limit: opts[:limit]
+          limit: opts[:limit],
+          cursor: opts[:cursor]
         }
 
         errors = ErrorTracking::ErrorsFinder.new(current_user, project, filter_opts).execute
 
+        pagination = {}
+        pagination[:next] = { cursor: errors.cursor_for_next_page } if errors.has_next_page?
+        pagination[:previous] = { cursor: errors.cursor_for_previous_page } if errors.has_previous_page?
+
         # We use the same response format as project_error_tracking_setting
         # method below for compatibility with existing code.
         {
           issues: errors.map(&:to_sentry_error),
-          pagination: {}
+          pagination: pagination
         }
       else
         project_error_tracking_setting.list_sentry_issues(**opts)

db/migrate/20210928155022_improve_index_for_error_tracking.rb

+# frozen_string_literal: true
+
+class ImproveIndexForErrorTracking < Gitlab::Database::Migration[1.0]
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :error_tracking_errors, %i(project_id status last_seen_at id),
+      order: { last_seen_at: :desc, id: :desc },
+      name: 'index_et_errors_on_project_id_and_status_last_seen_at_id_desc'
+
+    add_concurrent_index :error_tracking_errors, %i(project_id status first_seen_at id),
+      order: { first_seen_at: :desc, id: :desc },
+      name: 'index_et_errors_on_project_id_and_status_first_seen_at_id_desc'
+
+    add_concurrent_index :error_tracking_errors, %i(project_id status events_count id),
+      order: { events_count: :desc, id: :desc },
+      name: 'index_et_errors_on_project_id_and_status_events_count_id_desc'
+
+    remove_concurrent_index :error_tracking_errors, [:project_id, :status, :last_seen_at], name: 'index_et_errors_on_project_id_and_status_and_last_seen_at'
+    remove_concurrent_index :error_tracking_errors, [:project_id, :status, :first_seen_at], name: 'index_et_errors_on_project_id_and_status_and_first_seen_at'
+    remove_concurrent_index :error_tracking_errors, [:project_id, :status, :events_count], name: 'index_et_errors_on_project_id_and_status_and_events_count'
+  end
+
+  def down
+    add_concurrent_index :error_tracking_errors, [:project_id, :status, :last_seen_at], name: 'index_et_errors_on_project_id_and_status_and_last_seen_at'
+    add_concurrent_index :error_tracking_errors, [:project_id, :status, :first_seen_at], name: 'index_et_errors_on_project_id_and_status_and_first_seen_at'
+    add_concurrent_index :error_tracking_errors, [:project_id, :status, :events_count], name: 'index_et_errors_on_project_id_and_status_and_events_count'
+
+    remove_concurrent_index :error_tracking_errors, [:project_id, :status, :last_seen_at, :id], name: 'index_et_errors_on_project_id_and_status_last_seen_at_id_desc'
+    remove_concurrent_index :error_tracking_errors, [:project_id, :status, :first_seen_at, :id], name: 'index_et_errors_on_project_id_and_status_first_seen_at_id_desc'
+    remove_concurrent_index :error_tracking_errors, [:project_id, :status, :events_count, :id], name: 'index_et_errors_on_project_id_and_status_events_count_id_desc'
+  end
+end

db/schema_migrations/20210928155022

+41ea0971cd62ba43bf98c1901169e7bb8fcebe68025d947f26b0ccf6806c976e
\ No newline at end of file

db/structure.sql

@@ -25031,13 +25031,13 @@ CREATE UNIQUE INDEX index_escalation_rules_on_all_attributes ON incident_managem
 
 CREATE INDEX index_escalation_rules_on_user ON incident_management_escalation_rules USING btree (user_id);
 
-CREATE INDEX index_et_errors_on_project_id_and_status_and_events_count ON error_tracking_errors USING btree (project_id, status, events_count);
+CREATE INDEX index_et_errors_on_project_id_and_status_and_id ON error_tracking_errors USING btree (project_id, status, id);
 
-CREATE INDEX index_et_errors_on_project_id_and_status_and_first_seen_at ON error_tracking_errors USING btree (project_id, status, first_seen_at);
+CREATE INDEX index_et_errors_on_project_id_and_status_events_count_id_desc ON error_tracking_errors USING btree (project_id, status, events_count DESC, id DESC);
 
-CREATE INDEX index_et_errors_on_project_id_and_status_and_id ON error_tracking_errors USING btree (project_id, status, id);
+CREATE INDEX index_et_errors_on_project_id_and_status_first_seen_at_id_desc ON error_tracking_errors USING btree (project_id, status, first_seen_at DESC, id DESC);
 
-CREATE INDEX index_et_errors_on_project_id_and_status_and_last_seen_at ON error_tracking_errors USING btree (project_id, status, last_seen_at);
+CREATE INDEX index_et_errors_on_project_id_and_status_last_seen_at_id_desc ON error_tracking_errors USING btree (project_id, status, last_seen_at DESC, id DESC);
 
 CREATE INDEX index_events_on_action ON events USING btree (action);
 

doc/administration/get_started.md

@@ -128,7 +128,7 @@ The routine differs, depending on whether you deployed with Omnibus or the Helm
 
 When you backing up an Omnibus (single node) GitLab server, you can use a single Rake task.
 
-Learn about [backing up Omnibus or Helm variations](../raketasks/backup_restore.md#back-up-gitlab).
+Learn about [backing up Omnibus or Helm variations](../raketasks/backup_restore.md).
 This process backs up your entire instance, but does not back up the configuration files. Ensure those are backed up separately.
 Keep your configuration files and backup archives in a separate location to ensure the encryption keys are not kept with the encrypted data.
 

doc/administration/logs.md

@@ -1075,7 +1075,7 @@ GitLab also tracks [Prometheus metrics for Praefect](gitaly/#monitor-gitaly-clus
 
 For Omnibus installations, the backup log is located at `/var/log/gitlab/gitlab-rails/backup_json.log`.
 
-This log is populated when a [GitLab backup is created](../raketasks/backup_restore.md#back-up-gitlab). You can use this log to understand how the backup process performed.
+This log is populated when a [GitLab backup is created](../raketasks/backup_restore.md). You can use this log to understand how the backup process performed.
 
 ## Performance bar stats
 

doc/administration/read_only_gitlab.md

@@ -66,7 +66,7 @@ sudo gitlab-ctl start puma
 If you want to allow users to use the GitLab UI, then you'll need to ensure that
 the database is read-only:
 
-1. Take a [GitLab backup](../raketasks/backup_restore.md#back-up-gitlab)
+1. Take a [GitLab backup](../raketasks/backup_restore.md)
    in case things don't go as expected.
 1. Enter PostgreSQL on the console as an administrator user:
 

doc/install/aws/manual_install_aws.md

@@ -748,7 +748,7 @@ Read more on configuring an
 
 ## Backup and restore
 
-GitLab provides [a tool to back up](../../raketasks/backup_restore.md#back-up-gitlab)
+GitLab provides [a tool to back up](../../raketasks/backup_restore.md)
 and restore its Git data, database, attachments, LFS objects, and so on.
 
 Some important things to know:

doc/integration/mattermost/index.md

@@ -175,7 +175,7 @@ sudo gitlab-psql -d mattermost_production
 
 ### Back up GitLab Mattermost
 
-GitLab Mattermost is not included in the regular [Omnibus GitLab backup](../../raketasks/backup_restore.md#back-up-gitlab) Rake task.
+GitLab Mattermost is not included in the regular [Omnibus GitLab backup](../../raketasks/backup_restore.md) Rake task.
 
 The general Mattermost [backup and disaster recovery](https://docs.mattermost.com/deploy/backup-disaster-recovery.html) documentation can be used as a guide
 on what needs to be backed up.

doc/update/index.md

@@ -193,7 +193,7 @@ upgrade paths.
 
 | Target version | Your version | Supported upgrade path                                                                               | Note                                                                                                                              |
 | -------------- | ------------ | ---------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------- |
-| `14.1.2`       | `13.9.2`     | `13.9.2` -> `13.12.9` -> `14.0.7` -> `14.1.2`                                                        | Two intermediate versions are required: `13.12` and `14.0`, then `14.1`.                                                          |
+| `14.1.6`       | `13.9.2`     | `13.9.2` -> `13.12.12` -> `14.0.11` -> `14.1.6`                                                        | Two intermediate versions are required: `13.12` and `14.0`, then `14.1`.                                                          |
 | `13.12.10`     | `12.9.2`     | `12.9.2` -> `12.10.14` -> `13.0.14`  -> `13.1.11` -> `13.8.8` -> `13.12.10`                          | Four intermediate versions are required: `12.10`, `13.0`, `13.1` and `13.8.8`, then `13.12.10`.                               |
 | `13.2.10`      | `11.5.0`     | `11.5.0` -> `11.11.8` -> `12.0.12` -> `12.1.17` -> `12.10.14` -> `13.0.14` -> `13.1.11` -> `13.2.10` | Six intermediate versions are required: `11.11`, `12.0`, `12.1`, `12.10`, `13.0` and `13.1`, then `13.2.10`.                      |
 | `12.10.14`     | `11.3.4`     | `11.3.4` -> `11.11.8` -> `12.0.12` -> `12.1.17` -> `12.10.14`                                        | Three intermediate versions are required: `11.11`, `12.0` and `12.1`, then `12.10.14`.                                            |

doc/update/package/downgrade.md

@@ -12,7 +12,7 @@ of a package.
 WARNING:
 You must at least have a database backup created under the version you are
 downgrading to. Ideally, you should have a
-[full backup archive](../../raketasks/backup_restore.md#back-up-gitlab)
+[full backup archive](../../raketasks/backup_restore.md)
 on hand.
 
 The example below demonstrates the downgrade procedure when downgrading between minor

doc/update/plan_your_upgrade.md

@@ -75,7 +75,7 @@ Create a backup of GitLab and all its data (database, repos, uploads, builds,
 artifacts, LFS objects, registry, pages). This is vital for making it possible
 to roll back GitLab to a working state if there's a problem with the upgrade:
 
-- Create a [GitLab backup](../raketasks/backup_restore.md#back-up-gitlab).
+- Create a [GitLab backup](../raketasks/backup_restore.md).
   Make sure to follow the instructions based on your installation method.
   Don't forget to back up the [secrets and configuration files](../raketasks/backup_restore.md#storing-configuration-files).
 - Alternatively, create a snapshot of your instance. If this is a multi-node

doc/user/project/file_lock.md

@@ -49,7 +49,7 @@ This process allows you to lock single files or file extensions and it is
 done through the command line. It doesn't require GitLab paid subscriptions.
 
 Git LFS is well known for tracking files to reduce the storage of
-Git repositories, but it can also be user for [locking files](https://github.com/git-lfs/git-lfs/wiki/File-Locking).
+Git repositories, but it can also be used for [locking files](https://github.com/git-lfs/git-lfs/wiki/File-Locking).
 This is the method used for Exclusive File Locks.
 
 ### Install Git LFS

doc/user/project/issues/managing_issues.md

@@ -212,7 +212,7 @@ that are not in status **closed** from one project to another.
 
 To access rails console run `sudo gitlab-rails console` on the GitLab server and run the below
 script. Please be sure to change `project`, `admin_user`, and `target_project` to your values.
-We do also recommend [creating a backup](../../../raketasks/backup_restore.md#back-up-gitlab) before
+We do also recommend [creating a backup](../../../raketasks/backup_restore.md) before
 attempting any changes in the console.
 
 ```ruby

spec/finders/error_tracking/errors_finder_spec.rb

@@ -29,14 +29,25 @@ RSpec.describe ErrorTracking::ErrorsFinder do
     context 'with sort parameter' do
       let(:params) { { status: 'unresolved', sort: 'first_seen' } }
 
-      it { is_expected.to eq([error, error_yesterday]) }
+      it { expect(subject.to_a).to eq([error, error_yesterday]) }
     end
 
-    context 'with limit parameter' do
+    context 'pagination' do
       let(:params) { { limit: '1', sort: 'first_seen' } }
 
       # Sort by first_seen is DESC by default, so the most recent error is `error`
       it { is_expected.to contain_exactly(error) }
+
+      it { expect(subject.has_next_page?).to be_truthy }
+
+      it 'returns next page by cursor' do
+        params_with_cursor = params.merge(cursor: subject.cursor_for_next_page)
+        errors = described_class.new(user, project, params_with_cursor).execute
+
+        expect(errors).to contain_exactly(error_resolved)
+        expect(errors.has_next_page?).to be_truthy
+        expect(errors.has_previous_page?).to be_truthy
+      end
     end
   end
 end

spec/services/error_tracking/list_issues_service_spec.rb

@@ -5,56 +5,71 @@ require 'spec_helper'
 RSpec.describe ErrorTracking::ListIssuesService do
   include_context 'sentry error tracking context'
 
-  let(:params) { { search_term: 'something', sort: 'last_seen', cursor: 'some-cursor' } }
-  let(:list_sentry_issues_args) do
-    {
-      issue_status: 'unresolved',
-      limit: 20,
-      search_term: 'something',
-      sort: 'last_seen',
-      cursor: 'some-cursor'
-    }
-  end
+  let(:params) { {} }
 
   subject { described_class.new(project, user, params) }
 
   describe '#execute' do
-    context 'with authorized user' do
-      let(:issues) { [] }
+    context 'Sentry backend' do
+      let(:params) { { search_term: 'something', sort: 'last_seen', cursor: 'some-cursor' } }
+
+      let(:list_sentry_issues_args) do
+        {
+          issue_status: 'unresolved',
+          limit: 20,
+          search_term: 'something',
+          sort: 'last_seen',
+          cursor: 'some-cursor'
+        }
+      end
+
+      context 'with authorized user' do
+        let(:issues) { [] }
+
+        described_class::ISSUE_STATUS_VALUES.each do |status|
+          it "returns the issues with #{status} issue_status" do
+            params[:issue_status] = status
+            list_sentry_issues_args[:issue_status] = status
+            expect_list_sentry_issues_with(list_sentry_issues_args)
+
+            expect(result).to eq(status: :success, pagination: {}, issues: issues)
+          end
+        end
 
-      described_class::ISSUE_STATUS_VALUES.each do |status|
-        it "returns the issues with #{status} issue_status" do
-          params[:issue_status] = status
-          list_sentry_issues_args[:issue_status] = status
+        it 'returns the issues with no issue_status' do
           expect_list_sentry_issues_with(list_sentry_issues_args)
 
           expect(result).to eq(status: :success, pagination: {}, issues: issues)
         end
-      end
 
-      it 'returns the issues with no issue_status' do
-        expect_list_sentry_issues_with(list_sentry_issues_args)
+        it 'returns bad request for an issue_status not on the whitelist' do
+          params[:issue_status] = 'assigned'
+
+          expect(error_tracking_setting).not_to receive(:list_sentry_issues)
+          expect(result).to eq(message: "Bad Request: Invalid issue_status", status: :error, http_status: :bad_request)
+        end
 
-        expect(result).to eq(status: :success, pagination: {}, issues: issues)
+        include_examples 'error tracking service data not ready', :list_sentry_issues
+        include_examples 'error tracking service sentry error handling', :list_sentry_issues
+        include_examples 'error tracking service http status handling', :list_sentry_issues
       end
 
-      it 'returns bad request for an issue_status not on the whitelist' do
-        params[:issue_status] = 'assigned'
+      include_examples 'error tracking service unauthorized user'
+      include_examples 'error tracking service disabled'
 
-        expect(error_tracking_setting).not_to receive(:list_sentry_issues)
-        expect(result).to eq(message: "Bad Request: Invalid issue_status", status: :error, http_status: :bad_request)
+      def expect_list_sentry_issues_with(list_sentry_issues_args)
+        expect(error_tracking_setting)
+          .to receive(:list_sentry_issues)
+          .with(list_sentry_issues_args)
+          .and_return(issues: [], pagination: {})
       end
-
-      include_examples 'error tracking service data not ready', :list_sentry_issues
-      include_examples 'error tracking service sentry error handling', :list_sentry_issues
-      include_examples 'error tracking service http status handling', :list_sentry_issues
     end
 
-    include_examples 'error tracking service unauthorized user'
-    include_examples 'error tracking service disabled'
+    context 'GitLab backend' do
+      let_it_be(:error1) { create(:error_tracking_error, name: 'foo', project: project) }
+      let_it_be(:error2) { create(:error_tracking_error, name: 'bar', project: project) }
 
-    context 'integrated error tracking' do
-      let_it_be(:error) { create(:error_tracking_error, project: project) }
+      let(:params) { { limit: '1' } }
 
       before do
         error_tracking_setting.update!(integrated: true)
@@ -63,7 +78,9 @@ RSpec.describe ErrorTracking::ListIssuesService do
       it 'returns the error in expected format' do
         expect(result[:status]).to eq(:success)
         expect(result[:issues].size).to eq(1)
-        expect(result[:issues].first.to_json).to eq(error.to_sentry_error.to_json)
+        expect(result[:issues].first.to_json).to eq(error2.to_sentry_error.to_json)
+        expect(result[:pagination][:next][:cursor]).to be_present
+        expect(result[:pagination][:previous]).to be_nil
       end
     end
   end
@@ -76,10 +93,3 @@ RSpec.describe ErrorTracking::ListIssuesService do
     end
   end
 end
-
-def expect_list_sentry_issues_with(list_sentry_issues_args)
-  expect(error_tracking_setting)
-    .to receive(:list_sentry_issues)
-    .with(list_sentry_issues_args)
-    .and_return(issues: [], pagination: {})
-end