Merge branch 'remove-health-endpoints-from-rack-metrics' into 'master'

Exclude health checks from http_request_duration_seconds bucket

See merge request gitlab-org/gitlab!43409

lib/gitlab/metrics/requests_rack_middleware.rb

@@ -48,9 +48,10 @@ module Gitlab
         method = env['REQUEST_METHOD'].downcase
         method = 'INVALID' unless HTTP_METHODS.key?(method)
         started = Time.now.to_f
+        health_endpoint = health_endpoint?(env['PATH_INFO'])
 
         begin
-          if health_endpoint?(env['PATH_INFO'])
+          if health_endpoint
             RequestsRackMiddleware.http_health_requests_total.increment(method: method)
           else
             RequestsRackMiddleware.http_request_total.increment(method: method)
@@ -59,7 +60,10 @@ module Gitlab
           status, headers, body = @app.call(env)
 
           elapsed = Time.now.to_f - started
+
+          unless health_endpoint
             RequestsRackMiddleware.http_request_duration_seconds.observe({ method: method, status: status.to_s }, elapsed)
+          end
 
           [status, headers, body]
         rescue

spec/lib/gitlab/metrics/requests_rack_middleware_spec.rb

@@ -38,73 +38,53 @@ RSpec.describe Gitlab::Metrics::RequestsRackMiddleware do
       end
 
       context 'request is a health check endpoint' do
-        it 'increments health endpoint counter' do
-          env['PATH_INFO'] = '/-/liveness'
-
-          expect(described_class).to receive_message_chain(:http_health_requests_total, :increment).with(method: 'get')
-
-          subject.call(env)
-        end
-
-        context 'with trailing slash' do
+        ['/-/liveness', '/-/liveness/', '/-/%6D%65%74%72%69%63%73'].each do |path|
+          context "when path is #{path}" do
             before do
-            env['PATH_INFO'] = '/-/liveness/'
+              env['PATH_INFO'] = path
             end
 
-          it 'increments health endpoint counter' do
+            it 'increments health endpoint counter rather than overall counter' do
               expect(described_class).to receive_message_chain(:http_health_requests_total, :increment).with(method: 'get')
+              expect(described_class).not_to receive(:http_request_total)
 
               subject.call(env)
             end
-        end
 
-        context 'with percent encoded values' do
-          before do
-            env['PATH_INFO'] = '/-/%6D%65%74%72%69%63%73' # /-/metrics
-          end
-
-          it 'increments health endpoint counter' do
-            expect(described_class).to receive_message_chain(:http_health_requests_total, :increment).with(method: 'get')
+            it 'does not record the request duration' do
+              expect(described_class).not_to receive(:http_request_duration_seconds)
 
               subject.call(env)
             end
           end
         end
-
-      context 'request is not a health check endpoint' do
-        it 'does not increment health endpoint counter' do
-          env['PATH_INFO'] = '/-/ordinary-requests'
-
-          expect(described_class).not_to receive(:http_health_requests_total)
-
-          subject.call(env)
       end
 
-        context 'path info is a root path' do
+      context 'request is not a health check endpoint' do
+        ['/-/ordinary-requests', '/-/', '/-/health/subpath'].each do |path|
+          context "when path is #{path}" do
             before do
-            env['PATH_INFO'] = '/-/'
+              env['PATH_INFO'] = path
             end
 
-          it 'does not increment health endpoint counter' do
+            it 'increments overall counter rather than health endpoint counter' do
+              expect(described_class).to receive_message_chain(:http_request_total, :increment).with(method: 'get')
               expect(described_class).not_to receive(:http_health_requests_total)
 
               subject.call(env)
             end
-        end
-
-        context 'path info is a subpath' do
-          before do
-            env['PATH_INFO'] = '/-/health/subpath'
-          end
 
-          it 'does not increment health endpoint counter' do
-            expect(described_class).not_to receive(:http_health_requests_total)
+            it 'records the request duration' do
+              expect(described_class)
+                .to receive_message_chain(:http_request_duration_seconds, :observe)
+                      .with({ method: 'get', status: '200' }, a_positive_execution_time)
 
               subject.call(env)
             end
           end
         end
       end
+    end
 
     context '@app.call throws exception' do
       let(:http_request_duration_seconds) { double('http_request_duration_seconds') }