diff --git a/.rubocop_todo/gitlab/strong_memoize_attr.yml b/.rubocop_todo/gitlab/strong_memoize_attr.yml index bd0dbda439bbff07a76c6bbfd4da2a3d39d9feb1..d5477a47fafdbf6207a89b535f51a4983cdd64d5 100644 --- a/.rubocop_todo/gitlab/strong_memoize_attr.yml +++ b/.rubocop_todo/gitlab/strong_memoize_attr.yml @@ -174,8 +174,6 @@ Gitlab/StrongMemoizeAttr: - 'app/services/members/invitation_reminder_email_service.rb' - 'app/services/merge_requests/build_service.rb' - 'app/services/merge_requests/mergeability/detailed_merge_status_service.rb' - - 'app/services/merge_requests/mergeability/logger.rb' - - 'app/services/merge_requests/mergeability/run_checks_service.rb' - 'app/services/merge_requests/mergeability_check_service.rb' - 'app/services/merge_requests/outdated_discussion_diff_lines_service.rb' - 'app/services/merge_requests/pushed_branches_service.rb' diff --git a/app/services/merge_requests/mergeability/logger.rb b/app/services/merge_requests/mergeability/logger.rb index 612c79f0aaea3e553598770d4dd9021f85a726bf..cf4b836e8083f9a62094af27fd57aaea7a2afbca 100644 --- a/app/services/merge_requests/mergeability/logger.rb +++ b/app/services/merge_requests/mergeability/logger.rb @@ -34,9 +34,8 @@ def instrument(mergeability_name:) attr_reader :destination, :merge_request, :stored_result def observe_result(name, result) - return unless result.respond_to?(:success?) - - observe("mergeability.#{name}.successful", result.success?) + observe("mergeability.#{name}.successful", result.success?) if result.respond_to?(:success?) + observe("mergeability.#{name}.status", result.status.to_s) if result.respond_to?(:status) end def observe(name, value) @@ -69,10 +68,9 @@ def observations_hash end def observations - strong_memoize(:observations) do - Hash.new { |hash, key| hash[key] = [] } - end + Hash.new { |hash, key| hash[key] = [] } end + strong_memoize_attr :observations def observe_sql_counters(name, start_db_counters, end_db_counters) end_db_counters.each do |key, value| diff --git a/app/services/merge_requests/mergeability/run_checks_service.rb b/app/services/merge_requests/mergeability/run_checks_service.rb index 941de0061fc62e60cf2d17bea52656c1913e0058..931df0b1233f5e9a380bc5d715eb3f068635686a 100644 --- a/app/services/merge_requests/mergeability/run_checks_service.rb +++ b/app/services/merge_requests/mergeability/run_checks_service.rb @@ -26,7 +26,7 @@ def execute(checks, execute_all: false) logger.commit - return ServiceResponse.success(payload: { results: results }) if all_results_success? + return ServiceResponse.success(payload: { results: results }) if no_result_unsuccessful? ServiceResponse.error( message: 'Checks were not successful', @@ -53,18 +53,18 @@ def run_check(check) end def cached_results - strong_memoize(:cached_results) do - Gitlab::MergeRequests::Mergeability::ResultsStore.new(merge_request: merge_request) - end + Gitlab::MergeRequests::Mergeability::ResultsStore.new(merge_request: merge_request) end + strong_memoize_attr :cached_results def logger - strong_memoize(:logger) do - MergeRequests::Mergeability::Logger.new(merge_request: merge_request) - end + MergeRequests::Mergeability::Logger.new(merge_request: merge_request) end + strong_memoize_attr :logger - def all_results_success? + # This name may seem like a double-negative, but it is meaningful because + # #success? is _not_ the inverse of #unsuccessful? + def no_result_unsuccessful? results.none?(&:unsuccessful?) end diff --git a/spec/services/merge_requests/mergeability/logger_spec.rb b/spec/services/merge_requests/mergeability/logger_spec.rb index 501532e762aa5bf4dfbb493d57366b30d4b588b7..0212d26fce24f6c92c433d756571b6e0e0a6e7f6 100644 --- a/spec/services/merge_requests/mergeability/logger_spec.rb +++ b/spec/services/merge_requests/mergeability/logger_spec.rb @@ -71,6 +71,26 @@ def loggable_data(**extras) end end + context 'when block value responds to #status' do + let(:check_result) { instance_double(Gitlab::MergeRequests::Mergeability::CheckResult, status: :inactive) } + + let(:extra_data) do + { + 'mergeability.expensive_operation.status.values' => ['inactive'] + } + end + + it 'records operation status value' do + expect_next_instance_of(Gitlab::AppJsonLogger) do |app_logger| + expect(app_logger).to receive(:info).with(match(a_hash_including(loggable_data(**extra_data)))) + end + + expect(logger.instrument(mergeability_name: :expensive_operation) { check_result }).to eq(check_result) + + logger.commit + end + end + context 'with multiple observations' do let(:operation_count) { 2 }