Merge branch 'log-status-in-mergeability-logger' into 'master'

Log check result status in mergeability logger

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/173419



Merged-by: Kerri Miller <kerrizor@kerrizor.com>
Approved-by: Keeyan Nejad <knejad@gitlab.com>
Approved-by: Kerri Miller <kerrizor@kerrizor.com>
Reviewed-by: Keeyan Nejad <knejad@gitlab.com>
Co-authored-by: Hordur Freyr Yngvason <hfyngvason@gitlab.com>

.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'

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|

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
 

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 }