Address MR comments

- Add specs for measuring
- Rename base_data

app/services/projects/gitlab_projects_import_service.rb

@@ -18,7 +18,7 @@ module Projects
       measurement_enabled = !!options[:measurement_enabled]
       measurement_logger = options[:measurement_logger]
 
-      ::Gitlab::Utils::Measuring.execute_with(measurement_enabled, measurement_logger, base_data) do
+      ::Gitlab::Utils::Measuring.execute_with(measurement_enabled, measurement_logger, base_log_data) do
         prepare_template_environment(template_file)
 
         prepare_import_params
@@ -29,19 +29,19 @@ module Projects
 
     private
 
-    def base_data
-      base_data = {
+    def base_log_data
+      base_log_data = {
         class: self.class.name,
         current_user: current_user.name,
         project_full_path: project_path
       }
 
       if template_file
-        base_data[:import_type] = 'gitlab_project'
-        base_data[:file_path] = template_file.path
+        base_log_data[:import_type] = 'gitlab_project'
+        base_log_data[:file_path] = template_file.path
       end
 
-      base_data
+      base_log_data
     end
 
     def overwrite_project?
@@ -61,9 +61,7 @@ module Projects
     # rubocop: enable CodeReuse/ActiveRecord
 
     def project_path
-      strong_memoize(:project_path) do
-        "#{current_namespace.full_path}/#{params[:path]}"
-      end
+      "#{current_namespace.full_path}/#{params[:path]}"
     end
 
     def overwrite?

lib/gitlab/utils/measuring.rb

@@ -6,20 +6,20 @@ module Gitlab
   module Utils
     class Measuring
       class << self
-        def execute_with(measurement_enabled, logger, base_data)
-          measurement_enabled ? measuring(logger, base_data).with_measuring { yield } : yield
+        def execute_with(measurement_enabled, logger, base_log_data)
+          measurement_enabled ? measuring(logger, base_log_data).with_measuring { yield } : yield
         end
 
         private
 
-        def measuring(logger, base_data)
-          Gitlab::Utils::Measuring.new(logger: logger, base_data: base_data)
+        def measuring(logger, base_log_data)
+          Gitlab::Utils::Measuring.new(logger: logger, base_log_data: base_log_data)
         end
       end
 
-      def initialize(logger: nil, base_data: {})
-        @logger = logger || Logger.new($stdout)
-        @base_data = base_data
+      def initialize(logger: nil, base_log_data: {})
+        self.logger = logger || Logger.new($stdout)
+        self.base_log_data = base_log_data
       end
 
       def with_measuring
@@ -32,9 +32,9 @@ module Gitlab
         end
 
         log_info(
-          gc_stats: @gc_stats,
-          time_to_finish: @time_to_finish,
-          number_of_sql_calls: @count,
+          gc_stats: gc_stats,
+          time_to_finish: time_to_finish,
+          number_of_sql_calls: sql_calls_count,
           memory_usage: "#{Gitlab::Metrics::System.memory_usage.to_f / 1024 / 1024} MiB",
           label: ::Prometheus::PidProvider.worker_id
         )
@@ -44,20 +44,20 @@ module Gitlab
 
       private
 
-      attr_reader :logger, :base_data
+      attr_accessor :gc_stats, :time_to_finish, :sql_calls_count, :logger, :base_log_data
 
       def with_count_queries(&block)
-        @count = 0
+        self.sql_calls_count = 0
 
         counter_f = ->(_name, _started, _finished, _unique_id, payload) {
-          @count += 1 unless payload[:name].in? %w[CACHE SCHEMA]
+          self.sql_calls_count += 1 unless payload[:name].in? %w[CACHE SCHEMA]
         }
 
         ActiveSupport::Notifications.subscribed(counter_f, "sql.active_record", &block)
       end
 
       def log_info(details)
-        details = base_data.merge(details)
+        details = base_log_data.merge(details)
         details = details.to_yaml if ActiveSupport::Logger.logger_outputs_to?(logger, STDOUT)
         logger.info(details)
       end
@@ -67,7 +67,7 @@ module Gitlab
         stats_before = GC.stat
         result = yield
         stats_after = GC.stat
-        @gc_stats = stats_after.map do |key, after_value|
+        self.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
@@ -75,8 +75,8 @@ module Gitlab
       end
 
       def with_measure_time
-        result = 0
-        @time_to_finish = Benchmark.realtime do
+        result = nil
+        self.time_to_finish = Benchmark.realtime do
           result = yield
         end
 

spec/lib/gitlab/utils/measuring_spec.rb

@@ -3,16 +3,61 @@
 require 'fast_spec_helper'
 
 describe Gitlab::Utils::Measuring do
+  describe '#execute_with' do
+    let(:measurement_logger) { double(:logger) }
+    let(:measurement_enabled) { true }
+    let(:base_log_data) do
+      {
+        class: described_class.name
+      }
+    end
+    let(:result_block) { 'result' }
+
+    subject { described_class.execute_with(measurement_enabled, measurement_logger, base_log_data) { result_block } }
+
+    context 'when measurement is enabled' do
+      let!(:measurement) { described_class.new(logger: measurement_logger, base_log_data: base_log_data) }
+
+      before do
+        allow(measurement_logger).to receive(:info)
+      end
+
+      it 'measure execution with Gitlab::Utils::Measuring instance', :aggregate_failure do
+        expect(described_class).to receive(:new).with(logger: measurement_logger, base_log_data: base_log_data){ measurement }
+        expect(measurement).to receive(:with_measuring)
+
+        subject
+      end
+
+      it 'returns result from yielded block' do
+        is_expected.to eq(result_block)
+      end
+    end
+    context 'when measurement is disabled' do
+      let(:measurement_enabled) { false }
+
+      it 'does not measure service execution' do
+        expect(Gitlab::Utils::Measuring).not_to receive(:new)
+
+        subject
+      end
+
+      it 'returns result from yielded block' do
+        is_expected.to eq(result_block)
+      end
+    end
+  end
+
   describe '#with_measuring' do
     let(:logger) { double(:logger) }
-    let(:base_data) { {} }
+    let(:base_log_data) { {} }
     let(:result) { "result" }
 
     before do
       allow(logger).to receive(:info)
     end
 
-    let(:measurement) { described_class.new(logger: logger, base_data: base_data) }
+    let(:measurement) { described_class.new(logger: logger, base_log_data: base_log_data) }
 
     subject do
       measurement.with_measuring { result }
@@ -28,8 +73,8 @@ describe Gitlab::Utils::Measuring do
       is_expected.to eq(result)
     end
 
-    context 'with base_data provided' do
-      let(:base_data) { { test: "data" } }
+    context 'with base_log_data provided' do
+      let(:base_log_data) { { test: "data" } }
 
       it 'logs includes base data' do
         expect(logger).to receive(:info).with(including(:test, :gc_stats, :time_to_finish, :number_of_sql_calls, :memory_usage, :label))

spec/support/shared_examples/services/gitlab_projects_import_service_shared_examples.rb

@@ -55,15 +55,15 @@ RSpec.shared_examples 'gitlab projects import validations' do
   end
 
   context 'when measurable params are provided' do
-    let(:base_data) do
-      base_data = {
+    let(:base_log_data) do
+      base_log_data = {
         class: described_class.name,
         current_user: namespace.owner.name,
         project_full_path: "#{namespace.full_path}/#{path}"
       }
-      base_data.merge!({ import_type: 'gitlab_project', file_path: import_params[:file].path }) if import_params[:file]
+      base_log_data.merge!({ import_type: 'gitlab_project', file_path: import_params[:file].path }) if import_params[:file]
 
-      base_data
+      base_log_data
     end
 
     subject { described_class.new(namespace.owner, import_params) }
@@ -82,7 +82,7 @@ RSpec.shared_examples 'gitlab projects import validations' do
       end
 
       it 'measure service execution with Gitlab::Utils::Measuring' do
-        expect(Gitlab::Utils::Measuring).to receive(:execute_with).with(true, logger, base_data).and_call_original
+        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
@@ -98,7 +98,7 @@ RSpec.shared_examples 'gitlab projects import validations' do
       end
 
       it 'does not measure service execution' do
-        expect(Gitlab::Utils::Measuring).to receive(:execute_with).with(false, nil, base_data).and_call_original
+        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)
 
         subject.execute(measurable_options)