Refactor simplify rack metrics for elasticsearch/redis

Firstly we move the initialization of the metrics into initialize so
that they aren't invoked multiple times.

Secondly we remove the conditional logic which is avoiding adding
metrics when metrics are disabled. This is not necessary because these
[rack middleware are only inserted when metrics are
enabled](https://gitlab.com/gitlab-org/gitlab/-/blob/9c8f75e10b4dff4d3d13c6cf1ab75c0da252a352/config/initializers/zz_metrics.rb#L136)
and there will always be a transaction present as it is [unconditionally
created in
`Gitlab::Metrics::RackMiddleware`](https://gitlab.com/gitlab-org/gitlab/-/blob/9c8f75e10b4dff4d3d13c6cf1ab75c0da252a352/lib/gitlab/metrics/rack_middleware.rb#L13).

lib/gitlab/metrics/elasticsearch_rack_middleware.rb

@@ -8,6 +8,14 @@ module Gitlab
 
       def initialize(app)
         @app = app
+
+        @requests_total_counter = Gitlab::Metrics.counter(:http_elasticsearch_requests_total,
+                                                         'Amount of calls to Elasticsearch servers during web requests',
+                                                         Gitlab::Metrics::Transaction::BASE_LABELS)
+        @requests_duration_histogram = Gitlab::Metrics.histogram(:http_elasticsearch_requests_duration_seconds,
+                                                                 'Query time for Elasticsearch servers during web requests',
+                                                                 Gitlab::Metrics::Transaction::BASE_LABELS,
+                                                                 HISTOGRAM_BUCKETS)
       end
 
       def call(env)
@@ -15,7 +23,7 @@ module Gitlab
 
         @app.call(env)
       ensure
-        record_metrics(transaction) if transaction
+        record_metrics(transaction)
       end
 
       private
@@ -25,14 +33,8 @@ module Gitlab
         query_time = ::Gitlab::Instrumentation::ElasticsearchTransport.query_time
         request_count = ::Gitlab::Instrumentation::ElasticsearchTransport.get_request_count
 
-        Gitlab::Metrics.counter(:http_elasticsearch_requests_total,
-                                'Amount of calls to Elasticsearch servers during web requests',
-                                Gitlab::Metrics::Transaction::BASE_LABELS).increment(labels, request_count)
-
-        Gitlab::Metrics.histogram(:http_elasticsearch_requests_duration_seconds,
-                                  'Query time for Elasticsearch servers during web requests',
-                                  Gitlab::Metrics::Transaction::BASE_LABELS,
-                                  HISTOGRAM_BUCKETS).observe(labels, query_time)
+        @requests_total_counter.increment(labels, request_count)
+        @requests_duration_histogram.observe(labels, query_time)
       end
     end
   end

lib/gitlab/metrics/redis_rack_middleware.rb

@@ -6,6 +6,14 @@ module Gitlab
     class RedisRackMiddleware
       def initialize(app)
         @app = app
+
+        @requests_total_counter = Gitlab::Metrics.counter(:http_redis_requests_total,
+                                                          'Amount of calls to Redis servers during web requests',
+                                                          Gitlab::Metrics::Transaction::BASE_LABELS)
+        @requests_duration_histogram = Gitlab::Metrics.histogram(:http_redis_requests_duration_seconds,
+                                                                 'Query time for Redis servers during web requests',
+                                                                 Gitlab::Metrics::Transaction::BASE_LABELS,
+                                                                 Gitlab::Instrumentation::Redis::QUERY_TIME_BUCKETS)
       end
 
       def call(env)
@@ -13,7 +21,7 @@ module Gitlab
 
         @app.call(env)
       ensure
-        record_metrics(transaction) if transaction
+        record_metrics(transaction)
       end
 
       private
@@ -23,14 +31,8 @@ module Gitlab
         query_time = Gitlab::Instrumentation::Redis.query_time
         request_count = Gitlab::Instrumentation::Redis.get_request_count
 
-        Gitlab::Metrics.counter(:http_redis_requests_total,
-                                'Amount of calls to Redis servers during web requests',
-                                Gitlab::Metrics::Transaction::BASE_LABELS).increment(labels, request_count)
-
-        Gitlab::Metrics.histogram(:http_redis_requests_duration_seconds,
-                                  'Query time for Redis servers during web requests',
-                                  Gitlab::Metrics::Transaction::BASE_LABELS,
-                                  Gitlab::Instrumentation::Redis::QUERY_TIME_BUCKETS).observe(labels, query_time)
+        @requests_total_counter.increment(labels, request_count)
+        @requests_duration_histogram.observe(labels, query_time)
       end
     end
   end

spec/lib/gitlab/metrics/elasticsearch_rack_middleware_spec.rb

@@ -9,68 +9,49 @@ describe Gitlab::Metrics::ElasticsearchRackMiddleware do
   let(:transaction) { Gitlab::Metrics::WebTransaction.new(env) }
 
   describe '#call' do
-    context 'when metrics are disabled' do
-      before do
-        allow(Gitlab::Metrics).to receive(:current_transaction).and_return(nil)
-      end
-
-      it 'calls the app' do
-        expect(middleware.call(env)).to eq('app call result')
-      end
-
-      it 'does not record metrics' do
-        expect(Gitlab::Metrics).not_to receive(:counter)
-        expect(Gitlab::Metrics).not_to receive(:histogram)
-
-        middleware.call(env)
-      end
+    let(:counter) { instance_double(Prometheus::Client::Counter, increment: nil) }
+    let(:histogram) { instance_double(Prometheus::Client::Histogram, observe: nil) }
+    let(:elasticsearch_query_time) { 0.1 }
+    let(:elasticsearch_requests_count) { 2 }
+
+    before do
+      allow(Gitlab::Instrumentation::ElasticsearchTransport).to receive(:query_time) { elasticsearch_query_time }
+      allow(Gitlab::Instrumentation::ElasticsearchTransport).to receive(:get_request_count) { elasticsearch_requests_count }
+
+      allow(Gitlab::Metrics).to receive(:counter)
+        .with(:http_elasticsearch_requests_total,
+              an_instance_of(String),
+              Gitlab::Metrics::Transaction::BASE_LABELS)
+        .and_return(counter)
+
+      allow(Gitlab::Metrics).to receive(:histogram)
+        .with(:http_elasticsearch_requests_duration_seconds,
+              an_instance_of(String),
+              Gitlab::Metrics::Transaction::BASE_LABELS,
+              described_class::HISTOGRAM_BUCKETS)
+        .and_return(histogram)
+
+      allow(Gitlab::Metrics).to receive(:current_transaction).and_return(transaction)
     end
 
-    context 'when metrics are enabled' do
-      let(:counter) { instance_double(Prometheus::Client::Counter, increment: nil) }
-      let(:histogram) { instance_double(Prometheus::Client::Histogram, observe: nil) }
-      let(:elasticsearch_query_time) { 0.1 }
-      let(:elasticsearch_requests_count) { 2 }
-
-      before do
-        allow(Gitlab::Instrumentation::ElasticsearchTransport).to receive(:query_time) { elasticsearch_query_time }
-        allow(Gitlab::Instrumentation::ElasticsearchTransport).to receive(:get_request_count) { elasticsearch_requests_count }
-
-        allow(Gitlab::Metrics).to receive(:counter)
-          .with(:http_elasticsearch_requests_total,
-                an_instance_of(String),
-                Gitlab::Metrics::Transaction::BASE_LABELS)
-          .and_return(counter)
-
-        allow(Gitlab::Metrics).to receive(:histogram)
-          .with(:http_elasticsearch_requests_duration_seconds,
-                an_instance_of(String),
-                Gitlab::Metrics::Transaction::BASE_LABELS,
-                described_class::HISTOGRAM_BUCKETS)
-          .and_return(histogram)
-
-        allow(Gitlab::Metrics).to receive(:current_transaction).and_return(transaction)
-      end
-
-      it 'calls the app' do
-        expect(middleware.call(env)).to eq('app call result')
-      end
+    it 'calls the app' do
+      expect(middleware.call(env)).to eq('app call result')
+    end
 
-      it 'records elasticsearch metrics' do
-        expect(counter).to receive(:increment).with(transaction.labels, elasticsearch_requests_count)
-        expect(histogram).to receive(:observe).with(transaction.labels, elasticsearch_query_time)
+    it 'records elasticsearch metrics' do
+      expect(counter).to receive(:increment).with(transaction.labels, elasticsearch_requests_count)
+      expect(histogram).to receive(:observe).with(transaction.labels, elasticsearch_query_time)
 
-        middleware.call(env)
-      end
+      middleware.call(env)
+    end
 
-      it 'records elasticsearch metrics if an error is raised' do
-        expect(counter).to receive(:increment).with(transaction.labels, elasticsearch_requests_count)
-        expect(histogram).to receive(:observe).with(transaction.labels, elasticsearch_query_time)
+    it 'records elasticsearch metrics if an error is raised' do
+      expect(counter).to receive(:increment).with(transaction.labels, elasticsearch_requests_count)
+      expect(histogram).to receive(:observe).with(transaction.labels, elasticsearch_query_time)
 
-        allow(app).to receive(:call).with(env).and_raise(StandardError)
+      allow(app).to receive(:call).with(env).and_raise(StandardError)
 
-        expect { middleware.call(env) }.to raise_error(StandardError)
-      end
+      expect { middleware.call(env) }.to raise_error(StandardError)
     end
   end
 end

spec/lib/gitlab/metrics/redis_rack_middleware_spec.rb

@@ -13,68 +13,49 @@ describe Gitlab::Metrics::RedisRackMiddleware do
   end
 
   describe '#call' do
-    context 'when metrics are disabled' do
-      before do
-        allow(Gitlab::Metrics).to receive(:current_transaction).and_return(nil)
-      end
-
-      it 'calls the app' do
-        expect(middleware.call(env)).to eq('wub wub')
-      end
-
-      it 'does not record metrics' do
-        expect(Gitlab::Metrics).not_to receive(:counter)
-        expect(Gitlab::Metrics).not_to receive(:histogram)
-
-        middleware.call(env)
-      end
+    let(:counter) { double(Prometheus::Client::Counter, increment: nil) }
+    let(:histogram) { double(Prometheus::Client::Histogram, observe: nil) }
+    let(:redis_query_time) { 0.1 }
+    let(:redis_requests_count) { 2 }
+
+    before do
+      allow(Gitlab::Instrumentation::Redis).to receive(:query_time) { redis_query_time }
+      allow(Gitlab::Instrumentation::Redis).to receive(:get_request_count) { redis_requests_count }
+
+      allow(Gitlab::Metrics).to receive(:counter)
+        .with(:http_redis_requests_total,
+              an_instance_of(String),
+              Gitlab::Metrics::Transaction::BASE_LABELS)
+        .and_return(counter)
+
+      allow(Gitlab::Metrics).to receive(:histogram)
+        .with(:http_redis_requests_duration_seconds,
+              an_instance_of(String),
+              Gitlab::Metrics::Transaction::BASE_LABELS,
+              Gitlab::Instrumentation::Redis::QUERY_TIME_BUCKETS)
+        .and_return(histogram)
+
+      allow(Gitlab::Metrics).to receive(:current_transaction).and_return(transaction)
     end
 
-    context 'when metrics are enabled' do
-      let(:counter) { double(Prometheus::Client::Counter, increment: nil) }
-      let(:histogram) { double(Prometheus::Client::Histogram, observe: nil) }
-      let(:redis_query_time) { 0.1 }
-      let(:redis_requests_count) { 2 }
-
-      before do
-        allow(Gitlab::Instrumentation::Redis).to receive(:query_time) { redis_query_time }
-        allow(Gitlab::Instrumentation::Redis).to receive(:get_request_count) { redis_requests_count }
-
-        allow(Gitlab::Metrics).to receive(:counter)
-          .with(:http_redis_requests_total,
-                an_instance_of(String),
-                Gitlab::Metrics::Transaction::BASE_LABELS)
-          .and_return(counter)
-
-        allow(Gitlab::Metrics).to receive(:histogram)
-          .with(:http_redis_requests_duration_seconds,
-                an_instance_of(String),
-                Gitlab::Metrics::Transaction::BASE_LABELS,
-                Gitlab::Instrumentation::Redis::QUERY_TIME_BUCKETS)
-          .and_return(histogram)
-
-        allow(Gitlab::Metrics).to receive(:current_transaction).and_return(transaction)
-      end
-
-      it 'calls the app' do
-        expect(middleware.call(env)).to eq('wub wub')
-      end
+    it 'calls the app' do
+      expect(middleware.call(env)).to eq('wub wub')
+    end
 
-      it 'records redis metrics' do
-        expect(counter).to receive(:increment).with(transaction.labels, redis_requests_count)
-        expect(histogram).to receive(:observe).with(transaction.labels, redis_query_time)
+    it 'records redis metrics' do
+      expect(counter).to receive(:increment).with(transaction.labels, redis_requests_count)
+      expect(histogram).to receive(:observe).with(transaction.labels, redis_query_time)
 
-        middleware.call(env)
-      end
+      middleware.call(env)
+    end
 
-      it 'records redis metrics if an error is raised' do
-        expect(counter).to receive(:increment).with(transaction.labels, redis_requests_count)
-        expect(histogram).to receive(:observe).with(transaction.labels, redis_query_time)
+    it 'records redis metrics if an error is raised' do
+      expect(counter).to receive(:increment).with(transaction.labels, redis_requests_count)
+      expect(histogram).to receive(:observe).with(transaction.labels, redis_query_time)
 
-        allow(app).to receive(:call).with(env).and_raise(StandardError)
+      allow(app).to receive(:call).with(env).and_raise(StandardError)
 
-        expect { middleware.call(env) }.to raise_error(StandardError)
-      end
+      expect { middleware.call(env) }.to raise_error(StandardError)
     end
   end
 end