Merge branch '31063-ensure-diff-collection-limits-are-configurable' into 'master'

Resolve "Ensure diff collection limits are monitorable and configurable"

See merge request gitlab-org/gitlab!57790

app/helpers/diff_helper.rb

@@ -190,12 +190,8 @@ module DiffHelper
   def render_overflow_warning?(diffs_collection)
     diff_files = diffs_collection.raw_diff_files
 
-    if diff_files.any?(&:too_large?)
-      Gitlab::Metrics.add_event(:diffs_overflow_single_file_limits)
-    end
-
     diff_files.overflow?.tap do |overflown|
-      Gitlab::Metrics.add_event(:diffs_overflow_collection_limits) if overflown
+      log_overflow_limits(diff_files)
     end
   end
 
@@ -286,4 +282,18 @@ module DiffHelper
 
     conflicts_service.conflicts.files.index_by(&:our_path)
   end
+
+  def log_overflow_limits(diff_files)
+    if diff_files.any?(&:too_large?)
+      Gitlab::Metrics.add_event(:diffs_overflow_single_file_limits)
+    end
+
+    Gitlab::Metrics.add_event(:diffs_overflow_collection_limits) if diff_files.overflow?
+    Gitlab::Metrics.add_event(:diffs_overflow_max_bytes_limits) if diff_files.overflow_max_bytes?
+    Gitlab::Metrics.add_event(:diffs_overflow_max_files_limits) if diff_files.overflow_max_files?
+    Gitlab::Metrics.add_event(:diffs_overflow_max_lines_limits) if diff_files.overflow_max_lines?
+    Gitlab::Metrics.add_event(:diffs_overflow_collapsed_bytes_limits) if diff_files.collapsed_safe_bytes?
+    Gitlab::Metrics.add_event(:diffs_overflow_collapsed_files_limits) if diff_files.collapsed_safe_files?
+    Gitlab::Metrics.add_event(:diffs_overflow_collapsed_lines_limits) if diff_files.collapsed_safe_lines?
+  end
 end

changelogs/unreleased/31063-ensure-diff-collection-limits-are-configurable.yml

+---
+title: Track the different overflows for diff collections
+merge_request: 57790
+author:
+type: other

lib/gitlab/git/diff_collection.rb

@@ -82,6 +82,30 @@ module Gitlab
         !!@overflow
       end
 
+      def overflow_max_lines?
+        !!@overflow_max_lines
+      end
+
+      def overflow_max_bytes?
+        !!@overflow_max_bytes
+      end
+
+      def overflow_max_files?
+        !!@overflow_max_files
+      end
+
+      def collapsed_safe_lines?
+        !!@collapsed_safe_lines
+      end
+
+      def collapsed_safe_files?
+        !!@collapsed_safe_files
+      end
+
+      def collapsed_safe_bytes?
+        !!@collapsed_safe_bytes
+      end
+
       def size
         @size ||= count # forces a loop using each method
       end
@@ -121,7 +145,15 @@ module Gitlab
       end
 
       def over_safe_limits?(files)
-        files >= safe_max_files || @line_count > safe_max_lines || @byte_count >= safe_max_bytes
+        if files >= safe_max_files
+          @collapsed_safe_files = true
+        elsif @line_count > safe_max_lines
+          @collapsed_safe_lines = true
+        elsif @byte_count >= safe_max_bytes
+          @collapsed_safe_bytes = true
+        end
+
+        @collapsed_safe_files || @collapsed_safe_lines || @collapsed_safe_bytes
       end
 
       def expand_diff?
@@ -154,6 +186,7 @@ module Gitlab
 
           if @enforce_limits && i >= max_files
             @overflow = true
+            @overflow_max_files = true
             break
           end
 
@@ -166,10 +199,19 @@ module Gitlab
           @line_count += diff.line_count
           @byte_count += diff.diff.bytesize
 
-          if @enforce_limits && (@line_count >= max_lines || @byte_count >= max_bytes)
+          if @enforce_limits && @line_count >= max_lines
+            # This last Diff instance pushes us over the lines limit. We stop and
+            # discard it.
+            @overflow = true
+            @overflow_max_lines = true
+            break
+          end
+
+          if @enforce_limits && @byte_count >= max_bytes
             # This last Diff instance pushes us over the lines limit. We stop and
             # discard it.
             @overflow = true
+            @overflow_max_bytes = true
             break
           end
 

spec/helpers/diff_helper_spec.rb

@@ -291,6 +291,8 @@ RSpec.describe DiffHelper do
   end
 
   describe '#render_overflow_warning?' do
+    using RSpec::Parameterized::TableSyntax
+
     let(:diffs_collection) { instance_double(Gitlab::Diff::FileCollection::MergeRequestDiff, raw_diff_files: diff_files) }
     let(:diff_files) { Gitlab::Git::DiffCollection.new(files) }
     let(:safe_file) { { too_large: false, diff: '' } }
@@ -299,13 +301,42 @@ RSpec.describe DiffHelper do
 
     before do
       allow(diff_files).to receive(:overflow?).and_return(false)
+      allow(diff_files).to receive(:overflow_max_bytes?).and_return(false)
+      allow(diff_files).to receive(:overflow_max_files?).and_return(false)
+      allow(diff_files).to receive(:overflow_max_lines?).and_return(false)
+      allow(diff_files).to receive(:collapsed_safe_bytes?).and_return(false)
+      allow(diff_files).to receive(:collapsed_safe_files?).and_return(false)
+      allow(diff_files).to receive(:collapsed_safe_lines?).and_return(false)
     end
 
-    context 'when neither collection nor individual file hit the limit' do
+    context 'when no limits are hit' do
       it 'returns false and does not log any overflow events' do
         expect(Gitlab::Metrics).not_to receive(:add_event).with(:diffs_overflow_collection_limits)
         expect(Gitlab::Metrics).not_to receive(:add_event).with(:diffs_overflow_single_file_limits)
+        expect(Gitlab::Metrics).not_to receive(:add_event).with(:diffs_overflow_max_bytes_limits)
+        expect(Gitlab::Metrics).not_to receive(:add_event).with(:diffs_overflow_max_files_limits)
+        expect(Gitlab::Metrics).not_to receive(:add_event).with(:diffs_overflow_max_lines_limits)
+        expect(Gitlab::Metrics).not_to receive(:add_event).with(:diffs_overflow_collapsed_bytes_limits)
+        expect(Gitlab::Metrics).not_to receive(:add_event).with(:diffs_overflow_collapsed_files_limits)
+        expect(Gitlab::Metrics).not_to receive(:add_event).with(:diffs_overflow_collapsed_lines_limits)
+
+        expect(render_overflow_warning?(diffs_collection)).to be false
+      end
+    end
+
+    where(:overflow_method, :event_name) do
+      :overflow_max_bytes?      | :diffs_overflow_max_bytes_limits
+      :overflow_max_files?      | :diffs_overflow_max_files_limits
+      :overflow_max_lines?      | :diffs_overflow_max_lines_limits
+      :collapsed_safe_bytes?    | :diffs_overflow_collapsed_bytes_limits
+      :collapsed_safe_files?    | :diffs_overflow_collapsed_files_limits
+      :collapsed_safe_lines?    | :diffs_overflow_collapsed_lines_limits
+    end
 
+    with_them do
+      it 'returns false and only logs the correct collection overflow event' do
+        allow(diff_files).to receive(overflow_method).and_return(true)
+        expect(Gitlab::Metrics).to receive(:add_event).with(event_name).once
         expect(render_overflow_warning?(diffs_collection)).to be false
       end
     end
@@ -315,9 +346,8 @@ RSpec.describe DiffHelper do
         allow(diff_files).to receive(:overflow?).and_return(true)
       end
 
-      it 'returns false and only logs collection overflow event' do
-        expect(Gitlab::Metrics).to receive(:add_event).with(:diffs_overflow_collection_limits).exactly(:once)
-        expect(Gitlab::Metrics).not_to receive(:add_event).with(:diffs_overflow_single_file_limits)
+      it 'returns true and only logs all the correct collection overflow event' do
+        expect(Gitlab::Metrics).to receive(:add_event).with(:diffs_overflow_collection_limits).once
 
         expect(render_overflow_warning?(diffs_collection)).to be true
       end

spec/lib/gitlab/git/diff_collection_spec.rb

@@ -31,6 +31,19 @@ RSpec.describe Gitlab::Git::DiffCollection, :seed_helper do
     end
   end
 
+  let(:overflow_max_bytes) { false }
+  let(:overflow_max_files) { false }
+  let(:overflow_max_lines) { false }
+
+  shared_examples 'overflow stuff' do
+    it 'returns the expected overflow values' do
+      subject.overflow?
+      expect(subject.overflow_max_bytes?).to eq(overflow_max_bytes)
+      expect(subject.overflow_max_files?).to eq(overflow_max_files)
+      expect(subject.overflow_max_lines?).to eq(overflow_max_lines)
+    end
+  end
+
   subject do
     Gitlab::Git::DiffCollection.new(
       iterator,
@@ -76,12 +89,19 @@ RSpec.describe Gitlab::Git::DiffCollection, :seed_helper do
   end
 
   context 'overflow handling' do
+    subject { super() }
+
+    let(:collapsed_safe_files) { false }
+    let(:collapsed_safe_lines) { false }
+
     context 'adding few enough files' do
       let(:file_count) { 3 }
 
       context 'and few enough lines' do
         let(:line_count) { 10 }
 
+        it_behaves_like 'overflow stuff'
+
         describe '#overflow?' do
           subject { super().overflow? }
 
@@ -117,6 +137,11 @@ RSpec.describe Gitlab::Git::DiffCollection, :seed_helper do
 
         context 'when limiting is disabled' do
           let(:limits) { false }
+          let(:overflow_max_bytes) { false }
+          let(:overflow_max_files) { false }
+          let(:overflow_max_lines) { false }
+
+          it_behaves_like 'overflow stuff'
 
           describe '#overflow?' do
             subject { super().overflow? }
@@ -155,6 +180,9 @@ RSpec.describe Gitlab::Git::DiffCollection, :seed_helper do
 
       context 'and too many lines' do
         let(:line_count) { 1000 }
+        let(:overflow_max_lines) { true }
+
+        it_behaves_like 'overflow stuff'
 
         describe '#overflow?' do
           subject { super().overflow? }
@@ -184,6 +212,11 @@ RSpec.describe Gitlab::Git::DiffCollection, :seed_helper do
 
         context 'when limiting is disabled' do
           let(:limits) { false }
+          let(:overflow_max_bytes) { false }
+          let(:overflow_max_files) { false }
+          let(:overflow_max_lines) { false }
+
+          it_behaves_like 'overflow stuff'
 
           describe '#overflow?' do
             subject { super().overflow? }
@@ -216,10 +249,13 @@ RSpec.describe Gitlab::Git::DiffCollection, :seed_helper do
 
     context 'adding too many files' do
       let(:file_count) { 11 }
+      let(:overflow_max_files) { true }
 
       context 'and few enough lines' do
         let(:line_count) { 1 }
 
+        it_behaves_like 'overflow stuff'
+
         describe '#overflow?' do
           subject { super().overflow? }
 
@@ -248,6 +284,11 @@ RSpec.describe Gitlab::Git::DiffCollection, :seed_helper do
 
         context 'when limiting is disabled' do
           let(:limits) { false }
+          let(:overflow_max_bytes) { false }
+          let(:overflow_max_files) { false }
+          let(:overflow_max_lines) { false }
+
+          it_behaves_like 'overflow stuff'
 
           describe '#overflow?' do
             subject { super().overflow? }
@@ -279,6 +320,10 @@ RSpec.describe Gitlab::Git::DiffCollection, :seed_helper do
 
       context 'and too many lines' do
         let(:line_count) { 30 }
+        let(:overflow_max_lines) { true }
+        let(:overflow_max_files) { false }
+
+        it_behaves_like 'overflow stuff'
 
         describe '#overflow?' do
           subject { super().overflow? }
@@ -308,6 +353,11 @@ RSpec.describe Gitlab::Git::DiffCollection, :seed_helper do
 
         context 'when limiting is disabled' do
           let(:limits) { false }
+          let(:overflow_max_bytes) { false }
+          let(:overflow_max_files) { false }
+          let(:overflow_max_lines) { false }
+
+          it_behaves_like 'overflow stuff'
 
           describe '#overflow?' do
             subject { super().overflow? }
@@ -344,6 +394,8 @@ RSpec.describe Gitlab::Git::DiffCollection, :seed_helper do
       context 'and few enough lines' do
         let(:line_count) { 1 }
 
+        it_behaves_like 'overflow stuff'
+
         describe '#overflow?' do
           subject { super().overflow? }
 
@@ -375,6 +427,9 @@ RSpec.describe Gitlab::Git::DiffCollection, :seed_helper do
     context 'adding too many bytes' do
       let(:file_count) { 10 }
       let(:line_length) { 5200 }
+      let(:overflow_max_bytes) { true }
+
+      it_behaves_like 'overflow stuff'
 
       describe '#overflow?' do
         subject { super().overflow? }
@@ -404,6 +459,11 @@ RSpec.describe Gitlab::Git::DiffCollection, :seed_helper do
 
       context 'when limiting is disabled' do
         let(:limits) { false }
+        let(:overflow_max_bytes) { false }
+        let(:overflow_max_files) { false }
+        let(:overflow_max_lines) { false }
+
+        it_behaves_like 'overflow stuff'
 
         describe '#overflow?' do
           subject { super().overflow? }
@@ -437,6 +497,8 @@ RSpec.describe Gitlab::Git::DiffCollection, :seed_helper do
   describe 'empty collection' do
     subject { Gitlab::Git::DiffCollection.new([]) }
 
+    it_behaves_like 'overflow stuff'
+
     describe '#overflow?' do
       subject { super().overflow? }
 
@@ -555,7 +617,7 @@ RSpec.describe Gitlab::Git::DiffCollection, :seed_helper do
               .and_return({ max_files: 2, max_lines: max_lines })
           end
 
-          it 'prunes diffs by default even little ones' do
+          it 'prunes diffs by default even little ones and sets collapsed_safe_files true' do
             subject.each_with_index do |d, i|
               if i < 2
                 expect(d.diff).not_to eq('')
@@ -563,6 +625,8 @@ RSpec.describe Gitlab::Git::DiffCollection, :seed_helper do
                 expect(d.diff).to eq('')
               end
             end
+
+            expect(subject.collapsed_safe_files?).to eq(true)
           end
         end
 
@@ -582,7 +646,7 @@ RSpec.describe Gitlab::Git::DiffCollection, :seed_helper do
               .and_return({ max_files: max_files, max_lines: 80 })
           end
 
-          it 'prunes diffs by default even little ones' do
+          it 'prunes diffs by default even little ones and sets collapsed_safe_lines true' do
             subject.each_with_index do |d, i|
               if i < 2
                 expect(d.diff).not_to eq('')
@@ -590,26 +654,30 @@ RSpec.describe Gitlab::Git::DiffCollection, :seed_helper do
                 expect(d.diff).to eq('')
               end
             end
+
+            expect(subject.collapsed_safe_lines?).to eq(true)
           end
         end
 
         context 'when go over safe limits on bytes' do
           let(:iterator) do
             [
-              fake_diff(1, 45),
-              fake_diff(1, 45),
-              fake_diff(1, 20480),
-              fake_diff(1, 1)
+              fake_diff(5, 10),
+              fake_diff(5000, 10),
+              fake_diff(5, 10),
+              fake_diff(5, 10)
             ]
           end
 
           before do
+            allow(Gitlab::CurrentSettings).to receive(:diff_max_patch_bytes).and_return(1.megabyte)
+
             allow(Gitlab::Git::DiffCollection)
               .to receive(:default_limits)
-              .and_return({ max_files: max_files, max_lines: 80 })
+              .and_return({ max_files: 4, max_lines: 3000 })
           end
 
-          it 'prunes diffs by default even little ones' do
+          it 'prunes diffs by default even little ones and sets collapsed_safe_bytes true' do
             subject.each_with_index do |d, i|
               if i < 2
                 expect(d.diff).not_to eq('')
@@ -617,6 +685,8 @@ RSpec.describe Gitlab::Git::DiffCollection, :seed_helper do
                 expect(d.diff).to eq('')
               end
             end
+
+            expect(subject.collapsed_safe_bytes?).to eq(true)
           end
         end
       end