Merge branch 'sh-clean-up-bitbucket-import-errors' into 'master'

Avoid storing backtraces from Bitbucket Cloud imports in the database

See merge request gitlab-org/gitlab-ce!29862

3 files changed, 53 insertions, 7 deletions

diff --git a/changelogs/unreleased/sh-clean-up-bitbucket-import-errors.yml b/changelogs/unreleased/sh-clean-up-bitbucket-import-errors.yml
new file mode 100644
index 00000000000..e4c9de74e6a
--- /dev/null
+++ b/changelogs/unreleased/sh-clean-up-bitbucket-import-errors.yml
@@ -0,0 +1,5 @@
+---
+title: Avoid storing backtraces from Bitbucket Cloud imports in the database
+merge_request: 29862
+author:
+type: performance
diff --git a/lib/gitlab/bitbucket_import/importer.rb b/lib/gitlab/bitbucket_import/importer.rb
index c9f0ed66a54..8047ef4fa15 100644
--- a/lib/gitlab/bitbucket_import/importer.rb
+++ b/lib/gitlab/bitbucket_import/importer.rb
@@ -11,6 +11,7 @@ module Gitlab
                 { title: 'task', color: '#7F8C8D' }].freeze
 
       attr_reader :project, :client, :errors, :users
+      attr_accessor :logger
 
       def initialize(project)
         @project = project
@@ -19,6 +20,7 @@ module Gitlab
         @labels = {}
         @errors = []
         @users = {}
+        @logger = Gitlab::Import::Logger.build
       end
 
       def execute
@@ -41,6 +43,18 @@ module Gitlab
         }.to_json)
       end
 
+      def store_pull_request_error(pull_request, ex)
+        backtrace = Gitlab::Profiler.clean_backtrace(ex.backtrace)
+        error = { type: :pull_request, iid: pull_request.iid, errors: ex.message, trace: backtrace, raw_response: pull_request.raw }
+
+        log_error(error)
+        # Omit the details from the database to avoid blowing up usage in the error column
+        error.delete(:trace)
+        error.delete(:raw_response)
+
+        errors << error
+      end
+
       def gitlab_user_id(project, username)
         find_user_id(username) || project.creator_id
       end
@@ -176,7 +190,7 @@ module Gitlab
 
           import_pull_request_comments(pull_request, merge_request) if merge_request.persisted?
         rescue StandardError => e
-          errors << { type: :pull_request, iid: pull_request.iid, errors: e.message, trace: e.backtrace.join("\n"), raw_response: pull_request.raw }
+          store_pull_request_error(pull_request, e)
         end
       end
 
@@ -254,6 +268,18 @@ module Gitlab
           updated_at: comment.updated_at
         }
       end
+
+      def log_error(details)
+        logger.error(log_base_data.merge(details))
+      end
+
+      def log_base_data
+        {
+          class: self.class.name,
+          project_id: project.id,
+          project_path: project.full_path
+        }
+      end
     end
   end
 end
diff --git a/spec/lib/gitlab/bitbucket_import/importer_spec.rb b/spec/lib/gitlab/bitbucket_import/importer_spec.rb
index 2e90f6c7f71..35700e0b588 100644
--- a/spec/lib/gitlab/bitbucket_import/importer_spec.rb
+++ b/spec/lib/gitlab/bitbucket_import/importer_spec.rb
@@ -98,12 +98,8 @@ describe Gitlab::BitbucketImport::Importer do
   describe '#import_pull_requests' do
     let(:source_branch_sha) { sample.commits.last }
     let(:target_branch_sha) { sample.commits.first }
-
-    before do
-      allow(subject).to receive(:import_wiki)
-      allow(subject).to receive(:import_issues)
-
-      pull_request = instance_double(
+    let(:pull_request) do
+      instance_double(
         Bitbucket::Representation::PullRequest,
         iid: 10,
         source_branch_sha: source_branch_sha,
@@ -116,6 +112,11 @@ describe Gitlab::BitbucketImport::Importer do
         author: 'other',
         created_at: Time.now,
         updated_at: Time.now)
+    end
+
+    before do
+      allow(subject).to receive(:import_wiki)
+      allow(subject).to receive(:import_issues)
 
       # https://gitlab.com/gitlab-org/gitlab-test/compare/c1acaa58bbcbc3eafe538cb8274ba387047b69f8...5937ac0a7beb003549fc5fd26fc247ad
       @inline_note = instance_double(
@@ -167,6 +168,20 @@ describe Gitlab::BitbucketImport::Importer do
       expect(reply_note.note).to eq(@reply.note)
     end
 
+    context 'when importing a pull request throws an exception' do
+      before do
+        allow(pull_request).to receive(:raw).and_return('hello world')
+        allow(subject.client).to receive(:pull_request_comments).and_raise(HTTParty::Error)
+      end
+
+      it 'logs an error without the backtrace' do
+        subject.execute
+
+        expect(subject.errors.count).to eq(1)
+        expect(subject.errors.first.keys).to match_array(%i(type iid errors))
+      end
+    end
+
     context "when branches' sha is not found in the repository" do
       let(:source_branch_sha) { 'a' * Commit::MIN_SHA_LENGTH }
       let(:target_branch_sha) { 'b' * Commit::MIN_SHA_LENGTH }