Merge branch 'report-ci-minutes-reset-error-after-retries' into 'master'

Report BatchNotResetError to Sentry only after retries

See merge request gitlab-org/gitlab!42074

ee/app/services/ci/minutes/batch_reset_service.rb

@@ -3,12 +3,26 @@
 module Ci
   module Minutes
     class BatchResetService
-      BatchNotResetError = Class.new(StandardError)
+      class BatchNotResetError < StandardError
+        def initialize(failed_batches)
+          @failed_batches = failed_batches
+        end
+
+        def message
+          'Some namespace shared runner minutes were not reset'
+        end
+
+        def sentry_extra_data
+          {
+            failed_batches: @failed_batches
+          }
+        end
+      end
 
       BATCH_SIZE = 1000.freeze
 
       def initialize
-        @errors = []
+        @failed_batches = []
       end
 
       def execute!(ids_range: nil, batch_size: BATCH_SIZE)
@@ -18,10 +32,7 @@ module Ci
           reset_ci_minutes!(namespaces)
         end
 
-        if @errors.any?
-          exception = BatchNotResetError.new('Some namespace shared runner minutes were not reset.')
-          Gitlab::ErrorTracking.track_and_raise_exception(exception, namespace_ranges: @errors)
-        end
+        raise BatchNotResetError.new(@failed_batches) if @failed_batches.any?
       end
 
       private
@@ -36,7 +47,15 @@ module Ci
           reset_ci_minutes_notifications!(namespaces)
         end
       rescue ActiveRecord::ActiveRecordError => e
-        @errors << { count: namespaces.size, first_id: namespaces.first.id, last_id: namespaces.last.id, error: e.message }
+        # We cleanup the backtrace for intermediate errors so they remain compact and
+        # relevant due to the possibility of having many failed batches.
+        @failed_batches << {
+          count: namespaces.size,
+          first_namespace_id: namespaces.first.id,
+          last_namespace_id: namespaces.last.id,
+          error_message: e.message,
+          error_backtrace: ::Gitlab::BacktraceCleaner.clean_backtrace(e.backtrace)
+        }
       end
 
       # This service is responsible for the logic that recalculates the extra shared runners

ee/spec/services/ci/minutes/batch_reset_service_spec.rb

@@ -142,25 +142,23 @@ RSpec.describe Ci::Minutes::BatchResetService do
           expect(Namespace).to receive(:transaction).once.ordered.and_call_original
         end
 
-        it 'continues its progress' do
+        it 'continues its progress and raises exception at the end' do
           expect(service).to receive(:reset_ci_minutes!).with([namespace_1, namespace_2, namespace_3]).and_call_original
           expect(service).to receive(:reset_ci_minutes!).with([namespace_4, namespace_5, namespace_6]).and_call_original
 
-          expect(Gitlab::ErrorTracking).to receive(:track_and_raise_exception)
-          subject
+          expect { subject }
+            .to raise_error(described_class::BatchNotResetError) do |error|
+              expect(error.message).to eq('Some namespace shared runner minutes were not reset')
+              expect(error.sentry_extra_data[:failed_batches]).to contain_exactly(
+                {
+                  count: 3,
+                  first_namespace_id: 1,
+                  last_namespace_id: 3,
+                  error_message: 'something went wrong',
+                  error_backtrace: kind_of(Array)
+                }
+              )
             end
-
-        it 'raises exception with namespace details' do
-          expect(Gitlab::ErrorTracking).to receive(
-            :track_and_raise_exception
-          ).with(
-            Ci::Minutes::BatchResetService::BatchNotResetError.new(
-              'Some namespace shared runner minutes were not reset.'
-            ),
-            { namespace_ranges: [{ count: 3, first_id: 1, last_id: 3, error: 'something went wrong' }] }
-          ).once.and_call_original
-
-          expect { subject }.to raise_error(Ci::Minutes::BatchResetService::BatchNotResetError)
         end
       end
     end

ee/spec/workers/clear_shared_runners_minutes_worker_spec.rb

@@ -50,7 +50,7 @@ RSpec.describe ClearSharedRunnersMinutesWorker do
           it 'raises an exception' do
             expect { worker.perform }.to raise_error(
               Ci::Minutes::BatchResetService::BatchNotResetError,
-              'Some namespace shared runner minutes were not reset.'
+              'Some namespace shared runner minutes were not reset'
             )
           end
         end