Skip to content

Commit

Permalink
Clean Sidekiq metrics from multiproc dir on start
Browse files Browse the repository at this point in the history
After moving the multiproc dir cleanup into `config.ru`:`warmup`, we
stopped cleaning Sidekiq metrics dir which is not correct.
This MR intended to fix that. More details:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/31668
  • Loading branch information
Aleksei Lipniagov authored and ayufan committed Aug 19, 2019
1 parent 1d604d4 commit dcfaf49
Show file tree
Hide file tree
Showing 4 changed files with 82 additions and 13 deletions.
18 changes: 5 additions & 13 deletions config.ru
Expand Up @@ -17,24 +17,16 @@ end

require ::File.expand_path('../config/environment', __FILE__)

# The following is necessary to ensure stale Prometheus metrics don't accumulate over time.
# It needs to be done as early as here to ensure metrics files aren't deleted.
# After we hit our app in `warmup`, first metrics and corresponding files already being created,
# for example in `lib/gitlab/metrics/requests_rack_middleware.rb`.
def cleanup_prometheus_multiproc_dir
if dir = ::Prometheus::Client.configuration.multiprocess_files_dir
old_metrics = Dir[File.join(dir, '*.db')]

FileUtils.rm_rf(old_metrics)
end
end

def master_process?
Prometheus::PidProvider.worker_id.in? %w(unicorn_master puma_master)
end

warmup do |app|
cleanup_prometheus_multiproc_dir if master_process?
# The following is necessary to ensure stale Prometheus metrics don't accumulate over time.
# It needs to be done as early as here to ensure metrics files aren't deleted.
# After we hit our app in `warmup`, first metrics and corresponding files already being created,
# for example in `lib/gitlab/metrics/requests_rack_middleware.rb`.
Prometheus::CleanupMultiprocDirService.new.execute if master_process?

client = Rack::MockRequest.new(app)
client.get('/')
Expand Down
3 changes: 3 additions & 0 deletions config/initializers/7_prometheus_metrics.rb
Expand Up @@ -32,6 +32,9 @@ def prometheus_default_multiproc_dir

Sidekiq.configure_server do |config|
config.on(:startup) do
# webserver metrics are cleaned up in config.ru: `warmup` block
Prometheus::CleanupMultiprocDirService.new.execute

Gitlab::Metrics::SidekiqMetricsExporter.instance.start
end
end
Expand Down
23 changes: 23 additions & 0 deletions lib/prometheus/cleanup_multiproc_dir_service.rb
@@ -0,0 +1,23 @@
# frozen_string_literal: true

module Prometheus
class CleanupMultiprocDirService
include Gitlab::Utils::StrongMemoize

def execute
FileUtils.rm_rf(old_metrics) if old_metrics
end

private

def old_metrics
strong_memoize(:old_metrics) do
Dir[File.join(multiprocess_files_dir, '*.db')] if multiprocess_files_dir
end
end

def multiprocess_files_dir
::Prometheus::Client.configuration.multiprocess_files_dir
end
end
end
51 changes: 51 additions & 0 deletions spec/lib/prometheus/cleanup_multiproc_dir_service_spec.rb
@@ -0,0 +1,51 @@
# frozen_string_literal: true

require 'spec_helper'

describe Prometheus::CleanupMultiprocDirService do
describe '.call' do
subject { described_class.new.execute }

let(:metrics_multiproc_dir) { Dir.mktmpdir }
let(:metrics_file_path) { File.join(metrics_multiproc_dir, 'counter_puma_master-0.db') }

before do
FileUtils.touch(metrics_file_path)
end

after do
FileUtils.rm_r(metrics_multiproc_dir)
end

context 'when `multiprocess_files_dir` is defined' do
before do
expect(Prometheus::Client.configuration)
.to receive(:multiprocess_files_dir)
.and_return(metrics_multiproc_dir)
.at_least(:once)
end

it 'removes old metrics' do
expect { subject }
.to change { File.exist?(metrics_file_path) }
.from(true)
.to(false)
end
end

context 'when `multiprocess_files_dir` is not defined' do
before do
expect(Prometheus::Client.configuration)
.to receive(:multiprocess_files_dir)
.and_return(nil)
.at_least(:once)
end

it 'does not remove any files' do
expect { subject }
.not_to change { File.exist?(metrics_file_path) }
.from(true)
end
end
end
end

0 comments on commit dcfaf49

Please sign in to comment.