Merge branch 'dz-integrated-error-tracking-pagination' into 'master'

Add pagination to integrated error tracking

See merge request gitlab-org/gitlab!70945

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);
 

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