Commit 30f52b69 authored by Stan Hu's avatar Stan Hu

Avoid storing backtraces from Bitbucket Cloud imports in the database

We noticed in
https://gitlab.com/gitlab-com/gl-infra/production/issues/908 some
Bitbucket imports took over a second to load their projects row because
`import_error` was huge due to errors. To prevent this, we now:

1. Clean the backtraces
2. Log the details into importer.log
3. Omit the details from the database
parent 0d2537bf
---
title: Avoid storing backtraces from Bitbucket Cloud imports in the database
merge_request: 29862
author:
type: performance
...@@ -11,6 +11,7 @@ module Gitlab ...@@ -11,6 +11,7 @@ module Gitlab
{ title: 'task', color: '#7F8C8D' }].freeze { title: 'task', color: '#7F8C8D' }].freeze
attr_reader :project, :client, :errors, :users attr_reader :project, :client, :errors, :users
attr_accessor :logger
def initialize(project) def initialize(project)
@project = project @project = project
...@@ -19,6 +20,7 @@ module Gitlab ...@@ -19,6 +20,7 @@ module Gitlab
@labels = {} @labels = {}
@errors = [] @errors = []
@users = {} @users = {}
@logger = Gitlab::Import::Logger.build
end end
def execute def execute
...@@ -41,6 +43,18 @@ module Gitlab ...@@ -41,6 +43,18 @@ module Gitlab
}.to_json) }.to_json)
end 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) def gitlab_user_id(project, username)
find_user_id(username) || project.creator_id find_user_id(username) || project.creator_id
end end
...@@ -176,7 +190,7 @@ module Gitlab ...@@ -176,7 +190,7 @@ module Gitlab
import_pull_request_comments(pull_request, merge_request) if merge_request.persisted? import_pull_request_comments(pull_request, merge_request) if merge_request.persisted?
rescue StandardError => e 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
end end
...@@ -254,6 +268,18 @@ module Gitlab ...@@ -254,6 +268,18 @@ module Gitlab
updated_at: comment.updated_at updated_at: comment.updated_at
} }
end 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 end
end end
...@@ -98,12 +98,8 @@ describe Gitlab::BitbucketImport::Importer do ...@@ -98,12 +98,8 @@ describe Gitlab::BitbucketImport::Importer do
describe '#import_pull_requests' do describe '#import_pull_requests' do
let(:source_branch_sha) { sample.commits.last } let(:source_branch_sha) { sample.commits.last }
let(:target_branch_sha) { sample.commits.first } let(:target_branch_sha) { sample.commits.first }
let(:pull_request) do
before do instance_double(
allow(subject).to receive(:import_wiki)
allow(subject).to receive(:import_issues)
pull_request = instance_double(
Bitbucket::Representation::PullRequest, Bitbucket::Representation::PullRequest,
iid: 10, iid: 10,
source_branch_sha: source_branch_sha, source_branch_sha: source_branch_sha,
...@@ -116,6 +112,11 @@ describe Gitlab::BitbucketImport::Importer do ...@@ -116,6 +112,11 @@ describe Gitlab::BitbucketImport::Importer do
author: 'other', author: 'other',
created_at: Time.now, created_at: Time.now,
updated_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 # https://gitlab.com/gitlab-org/gitlab-test/compare/c1acaa58bbcbc3eafe538cb8274ba387047b69f8...5937ac0a7beb003549fc5fd26fc247ad
@inline_note = instance_double( @inline_note = instance_double(
...@@ -167,6 +168,20 @@ describe Gitlab::BitbucketImport::Importer do ...@@ -167,6 +168,20 @@ describe Gitlab::BitbucketImport::Importer do
expect(reply_note.note).to eq(@reply.note) expect(reply_note.note).to eq(@reply.note)
end 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 context "when branches' sha is not found in the repository" do
let(:source_branch_sha) { 'a' * Commit::MIN_SHA_LENGTH } let(:source_branch_sha) { 'a' * Commit::MIN_SHA_LENGTH }
let(:target_branch_sha) { 'b' * Commit::MIN_SHA_LENGTH } let(:target_branch_sha) { 'b' * Commit::MIN_SHA_LENGTH }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment