From dcfaf49550866a291c316f2901e4274d587e7d37 Mon Sep 17 00:00:00 2001
From: Aleksei Lipniagov <alipniagov@gitlab.com>
Date: Mon, 19 Aug 2019 12:52:07 +0000
Subject: Clean Sidekiq metrics from multiproc dir on start

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
---
 config.ru                                          | 18 +++-----
 config/initializers/7_prometheus_metrics.rb        |  3 ++
 lib/prometheus/cleanup_multiproc_dir_service.rb    | 23 ++++++++++
 .../cleanup_multiproc_dir_service_spec.rb          | 51 ++++++++++++++++++++++
 4 files changed, 82 insertions(+), 13 deletions(-)
 create mode 100644 lib/prometheus/cleanup_multiproc_dir_service.rb
 create mode 100644 spec/lib/prometheus/cleanup_multiproc_dir_service_spec.rb

diff --git a/config.ru b/config.ru
index f6a7dca0542..750c84f7642 100644
--- a/config.ru
+++ b/config.ru
@@ -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('/')
diff --git a/config/initializers/7_prometheus_metrics.rb b/config/initializers/7_prometheus_metrics.rb
index 70e5dcd042e..143b34b5368 100644
--- a/config/initializers/7_prometheus_metrics.rb
+++ b/config/initializers/7_prometheus_metrics.rb
@@ -32,6 +32,9 @@ end
 
 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
diff --git a/lib/prometheus/cleanup_multiproc_dir_service.rb b/lib/prometheus/cleanup_multiproc_dir_service.rb
new file mode 100644
index 00000000000..6418b4de166
--- /dev/null
+++ b/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
diff --git a/spec/lib/prometheus/cleanup_multiproc_dir_service_spec.rb b/spec/lib/prometheus/cleanup_multiproc_dir_service_spec.rb
new file mode 100644
index 00000000000..c7302a1a656
--- /dev/null
+++ b/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
-- 
cgit v1.2.1