Merge branch '21300-push-measurement-to-export-service' into 'master'

Move measurement to export service layer

See merge request gitlab-org/gitlab!29899

app/services/projects/import_export/export_service.rb

@@ -10,8 +10,13 @@ module Projects
 
         @shared = project.import_export_shared
 
-        save_all!
-        execute_after_export_action(after_export_strategy)
+        measurement_enabled = !!options[:measurement_enabled]
+        measurement_logger = options[:measurement_logger]
+
+        ::Gitlab::Utils::Measuring.execute_with(measurement_enabled, measurement_logger, base_log_data) do
+          save_all!
+          execute_after_export_action(after_export_strategy)
+        end
       ensure
         cleanup
       end
@@ -20,6 +25,15 @@ module Projects
 
       attr_accessor :shared
 
+      def base_log_data
+        {
+          class: self.class.name,
+          current_user: current_user.name,
+          project_full_path: project.full_path,
+          file_path: shared.export_path
+        }
+      end
+
       def execute_after_export_action(after_export_strategy)
         return unless after_export_strategy
 

lib/gitlab/import_export/project/base_task.rb

@@ -12,16 +12,18 @@ module Gitlab
           @namespace    = Namespace.find_by_full_path(opts.fetch(:namespace_path))
           @current_user = User.find_by_username(opts.fetch(:username))
           @measurement_enabled = opts.fetch(:measurement_enabled)
-          @measurement = Gitlab::Utils::Measuring.new(logger: logger) if @measurement_enabled
           @logger = logger
         end
 
         private
 
-        attr_reader :measurement, :project, :namespace, :current_user, :file_path, :project_path, :logger
+        attr_reader :project, :namespace, :current_user, :file_path, :project_path, :logger, :measurement_enabled
 
-        def measurement_enabled?
-          @measurement_enabled
+        def measurement_options
+          {
+            measurement_enabled: measurement_enabled,
+            measurement_logger: logger
+          }
         end
 
         def success(message)
@@ -30,13 +32,6 @@ module Gitlab
           true
         end
 
-        def measurement_options
-          {
-            measurement_enabled: measurement_enabled?,
-            measurement_logger: logger
-          }
-        end
-
         def error(message)
           logger.error(message)
 

lib/gitlab/import_export/project/export_task.rb

@@ -16,7 +16,7 @@ module Gitlab
 
           with_export do
             ::Projects::ImportExport::ExportService.new(project, current_user)
-              .execute(Gitlab::ImportExport::AfterExportStrategies::MoveFileStrategy.new(archive_path: file_path))
+              .execute(Gitlab::ImportExport::AfterExportStrategies::MoveFileStrategy.new(archive_path: file_path), measurement_options)
           end
 
           success('Done!')
@@ -33,7 +33,7 @@ module Gitlab
         def with_export
           with_request_store do
             ::Gitlab::GitalyClient.allow_n_plus_1_calls do
-              measurement_enabled? ? measurement.with_measuring { yield } : yield
+              yield
             end
           end
         end

lib/gitlab/utils/measuring.rb

@@ -23,10 +23,11 @@ module Gitlab
       end
 
       def with_measuring
-        result = with_gc_stats do
+        result = nil
+        with_gc_stats do
           with_count_queries do
             with_measure_time do
-              yield
+              result = yield
             end
           end
         end
@@ -56,31 +57,27 @@ module Gitlab
         ActiveSupport::Notifications.subscribed(counter_f, "sql.active_record", &block)
       end
 
-      def log_info(details)
-        details = base_log_data.merge(details)
-        details = details.to_yaml if ActiveSupport::Logger.logger_outputs_to?(logger, STDOUT)
-        logger.info(details)
-      end
-
       def with_gc_stats
         GC.start # perform a full mark-and-sweep
         stats_before = GC.stat
-        result = yield
+        yield
         stats_after = GC.stat
         @gc_stats = stats_after.map do |key, after_value|
           before_value = stats_before[key]
           [key, before: before_value, after: after_value, diff: after_value - before_value]
         end.to_h
-        result
       end
 
       def with_measure_time
-        result = nil
         @time_to_finish = Benchmark.realtime do
-          result = yield
+          yield
         end
+      end
 
-        result
+      def log_info(details)
+        details = base_log_data.merge(details)
+        details = details.to_yaml if ActiveSupport::Logger.logger_outputs_to?(logger, STDOUT)
+        logger.info(details)
       end
 
       def duration_in_numbers(duration_in_seconds)

spec/lib/gitlab/utils/measuring_spec.rb

@@ -3,7 +3,7 @@
 require 'fast_spec_helper'
 
 describe Gitlab::Utils::Measuring do
-  describe '#execute_with' do
+  describe '.execute_with' do
     let(:measurement_logger) { double(:logger) }
     let(:base_log_data) do
       {

spec/services/projects/import_export/export_service_spec.rb

@@ -177,5 +177,56 @@ describe Projects::ImportExport::ExportService do
         expect { service.execute }.to raise_error(Gitlab::ImportExport::Error).with_message(expected_message)
       end
     end
+
+    context 'when measurable params are provided' do
+      let(:base_log_data) do
+        {
+          class: described_class.name,
+          current_user: user.name,
+          project_full_path: project.full_path,
+          file_path: shared.export_path
+        }
+      end
+
+      subject(:service) { described_class.new(project, user) }
+
+      context 'when measurement is enabled' do
+        let(:logger) { double(:logger) }
+        let(:measurable_options) do
+          {
+            measurement_enabled: true,
+            measurement_logger: logger
+          }
+        end
+
+        before do
+          allow(logger).to receive(:info)
+        end
+
+        it 'measure service execution with Gitlab::Utils::Measuring' do
+          expect(Gitlab::Utils::Measuring).to receive(:execute_with).with(true, logger, base_log_data).and_call_original
+          expect_next_instance_of(Gitlab::Utils::Measuring) do |measuring|
+            expect(measuring).to receive(:with_measuring).and_call_original
+          end
+
+          service.execute(after_export_strategy, measurable_options)
+        end
+      end
+
+      context 'when measurement is disabled' do
+        let(:measurable_options) do
+          {
+            measurement_enabled: false
+          }
+        end
+
+        it 'does not measure service execution' do
+          expect(Gitlab::Utils::Measuring).to receive(:execute_with).with(false, nil, base_log_data).and_call_original
+          expect(Gitlab::Utils::Measuring).not_to receive(:new)
+
+          service.execute(after_export_strategy, measurable_options)
+        end
+      end
+    end
   end
 end