Fix transaction labels

- Fix default bucket for histogram
- Add specs for WebTransaction
- Add specs for BackgroundTransaction
- Add changelog for inline metrics
- Fix db count metric names

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

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

lib/gitlab/database.rb

@@ -2,7 +2,6 @@
 
 module Gitlab
   module Database
-
     # Minimum PostgreSQL version requirement per documentation:
     # https://docs.gitlab.com/ee/install/requirements.html#postgresql-requirements
     MINIMUM_POSTGRES_VERSION = 11

lib/gitlab/metrics/subscribers/active_record.rb

@@ -53,7 +53,7 @@ module Gitlab
         end
 
         def increment(counter)
-          current_transaction.increment(counter, 1)
+          current_transaction.increment("gitlab_transaction_#{counter}_total".to_sym, 1)
 
           if Gitlab::SafeRequestStore.active?
             Gitlab::SafeRequestStore[counter] = Gitlab::SafeRequestStore[counter].to_i + 1

lib/gitlab/metrics/transaction.rb

@@ -31,7 +31,7 @@ module Gitlab
             # set default metric options
             docstring "#{name.to_s.humanize} #{type}"
             multiprocess_mode :livesum if type == :gauge
-            buckets SMALL_BUCKETS if type == :histogram
+            buckets ::Prometheus::Client::Histogram::DEFAULT_BUCKETS if type == :histogram
 
             evaluate(&block)
             # always filter sensitive labels and merge with base ones
@@ -74,8 +74,12 @@ module Gitlab
         @memory_after = System.memory_usage_rss
         @finished_at = System.monotonic_time
 
-        observe(:cputime_seconds, thread_cpu_duration)
-        observe(:duration_seconds, duration)
+        observe(:cputime_seconds, thread_cpu_duration) do
+          buckets SMALL_BUCKETS
+        end
+        observe(:duration_seconds, duration) do
+          buckets SMALL_BUCKETS
+        end
         observe(:allocated_memory_bytes, allocated_memory * 1024.0) do
           buckets BIG_BUCKETS
         end
@@ -126,7 +130,7 @@ module Gitlab
       def increment(name, value = 1, &block)
         counter = self.class.prometheus_metric(name, :counter, &block)
 
-        counter.increment(counter.base_labels, value)
+        counter.increment(counter.base_labels&.merge(labels), value)
       end
 
       # Set gauge metric
@@ -144,7 +148,7 @@ module Gitlab
       def set(name, value, &block)
         gauge = self.class.prometheus_metric(name, :gauge, &block)
 
-        gauge.set(gauge.base_labels, value)
+        gauge.set(gauge.base_labels&.merge(labels), value)
       end
 
       # Observe histogram metric
@@ -162,7 +166,7 @@ module Gitlab
       def observe(name, value, &block)
         histogram = self.class.prometheus_metric(name, :histogram, &block)
 
-        histogram.observe(histogram.base_labels, value)
+        histogram.observe(histogram.base_labels&.merge(labels), value)
       end
 
       def labels

spec/lib/gitlab/metrics/background_transaction_spec.rb

@@ -4,9 +4,23 @@ require 'spec_helper'
 
 RSpec.describe Gitlab::Metrics::BackgroundTransaction do
   let(:test_worker_class) { double(:class, name: 'TestWorker') }
+  let(:prometheus_metric) { instance_double(Prometheus::Client::Metric, base_labels: {}) }
+
+  before do
+    allow(described_class).to receive(:prometheus_metric).and_return(prometheus_metric)
+  end
 
   subject { described_class.new(test_worker_class) }
 
+  RSpec.shared_examples 'metric with worker labels' do |metric_method|
+    it 'measures with correct labels and value' do
+      value = 1
+      expect(prometheus_metric).to receive(metric_method).with({ controller: 'TestWorker', action: 'perform', feature_category: '' }, value)
+
+      subject.send(metric_method, :bau, value)
+    end
+  end
+
   describe '#label' do
     it 'returns labels based on class name' do
       expect(subject.labels).to eq(controller: 'TestWorker', action: 'perform', feature_category: '')
@@ -21,4 +35,22 @@ RSpec.describe Gitlab::Metrics::BackgroundTransaction do
       expect(subject.labels).to include(feature_category: 'source_code_management')
     end
   end
+
+  describe '#increment' do
+    let(:prometheus_metric) { instance_double(Prometheus::Client::Counter, :increment, base_labels: {}) }
+
+    it_behaves_like 'metric with worker labels', :increment
+  end
+
+  describe '#set' do
+    let(:prometheus_metric) { instance_double(Prometheus::Client::Gauge, :set, base_labels: {}) }
+
+    it_behaves_like 'metric with worker labels', :set
+  end
+
+  describe '#observe' do
+    let(:prometheus_metric) { instance_double(Prometheus::Client::Histogram, :observe, base_labels: {}) }
+
+    it_behaves_like 'metric with worker labels', :observe
+  end
 end

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

@@ -37,10 +37,11 @@ RSpec.describe Gitlab::Metrics::Subscribers::ActiveRecord do
 
         it 'increments only db count value' do
           described_class::DB_COUNTERS.each do |counter|
+            prometheus_counter = "gitlab_transaction_#{counter}_total".to_sym
             if expected_counters[counter] > 0
-              expect(transaction).to receive(:increment).with(counter, 1)
+              expect(transaction).to receive(:increment).with(prometheus_counter, 1)
             else
-              expect(transaction).not_to receive(:increment).with(counter, 1)
+              expect(transaction).not_to receive(:increment).with(prometheus_counter, 1)
             end
           end
 

spec/lib/gitlab/metrics/web_transaction_spec.rb

@@ -9,10 +9,38 @@ RSpec.describe Gitlab::Metrics::WebTransaction do
 
   before do
     allow(described_class).to receive(:prometheus_metric).and_return(prometheus_metric)
+  end
+
+  RSpec.shared_context 'ActionController request' do
+    let(:request) { double(:request, format: double(:format, ref: :html)) }
+    let(:controller_class) { double(:controller_class, name: 'TestController') }
+
+    before do
+      controller = double(:controller, class: controller_class, action_name: 'show', request: request)
+      env['action_controller.instance'] = controller
+    end
+  end
+
+  RSpec.shared_context 'transaction observe metrics' do
+    before do
       allow(transaction).to receive(:observe)
     end
+  end
+
+  RSpec.shared_examples 'metric with labels' do |metric_method|
+    include_context 'ActionController request'
+
+    it 'measures with correct labels and value' do
+      value = 1
+      expect(prometheus_metric).to receive(metric_method).with({ controller: 'TestController', action: 'show', feature_category: '' }, value)
+
+      transaction.send(metric_method, :bau, value)
+    end
+  end
 
   describe '#duration' do
+    include_context 'transaction observe metrics'
+
     it 'returns the duration of a transaction in seconds' do
       transaction.run { sleep(0.5) }
 
@@ -21,6 +49,8 @@ RSpec.describe Gitlab::Metrics::WebTransaction do
   end
 
   describe '#allocated_memory' do
+    include_context 'transaction observe metrics'
+
     it 'returns the allocated memory in bytes' do
       transaction.run { 'a' * 32 }
 
@@ -29,6 +59,8 @@ RSpec.describe Gitlab::Metrics::WebTransaction do
   end
 
   describe '#run' do
+    include_context 'transaction observe metrics'
+
     it 'yields the supplied block' do
       expect { |b| transaction.run(&b) }.to yield_control
     end
@@ -55,9 +87,6 @@ RSpec.describe Gitlab::Metrics::WebTransaction do
   end
 
   describe '#labels' do
-    let(:request) { double(:request, format: double(:format, ref: :html)) }
-    let(:controller_class) { double(:controller_class, name: 'TestController') }
-
     context 'when request goes to Grape endpoint' do
       before do
         route = double(:route, request_method: 'GET', path: '/:version/projects/:id/archive(.:format)')
@@ -85,11 +114,7 @@ RSpec.describe Gitlab::Metrics::WebTransaction do
     end
 
     context 'when request goes to ActionController' do
-      before do
-        controller = double(:controller, class: controller_class, action_name: 'show', request: request)
-
-        env['action_controller.instance'] = controller
-      end
+      include_context 'ActionController request'
 
       it 'tags a transaction with the name and action of a controller' do
         expect(transaction.labels).to eq({ controller: 'TestController', action: 'show', feature_category: '' })
@@ -143,4 +168,22 @@ RSpec.describe Gitlab::Metrics::WebTransaction do
       transaction.add_event(:bau, animal: 'dog')
     end
   end
+
+  describe '#increment' do
+    let(:prometheus_metric) { instance_double(Prometheus::Client::Counter, :increment, base_labels: {}) }
+
+    it_behaves_like 'metric with labels', :increment
+  end
+
+  describe '#set' do
+    let(:prometheus_metric) { instance_double(Prometheus::Client::Gauge, :set, base_labels: {}) }
+
+    it_behaves_like 'metric with labels', :set
+  end
+
+  describe '#observe' do
+    let(:prometheus_metric) { instance_double(Prometheus::Client::Histogram, :observe, base_labels: {}) }
+
+    it_behaves_like 'metric with labels', :observe
+  end
 end