Addressed maintainer comments

- Clear base labels when label keys are set
- Fall back to original caller in evaluate method
- Revert metric_cache_operation_duration_seconds metric
- Rename FILTERED_LABELS to FILTERED_LABEL_KEYS

lib/gitlab/metrics/method_call.rb

@@ -34,10 +34,9 @@ module Gitlab
         @call_count += 1
 
         if above_threshold? && transaction
-          label_keys = labels.keys
           transaction.observe(:gitlab_method_call_duration_seconds, real_time, labels) do
             docstring 'Method calls real duration'
-            label_keys label_keys
+            label_keys labels.keys
             buckets [0.01, 0.05, 0.1, 0.5, 1]
             with_feature :prometheus_metrics_method_instrumentation
           end

lib/gitlab/metrics/methods/metric_options.rb

@@ -7,7 +7,7 @@ module Gitlab
         def initialize(options = {})
           @multiprocess_mode = options[:multiprocess_mode] || :all
           @buckets = options[:buckets] || ::Prometheus::Client::Histogram::DEFAULT_BUCKETS
-          @base_labels = options[:base_labels] || {}
+          @base_labels = options[:base_labels] || nil
           @docstring = options[:docstring]
           @with_feature = options[:with_feature]
           @label_keys = options[:label_keys] || []
@@ -40,23 +40,20 @@ module Gitlab
 
         # Base labels are merged with per metric labels
         def base_labels
-          seed_labels if @base_labels.empty?
+          @base_labels ||= @label_keys.product([nil]).to_h
 
           @base_labels
         end
 
         def label_keys(label_keys = nil)
-          @label_keys = label_keys unless label_keys.nil?
+          unless label_keys.nil?
+            @label_keys = label_keys
+            @base_labels = nil
+          end
 
           @label_keys
         end
 
-        def seed_labels
-          @label_keys.each do |key|
-            @base_labels[key] = nil
-          end
-        end
-
         # Use feature toggle to control whether certain metric is enabled/disabled
         def with_feature(name = nil)
           @with_feature = name unless name.nil?
@@ -65,9 +62,17 @@ module Gitlab
         end
 
         def evaluate(&block)
-          instance_eval(&block) if block_given?
+          if block_given?
+            @self_before_instance_eval = eval "self", block.binding, __FILE__, __LINE__
+            instance_eval(&block)
+          end
+
           self
         end
+
+        def method_missing(method, *args, &block)
+          @self_before_instance_eval ? @self_before_instance_eval.send(method, *args, &block) : super # rubocop:disable GitlabSecurity/PublicSend
+        end
       end
     end
   end

lib/gitlab/metrics/subscribers/rails_cache.rb

@@ -53,16 +53,13 @@ module Gitlab
           return unless current_transaction
 
           labels = { operation: key }
+
           current_transaction.increment(:gitlab_cache_operations_total, 1, labels) do
             docstring 'Cache operations'
             label_keys labels.keys
           end
 
-          current_transaction.observe(:gitlab_cache_operation_duration_seconds, duration / 1000.0, labels) do
-            docstring 'Cache access time'
-            buckets [0.00001, 0.0001, 0.001, 0.01, 0.1, 1.0]
-            label_keys labels.keys
-          end
+          metric_cache_operation_duration_seconds.observe(labels, duration / 1000.0)
         end
 
         private
@@ -70,6 +67,15 @@ module Gitlab
         def current_transaction
           Transaction.current
         end
+
+        def metric_cache_operation_duration_seconds
+          @metric_cache_operation_duration_seconds ||= ::Gitlab::Metrics.histogram(
+            :gitlab_cache_operation_duration_seconds,
+            'Cache access time',
+            {},
+            [0.00001, 0.0001, 0.001, 0.01, 0.1, 1.0]
+          )
+        end
       end
     end
   end

lib/gitlab/metrics/transaction.rb

@@ -9,7 +9,7 @@ module Gitlab
       # base label keys shared among all transactions
       BASE_LABEL_KEYS = %i(controller action feature_category).freeze
       # labels that potentially contain sensitive information and will be filtered
-      FILTERED_LABELS = %i(branch path).freeze
+      FILTERED_LABEL_KEYS = %i(branch path).freeze
 
       THREAD_KEY = :_gitlab_metrics_transaction
 
@@ -34,7 +34,7 @@ module Gitlab
 
             evaluate(&block)
             # always filter sensitive labels and merge with base ones
-            label_keys BASE_LABEL_KEYS | (label_keys - FILTERED_LABELS)
+            label_keys BASE_LABEL_KEYS | (label_keys - FILTERED_LABEL_KEYS)
           end
         end
       end
@@ -169,13 +169,11 @@ module Gitlab
       end
 
       def labels
-        BASE_LABEL_KEYS.each_with_object( {} ) do |key, hash|
-          hash[key] = nil
-        end
+        BASE_LABEL_KEYS.product([nil]).to_h
       end
 
       def filter_labels(labels)
-        labels.empty? ? self.labels : labels.without(*FILTERED_LABELS).merge(self.labels)
+        labels.empty? ? self.labels : labels.without(*FILTERED_LABEL_KEYS).merge(self.labels)
       end
     end
   end

spec/lib/gitlab/metrics/subscribers/rails_cache_spec.rb

@@ -156,9 +156,9 @@ RSpec.describe Gitlab::Metrics::Subscribers::RailsCache do
       end
 
       it 'observes cache metric' do
-        expect(transaction)
+        expect(subscriber.send(:metric_cache_operation_duration_seconds))
           .to receive(:observe)
-          .with(:gitlab_cache_operation_duration_seconds, event.duration / 1000.0, { operation: :delete })
+          .with({ operation: :delete }, event.duration / 1000.0)
 
         subscriber.observe(:delete, event.duration)
       end