From c34240d26fdcf447ee86172a81c0fc56fcaf9cbc Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Tue, 3 Sep 2019 23:34:52 -0700
Subject: Log errors for failed pipeline creation in PostReceive

When a pipeline fails to create in `PostReceive`, the error is silently
discarded, making it difficult to understand why a pipeline was not
created. We now add a Sidekiq warning message for this. Adding a Sentry
exception when this happens would generate a lot of noise for invalid CI
files.

Relates to https://gitlab.com/gitlab-org/gitlab-ee/issues/14720
---
 app/services/git/base_hooks_service.rb             | 28 +++++++++++++++++++++-
 .../sh-add-sidekiq-logging-for-bad-ci.yml          |  5 ++++
 spec/services/git/branch_push_service_spec.rb      | 14 +++++++++++
 3 files changed, 46 insertions(+), 1 deletion(-)
 create mode 100644 changelogs/unreleased/sh-add-sidekiq-logging-for-bad-ci.yml

diff --git a/app/services/git/base_hooks_service.rb b/app/services/git/base_hooks_service.rb
index 47c308c8280..35a4d2015fa 100644
--- a/app/services/git/base_hooks_service.rb
+++ b/app/services/git/base_hooks_service.rb
@@ -57,7 +57,9 @@ module Git
 
       Ci::CreatePipelineService
         .new(project, current_user, pipeline_params)
-        .execute(:push, pipeline_options)
+        .execute!(:push, pipeline_options)
+    rescue Ci::CreatePipelineService::CreateError => ex
+      log_pipeline_errors(ex)
     end
 
     def execute_project_hooks
@@ -125,5 +127,29 @@ module Git
       project.mark_stuck_remote_mirrors_as_failed!
       project.update_remote_mirrors
     end
+
+    def log_pipeline_errors(exception)
+      data = {
+        class: self.class.name,
+        correlation_id: Labkit::Correlation::CorrelationId.current_id.to_s,
+        project_id: project.id,
+        project_path: project.full_path,
+        message: "Error creating pipeline",
+        errors: exception.to_s,
+        pipeline_params: pipeline_params
+      }
+
+      logger.warn(data)
+    end
+
+    def logger
+      if Sidekiq.server?
+        Sidekiq.logger
+      else
+        # This service runs in Sidekiq, so this shouldn't ever be
+        # called, but this is included just in case.
+        Gitlab::ProjectServiceLogger
+      end
+    end
   end
 end
diff --git a/changelogs/unreleased/sh-add-sidekiq-logging-for-bad-ci.yml b/changelogs/unreleased/sh-add-sidekiq-logging-for-bad-ci.yml
new file mode 100644
index 00000000000..b334355cab6
--- /dev/null
+++ b/changelogs/unreleased/sh-add-sidekiq-logging-for-bad-ci.yml
@@ -0,0 +1,5 @@
+---
+title: Log errors for failed pipeline creation in PostReceive
+merge_request: 32633
+author:
+type: other
diff --git a/spec/services/git/branch_push_service_spec.rb b/spec/services/git/branch_push_service_spec.rb
index d9e607cd251..c3a4f3dbe3f 100644
--- a/spec/services/git/branch_push_service_spec.rb
+++ b/spec/services/git/branch_push_service_spec.rb
@@ -99,6 +99,20 @@ describe Git::BranchPushService, services: true do
       expect(pipeline).to be_push
       expect(Gitlab::Git::BRANCH_REF_PREFIX + pipeline.ref).to eq(ref)
     end
+
+    context 'when pipeline has errors' do
+      before do
+        config = YAML.dump({ test: { script: 'ls', only: ['feature'] } })
+        stub_ci_pipeline_yaml_file(config)
+      end
+
+      it 'reports an error' do
+        allow(Sidekiq).to receive(:server?).and_return(true)
+        expect(Sidekiq.logger).to receive(:warn)
+
+        expect { subject }.not_to change { Ci::Pipeline.count }
+      end
+    end
   end
 
   describe "Updates merge requests" do
-- 
cgit v1.2.1