Metrics server can be spawned instead of forked

This provides better memory use for Puma, and we
will require this for running a non-Ruby server
in the future.

This does not actually start the server for Puma
yet.

bin/metrics-server

@@ -9,4 +9,5 @@ raise "METRICS_SERVER_TARGET cannot be blank" if target.blank?
 metrics_dir = ENV["prometheus_multiproc_dir"] || File.absolute_path("tmp/prometheus_multiproc_dir/#{target}")
 wipe_metrics_dir = Gitlab::Utils.to_boolean(ENV['WIPE_METRICS_DIR']) || false
 
-Process.wait(MetricsServer.spawn(target, metrics_dir: metrics_dir, wipe_metrics_dir: wipe_metrics_dir))
+server = MetricsServer.new(target, metrics_dir, wipe_metrics_dir)
+server.start

metrics_server/metrics_server.rb

@@ -6,8 +6,23 @@ require_relative 'dependencies'
 
 class MetricsServer # rubocop:disable Gitlab/NamespacedClass
   class << self
-    def spawn(target, metrics_dir:, wipe_metrics_dir: false, trapped_signals: [])
-      raise "Target must be one of [puma,sidekiq]" unless %w(puma sidekiq).include?(target)
+    def spawn(target, metrics_dir:, gitlab_config: nil, wipe_metrics_dir: false)
+      ensure_valid_target!(target)
+
+      cmd = "#{Rails.root}/bin/metrics-server"
+      env = {
+        'METRICS_SERVER_TARGET' => target,
+        'WIPE_METRICS_DIR' => wipe_metrics_dir ? '1' : '0'
+      }
+      env['GITLAB_CONFIG'] = gitlab_config if gitlab_config
+
+      Process.spawn(env, cmd, err: $stderr, out: $stdout, pgroup: true).tap do |pid|
+        Process.detach(pid)
+      end
+    end
+
+    def fork(target, metrics_dir:, wipe_metrics_dir: false, reset_signals: [])
+      ensure_valid_target!(target)
 
       pid = Process.fork
 
@@ -15,7 +30,7 @@ class MetricsServer # rubocop:disable Gitlab/NamespacedClass
         # Remove any custom signal handlers the parent process had registered, since we do
         # not want to inherit them, and Ruby forks with a `clone` that has the `CLONE_SIGHAND`
         # flag set.
-        Gitlab::ProcessManagement.modify_signals(trapped_signals, 'DEFAULT')
+        Gitlab::ProcessManagement.modify_signals(reset_signals, 'DEFAULT')
 
         server = MetricsServer.new(target, metrics_dir, wipe_metrics_dir)
         # This rewrites /proc/cmdline, since otherwise tools like `top` will show the
@@ -29,6 +44,12 @@ class MetricsServer # rubocop:disable Gitlab/NamespacedClass
 
       pid
     end
+
+    private
+
+    def ensure_valid_target!(target)
+      raise "Target must be one of [puma,sidekiq]" unless %w(puma sidekiq).include?(target)
+    end
   end
 
   def initialize(target, metrics_dir, wipe_metrics_dir)
@@ -40,7 +61,7 @@ class MetricsServer # rubocop:disable Gitlab/NamespacedClass
   def start
     ::Prometheus::Client.configure do |config|
       config.multiprocess_files_dir = @metrics_dir
-      config.pid_provider = proc { "#{@target}_exporter" }
+      config.pid_provider = proc { name }
     end
 
     FileUtils.mkdir_p(@metrics_dir, mode: 0700)
@@ -57,16 +78,18 @@ class MetricsServer # rubocop:disable Gitlab/NamespacedClass
       case @target
       when 'puma'
         Gitlab::Metrics::Exporter::WebExporter.instance(**default_opts)
-      else
-        exporter_class = "Gitlab::Metrics::Exporter::#{@target.camelize}Exporter".constantize
+      when 'sidekiq'
         settings = Settings.new(Settings.monitoring[name])
-        exporter_class.instance(settings, **default_opts)
+        Gitlab::Metrics::Exporter::SidekiqExporter.instance(settings, **default_opts)
       end
 
     exporter.start
   end
 
   def name
-    "#{@target}_exporter"
+    case @target
+    when 'puma' then 'web_exporter'
+    when 'sidekiq' then 'sidekiq_exporter'
+    end
   end
 end

sidekiq_cluster/cli.rb

@@ -191,11 +191,11 @@ module Gitlab
         return unless metrics_server_enabled?
 
         @logger.info("Starting metrics server on port #{sidekiq_exporter_port}")
-        @metrics_server_pid = MetricsServer.spawn(
+        @metrics_server_pid = MetricsServer.fork(
           'sidekiq',
           metrics_dir: @metrics_dir,
           wipe_metrics_dir: wipe_metrics_dir,
-          trapped_signals: TERMINATE_SIGNALS + FORWARD_SIGNALS
+          reset_signals: TERMINATE_SIGNALS + FORWARD_SIGNALS
         )
       end
 

spec/commands/metrics_server/metrics_server_spec.rb

@@ -38,13 +38,7 @@ RSpec.describe 'bin/metrics-server', :aggregate_failures do
         config_file.write(YAML.dump(config))
         config_file.close
 
-        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)
+        @pid = MetricsServer.spawn(target, metrics_dir: metrics_dir, gitlab_config: config_file.path, wipe_metrics_dir: true)
       end
 
       after do

spec/commands/sidekiq_cluster/cli_spec.rb

@@ -303,7 +303,7 @@ RSpec.describe Gitlab::SidekiqCluster::CLI, stub_settings_source: true do # rubo
             end
 
             it 'does not start a sidekiq metrics server' do
-              expect(MetricsServer).not_to receive(:spawn)
+              expect(MetricsServer).not_to receive(:fork)
 
               cli.run(%w(foo))
             end
@@ -320,7 +320,7 @@ RSpec.describe Gitlab::SidekiqCluster::CLI, stub_settings_source: true do # rubo
             end
 
             it 'does not start a sidekiq metrics server' do
-              expect(MetricsServer).not_to receive(:spawn)
+              expect(MetricsServer).not_to receive(:fork)
 
               cli.run(%w(foo))
             end
@@ -350,7 +350,7 @@ RSpec.describe Gitlab::SidekiqCluster::CLI, stub_settings_source: true do # rubo
             end
 
             it 'does not start a sidekiq metrics server' do
-              expect(MetricsServer).not_to receive(:spawn)
+              expect(MetricsServer).not_to receive(:fork)
 
               cli.run(%w(foo))
             end
@@ -376,7 +376,7 @@ RSpec.describe Gitlab::SidekiqCluster::CLI, stub_settings_source: true do # rubo
             end
 
             it 'does not start a sidekiq metrics server' do
-              expect(MetricsServer).not_to receive(:spawn)
+              expect(MetricsServer).not_to receive(:fork)
 
               cli.run(%w(foo))
             end
@@ -406,9 +406,9 @@ RSpec.describe Gitlab::SidekiqCluster::CLI, stub_settings_source: true do # rubo
 
               specify do
                 if start_metrics_server
-                  expect(MetricsServer).to receive(:spawn).with('sidekiq', metrics_dir: metrics_dir, wipe_metrics_dir: true, trapped_signals: trapped_signals)
+                  expect(MetricsServer).to receive(:fork).with('sidekiq', metrics_dir: metrics_dir, wipe_metrics_dir: true, reset_signals: trapped_signals)
                 else
-                  expect(MetricsServer).not_to receive(:spawn)
+                  expect(MetricsServer).not_to receive(:fork)
                 end
 
                 cli.run(%w(foo))
@@ -421,7 +421,7 @@ RSpec.describe Gitlab::SidekiqCluster::CLI, stub_settings_source: true do # rubo
           let(:sidekiq_exporter_enabled) { true }
 
           it 'does not start the server' do
-            expect(MetricsServer).not_to receive(:spawn)
+            expect(MetricsServer).not_to receive(:fork)
 
             cli.run(%w(foo --dryrun))
           end
@@ -434,7 +434,7 @@ RSpec.describe Gitlab::SidekiqCluster::CLI, stub_settings_source: true do # rubo
 
         before do
           allow(cli).to receive(:sleep).with(a_kind_of(Numeric))
-          allow(MetricsServer).to receive(:spawn).and_return(99)
+          allow(MetricsServer).to receive(:fork).and_return(99)
           cli.start_metrics_server
         end
 
@@ -453,8 +453,8 @@ RSpec.describe Gitlab::SidekiqCluster::CLI, stub_settings_source: true do # rubo
           allow(Gitlab::ProcessManagement).to receive(:all_alive?).with(an_instance_of(Array)).and_return(false)
           allow(cli).to receive(:stop_metrics_server)
 
-          expect(MetricsServer).to receive(:spawn).with(
-            'sidekiq', metrics_dir: metrics_dir, wipe_metrics_dir: false, trapped_signals: trapped_signals
+          expect(MetricsServer).to receive(:fork).with(
+            'sidekiq', metrics_dir: metrics_dir, wipe_metrics_dir: false, reset_signals: trapped_signals
           )
 
           cli.start_loop

spec/metrics_server/metrics_server_spec.rb

@@ -36,13 +36,13 @@ RSpec.describe MetricsServer do # rubocop:disable RSpec/FilePath
 
   %w(puma sidekiq).each do |target|
     context "when targeting #{target}" do
-      describe '.spawn' do
+      describe '.fork' 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(target, metrics_dir: metrics_dir)
+            described_class.fork(target, metrics_dir: metrics_dir)
           end
         end
 
@@ -58,13 +58,47 @@ RSpec.describe MetricsServer do # rubocop:disable RSpec/FilePath
               expect(server).to receive(:start)
             end
 
-            described_class.spawn(target, metrics_dir: metrics_dir)
+            described_class.fork(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')
 
-            described_class.spawn(target, metrics_dir: metrics_dir, trapped_signals: %i[A B])
+            described_class.fork(target, metrics_dir: metrics_dir, reset_signals: %i[A B])
+          end
+        end
+      end
+
+      describe '.spawn' do
+        let(:expected_env) do
+          {
+            'METRICS_SERVER_TARGET' => target,
+            'WIPE_METRICS_DIR' => '0'
+          }
+        end
+
+        it 'spawns a new server process and returns its PID' do
+          expect(Process).to receive(:spawn).with(
+            expected_env,
+            end_with('bin/metrics-server'),
+            hash_including(pgroup: true)
+          ).and_return(99)
+          expect(Process).to receive(:detach).with(99)
+
+          pid = described_class.spawn(target, metrics_dir: metrics_dir)
+
+          expect(pid).to eq(99)
+        end
+
+        context 'when path to gitlab.yml is passed' do
+          it 'sets the GITLAB_CONFIG environment variable' do
+            expect(Process).to receive(:spawn).with(
+              expected_env.merge('GITLAB_CONFIG' => 'path/to/config/gitlab.yml'),
+              end_with('bin/metrics-server'),
+              hash_including(pgroup: true)
+            ).and_return(99)
+
+            described_class.spawn(target, metrics_dir: metrics_dir, gitlab_config: 'path/to/config/gitlab.yml')
           end
         end
       end
@@ -72,6 +106,14 @@ RSpec.describe MetricsServer do # rubocop:disable RSpec/FilePath
   end
 
   context 'when targeting invalid target' do
+    describe '.fork' do
+      it 'raises an error' do
+        expect { described_class.fork('unsupported', metrics_dir: metrics_dir) }.to(
+          raise_error('Target must be one of [puma,sidekiq]')
+        )
+      end
+    end
+
     describe '.spawn' do
       it 'raises an error' do
         expect { described_class.spawn('unsupported', metrics_dir: metrics_dir) }.to(
@@ -81,64 +123,86 @@ RSpec.describe MetricsServer do # rubocop:disable RSpec/FilePath
     end
   end
 
-  describe '#start' do
-    let(:exporter_class) { Class.new(Gitlab::Metrics::Exporter::BaseExporter) }
-    let(:exporter_double) { double('fake_exporter', start: true) }
-    let(:settings) { { "fake_exporter" => { "enabled" => true } } }
+  shared_examples 'a metrics exporter' do |target, expected_name|
+    describe '#start' do
+      let(:exporter_double) { double('exporter', start: true) }
+      let(:wipe_metrics_dir) { true }
 
-    subject(:metrics_server) { described_class.new('fake', metrics_dir, true)}
+      subject(:metrics_server) { described_class.new(target, metrics_dir, wipe_metrics_dir) }
 
-    before do
-      stub_const('Gitlab::Metrics::Exporter::FakeExporter', exporter_class)
-      expect(exporter_class).to receive(:instance).with(
-        settings['fake_exporter'], gc_requests: true, synchronous: true
-      ).and_return(exporter_double)
-      expect(Settings).to receive(:monitoring).and_return(settings)
-    end
+      it 'configures ::Prometheus::Client' do
+        metrics_server.start
 
-    it 'configures ::Prometheus::Client' do
-      metrics_server.start
+        expect(prometheus_config.multiprocess_files_dir).to eq metrics_dir
+        expect(::Prometheus::Client.configuration.pid_provider.call).to eq expected_name
+      end
 
-      expect(prometheus_config.multiprocess_files_dir).to eq metrics_dir
-      expect(::Prometheus::Client.configuration.pid_provider.call).to eq 'fake_exporter'
-    end
+      it 'ensures that metrics directory exists in correct mode (0700)' do
+        expect(FileUtils).to receive(:mkdir_p).with(metrics_dir, mode: 0700)
 
-    it 'ensures that metrics directory exists in correct mode (0700)' do
-      expect(FileUtils).to receive(:mkdir_p).with(metrics_dir, mode: 0700)
+        metrics_server.start
+      end
 
-      metrics_server.start
-    end
+      context 'when wipe_metrics_dir is true' do
+        it 'removes any old metrics files' do
+          FileUtils.touch("#{metrics_dir}/remove_this.db")
 
-    context 'when wipe_metrics_dir is true' do
-      subject(:metrics_server) { described_class.new('fake', metrics_dir, true)}
+          expect { metrics_server.start }.to change { Dir.empty?(metrics_dir) }.from(false).to(true)
+        end
+      end
 
-      it 'removes any old metrics files' do
-        FileUtils.touch("#{metrics_dir}/remove_this.db")
+      context 'when wipe_metrics_dir is false' do
+        let(:wipe_metrics_dir) { false }
 
-        expect { metrics_server.start }.to change { Dir.empty?(metrics_dir) }.from(false).to(true)
+        it 'does not remove any old metrics files' do
+          FileUtils.touch("#{metrics_dir}/remove_this.db")
+
+          expect { metrics_server.start }.not_to change { Dir.empty?(metrics_dir) }.from(false)
+        end
       end
-    end
 
-    context 'when wipe_metrics_dir is false' do
-      subject(:metrics_server) { described_class.new('fake', metrics_dir, false)}
+      it 'starts a metrics server' do
+        expect(exporter_double).to receive(:start)
+
+        metrics_server.start
+      end
 
-      it 'does not remove any old metrics files' do
-        FileUtils.touch("#{metrics_dir}/remove_this.db")
+      it 'starts a RubySampler instance' do
+        expect(ruby_sampler_double).to receive(:start)
 
-        expect { metrics_server.start }.not_to change { Dir.empty?(metrics_dir) }.from(false)
+        subject.start
       end
     end
 
-    it 'starts a metrics server' do
-      expect(exporter_double).to receive(:start)
+    describe '#name' do
+      let(:exporter_double) { double('exporter', start: true) }
 
-      metrics_server.start
+      subject(:name) { described_class.new(target, metrics_dir, true).name }
+
+      it { is_expected.to eq(expected_name) }
     end
+  end
 
-    it 'starts a RubySampler instance' do
-      expect(ruby_sampler_double).to receive(:start)
+  context 'for puma' do
+    before do
+      allow(Gitlab::Metrics::Exporter::WebExporter).to receive(:instance).with(
+        gc_requests: true, synchronous: true
+      ).and_return(exporter_double)
+    end
 
-      subject.start
+    it_behaves_like 'a metrics exporter', 'puma', 'web_exporter'
+  end
+
+  context 'for sidekiq' do
+    let(:settings) { { "sidekiq_exporter" => { "enabled" => true } } }
+
+    before do
+      allow(::Settings).to receive(:monitoring).and_return(settings)
+      allow(Gitlab::Metrics::Exporter::SidekiqExporter).to receive(:instance).with(
+        settings['sidekiq_exporter'], gc_requests: true, synchronous: true
+      ).and_return(exporter_double)
     end
+
+    it_behaves_like 'a metrics exporter', 'sidekiq', 'sidekiq_exporter'
   end
 end