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 6918953c6936a6775be69740e3a83e7680442a15..43acf000a10281118593123f7dd339cfc57e0d5a 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 36616635cc747b5cc79947e4693a3bb39cc07df4..76e2383411c4e72bb0248860a66b1a7370781d00 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 839616956b5825120e27f27aa342dd3b68b645ef..f293e4c3ac256f4dc4b7aba100fc6ca60e1df9b3 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 448d2428947cbbbd117ce00e066a161a3f4af9f3..5f08fefbbac195510378eedeb57da0083730aa78 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 63fe6b44ab84cc25125d94e4799fb7483d82af73..3db443b3c185a18073ac52fddd072c678d421a9e 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