Merge branch 'revert-24e6ba7f' into 'master'

Revert "Merge branch '283807-failure-reason-dashboard' into 'master'"

See merge request gitlab-org/gitlab!58645

app/models/ci/pipeline.rb

@@ -286,11 +286,9 @@ module Ci
       end
 
       after_transition any => [:failed] do |pipeline|
-        pipeline.run_after_commit do
-          ::Gitlab::Ci::Pipeline::Metrics.pipeline_failure_reason_counter.increment(reason: pipeline.failure_reason)
+        next unless pipeline.auto_devops_source?
 
-          AutoDevops::DisableWorker.perform_async(pipeline.id) if pipeline.auto_devops_source?
-        end
+        pipeline.run_after_commit { AutoDevops::DisableWorker.perform_async(pipeline.id) }
       end
     end
 

app/models/commit_status.rb

@@ -181,16 +181,15 @@ class CommitStatus < ApplicationRecord
     end
 
     after_transition any => :failed do |commit_status|
-      commit_status.run_after_commit do
-        ::Gitlab::Ci::Pipeline::Metrics.job_failure_reason_counter.increment(reason: commit_status.failure_reason)
-
-        # rubocop: disable CodeReuse/ServiceClass
       next if Feature.enabled?(:async_add_build_failure_todo, commit_status.project, default_enabled: :yaml)
       next unless commit_status.project
 
-        MergeRequests::AddTodoWhenBuildFailsService.new(project, nil).execute(self)
-        # rubocop: enable CodeReuse/ServiceClass
+      # rubocop: disable CodeReuse/ServiceClass
+      commit_status.run_after_commit do
+        MergeRequests::AddTodoWhenBuildFailsService
+          .new(project, nil).execute(self)
       end
+      # rubocop: enable CodeReuse/ServiceClass
     end
   end
 

app/services/ci/process_pipeline_service.rb

@@ -19,7 +19,7 @@ module Ci
     end
 
     def metrics
-      @metrics ||= ::Gitlab::Ci::Pipeline::Metrics
+      @metrics ||= ::Gitlab::Ci::Pipeline::Metrics.new
     end
 
     private

ee/spec/lib/gitlab/ci/pipeline/chain/limit/size_spec.rb

@@ -83,8 +83,7 @@ RSpec.describe ::Gitlab::Ci::Pipeline::Chain::Limit::Size do
           project: project,
           current_user: user,
           save_incompleted: false,
-          pipeline_seed: double(:seed, size: 2),
-          increment_pipeline_failure_reason_counter: true)
+          pipeline_seed: double(:seed, size: 2))
       end
 
       it 'does not drop the pipeline' do
@@ -98,12 +97,6 @@ RSpec.describe ::Gitlab::Ci::Pipeline::Chain::Limit::Size do
 
         expect(step.break?).to be true
       end
-
-      it 'increments the error metric' do
-        expect(command).to receive(:increment_pipeline_failure_reason_counter).with(:size_limit_exceeded)
-
-        subject
-      end
     end
   end
 

lib/gitlab/ci/pipeline/chain/command.rb

@@ -84,7 +84,7 @@ module Gitlab
           end
 
           def metrics
-            @metrics ||= ::Gitlab::Ci::Pipeline::Metrics
+            @metrics ||= ::Gitlab::Ci::Pipeline::Metrics.new
           end
 
           def observe_creation_duration(duration)
@@ -97,11 +97,6 @@ module Gitlab
               .observe({ source: pipeline.source.to_s }, pipeline.total_size)
           end
 
-          def increment_pipeline_failure_reason_counter(reason)
-            metrics.pipeline_failure_reason_counter
-              .increment(reason: (reason || :unknown_failure).to_s)
-          end
-
           def dangling_build?
             %i[ondemand_dast_scan webide].include?(source)
           end

lib/gitlab/ci/pipeline/chain/helpers.rb

@@ -12,8 +12,7 @@ module Gitlab
             end
 
             pipeline.add_error_message(message)
-
-            drop_pipeline!(drop_reason)
+            pipeline.drop!(drop_reason) if drop_reason && persist_pipeline?
 
             # TODO: consider not to rely on AR errors directly as they can be
             # polluted with other unrelated errors (e.g. state machine)
@@ -25,16 +24,8 @@ module Gitlab
             pipeline.add_warning_message(message)
           end
 
-          private
-
-          def drop_pipeline!(drop_reason)
-            return if pipeline.readonly?
-
-            if drop_reason && command.save_incompleted
-              pipeline.drop!(drop_reason)
-            else
-              command.increment_pipeline_failure_reason_counter(drop_reason)
-            end
+          def persist_pipeline?
+            command.save_incompleted && !pipeline.readonly?
           end
         end
       end

lib/gitlab/ci/pipeline/chain/metrics.rb

@@ -14,7 +14,7 @@ module Gitlab
           end
 
           def counter
-            ::Gitlab::Ci::Pipeline::Metrics.pipelines_created_counter
+            ::Gitlab::Ci::Pipeline::Metrics.new.pipelines_created_counter
           end
         end
       end

lib/gitlab/ci/pipeline/metrics.rb

@@ -4,9 +4,9 @@ module Gitlab
   module Ci
     module Pipeline
       class Metrics
-        extend Gitlab::Utils::StrongMemoize
+        include Gitlab::Utils::StrongMemoize
 
-        def self.pipeline_creation_duration_histogram
+        def pipeline_creation_duration_histogram
           strong_memoize(:pipeline_creation_duration_histogram) do
             name = :gitlab_ci_pipeline_creation_duration_seconds
             comment = 'Pipeline creation duration'
@@ -17,7 +17,7 @@ module Gitlab
           end
         end
 
-        def self.pipeline_size_histogram
+        def pipeline_size_histogram
           strong_memoize(:pipeline_size_histogram) do
             name = :gitlab_ci_pipeline_size_builds
             comment = 'Pipeline size'
@@ -28,7 +28,7 @@ module Gitlab
           end
         end
 
-        def self.pipeline_processing_events_counter
+        def pipeline_processing_events_counter
           strong_memoize(:pipeline_processing_events_counter) do
             name = :gitlab_ci_pipeline_processing_events_total
             comment = 'Total amount of pipeline processing events'
@@ -37,7 +37,7 @@ module Gitlab
           end
         end
 
-        def self.pipelines_created_counter
+        def pipelines_created_counter
           strong_memoize(:pipelines_created_count) do
             name = :pipelines_created_total
             comment = 'Counter of pipelines created'
@@ -46,7 +46,7 @@ module Gitlab
           end
         end
 
-        def self.legacy_update_jobs_counter
+        def legacy_update_jobs_counter
           strong_memoize(:legacy_update_jobs_counter) do
             name = :ci_legacy_update_jobs_as_retried_total
             comment = 'Counter of occurrences when jobs were not being set as retried before update_retried'
@@ -54,24 +54,6 @@ module Gitlab
             Gitlab::Metrics.counter(name, comment)
           end
         end
-
-        def self.pipeline_failure_reason_counter
-          strong_memoize(:pipeline_failure_reason_counter) do
-            name = :gitlab_ci_pipeline_failure_reasons
-            comment = 'Counter of pipeline failure reasons'
-
-            Gitlab::Metrics.counter(name, comment)
-          end
-        end
-
-        def self.job_failure_reason_counter
-          strong_memoize(:job_failure_reason_counter) do
-            name = :gitlab_ci_job_failure_reasons
-            comment = 'Counter of job failure reasons'
-
-            Gitlab::Metrics.counter(name, comment)
-          end
-        end
       end
     end
   end

spec/lib/gitlab/ci/pipeline/chain/command_spec.rb

@@ -321,25 +321,4 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Command do
       it { is_expected.to be_falsey }
     end
   end
-
-  describe '#increment_pipeline_failure_reason_counter' do
-    let(:command) { described_class.new }
-    let(:reason) { :size_limit_exceeded }
-
-    subject { command.increment_pipeline_failure_reason_counter(reason) }
-
-    it 'increments the error metric' do
-      counter = Gitlab::Metrics.counter(:gitlab_ci_pipeline_failure_reasons, 'desc')
-      expect { subject }.to change { counter.get(reason: reason.to_s) }.by(1)
-    end
-
-    context 'when the reason is nil' do
-      let(:reason) { nil }
-
-      it 'increments the error metric with unknown_failure' do
-        counter = Gitlab::Metrics.counter(:gitlab_ci_pipeline_failure_reasons, 'desc')
-        expect { subject }.to change { counter.get(reason: 'unknown_failure') }.by(1)
-      end
-    end
-  end
 end

spec/lib/gitlab/ci/pipeline/chain/limit/deployments_spec.rb

@@ -11,7 +11,7 @@ RSpec.describe ::Gitlab::Ci::Pipeline::Chain::Limit::Deployments do
   let(:save_incompleted) { false }
 
   let(:command) do
-    Gitlab::Ci::Pipeline::Chain::Command.new(
+    double(:command,
       project: project,
       pipeline_seed: pipeline_seed,
       save_incompleted: save_incompleted
@@ -49,11 +49,6 @@ RSpec.describe ::Gitlab::Ci::Pipeline::Chain::Limit::Deployments do
 
         expect(pipeline.deployments_limit_exceeded?).to be true
       end
-
-      it 'calls increment_pipeline_failure_reason_counter' do
-        counter = Gitlab::Metrics.counter(:gitlab_ci_pipeline_failure_reasons, 'desc')
-        expect { perform }.to change { counter.get(reason: 'deployments_limit_exceeded') }.by(1)
-      end
     end
 
     context 'when not saving incomplete pipelines' do
@@ -76,12 +71,6 @@ RSpec.describe ::Gitlab::Ci::Pipeline::Chain::Limit::Deployments do
 
         expect(pipeline.errors.messages).to include(base: ['Pipeline has too many deployments! Requested 2, but the limit is 1.'])
       end
-
-      it 'increments the error metric' do
-        expect(command).to receive(:increment_pipeline_failure_reason_counter).with(:deployments_limit_exceeded)
-
-        perform
-      end
     end
 
     it 'logs the error' do

spec/lib/gitlab/ci/pipeline/chain/populate_spec.rb

@@ -96,11 +96,6 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Populate do
     it 'wastes pipeline iid' do
       expect(InternalId.ci_pipelines.where(project_id: project.id).last.last_value).to be > 0
     end
-
-    it 'increments the error metric' do
-      counter = Gitlab::Metrics.counter(:gitlab_ci_pipeline_failure_reasons, 'desc')
-      expect { run_chain }.to change { counter.get(reason: 'unknown_failure') }.by(1)
-    end
   end
 
   describe 'pipeline protect' do

spec/models/ci/pipeline_spec.rb

@@ -3867,16 +3867,6 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
           pipeline.drop
         end
       end
-
-      context 'with failure_reason' do
-        let(:pipeline) { create(:ci_pipeline, :running) }
-        let(:failure_reason) { 'config_error' }
-        let(:counter) { Gitlab::Metrics.counter(:gitlab_ci_pipeline_failure_reasons, 'desc') }
-
-        it 'increments the counter with the failure_reason' do
-          expect { pipeline.drop!(failure_reason) }.to change { counter.get(reason: failure_reason) }.by(1)
-        end
-      end
     end
   end
 

spec/models/commit_status_spec.rb

@@ -629,45 +629,30 @@ RSpec.describe CommitStatus do
     end
   end
 
-  describe '#drop' do
+  describe 'set failure_reason when drop' do
     let(:commit_status) { create(:commit_status, :created) }
-    let(:counter) { Gitlab::Metrics.counter(:gitlab_ci_job_failure_reasons, 'desc') }
-    let(:failure_reason) { reason.to_s }
 
     subject do
       commit_status.drop!(reason)
       commit_status
     end
 
-    shared_examples 'incrementing failure reason counter' do
-      it 'increments the counter with the failure_reason' do
-        expect { subject }.to change { counter.get(reason: failure_reason) }.by(1)
-      end
-    end
-
     context 'when failure_reason is nil' do
       let(:reason) { }
-      let(:failure_reason) { 'unknown_failure' }
 
       it { is_expected.to be_unknown_failure }
-
-      it_behaves_like 'incrementing failure reason counter'
     end
 
     context 'when failure_reason is script_failure' do
       let(:reason) { :script_failure }
 
       it { is_expected.to be_script_failure }
-
-      it_behaves_like 'incrementing failure reason counter'
     end
 
     context 'when failure_reason is unmet_prerequisites' do
       let(:reason) { :unmet_prerequisites }
 
       it { is_expected.to be_unmet_prerequisites }
-
-      it_behaves_like 'incrementing failure reason counter'
     end
   end
 

spec/services/ci/create_pipeline_service_spec.rb

@@ -71,21 +71,19 @@ RSpec.describe Ci::CreatePipelineService do
       end
 
       it 'increments the prometheus counter' do
-        counter = spy('pipeline created counter')
-
-        allow(Gitlab::Ci::Pipeline::Metrics)
-          .to receive(:pipelines_created_counter).and_return(counter)
+        expect(Gitlab::Metrics).to receive(:counter)
+          .with(:pipelines_created_total, "Counter of pipelines created")
+          .and_call_original
+        allow(Gitlab::Metrics).to receive(:counter).and_call_original # allow other counters
 
         pipeline
-
-        expect(counter).to have_received(:increment)
       end
 
       it 'records pipeline size in a prometheus histogram' do
         histogram = spy('pipeline size histogram')
 
         allow(Gitlab::Ci::Pipeline::Metrics)
-          .to receive(:pipeline_size_histogram).and_return(histogram)
+          .to receive(:new).and_return(histogram)
 
         execute_service
 
@@ -582,13 +580,6 @@ RSpec.describe Ci::CreatePipelineService do
 
       it_behaves_like 'a failed pipeline'
 
-      it 'increments the error metric' do
-        stub_ci_pipeline_yaml_file(ci_yaml)
-
-        counter = Gitlab::Metrics.counter(:gitlab_ci_pipeline_failure_reasons, 'desc')
-        expect { execute_service }.to change { counter.get(reason: 'config_error') }.by(1)
-      end
-
       context 'when receive git commit' do
         before do
           allow_any_instance_of(Ci::Pipeline).to receive(:git_commit_message) { message }

spec/services/ci/process_pipeline_service_spec.rb

@@ -10,14 +10,6 @@ RSpec.describe Ci::ProcessPipelineService do
     create(:ci_empty_pipeline, ref: 'master', project: project)
   end
 
-  let(:pipeline_processing_events_counter) { double(increment: true) }
-  let(:legacy_update_jobs_counter) { double(increment: true) }
-
-  let(:metrics) do
-    double(pipeline_processing_events_counter: pipeline_processing_events_counter,
-           legacy_update_jobs_counter: legacy_update_jobs_counter)
-  end
-
   subject { described_class.new(pipeline) }
 
   before do
@@ -25,13 +17,22 @@ RSpec.describe Ci::ProcessPipelineService do
     stub_not_protect_default_branch
 
     project.add_developer(user)
-
-    allow(subject).to receive(:metrics).and_return(metrics)
   end
 
   describe 'processing events counter' do
+    let(:metrics) { double('pipeline metrics') }
+    let(:counter) { double('events counter') }
+
+    before do
+      allow(subject)
+        .to receive(:metrics).and_return(metrics)
+      allow(metrics)
+        .to receive(:pipeline_processing_events_counter)
+        .and_return(counter)
+    end
+
     it 'increments processing events counter' do
-      expect(pipeline_processing_events_counter).to receive(:increment)
+      expect(counter).to receive(:increment)
 
       subject.execute
     end
@@ -63,8 +64,18 @@ RSpec.describe Ci::ProcessPipelineService do
         expect(all_builds.retried).to contain_exactly(build_retried)
       end
 
+      context 'counter ci_legacy_update_jobs_as_retried_total' do
+        let(:counter) { double(increment: true) }
+
+        before do
+          allow(Gitlab::Metrics).to receive(:counter).and_call_original
+          allow(Gitlab::Metrics).to receive(:counter)
+                                      .with(:ci_legacy_update_jobs_as_retried_total, anything)
+                                      .and_return(counter)
+        end
+
         it 'increments the counter' do
-        expect(legacy_update_jobs_counter).to receive(:increment)
+          expect(counter).to receive(:increment)
 
           subject.execute
         end
@@ -75,12 +86,13 @@ RSpec.describe Ci::ProcessPipelineService do
           end
 
           it 'does not increment the counter' do
-          expect(legacy_update_jobs_counter).not_to receive(:increment)
+            expect(counter).not_to receive(:increment)
 
             subject.execute
           end
         end
       end
+    end
 
     private