Allow metrics_server to target Puma

So far we had only tested this with Sidekiq.
This change makes sure that WebExporter and
the puma metrics directory can be used.

This is a prerequisite for extracting WebExporter
into a separate process.

lib/gitlab/daemon.rb

@@ -20,6 +20,8 @@ module Gitlab
       !thread.nil?
     end
 
+    # Possible options:
+    # - synchronous [Boolean] if true, turns `start` into a blocking call
     def initialize(**options)
       @synchronous = options[:synchronous]
       @mutex = Mutex.new

lib/gitlab/metrics/exporter/base_exporter.rb

@@ -9,6 +9,10 @@ module Gitlab
       class BaseExporter < Daemon
         attr_reader :server
 
+        # @param settings [Hash] SettingsLogic hash containing the `*_exporter` config
+        # @param log_enabled [Boolean] whether to log HTTP requests
+        # @param log_file [String] path to where the server log should be located
+        # @param gc_requests [Boolean] whether to run a major GC after each scraper request
         def initialize(settings, log_enabled:, log_file:, gc_requests: false, **options)
           super(**options)
 

metrics_server/dependencies.rb

@@ -23,6 +23,7 @@ require_relative '../lib/gitlab/metrics/system'
 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/web_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'

metrics_server/metrics_server.rb

@@ -7,7 +7,7 @@ require_relative 'dependencies'
 class MetricsServer # rubocop:disable Gitlab/NamespacedClass
   class << self
     def spawn(target, metrics_dir:, wipe_metrics_dir: false, trapped_signals: [])
-      raise "The only valid target is 'sidekiq' currently" unless target == 'sidekiq'
+      raise "Target must be one of [puma,sidekiq]" unless %w(puma sidekiq).include?(target)
 
       pid = Process.fork
 
@@ -52,11 +52,18 @@ class MetricsServer # rubocop:disable Gitlab/NamespacedClass
     # Warming up ensures that these files exist prior to the exporter starting up.
     Gitlab::Metrics::Samplers::RubySampler.initialize_instance(prefix: name, warmup: true).start
 
-    exporter_class = "Gitlab::Metrics::Exporter::#{@target.camelize}Exporter".constantize
-    settings = Settings.new(Settings.monitoring[name])
-    server = exporter_class.instance(settings, gc_requests: true, synchronous: true)
+    default_opts = { gc_requests: true, synchronous: true }
+    exporter =
+      case @target
+      when 'puma'
+        Gitlab::Metrics::Exporter::WebExporter.instance(**default_opts)
+      else
+        exporter_class = "Gitlab::Metrics::Exporter::#{@target.camelize}Exporter".constantize
+        settings = Settings.new(Settings.monitoring[name])
+        exporter_class.instance(settings, **default_opts)
+      end
 
-    server.start
+    exporter.start
   end
 
   def name

spec/commands/metrics_server/metrics_server_spec.rb

@@ -12,6 +12,11 @@ RSpec.describe 'bin/metrics-server', :aggregate_failures do
     {
       'test' => {
         'monitoring' => {
+          'web_exporter' => {
+            'address' => 'localhost',
+            'enabled' => true,
+            'port' => 3807
+          },
           'sidekiq_exporter' => {
             'address' => 'localhost',
             'enabled' => true,
@@ -22,56 +27,58 @@ RSpec.describe 'bin/metrics-server', :aggregate_failures do
     }
   end
 
-  context 'with a running server' do
-    let(:metrics_dir) { Dir.mktmpdir }
+  %w(puma sidekiq).each do |target|
+    context "with a running server targeting #{target}" do
+      let(:metrics_dir) { Dir.mktmpdir }
 
-    before do
-      # We need to send a request to localhost
-      WebMock.allow_net_connect!
+      before do
+        # We need to send a request to localhost
+        WebMock.allow_net_connect!
 
-      config_file.write(YAML.dump(config))
-      config_file.close
+        config_file.write(YAML.dump(config))
+        config_file.close
 
-      env = {
-        'GITLAB_CONFIG' => config_file.path,
-        'METRICS_SERVER_TARGET' => 'sidekiq',
-        'WIPE_METRICS_DIR' => '1',
-        'prometheus_multiproc_dir' => metrics_dir
-      }
-      @pid = Process.spawn(env, 'bin/metrics-server', pgroup: true)
-    end
+        env = {
+          'GITLAB_CONFIG' => config_file.path,
+          'METRICS_SERVER_TARGET' => target,
+          'WIPE_METRICS_DIR' => '1',
+          'prometheus_multiproc_dir' => metrics_dir
+        }
+        @pid = Process.spawn(env, 'bin/metrics-server', pgroup: true)
+      end
 
-    after do
-      webmock_enable!
+      after do
+        webmock_enable!
 
-      if @pid
-        pgrp = Process.getpgid(@pid)
+        if @pid
+          pgrp = Process.getpgid(@pid)
 
-        Timeout.timeout(10) do
-          Process.kill('TERM', -pgrp)
-          Process.waitpid(@pid)
-        end
+          Timeout.timeout(10) do
+            Process.kill('TERM', -pgrp)
+            Process.waitpid(@pid)
+          end
 
-        expect(Gitlab::ProcessManagement.process_alive?(@pid)).to be(false)
+          expect(Gitlab::ProcessManagement.process_alive?(@pid)).to be(false)
+        end
+      rescue Errno::ESRCH => _
+        # 'No such process' means the process died before
+      ensure
+        config_file.unlink
+        FileUtils.rm_rf(metrics_dir, secure: true)
       end
-    rescue Errno::ESRCH => _
-      # '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
-      expect do
-        Timeout.timeout(10) do
-          http_ok = false
-          until http_ok
-            sleep 1
-            response = Gitlab::HTTP.try_get("http://localhost:3807/metrics", allow_local_requests: true)
-            http_ok = response&.success?
+      it 'serves /metrics endpoint' do
+        expect do
+          Timeout.timeout(10) do
+            http_ok = false
+            until http_ok
+              sleep 1
+              response = Gitlab::HTTP.try_get("http://localhost:3807/metrics", allow_local_requests: true)
+              http_ok = response&.success?
+            end
           end
-        end
-      end.not_to raise_error
+        end.not_to raise_error
+      end
     end
   end
 end

spec/metrics_server/metrics_server_spec.rb

@@ -15,9 +15,16 @@ RSpec.describe MetricsServer do # rubocop:disable RSpec/FilePath
   # we need to reset it after testing.
   let!(:old_multiprocess_files_dir) { prometheus_config.multiprocess_files_dir }
 
+  let(:ruby_sampler_double) { double(Gitlab::Metrics::Samplers::RubySampler) }
+
   before do
     # We do not want this to have knock-on effects on the test process.
     allow(Gitlab::ProcessManagement).to receive(:modify_signals)
+
+    # This being a singleton, we stub it out because only one instance is allowed
+    # to exist per process.
+    allow(Gitlab::Metrics::Samplers::RubySampler).to receive(:initialize_instance).and_return(ruby_sampler_double)
+    allow(ruby_sampler_double).to receive(:start)
   end
 
   after do
@@ -27,35 +34,49 @@ RSpec.describe MetricsServer do # rubocop:disable RSpec/FilePath
     FileUtils.rm_rf(metrics_dir, secure: true)
   end
 
-  describe '.spawn' do
-    context 'when in parent process' do
-      it 'forks into a new process and detaches it' do
-        expect(Process).to receive(:fork).and_return(99)
-        expect(Process).to receive(:detach).with(99)
+  %w(puma sidekiq).each do |target|
+    context "when targeting #{target}" do
+      describe '.spawn' do
+        context 'when in parent process' do
+          it 'forks into a new process and detaches it' do
+            expect(Process).to receive(:fork).and_return(99)
+            expect(Process).to receive(:detach).with(99)
 
-        described_class.spawn('sidekiq', metrics_dir: metrics_dir)
-      end
-    end
+            described_class.spawn(target, metrics_dir: metrics_dir)
+          end
+        end
 
-    context 'when in child process' do
-      before do
-        # This signals the process that it's "inside" the fork
-        expect(Process).to receive(:fork).and_return(nil)
-        expect(Process).not_to receive(:detach)
-      end
+        context 'when in child process' do
+          before do
+            # This signals the process that it's "inside" the fork
+            expect(Process).to receive(:fork).and_return(nil)
+            expect(Process).not_to receive(:detach)
+          end
 
-      it 'starts the metrics server with the given arguments' do
-        expect_next_instance_of(MetricsServer) do |server|
-          expect(server).to receive(:start)
-        end
+          it 'starts the metrics server with the given arguments' do
+            expect_next_instance_of(MetricsServer) do |server|
+              expect(server).to receive(:start)
+            end
 
-        described_class.spawn('sidekiq', metrics_dir: metrics_dir)
-      end
+            described_class.spawn(target, metrics_dir: metrics_dir)
+          end
 
-      it 'resets signal handlers from parent process' do
-        expect(Gitlab::ProcessManagement).to receive(:modify_signals).with(%i[A B], 'DEFAULT')
+          it 'resets signal handlers from parent process' do
+            expect(Gitlab::ProcessManagement).to receive(:modify_signals).with(%i[A B], 'DEFAULT')
 
-        described_class.spawn('sidekiq', metrics_dir: metrics_dir, trapped_signals: %i[A B])
+            described_class.spawn(target, metrics_dir: metrics_dir, trapped_signals: %i[A B])
+          end
+        end
+      end
+    end
+  end
+
+  context 'when targeting invalid target' do
+    describe '.spawn' do
+      it 'raises an error' do
+        expect { described_class.spawn('unsupported', metrics_dir: metrics_dir) }.to(
+          raise_error('Target must be one of [puma,sidekiq]')
+        )
       end
     end
   end
@@ -64,7 +85,6 @@ RSpec.describe MetricsServer do # rubocop:disable RSpec/FilePath
     let(:exporter_class) { Class.new(Gitlab::Metrics::Exporter::BaseExporter) }
     let(:exporter_double) { double('fake_exporter', start: true) }
     let(:settings) { { "fake_exporter" => { "enabled" => true } } }
-    let(:ruby_sampler_double) { double(Gitlab::Metrics::Samplers::RubySampler) }
 
     subject(:metrics_server) { described_class.new('fake', metrics_dir, true)}
 
@@ -74,9 +94,6 @@ RSpec.describe MetricsServer do # rubocop:disable RSpec/FilePath
         settings['fake_exporter'], gc_requests: true, synchronous: true
       ).and_return(exporter_double)
       expect(Settings).to receive(:monitoring).and_return(settings)
-
-      allow(Gitlab::Metrics::Samplers::RubySampler).to receive(:initialize_instance).and_return(ruby_sampler_double)
-      allow(ruby_sampler_double).to receive(:start)
     end
 
     it 'configures ::Prometheus::Client' do