Merge branch '348942-metrics-server-http-metrics' into 'master'

Export HTTP metrics for metrics-server

See merge request gitlab-org/gitlab!77114

lib/gitlab/metrics/exporter/base_exporter.rb

@@ -39,13 +39,8 @@ module Gitlab
 
           @server = ::WEBrick::HTTPServer.new(
             Port: settings.port, BindAddress: settings.address,
-            Logger: logger, AccessLog: access_log)
-          server.mount_proc '/readiness' do |req, res|
-            render_probe(readiness_probe, req, res)
-          end
-          server.mount_proc '/liveness' do |req, res|
-            render_probe(liveness_probe, req, res)
-          end
+            Logger: logger, AccessLog: access_log
+          )
           server.mount '/', Rack::Handler::WEBrick, rack_app
 
           true
@@ -72,8 +67,14 @@ module Gitlab
         end
 
         def rack_app
+          readiness = readiness_probe
+          liveness = liveness_probe
+          pid = thread_name
+
           Rack::Builder.app do
             use Rack::Deflater
+            use Gitlab::Metrics::Exporter::MetricsMiddleware, pid
+            use Gitlab::Metrics::Exporter::HealthChecksMiddleware, readiness, liveness
             use ::Prometheus::Client::Rack::Exporter if ::Gitlab::Metrics.metrics_folder_present?
             run -> (env) { [404, {}, ['']] }
           end
@@ -86,14 +87,6 @@ module Gitlab
         def liveness_probe
           ::Gitlab::HealthChecks::Probes::Collection.new
         end
-
-        def render_probe(probe, req, res)
-          result = probe.execute
-
-          res.status = result.http_status
-          res.content_type = 'application/json; charset=utf-8'
-          res.body = result.json.to_json
-        end
       end
     end
   end

lib/gitlab/metrics/exporter/health_checks_middleware.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Metrics
+    module Exporter
+      class HealthChecksMiddleware
+        def initialize(app, readiness_probe, liveness_probe)
+          @app = app
+          @readiness_probe = readiness_probe
+          @liveness_probe = liveness_probe
+        end
+
+        def call(env)
+          case env['PATH_INFO']
+          when '/readiness' then render_probe(@readiness_probe)
+          when '/liveness' then render_probe(@liveness_probe)
+          else @app.call(env)
+          end
+        end
+
+        private
+
+        def render_probe(probe)
+          result = probe.execute
+
+          [
+            result.http_status,
+            { 'Content-Type' => 'application/json; charset=utf-8' },
+            [result.json.to_json]
+          ]
+        end
+      end
+    end
+  end
+end

lib/gitlab/metrics/exporter/metrics_middleware.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Metrics
+    module Exporter
+      class MetricsMiddleware
+        def initialize(app, pid)
+          @app = app
+          default_labels = {
+            pid: pid
+          }
+          @requests_total = Gitlab::Metrics.counter(
+            :exporter_http_requests_total, 'Total number of HTTP requests', default_labels
+          )
+          @request_durations = Gitlab::Metrics.histogram(
+            :exporter_http_request_duration_seconds,
+            'HTTP request duration histogram (seconds)',
+            default_labels,
+            [0.025, 0.05, 0.1, 0.25, 0.5, 1, 2.5, 5, 10]
+          )
+        end
+
+        def call(env)
+          start = Gitlab::Metrics::System.monotonic_time
+          @app.call(env).tap do |response|
+            duration = Gitlab::Metrics::System.monotonic_time - start
+
+            labels = {
+              method: env['REQUEST_METHOD'].downcase,
+              path:   env['PATH_INFO'].to_s,
+              code:   response.first.to_s
+            }
+
+            @requests_total.increment(labels)
+            @request_durations.observe(labels, duration)
+          end
+        end
+      end
+    end
+  end
+end

metrics_server/dependencies.rb

@@ -24,6 +24,8 @@ require_relative '../lib/gitlab/metrics/samplers/base_sampler'
 require_relative '../lib/gitlab/metrics/samplers/ruby_sampler'
 require_relative '../lib/gitlab/metrics/exporter/base_exporter'
 require_relative '../lib/gitlab/metrics/exporter/sidekiq_exporter'
+require_relative '../lib/gitlab/metrics/exporter/metrics_middleware'
+require_relative '../lib/gitlab/metrics/exporter/health_checks_middleware'
 require_relative '../lib/gitlab/health_checks/probes/collection'
 require_relative '../lib/gitlab/health_checks/probes/status'
 require_relative '../lib/gitlab/process_management'

spec/commands/metrics_server/metrics_server_spec.rb

@@ -23,6 +23,8 @@ RSpec.describe 'bin/metrics-server', :aggregate_failures do
   end
 
   context 'with a running server' do
+    let(:metrics_dir) { Dir.mktmpdir }
+
     before do
       # We need to send a request to localhost
       WebMock.allow_net_connect!
@@ -33,7 +35,8 @@ RSpec.describe 'bin/metrics-server', :aggregate_failures do
       env = {
         'GITLAB_CONFIG' => config_file.path,
         'METRICS_SERVER_TARGET' => 'sidekiq',
-        'WIPE_METRICS_DIR' => '1'
+        'WIPE_METRICS_DIR' => '1',
+        'prometheus_multiproc_dir' => metrics_dir
       }
       @pid = Process.spawn(env, 'bin/metrics-server', pgroup: true)
     end
@@ -55,6 +58,7 @@ RSpec.describe 'bin/metrics-server', :aggregate_failures do
       # 'No such process' means the process died before
     ensure
       config_file.unlink
+      FileUtils.rm_rf(metrics_dir, secure: true)
     end
 
     it 'serves /metrics endpoint' do

spec/lib/gitlab/metrics/exporter/base_exporter_spec.rb

@@ -111,6 +111,18 @@ RSpec.describe Gitlab::Metrics::Exporter::BaseExporter do
   describe 'request handling' do
     using RSpec::Parameterized::TableSyntax
 
+    let(:fake_collector) do
+      Class.new do
+        def initialize(app, ...)
+          @app = app
+        end
+
+        def call(env)
+          @app.call(env)
+        end
+      end
+    end
+
     where(:method_class, :path, :http_status) do
       Net::HTTP::Get | '/metrics' | 200
       Net::HTTP::Get | '/liveness' | 200
@@ -123,6 +135,8 @@ RSpec.describe Gitlab::Metrics::Exporter::BaseExporter do
       allow(settings).to receive(:port).and_return(0)
       allow(settings).to receive(:address).and_return('127.0.0.1')
 
+      stub_const('Gitlab::Metrics::Exporter::MetricsMiddleware', fake_collector)
+
       # We want to wrap original method
       # and run handling of requests
       # in separate thread
@@ -134,8 +148,6 @@ RSpec.describe Gitlab::Metrics::Exporter::BaseExporter do
           # is raised as we close listeners
         end
       end
-
-      exporter.start.join
     end
 
     after do
@@ -146,12 +158,25 @@ RSpec.describe Gitlab::Metrics::Exporter::BaseExporter do
       let(:config) { exporter.server.config }
       let(:request) { method_class.new(path) }
 
-      it 'responds with proper http_status' do
+      subject(:response) do
         http = Net::HTTP.new(config[:BindAddress], config[:Port])
-        response = http.request(request)
+        http.request(request)
+      end
+
+      it 'responds with proper http_status' do
+        exporter.start.join
 
         expect(response.code).to eq(http_status.to_s)
       end
+
+      it 'collects request metrics' do
+        expect_next_instance_of(fake_collector) do |instance|
+          expect(instance).to receive(:call).and_call_original
+        end
+
+        exporter.start.join
+        response
+      end
     end
   end
 

spec/lib/gitlab/metrics/exporter/health_checks_middleware_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+
+RSpec.describe Gitlab::Metrics::Exporter::HealthChecksMiddleware do
+  let(:app) { double(:app) }
+  let(:env) { { 'PATH_INFO' => path } }
+
+  let(:readiness_probe) { double(:readiness_probe) }
+  let(:liveness_probe) { double(:liveness_probe) }
+  let(:probe_result) { Gitlab::HealthChecks::Probes::Status.new(200, { status: 'ok' }) }
+
+  subject(:middleware) { described_class.new(app, readiness_probe, liveness_probe) }
+
+  describe '#call' do
+    context 'handling /readiness requests' do
+      let(:path) { '/readiness' }
+
+      it 'handles the request' do
+        expect(readiness_probe).to receive(:execute).and_return(probe_result)
+
+        response = middleware.call(env)
+
+        expect(response).to eq([200, { 'Content-Type' => 'application/json; charset=utf-8' }, ['{"status":"ok"}']])
+      end
+    end
+
+    context 'handling /liveness requests' do
+      let(:path) { '/liveness' }
+
+      it 'handles the request' do
+        expect(liveness_probe).to receive(:execute).and_return(probe_result)
+
+        response = middleware.call(env)
+
+        expect(response).to eq([200, { 'Content-Type' => 'application/json; charset=utf-8' }, ['{"status":"ok"}']])
+      end
+    end
+
+    context 'handling other requests' do
+      let(:path) { '/other_path' }
+
+      it 'forwards them to the next middleware' do
+        expect(app).to receive(:call).with(env).and_return([201, {}, []])
+
+        response = middleware.call(env)
+
+        expect(response).to eq([201, {}, []])
+      end
+    end
+  end
+end

spec/lib/gitlab/metrics/exporter/metrics_middleware_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Metrics::Exporter::MetricsMiddleware do
+  let(:app) { double(:app) }
+  let(:pid) { 'fake_exporter' }
+  let(:env) { { 'PATH_INFO' => '/path', 'REQUEST_METHOD' => 'GET' } }
+
+  subject(:middleware) { described_class.new(app, pid) }
+
+  def metric(name, method, path, status)
+    metric = ::Prometheus::Client.registry.get(name)
+    return unless metric
+
+    values = metric.values.transform_keys { |k| k.slice(:method, :path, :pid, :code) }
+    values[{ method: method, path: path, pid: pid, code: status.to_s }]&.get
+  end
+
+  before do
+    expect(app).to receive(:call).with(env).and_return([200, {}, []])
+  end
+
+  describe '#call', :prometheus do
+    it 'records a total requests metric' do
+      response = middleware.call(env)
+
+      expect(response).to eq([200, {}, []])
+      expect(metric(:exporter_http_requests_total, 'get', '/path', 200)).to eq(1.0)
+    end
+
+    it 'records a request duration histogram' do
+      response = middleware.call(env)
+
+      expect(response).to eq([200, {}, []])
+      expect(metric(:exporter_http_request_duration_seconds, 'get', '/path', 200)).to be_a(Hash)
+    end
+  end
+end

spec/lib/gitlab/metrics/exporter/web_exporter_spec.rb

@@ -24,14 +24,14 @@ RSpec.describe Gitlab::Metrics::Exporter::WebExporter do
     exporter.stop
   end
 
-  context 'when running server' do
+  context 'when running server', :prometheus do
     it 'readiness probe returns succesful status' do
       expect(readiness_probe.http_status).to eq(200)
       expect(readiness_probe.json).to include(status: 'ok')
       expect(readiness_probe.json).to include('web_exporter' => [{ 'status': 'ok' }])
     end
 
-    it 'initializes request metrics', :prometheus do
+    it 'initializes request metrics' do
       expect(Gitlab::Metrics::RailsSlis).to receive(:initialize_request_slis_if_needed!).and_call_original
 
       http = Net::HTTP.new(exporter.server.config[:BindAddress], exporter.server.config[:Port])
@@ -42,7 +42,7 @@ RSpec.describe Gitlab::Metrics::Exporter::WebExporter do
   end
 
   describe '#mark_as_not_running!' do
-    it 'readiness probe returns a failure status' do
+    it 'readiness probe returns a failure status', :prometheus do
       exporter.mark_as_not_running!
 
       expect(readiness_probe.http_status).to eq(503)