From 6321a29006ede22a9e71031a901f3057a42bb1af Mon Sep 17 00:00:00 2001
From: Tan Le <tle@gitlab.com>
Date: Thu, 1 Dec 2022 21:53:56 +1100
Subject: [PATCH] Only retry on network error on suggested reviewers
We would like to track only network errors when calling the suggested
reviewers client. These types of errors will deplete the error budget.
Other application errors that are deep in the stack, that are not
network-related are not retriable but will be tracked and reported
internally in the suggested reviewers monitoring infrastructure.
---
.../fetch_suggested_reviewers_worker.rb | 8 +++++
ee/lib/gitlab/applied_ml/errors.rb | 1 +
.../applied_ml/suggested_reviewers/client.rb | 7 +++++
.../suggested_reviewers/client_spec.rb | 16 ++++++++--
.../fetch_suggested_reviewers_worker_spec.rb | 30 +++++++++++++++++--
5 files changed, 58 insertions(+), 4 deletions(-)
diff --git a/ee/app/workers/merge_requests/fetch_suggested_reviewers_worker.rb b/ee/app/workers/merge_requests/fetch_suggested_reviewers_worker.rb
index 6918953c6936a..43acf000a1028 100644
--- a/ee/app/workers/merge_requests/fetch_suggested_reviewers_worker.rb
+++ b/ee/app/workers/merge_requests/fetch_suggested_reviewers_worker.rb
@@ -31,6 +31,14 @@ def perform(merge_request_id)
else
logger.error(structured_payload({ result: result }))
end
+ rescue Gitlab::AppliedMl::Errors::ResourceNotAvailable => e
+ Gitlab::ErrorTracking.log_exception(
+ e,
+ project_id: merge_request.project.id,
+ merge_request_id: merge_request.id
+ )
+
+ nil
end
end
end
diff --git a/ee/lib/gitlab/applied_ml/errors.rb b/ee/lib/gitlab/applied_ml/errors.rb
index 36616635cc747..76e2383411c4e 100644
--- a/ee/lib/gitlab/applied_ml/errors.rb
+++ b/ee/lib/gitlab/applied_ml/errors.rb
@@ -8,6 +8,7 @@ module Errors
ProjectAlreadyExists = Class.new(BaseError)
ResourceNotAvailable = Class.new(BaseError)
ConfigurationError = Class.new(BaseError)
+ ConnectionFailed = Class.new(BaseError)
end
end
end
diff --git a/ee/lib/gitlab/applied_ml/suggested_reviewers/client.rb b/ee/lib/gitlab/applied_ml/suggested_reviewers/client.rb
index 839616956b582..f293e4c3ac256 100644
--- a/ee/lib/gitlab/applied_ml/suggested_reviewers/client.rb
+++ b/ee/lib/gitlab/applied_ml/suggested_reviewers/client.rb
@@ -15,6 +15,11 @@ class Client
SECRET_NAME = "SUGGESTED_REVIEWERS_SECRET"
SECRET_LENGTH = 64
+ NETWORK_ERRORS = [
+ GRPC::DeadlineExceeded,
+ GRPC::Unavailable
+ ].freeze
+
def self.default_rpc_url
if Gitlab.dev_or_test_env?
'suggested-reviewer.dev:443'
@@ -48,6 +53,8 @@ def suggested_reviewers(merge_request_iid:, project_id:, changes:, author_userna
top_n: response.top_n,
reviewers: response.reviewers
}
+ rescue *NETWORK_ERRORS => e
+ raise Errors::ConnectionFailed, e
rescue GRPC::BadStatus => e
raise Errors::ResourceNotAvailable, e
end
diff --git a/ee/spec/lib/gitlab/applied_ml/suggested_reviewers/client_spec.rb b/ee/spec/lib/gitlab/applied_ml/suggested_reviewers/client_spec.rb
index 448d2428947cb..5f08fefbbac19 100644
--- a/ee/spec/lib/gitlab/applied_ml/suggested_reviewers/client_spec.rb
+++ b/ee/spec/lib/gitlab/applied_ml/suggested_reviewers/client_spec.rb
@@ -2,7 +2,7 @@
require 'spec_helper'
-RSpec.describe Gitlab::AppliedMl::SuggestedReviewers::Client do
+RSpec.describe Gitlab::AppliedMl::SuggestedReviewers::Client, feature_category: :workflow_automation do
let(:stub_class) { Gitlab::AppliedMl::SuggestedReviewers::RecommenderServicesPb::Stub }
let(:rpc_url) { 'example.org:1234' }
@@ -141,13 +141,25 @@
it_behaves_like 'respecting channel credentials'
end
- context 'when a grpc bad status is received' do
+ context 'when a grpc connection error is received' do
before do
allow_next_instance_of(stub_class) do |stub|
allow(stub).to receive(:merge_request_recommendations_v2).and_raise(GRPC::Unavailable)
end
end
+ it 'raises a new error' do
+ expect { subject }.to raise_error(Gitlab::AppliedMl::Errors::ConnectionFailed)
+ end
+ end
+
+ context 'when a grpc bad status is received' do
+ before do
+ allow_next_instance_of(stub_class) do |stub|
+ allow(stub).to receive(:merge_request_recommendations_v2).and_raise(GRPC::Internal)
+ end
+ end
+
it 'raises a new error' do
expect { subject }.to raise_error(Gitlab::AppliedMl::Errors::ResourceNotAvailable)
end
diff --git a/ee/spec/workers/merge_requests/fetch_suggested_reviewers_worker_spec.rb b/ee/spec/workers/merge_requests/fetch_suggested_reviewers_worker_spec.rb
index 63fe6b44ab84c..3db443b3c185a 100644
--- a/ee/spec/workers/merge_requests/fetch_suggested_reviewers_worker_spec.rb
+++ b/ee/spec/workers/merge_requests/fetch_suggested_reviewers_worker_spec.rb
@@ -2,7 +2,9 @@
require 'spec_helper'
-RSpec.describe MergeRequests::FetchSuggestedReviewersWorker do
+RSpec.describe MergeRequests::FetchSuggestedReviewersWorker, feature_category: :workflow_automation do
+ include AfterNextHelpers
+
let_it_be(:merge_request) { create(:merge_request) }
subject { described_class.new }
@@ -19,7 +21,7 @@
context 'when merge request does not have changes' do
let_it_be(:merge_request) { create(:merge_request, :without_diffs) }
- it 'returns without calling the fetch suggested reviewer service', :aggregate_failures do
+ it 'returns without calling the fetch suggested reviewer service' do
expect(MergeRequests::FetchSuggestedReviewersService).not_to receive(:new)
subject.perform(merge_request.id)
@@ -88,6 +90,30 @@
expect(merge_request2.predictions&.suggested_reviewers).to be_nil
end
end
+
+ context 'when exceptions are raised' do
+ it 're-raises exception when it is retriable' do
+ allow_next(::MergeRequests::FetchSuggestedReviewersService)
+ .to receive(:execute)
+ .and_raise(Gitlab::AppliedMl::Errors::ConnectionFailed)
+
+ expect { subject.perform(merge_request.id) }.to raise_error(Gitlab::AppliedMl::Errors::ConnectionFailed)
+ end
+
+ it 'does not raise but logs exception when it is swallowable', :aggregate_failures do
+ allow_next(::MergeRequests::FetchSuggestedReviewersService)
+ .to receive(:execute)
+ .and_raise(Gitlab::AppliedMl::Errors::ResourceNotAvailable)
+
+ expect(Gitlab::ErrorTracking).to receive(:log_exception).with(
+ Gitlab::AppliedMl::Errors::ResourceNotAvailable,
+ project_id: merge_request.project.id,
+ merge_request_id: merge_request.id
+ )
+
+ expect { subject.perform(merge_request.id) }.not_to raise_error
+ end
+ end
end
end
end
--
GitLab