Introduce label_keys as metric options

- Fix specs for metric options
- Add labels as a param to increment, set and observe

changelogs/unreleased/219029-refactor-metrics-tranasction.yml

 ---
-title: Unify Prometheus metric initialization by always using inline transaction metrics.
+title: Unify Prometheus metric initialization by always using inline transaction metrics
 merge_request: 32980
 author:
 type: other

lib/gitlab/metrics/method_call.rb

@@ -34,9 +34,10 @@ module Gitlab
         @call_count += 1
 
         if above_threshold? && transaction
-          transaction.observe(:gitlab_method_call_duration_seconds, real_time) do
+          label_keys = labels.keys
+          transaction.observe(:gitlab_method_call_duration_seconds, real_time, labels) do
             docstring 'Method calls real duration'
-            base_labels @labels
+            label_keys label_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

@@ -4,14 +4,13 @@ module Gitlab
   module Metrics
     module Methods
       class MetricOptions
-        SMALL_NETWORK_BUCKETS = [0.005, 0.01, 0.1, 1, 10].freeze
-
         def initialize(options = {})
           @multiprocess_mode = options[:multiprocess_mode] || :all
-          @buckets = options[:buckets] || SMALL_NETWORK_BUCKETS
+          @buckets = options[:buckets] || ::Prometheus::Client::Histogram::DEFAULT_BUCKETS
           @base_labels = options[:base_labels] || {}
           @docstring = options[:docstring]
           @with_feature = options[:with_feature]
+          @label_keys = options[:label_keys] || []
         end
 
         # Documentation describing metric in metrics endpoint '/-/metrics'
@@ -40,12 +39,24 @@ module Gitlab
         end
 
         # Base labels are merged with per metric labels
-        def base_labels(base_labels = nil)
-          @base_labels = base_labels unless base_labels.nil?
+        def base_labels
+          seed_labels if @base_labels.empty?
 
           @base_labels
         end
 
+        def label_keys(label_keys = nil)
+          @label_keys = label_keys unless label_keys.nil?
+
+          @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?

lib/gitlab/metrics/subscribers/action_view.rb

@@ -19,9 +19,9 @@ module Gitlab
 
         def track(event)
           tags = tags_for(event)
-          current_transaction.observe(:gitlab_view_rendering_duration_seconds, event.duration) do
+          current_transaction.observe(:gitlab_view_rendering_duration_seconds, event.duration, tags) do
             docstring 'View rendering time'
-            base_labels tags
+            label_keys %i(view)
             buckets [0.001, 0.01, 0.1, 1, 10.0]
             with_feature :prometheus_metrics_view_instrumentation
           end

lib/gitlab/metrics/subscribers/rails_cache.rb

@@ -53,15 +53,15 @@ module Gitlab
           return unless current_transaction
 
           labels = { operation: key }
-          current_transaction.increment(:gitlab_cache_operations_total, 1) do
+          current_transaction.increment(:gitlab_cache_operations_total, 1, labels) do
             docstring 'Cache operations'
-            base_labels labels
+            label_keys labels.keys
           end
 
-          current_transaction.observe(:gitlab_cache_operation_duration_seconds, duration / 1000.0) do
+          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]
-            base_labels labels
+            label_keys labels.keys
           end
         end
 

lib/gitlab/metrics/transaction.rb

@@ -6,10 +6,10 @@ module Gitlab
     class Transaction
       include Gitlab::Metrics::Methods
 
-      # base labels shared among all transactions
-      BASE_LABELS = { controller: nil, action: nil, feature_category: nil }.freeze
+      # 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 = [:branch, :path].freeze
+      FILTERED_LABELS = %i(branch path).freeze
 
       THREAD_KEY = :_gitlab_metrics_transaction
 
@@ -31,11 +31,10 @@ module Gitlab
             # set default metric options
             docstring "#{name.to_s.humanize} #{type}"
             multiprocess_mode :livesum if type == :gauge
-            buckets ::Prometheus::Client::Histogram::DEFAULT_BUCKETS if type == :histogram
 
             evaluate(&block)
             # always filter sensitive labels and merge with base ones
-            base_labels base_labels.without(*FILTERED_LABELS).merge(BASE_LABELS)
+            label_keys BASE_LABEL_KEYS | (label_keys - FILTERED_LABELS)
           end
         end
       end
@@ -97,10 +96,10 @@ module Gitlab
       def add_event(event_name, tags = {})
         event_name = "gitlab_transaction_event_#{event_name}_total".to_sym
         metric = self.class.prometheus_metric(event_name, :counter) do
-          base_labels tags
+          label_keys tags.keys
         end
 
-        metric.increment(tags.without(*FILTERED_LABELS).merge(labels))
+        metric.increment(filter_labels(tags))
       end
 
       # Returns a MethodCall object for the given name.
@@ -124,13 +123,13 @@ module Gitlab
       #
       # transaction.increment(:mestric_name, 1) do
       #   docstring 'Custom title'
-      #   base_labels { sane: 'yes' }
+      #   label_keys %i(sane)
       # end
       # ```
-      def increment(name, value = 1, &block)
+      def increment(name, value = 1, labels = {}, &block)
         counter = self.class.prometheus_metric(name, :counter, &block)
 
-        counter.increment(counter.base_labels&.merge(labels), value)
+        counter.increment(filter_labels(labels), value)
       end
 
       # Set gauge metric
@@ -145,10 +144,10 @@ module Gitlab
       #   multiprocess_mode :all
       # end
       # ```
-      def set(name, value, &block)
+      def set(name, value, labels = {}, &block)
         gauge = self.class.prometheus_metric(name, :gauge, &block)
 
-        gauge.set(gauge.base_labels&.merge(labels), value)
+        gauge.set(filter_labels(labels), value)
       end
 
       # Observe histogram metric
@@ -163,14 +162,20 @@ module Gitlab
       #   buckets [100, 1000, 10000, 100000, 1000000, 10000000]
       # end
       # ```
-      def observe(name, value, &block)
+      def observe(name, value, labels = {}, &block)
         histogram = self.class.prometheus_metric(name, :histogram, &block)
 
-        histogram.observe(histogram.base_labels&.merge(labels), value)
+        histogram.observe(filter_labels(labels), value)
       end
 
       def labels
-        BASE_LABELS
+        BASE_LABEL_KEYS.each_with_object( {} ) do |key, hash|
+          hash[key] = nil
+        end
+      end
+
+      def filter_labels(labels)
+        labels.empty? ? self.labels : labels.without(*FILTERED_LABELS).merge(self.labels)
       end
     end
   end

spec/lib/gitlab/metrics/background_transaction_spec.rb

@@ -27,7 +27,7 @@ RSpec.describe Gitlab::Metrics::BackgroundTransaction do
     end
 
     it 'contains only the labels defined for metrics' do
-      expect(subject.labels.keys).to contain_exactly(*described_class.superclass::BASE_LABELS.keys)
+      expect(subject.labels.keys).to contain_exactly(*described_class.superclass::BASE_LABEL_KEYS)
     end
 
     it 'includes the feature category if there is one' do

spec/lib/gitlab/metrics/method_call_spec.rb

@@ -42,7 +42,7 @@ RSpec.describe Gitlab::Metrics::MethodCall do
 
         it 'observes the performance of the supplied block' do
           expect(transaction)
-            .to receive(:observe).with(:gitlab_method_call_duration_seconds, be_a_kind_of(Numeric))
+            .to receive(:observe).with(:gitlab_method_call_duration_seconds, be_a_kind_of(Numeric), { method: "#bar", module: :Foo })
 
           method_call.measure { 'foo' }
         end
@@ -55,7 +55,7 @@ RSpec.describe Gitlab::Metrics::MethodCall do
 
         it 'observes the performance of the supplied block' do
           expect(transaction)
-            .to receive(:observe).with(:gitlab_method_call_duration_seconds, be_a_kind_of(Numeric))
+            .to receive(:observe).with(:gitlab_method_call_duration_seconds, be_a_kind_of(Numeric), { method: "#bar", module: :Foo })
 
           method_call.measure { 'foo' }
         end

spec/lib/gitlab/metrics/methods_spec.rb

@@ -135,5 +135,5 @@ RSpec.describe Gitlab::Metrics::Methods do
 
   include_examples 'metric', :counter, {}
   include_examples 'metric', :gauge, {}, :all
-  include_examples 'metric', :histogram, {}, [0.005, 0.01, 0.1, 1, 10]
+  include_examples 'metric', :histogram, {}, ::Prometheus::Client::Histogram::DEFAULT_BUCKETS
 end

spec/lib/gitlab/metrics/sidekiq_middleware_spec.rb

@@ -12,7 +12,7 @@ RSpec.describe Gitlab::Metrics::SidekiqMiddleware do
 
       expect_next_instance_of(Gitlab::Metrics::BackgroundTransaction) do |transaction|
         expect(transaction).to receive(:set).with(:sidekiq_queue_duration, instance_of(Float))
-        expect(transaction).to receive(:increment).with(:db_count, 1)
+        expect(transaction).to receive(:increment).with(:gitlab_transaction_db_count_total, 1)
       end
 
       middleware.call(worker, message, :test) do

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

@@ -30,7 +30,7 @@ RSpec.describe Gitlab::Metrics::Subscribers::ActionView do
     it 'observes view rendering time' do
       expect(transaction)
         .to receive(:observe)
-        .with(:gitlab_view_rendering_duration_seconds, 2.1)
+        .with(:gitlab_view_rendering_duration_seconds, 2.1, { view: "app/views/x.html.haml" })
 
       subscriber.render_template(event)
     end

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

@@ -158,7 +158,7 @@ RSpec.describe Gitlab::Metrics::Subscribers::RailsCache do
       it 'observes cache metric' do
         expect(transaction)
           .to receive(:observe)
-          .with(:gitlab_cache_operation_duration_seconds, event.duration / 1000.0)
+          .with(:gitlab_cache_operation_duration_seconds, event.duration / 1000.0, { operation: :delete })
 
         subscriber.observe(:delete, event.duration)
       end
@@ -166,7 +166,7 @@ RSpec.describe Gitlab::Metrics::Subscribers::RailsCache do
       it 'increments the operations total' do
         expect(transaction)
           .to receive(:increment)
-          .with(:gitlab_cache_operations_total, 1)
+          .with(:gitlab_cache_operations_total, 1, { operation: :delete })
 
         subscriber.observe(:delete, event.duration)
       end

spec/lib/gitlab/metrics/transaction_spec.rb

@@ -116,17 +116,17 @@ RSpec.describe Gitlab::Metrics::Transaction do
         expect(::Gitlab::Metrics).to receive(:counter).with(:block_labels, 'Block labels counter', hash_including(:controller, :action, :sane)).and_return(prometheus_metric)
 
         labels = { sane: 'yes' }
-        transaction.increment(:block_labels, 1) do
-          base_labels labels
+        transaction.increment(:block_labels, 1, labels) do
+          label_keys %i(sane)
         end
       end
 
       it 'filters sensitive tags' do
         expect(::Gitlab::Metrics).to receive(:counter).with(:metric_with_sensitive_block, 'Metric with sensitive block counter', hash_excluding(sensitive_tags)).and_return(prometheus_metric)
 
-        labels = sensitive_tags
-        transaction.increment(:metric_with_sensitive_block, 1) do
-          base_labels labels
+        labels_keys = sensitive_tags.keys
+        transaction.increment(:metric_with_sensitive_block, 1, sensitive_tags) do
+          label_keys labels_keys
         end
       end
     end
@@ -155,17 +155,17 @@ RSpec.describe Gitlab::Metrics::Transaction do
         expect(::Gitlab::Metrics).to receive(:gauge).with(:block_labels_set, 'Block labels set gauge', hash_including(:controller, :action, :sane), :livesum).and_return(prometheus_metric)
 
         labels = { sane: 'yes' }
-        transaction.set(:block_labels_set, 1) do
-          base_labels labels
+        transaction.set(:block_labels_set, 1, labels) do
+          label_keys %i(sane)
         end
       end
 
       it 'filters sensitive tags' do
         expect(::Gitlab::Metrics).to receive(:gauge).with(:metric_set_with_sensitive_block, 'Metric set with sensitive block gauge', hash_excluding(sensitive_tags), :livesum).and_return(prometheus_metric)
 
-        labels = sensitive_tags
-        transaction.set(:metric_set_with_sensitive_block, 1) do
-          base_labels labels
+        label_keys = sensitive_tags.keys
+        transaction.set(:metric_set_with_sensitive_block, 1, sensitive_tags) do
+          label_keys label_keys
         end
       end
     end
@@ -194,17 +194,17 @@ RSpec.describe Gitlab::Metrics::Transaction do
         expect(::Gitlab::Metrics).to receive(:histogram).with(:block_labels_observe, 'Block labels observe histogram', hash_including(:controller, :action, :sane), kind_of(Array)).and_return(prometheus_metric)
 
         labels = { sane: 'yes' }
-        transaction.observe(:block_labels_observe, 1) do
-          base_labels labels
+        transaction.observe(:block_labels_observe, 1, labels) do
+          label_keys %i(sane)
         end
       end
 
       it 'filters sensitive tags' do
         expect(::Gitlab::Metrics).to receive(:histogram).with(:metric_observe_with_sensitive_block, 'Metric observe with sensitive block histogram', hash_excluding(sensitive_tags), kind_of(Array)).and_return(prometheus_metric)
 
-        labels = sensitive_tags
-        transaction.observe(:metric_observe_with_sensitive_block, 1) do
-          base_labels labels
+        label_keys = sensitive_tags.keys
+        transaction.observe(:metric_observe_with_sensitive_block, 1, sensitive_tags) do
+          label_keys label_keys
         end
       end
     end

spec/lib/gitlab/metrics/web_transaction_spec.rb

@@ -100,7 +100,7 @@ RSpec.describe Gitlab::Metrics::WebTransaction do
       end
 
       it 'contains only the labels defined for transactions' do
-        expect(transaction.labels.keys).to contain_exactly(*described_class.superclass::BASE_LABELS.keys)
+        expect(transaction.labels.keys).to contain_exactly(*described_class.superclass::BASE_LABEL_KEYS)
       end
 
       it 'does not provide labels if route infos are missing' do
@@ -121,7 +121,7 @@ RSpec.describe Gitlab::Metrics::WebTransaction do
       end
 
       it 'contains only the labels defined for transactions' do
-        expect(transaction.labels.keys).to contain_exactly(*described_class.superclass::BASE_LABELS.keys)
+        expect(transaction.labels.keys).to contain_exactly(*described_class.superclass::BASE_LABEL_KEYS)
       end
 
       context 'when the request content type is not :html' do