diff --git a/bin/metrics-server b/bin/metrics-server
index d8f2ed9faa48ea5fb620db1e09b60fdac63657a1..f8eca93908ed4aa8e7088260ee0655068ebd9ece 100755
--- a/bin/metrics-server
+++ b/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
diff --git a/metrics_server/metrics_server.rb b/metrics_server/metrics_server.rb
index f1bcd6f611eedc239f1953766379398640978ef3..70769459019c70f38dc1ea7b13d3dc7841d0d10c 100644
--- a/metrics_server/metrics_server.rb
+++ b/metrics_server/metrics_server.rb
@@ -6,8 +6,23 @@
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 @@ def spawn(target, metrics_dir:, wipe_metrics_dir: false, trapped_signals: [])
# 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 @@ def spawn(target, metrics_dir:, wipe_metrics_dir: false, trapped_signals: [])
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 @@ def initialize(target, metrics_dir, wipe_metrics_dir)
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 @@ def start
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
diff --git a/sidekiq_cluster/cli.rb b/sidekiq_cluster/cli.rb
index bbe95788a356b5763bbdf13984a1121f1be0ffdf..2feb77601b89cbd96bd8fb55e89201f45dfb5155 100644
--- a/sidekiq_cluster/cli.rb
+++ b/sidekiq_cluster/cli.rb
@@ -191,11 +191,11 @@ def start_metrics_server(wipe_metrics_dir: false)
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
diff --git a/spec/commands/metrics_server/metrics_server_spec.rb b/spec/commands/metrics_server/metrics_server_spec.rb
index 3b92c3c38fab6170040666b8a8db14083780d1b7..217aa185767969749fa384535fb9656046490dc7 100644
--- a/spec/commands/metrics_server/metrics_server_spec.rb
+++ b/spec/commands/metrics_server/metrics_server_spec.rb
@@ -38,13 +38,7 @@
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
diff --git a/spec/commands/sidekiq_cluster/cli_spec.rb b/spec/commands/sidekiq_cluster/cli_spec.rb
index 4d4e1f35cff9a7fb321095a3c4a31e5b95eaafac..15b738cacd1005c8e6bf87980e713afa82018e07 100644
--- a/spec/commands/sidekiq_cluster/cli_spec.rb
+++ b/spec/commands/sidekiq_cluster/cli_spec.rb
@@ -303,7 +303,7 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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
diff --git a/spec/metrics_server/metrics_server_spec.rb b/spec/metrics_server/metrics_server_spec.rb
index c90fd162814f49f4e2a2fb35f3a9839e6137decd..860a3299d853c9727c737e2331aab1e676ee1537 100644
--- a/spec/metrics_server/metrics_server_spec.rb
+++ b/spec/metrics_server/metrics_server_spec.rb
@@ -36,13 +36,13 @@
%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 @@
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 @@
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 @@
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