Merge branch '337060-raise-statement-invalid-for-hardening-methods' into 'master'

Add track_and_raise_for_dev_exception to usage_data methods

See merge request gitlab-org/gitlab!75182

ee/spec/lib/ee/gitlab/usage_data_spec.rb

@@ -711,43 +711,62 @@ RSpec.describe Gitlab::UsageData do
       )
     end
 
-    it 'has to resort to 0 for counting license scan' do
-      for_defined_days_back do
-        create(:security_scan)
+    context 'when count fails' do
+      subject { described_class.usage_activity_by_stage_secure(described_class.monthly_time_range_db_params) }
+
+      before do
+        allow(Gitlab::Database::BatchCount).to receive(:batch_distinct_count).and_raise(ActiveRecord::StatementInvalid)
+        allow(Gitlab::Database::BatchCount).to receive(:batch_count).and_raise(ActiveRecord::StatementInvalid)
+        allow(Gitlab::Database::PostgresHll::BatchDistinctCounter).to receive(:new).and_raise(ActiveRecord::StatementInvalid)
+        allow(::Ci::Build).to receive(:distinct_count_by).and_raise(ActiveRecord::StatementInvalid)
+        allow(Gitlab::ErrorTracking).to receive(:should_raise_for_dev?).and_return(should_raise_for_dev)
       end
 
-      allow(Gitlab::Database::BatchCount).to receive(:batch_distinct_count).and_raise(ActiveRecord::StatementInvalid)
-      allow(Gitlab::Database::BatchCount).to receive(:batch_count).and_raise(ActiveRecord::StatementInvalid)
-      allow(Gitlab::Database::PostgresHll::BatchDistinctCounter).to receive(:new).and_raise(ActiveRecord::StatementInvalid)
-      allow(::Ci::Build).to receive(:distinct_count_by).and_raise(ActiveRecord::StatementInvalid)
+      context 'with should_raise_for_dev? true' do
+        let(:should_raise_for_dev) { true }
 
-      expect(described_class.usage_activity_by_stage_secure(described_class.monthly_time_range_db_params)).to include(
-        user_preferences_group_overview_security_dashboard: -1,
-        user_api_fuzzing_jobs: -1,
-        user_api_fuzzing_dnd_jobs: -1,
-        user_container_scanning_jobs: -1,
-        user_coverage_fuzzing_jobs: -1,
-        user_dast_jobs: -1,
-        user_dependency_scanning_jobs: -1,
-        user_license_management_jobs: -1,
-        user_sast_jobs: -1,
-        user_secret_detection_jobs: -1,
-        sast_pipeline: -1,
-        sast_scans: -1,
-        dependency_scanning_pipeline: -1,
-        dependency_scanning_scans: -1,
-        container_scanning_pipeline: -1,
-        container_scanning_scans: -1,
-        dast_pipeline: -1,
-        dast_scans: -1,
-        secret_detection_pipeline: -1,
-        secret_detection_scans: -1,
-        coverage_fuzzing_pipeline: -1,
-        coverage_fuzzing_scans: -1,
-        api_fuzzing_pipeline: -1,
-        api_fuzzing_scans: -1,
-        user_unique_users_all_secure_scanners: -1
-      )
+        it 'raises an error' do
+          expect { subject }.to raise_error(ActiveRecord::StatementInvalid)
+        end
+      end
+
+      context 'with should_raise_for_dev? false' do
+        let(:should_raise_for_dev) { false }
+
+        it 'has to resort to 0 for counting license scan' do
+          for_defined_days_back do
+            create(:security_scan)
+          end
+
+          expect(subject).to include(
+            user_preferences_group_overview_security_dashboard: -1,
+            user_api_fuzzing_jobs: -1,
+            user_api_fuzzing_dnd_jobs: -1,
+            user_container_scanning_jobs: -1,
+            user_coverage_fuzzing_jobs: -1,
+            user_dast_jobs: -1,
+            user_dependency_scanning_jobs: -1,
+            user_license_management_jobs: -1,
+            user_sast_jobs: -1,
+            user_secret_detection_jobs: -1,
+            sast_pipeline: -1,
+            sast_scans: -1,
+            dependency_scanning_pipeline: -1,
+            dependency_scanning_scans: -1,
+            container_scanning_pipeline: -1,
+            container_scanning_scans: -1,
+            dast_pipeline: -1,
+            dast_scans: -1,
+            secret_detection_pipeline: -1,
+            secret_detection_scans: -1,
+            coverage_fuzzing_pipeline: -1,
+            coverage_fuzzing_scans: -1,
+            api_fuzzing_pipeline: -1,
+            api_fuzzing_scans: -1,
+            user_unique_users_all_secure_scanners: -1
+          )
+        end
+      end
     end
 
     it 'deprecates count for users who have run scans' do

lib/gitlab/usage_data.rb

@@ -406,7 +406,8 @@ module Gitlab
         results[:projects_jira_cloud_active] = jira_integration_data_hash[:projects_jira_cloud_active]
 
         results
-      rescue ActiveRecord::StatementInvalid
+      rescue ActiveRecord::StatementInvalid => error
+        Gitlab::ErrorTracking.track_and_raise_for_dev_exception(error)
         { projects_jira_server_active: FALLBACK, projects_jira_cloud_active: FALLBACK }
       end
       # rubocop: enable CodeReuse/ActiveRecord

lib/gitlab/utils/usage_data.rb

@@ -56,7 +56,8 @@ module Gitlab
         else
           relation.count
         end
-      rescue ActiveRecord::StatementInvalid
+      rescue ActiveRecord::StatementInvalid => error
+        Gitlab::ErrorTracking.track_and_raise_for_dev_exception(error)
         FALLBACK
       end
 
@@ -66,7 +67,8 @@ module Gitlab
         else
           relation.distinct_count_by(column)
         end
-      rescue ActiveRecord::StatementInvalid
+      rescue ActiveRecord::StatementInvalid => error
+        Gitlab::ErrorTracking.track_and_raise_for_dev_exception(error)
         FALLBACK
       end
 
@@ -78,7 +80,8 @@ module Gitlab
         yield buckets if block_given?
 
         buckets.estimated_distinct_count
-      rescue ActiveRecord::StatementInvalid
+      rescue ActiveRecord::StatementInvalid => error
+        Gitlab::ErrorTracking.track_and_raise_for_dev_exception(error)
         FALLBACK
       # catch all rescue should be removed as a part of feature flag rollout issue
       # https://gitlab.com/gitlab-org/gitlab/-/issues/285485
@@ -89,7 +92,8 @@ module Gitlab
 
       def sum(relation, column, batch_size: nil, start: nil, finish: nil)
         Gitlab::Database::BatchCount.batch_sum(relation, column, batch_size: batch_size, start: start, finish: finish)
-      rescue ActiveRecord::StatementInvalid
+      rescue ActiveRecord::StatementInvalid => error
+        Gitlab::ErrorTracking.track_and_raise_for_dev_exception(error)
         FALLBACK
       end
 
@@ -155,7 +159,8 @@ module Gitlab
           query: query.to_sql,
           message: e.message
         )
-
+        # Raises error for dev env
+        Gitlab::ErrorTracking.track_and_raise_for_dev_exception(e)
         HISTOGRAM_FALLBACK
       end
       # rubocop: enable CodeReuse/ActiveRecord

spec/lib/gitlab/usage_data_spec.rb

@@ -684,23 +684,50 @@ RSpec.describe Gitlab::UsageData, :aggregate_failures do
       end
     end
 
-    it 'works when queries time out' do
-      allow_any_instance_of(ActiveRecord::Relation)
-        .to receive(:count).and_raise(ActiveRecord::StatementInvalid.new(''))
+    context 'when queries time out' do
+      let(:metric_method) { :count }
+
+      before do
+        allow_any_instance_of(ActiveRecord::Relation).to receive(metric_method).and_raise(ActiveRecord::StatementInvalid)
+        allow(Gitlab::ErrorTracking).to receive(:should_raise_for_dev?).and_return(should_raise_for_dev)
+      end
+
+      context 'with should_raise_for_dev? true' do
+        let(:should_raise_for_dev) { true }
+
+        it 'raises an error' do
+          expect { subject }.to raise_error(ActiveRecord::StatementInvalid)
+        end
+
+        context 'when metric calls find_in_batches' do
+          let(:metric_method) { :find_in_batches }
+
+          it 'raises an error for jira_usage' do
+            expect { described_class.jira_usage }.to raise_error(ActiveRecord::StatementInvalid)
+          end
+        end
+      end
+
+      context 'with should_raise_for_dev? false' do
+        let(:should_raise_for_dev) { false }
+
+        it 'does not raise an error' do
+          expect { subject }.not_to raise_error
+        end
 
-      expect { subject }.not_to raise_error
+        context 'when metric calls find_in_batches' do
+          let(:metric_method) { :find_in_batches }
+
+          it 'does not raise an error for jira_usage' do
+            expect { described_class.jira_usage }.not_to raise_error
+          end
+        end
+      end
     end
 
     it 'includes a recording_ce_finished_at timestamp' do
       expect(subject[:recording_ce_finished_at]).to be_a(Time)
     end
-
-    it 'jira usage works when queries time out' do
-      allow_any_instance_of(ActiveRecord::Relation)
-        .to receive(:find_in_batches).and_raise(ActiveRecord::StatementInvalid.new(''))
-
-      expect { described_class.jira_usage }.not_to raise_error
-    end
   end
 
   describe '.system_usage_data_monthly' do
@@ -1355,46 +1382,58 @@ RSpec.describe Gitlab::UsageData, :aggregate_failures do
 
     context 'when queries time out' do
       before do
-        allow_any_instance_of(ActiveRecord::Relation)
-          .to receive(:count).and_raise(ActiveRecord::StatementInvalid.new(''))
+        allow_any_instance_of(ActiveRecord::Relation).to receive(:count).and_raise(ActiveRecord::StatementInvalid)
+        allow(Gitlab::ErrorTracking).to receive(:should_raise_for_dev?).and_return(should_raise_for_dev)
       end
 
-      it 'returns -1 for email campaign data' do
-        expected_data = {
-          "in_product_marketing_email_create_0_sent" => -1,
-          "in_product_marketing_email_create_0_cta_clicked" => -1,
-          "in_product_marketing_email_create_1_sent" => -1,
-          "in_product_marketing_email_create_1_cta_clicked" => -1,
-          "in_product_marketing_email_create_2_sent" => -1,
-          "in_product_marketing_email_create_2_cta_clicked" => -1,
-          "in_product_marketing_email_team_short_0_sent" => -1,
-          "in_product_marketing_email_team_short_0_cta_clicked" => -1,
-          "in_product_marketing_email_trial_short_0_sent" => -1,
-          "in_product_marketing_email_trial_short_0_cta_clicked" => -1,
-          "in_product_marketing_email_admin_verify_0_sent" => -1,
-          "in_product_marketing_email_admin_verify_0_cta_clicked" => -1,
-          "in_product_marketing_email_verify_0_sent" => -1,
-          "in_product_marketing_email_verify_0_cta_clicked" => -1,
-          "in_product_marketing_email_verify_1_sent" => -1,
-          "in_product_marketing_email_verify_1_cta_clicked" => -1,
-          "in_product_marketing_email_verify_2_sent" => -1,
-          "in_product_marketing_email_verify_2_cta_clicked" => -1,
-          "in_product_marketing_email_trial_0_sent" => -1,
-          "in_product_marketing_email_trial_0_cta_clicked" => -1,
-          "in_product_marketing_email_trial_1_sent" => -1,
-          "in_product_marketing_email_trial_1_cta_clicked" => -1,
-          "in_product_marketing_email_trial_2_sent" => -1,
-          "in_product_marketing_email_trial_2_cta_clicked" => -1,
-          "in_product_marketing_email_team_0_sent" => -1,
-          "in_product_marketing_email_team_0_cta_clicked" => -1,
-          "in_product_marketing_email_team_1_sent" => -1,
-          "in_product_marketing_email_team_1_cta_clicked" => -1,
-          "in_product_marketing_email_team_2_sent" => -1,
-          "in_product_marketing_email_team_2_cta_clicked" => -1,
-          "in_product_marketing_email_experience_0_sent" => -1
-        }
+      context 'with should_raise_for_dev? true' do
+        let(:should_raise_for_dev) { true }
 
-        expect(subject).to eq(expected_data)
+        it 'raises an error' do
+          expect { subject }.to raise_error(ActiveRecord::StatementInvalid)
+        end
+      end
+
+      context 'with should_raise_for_dev? false' do
+        let(:should_raise_for_dev) { false }
+
+        it 'returns -1 for email campaign data' do
+          expected_data = {
+            "in_product_marketing_email_create_0_sent" => -1,
+            "in_product_marketing_email_create_0_cta_clicked" => -1,
+            "in_product_marketing_email_create_1_sent" => -1,
+            "in_product_marketing_email_create_1_cta_clicked" => -1,
+            "in_product_marketing_email_create_2_sent" => -1,
+            "in_product_marketing_email_create_2_cta_clicked" => -1,
+            "in_product_marketing_email_team_short_0_sent" => -1,
+            "in_product_marketing_email_team_short_0_cta_clicked" => -1,
+            "in_product_marketing_email_trial_short_0_sent" => -1,
+            "in_product_marketing_email_trial_short_0_cta_clicked" => -1,
+            "in_product_marketing_email_admin_verify_0_sent" => -1,
+            "in_product_marketing_email_admin_verify_0_cta_clicked" => -1,
+            "in_product_marketing_email_verify_0_sent" => -1,
+            "in_product_marketing_email_verify_0_cta_clicked" => -1,
+            "in_product_marketing_email_verify_1_sent" => -1,
+            "in_product_marketing_email_verify_1_cta_clicked" => -1,
+            "in_product_marketing_email_verify_2_sent" => -1,
+            "in_product_marketing_email_verify_2_cta_clicked" => -1,
+            "in_product_marketing_email_trial_0_sent" => -1,
+            "in_product_marketing_email_trial_0_cta_clicked" => -1,
+            "in_product_marketing_email_trial_1_sent" => -1,
+            "in_product_marketing_email_trial_1_cta_clicked" => -1,
+            "in_product_marketing_email_trial_2_sent" => -1,
+            "in_product_marketing_email_trial_2_cta_clicked" => -1,
+            "in_product_marketing_email_team_0_sent" => -1,
+            "in_product_marketing_email_team_0_cta_clicked" => -1,
+            "in_product_marketing_email_team_1_sent" => -1,
+            "in_product_marketing_email_team_1_cta_clicked" => -1,
+            "in_product_marketing_email_team_2_sent" => -1,
+            "in_product_marketing_email_team_2_cta_clicked" => -1,
+            "in_product_marketing_email_experience_0_sent" => -1
+          }
+
+          expect(subject).to eq(expected_data)
+        end
       end
     end
 

spec/lib/gitlab/utils/usage_data_spec.rb

@@ -5,6 +5,30 @@ require 'spec_helper'
 RSpec.describe Gitlab::Utils::UsageData do
   include Database::DatabaseHelpers
 
+  shared_examples 'failing hardening method' do
+    before do
+      allow(Gitlab::ErrorTracking).to receive(:should_raise_for_dev?).and_return(should_raise_for_dev)
+      stub_const("Gitlab::Utils::UsageData::FALLBACK", fallback)
+      allow(failing_class).to receive(failing_method).and_raise(ActiveRecord::StatementInvalid)
+    end
+
+    context 'with should_raise_for_dev? false' do
+      let(:should_raise_for_dev) { false }
+
+      it 'returns the fallback' do
+        expect(subject).to eq(fallback)
+      end
+    end
+
+    context 'with should_raise_for_dev? true' do
+      let(:should_raise_for_dev) { true }
+
+      it 'raises an error' do
+        expect { subject }.to raise_error(ActiveRecord::StatementInvalid)
+      end
+    end
+  end
+
   describe '#add_metric' do
     let(:metric) { 'UuidMetric'}
 
@@ -22,11 +46,14 @@ RSpec.describe Gitlab::Utils::UsageData do
       expect(described_class.count(relation, batch: false)).to eq(1)
     end
 
-    it 'returns the fallback value when counting fails' do
-      stub_const("Gitlab::Utils::UsageData::FALLBACK", 15)
-      allow(relation).to receive(:count).and_raise(ActiveRecord::StatementInvalid.new(''))
+    context 'when counting fails' do
+      subject { described_class.count(relation, batch: false) }
 
-      expect(described_class.count(relation, batch: false)).to eq(15)
+      let(:fallback) { 15 }
+      let(:failing_class) { relation }
+      let(:failing_method) { :count }
+
+      it_behaves_like 'failing hardening method'
     end
   end
 
@@ -39,11 +66,14 @@ RSpec.describe Gitlab::Utils::UsageData do
       expect(described_class.distinct_count(relation, batch: false)).to eq(1)
     end
 
-    it 'returns the fallback value when counting fails' do
-      stub_const("Gitlab::Utils::UsageData::FALLBACK", 15)
-      allow(relation).to receive(:distinct_count_by).and_raise(ActiveRecord::StatementInvalid.new(''))
+    context 'when counting fails' do
+      subject { described_class.distinct_count(relation, batch: false) }
+
+      let(:fallback) { 15 }
+      let(:failing_class) { relation }
+      let(:failing_method) { :distinct_count_by }
 
-      expect(described_class.distinct_count(relation, batch: false)).to eq(15)
+      it_behaves_like 'failing hardening method'
     end
   end
 
@@ -155,14 +185,24 @@ RSpec.describe Gitlab::Utils::UsageData do
         stub_const("Gitlab::Utils::UsageData::DISTRIBUTED_HLL_FALLBACK", 4)
       end
 
-      it 'returns fallback if counter raises WRONG_CONFIGURATION_ERROR' do
-        expect(described_class.estimate_batch_distinct_count(relation, 'id', start: 1, finish: 0)).to eq 3
+      context 'when counter raises WRONG_CONFIGURATION_ERROR' do
+        subject { described_class.estimate_batch_distinct_count(relation, 'id', start: 1, finish: 0) }
+
+        let(:fallback) { 3 }
+        let(:failing_class) { Gitlab::Database::PostgresHll::BatchDistinctCounter }
+        let(:failing_method) { :new }
+
+        it_behaves_like 'failing hardening method'
       end
 
-      it 'returns default fallback value when counting fails due to database error' do
-        allow(Gitlab::Database::PostgresHll::BatchDistinctCounter).to receive(:new).and_raise(ActiveRecord::StatementInvalid.new(''))
+      context 'when counting fails due to database error' do
+        subject { described_class.estimate_batch_distinct_count(relation) }
 
-        expect(described_class.estimate_batch_distinct_count(relation)).to eq(3)
+        let(:fallback) { 3 }
+        let(:failing_class) { Gitlab::Database::PostgresHll::BatchDistinctCounter }
+        let(:failing_method) { :new }
+
+        it_behaves_like 'failing hardening method'
       end
 
       it 'logs error and returns DISTRIBUTED_HLL_FALLBACK value when counting raises any error', :aggregate_failures do
@@ -187,13 +227,14 @@ RSpec.describe Gitlab::Utils::UsageData do
       expect(described_class.sum(relation, :column, batch_size: 100, start: 2, finish: 3)).to eq(1)
     end
 
-    it 'returns the fallback value when counting fails' do
-      stub_const("Gitlab::Utils::UsageData::FALLBACK", 15)
-      allow(Gitlab::Database::BatchCount)
-        .to receive(:batch_sum)
-        .and_raise(ActiveRecord::StatementInvalid.new(''))
+    context 'when counting fails' do
+      subject { described_class.sum(relation, :column) }
+
+      let(:fallback) { 15 }
+      let(:failing_class) { Gitlab::Database::BatchCount }
+      let(:failing_method) { :batch_sum }
 
-      expect(described_class.sum(relation, :column)).to eq(15)
+      it_behaves_like 'failing hardening method'
     end
   end
 
@@ -273,23 +314,45 @@ RSpec.describe Gitlab::Utils::UsageData do
       expect(histogram).to eq('2' => 1)
     end
 
-    it 'returns fallback and logs canceled queries' do
-      create(:alert_management_http_integration, :active, project: project1)
+    context 'when query timeout' do
+      subject do
+        with_statement_timeout(0.001) do
+          relation = AlertManagement::HttpIntegration.select('pg_sleep(0.002)')
+          described_class.histogram(relation, column, buckets: 1..100)
+        end
+      end
 
-      expect(Gitlab::AppJsonLogger).to receive(:error).with(
-        event: 'histogram',
-        relation: relation.table_name,
-        operation: 'histogram',
-        operation_args: [column, 1, 100, 99],
-        query: kind_of(String),
-        message: /PG::QueryCanceled/
-      )
+      before do
+        allow(Gitlab::ErrorTracking).to receive(:should_raise_for_dev?).and_return(should_raise_for_dev)
+        create(:alert_management_http_integration, :active, project: project1)
+      end
+
+      context 'with should_raise_for_dev? false' do
+        let(:should_raise_for_dev) { false }
+
+        it 'logs canceled queries' do
+          expect(Gitlab::AppJsonLogger).to receive(:error).with(
+            event: 'histogram',
+            relation: relation.table_name,
+            operation: 'histogram',
+            operation_args: [column, 1, 100, 99],
+            query: kind_of(String),
+            message: /PG::QueryCanceled/
+          )
+          subject
+        end
 
-      with_statement_timeout(0.001) do
-        relation = AlertManagement::HttpIntegration.select('pg_sleep(0.002)')
-        histogram = described_class.histogram(relation, column, buckets: 1..100)
+        it 'returns fallback' do
+          expect(subject).to eq(fallback)
+        end
+      end
 
-        expect(histogram).to eq(fallback)
+      context 'with should_raise_for_dev? true' do
+        let(:should_raise_for_dev) { true }
+
+        it 'raises error' do
+          expect { subject }.to raise_error(ActiveRecord::QueryCanceled)
+        end
       end
     end
   end