Experimental approach w/ different pid providers64972-fix-unicorn-workers-metric-v2

7 files changed, 151 insertions, 69 deletions

diff --git a/config/initializers/7_prometheus_metrics.rb b/config/initializers/7_prometheus_metrics.rb
index 3f2691dde95..c1d6f58bd18 100644
--- a/config/initializers/7_prometheus_metrics.rb
+++ b/config/initializers/7_prometheus_metrics.rb
@@ -22,7 +22,7 @@ Prometheus::Client.configure do |config|
 
   config.multiprocess_files_dir = ENV['prometheus_multiproc_dir'] || prometheus_default_multiproc_dir
 
-  config.pid_provider = Prometheus::PidProvider.method(:worker_id)
+  config.pid_provider = Prometheus::PidProviderOld.method(:worker_id)
 end
 
 Gitlab::Application.configure do |config|
@@ -40,13 +40,29 @@ if !Rails.env.test? && Gitlab::Metrics.prometheus_metrics_enabled?
   Gitlab::Cluster::LifecycleEvents.on_worker_start do
     defined?(::Prometheus::Client.reinitialize_on_pid_change) && Prometheus::Client.reinitialize_on_pid_change
 
+    Prometheus::Client.configure do |config|
+      if defined?(::Unicorn)
+        config.pid_provider = Prometheus::UnicornPidProvider.new(for_master: false).method(:process_id)
+      elsif defined?(::Puma)
+        config.pid_provider = Prometheus::PumaPidProvider.new(for_master: false).method(:process_id)
+      end
+    end
+
     Gitlab::Metrics::Samplers::RubySampler.initialize_instance(Settings.monitoring.ruby_sampler_interval).start
   end
 
   Gitlab::Cluster::LifecycleEvents.on_master_start do
     if defined?(::Unicorn)
+      Prometheus::Client.configure do |config|
+        config.pid_provider = Prometheus::UnicornPidProvider.new(for_master: true).method(:process_id)
+      end
+
       Gitlab::Metrics::Samplers::UnicornSampler.initialize_instance(Settings.monitoring.unicorn_sampler_interval).start
     elsif defined?(::Puma)
+      Prometheus::Client.configure do |config|
+        config.pid_provider = Prometheus::PumaPidProvider.new(for_master: true).method(:process_id)
+      end
+
       Gitlab::Metrics::Samplers::PumaSampler.initialize_instance(Settings.monitoring.puma_sampler_interval).start
     end
   end
diff --git a/lib/gitlab/metrics/samplers/unicorn_sampler.rb b/lib/gitlab/metrics/samplers/unicorn_sampler.rb
index 355f938704e..3677c386948 100644
--- a/lib/gitlab/metrics/samplers/unicorn_sampler.rb
+++ b/lib/gitlab/metrics/samplers/unicorn_sampler.rb
@@ -18,16 +18,17 @@ module Gitlab
 
         def enabled?
           # Raindrops::Linux.tcp_listener_stats is only present on Linux
-          unicorn_with_listeners? && Raindrops::Linux.respond_to?(:tcp_listener_stats)
+          # unicorn_with_listeners? && Raindrops::Linux.respond_to?(:tcp_listener_stats)
+          true
         end
 
         def sample
-          Raindrops::Linux.tcp_listener_stats(tcp_listeners).each do |addr, stats|
-            set_unicorn_connection_metrics('tcp', addr, stats)
-          end
-          Raindrops::Linux.unix_listener_stats(unix_listeners).each do |addr, stats|
-            set_unicorn_connection_metrics('unix', addr, stats)
-          end
+          # Raindrops::Linux.tcp_listener_stats(tcp_listeners).each do |addr, stats|
+          #   set_unicorn_connection_metrics('tcp', addr, stats)
+          # end
+          # Raindrops::Linux.unix_listener_stats(unix_listeners).each do |addr, stats|
+          #   set_unicorn_connection_metrics('unix', addr, stats)
+          # end
 
           metrics[:unicorn_workers].set({}, unicorn_workers_count)
         end
diff --git a/lib/prometheus/pid_provider.rb b/lib/prometheus/pid_provider.rb
deleted file mode 100644
index 3c791147ccb..00000000000
--- a/lib/prometheus/pid_provider.rb
+++ /dev/null
@@ -1,51 +0,0 @@
-# frozen_string_literal: true
-
-require 'prometheus/client/support/unicorn'
-
-module Prometheus
-  module PidProvider
-    extend self
-
-    def worker_id
-      if Sidekiq.server?
-        'sidekiq'
-      elsif defined?(Unicorn::Worker)
-        unicorn_worker_id
-      elsif defined?(::Puma)
-        puma_worker_id
-      else
-        unknown_process_id
-      end
-    end
-
-    private
-
-    def unicorn_worker_id
-      if process_name =~ /unicorn_rails master/
-        'unicorn_master'
-      elsif match = process_name.match(/unicorn_rails worker\[([0-9]+)\]/)
-        "unicorn_#{match[1]}"
-      else
-        unknown_process_id
-      end
-    end
-
-    # This is not fully accurate as we don't really know if the nil returned
-    # is actually means we're on master or not.
-    # Follow up issue was created to address this problem and
-    # to introduce more structrured approach to a current process discovery:
-    # https://gitlab.com/gitlab-org/gitlab-ce/issues/64740
-    def puma_worker_id
-      match = process_name.match(/cluster worker ([0-9]+):/)
-      match ? "puma_#{match[1]}" : 'puma_master'
-    end
-
-    def unknown_process_id
-      "process_#{Process.pid}"
-    end
-
-    def process_name
-      $0
-    end
-  end
-end
diff --git a/lib/prometheus/pid_provider_old.rb b/lib/prometheus/pid_provider_old.rb
new file mode 100644
index 00000000000..301b52b5a45
--- /dev/null
+++ b/lib/prometheus/pid_provider_old.rb
@@ -0,0 +1,31 @@
+# frozen_string_literal: true
+
+module Prometheus
+  module PidProviderOld
+    extend self
+
+    def worker_id
+      if Sidekiq.server?
+        'sidekiq'
+      # still need to check this or similar as PidProvider is called even before / outside of the
+      # `Gitlab::Cluster::LifecycleEvents.on_master_start` hook
+      elsif process_name =~ /(unicorn|unicorn_rails)\z/
+        'unicorn_master'
+      elsif elsif process_name =~ /puma/
+        'puma_master'
+      else
+        unknown_process_id
+      end
+    end
+
+    private
+
+    def unknown_process_id
+      "process_#{Process.pid}"
+    end
+
+    def process_name
+      $0
+    end
+  end
+end
diff --git a/lib/prometheus/puma_pid_provider.rb b/lib/prometheus/puma_pid_provider.rb
new file mode 100644
index 00000000000..c12e41d826f
--- /dev/null
+++ b/lib/prometheus/puma_pid_provider.rb
@@ -0,0 +1,26 @@
+module Prometheus
+  class PumaPidProvider
+    def initialize(for_master:)
+      @for_master = for_master
+    end
+
+    def process_id
+      @for_master ? 'puma_master' : worker_id
+    end
+
+    private
+
+    def worker_id
+      match = process_name.match(/cluster worker ([0-9]+):/)
+      match ? "puma_#{match[1]}" : unknown_process_id
+    end
+
+    def unknown_process_id
+      "process_#{Process.pid}"
+    end
+
+    def process_name
+      $0
+    end
+  end
+end
diff --git a/lib/prometheus/unicorn_pid_provider.rb b/lib/prometheus/unicorn_pid_provider.rb
new file mode 100644
index 00000000000..28348896f82
--- /dev/null
+++ b/lib/prometheus/unicorn_pid_provider.rb
@@ -0,0 +1,31 @@
+# frozen_string_literal: true
+
+module Prometheus
+  class UnicornPidProvider
+    def initialize(for_master:)
+      @for_master = for_master
+    end
+
+    def process_id
+      @for_master ? 'unicorn_master' : worker_id
+    end
+
+    private
+
+    def worker_id
+      if match = process_name.match(/(unicorn|unicorn_rails) worker\[([0-9]+)\]/)
+        "unicorn_#{match[2]}"
+      else
+        unknown_process_id
+      end
+    end
+
+    def unknown_process_id
+      "process_#{Process.pid}"
+    end
+
+    def process_name
+      $0
+    end
+  end
+end
diff --git a/spec/lib/prometheus/pid_provider_spec.rb b/spec/lib/prometheus/pid_provider_old_spec.rb
index 32e03f5b4a5..84b54d33a92 100644
--- a/spec/lib/prometheus/pid_provider_spec.rb
+++ b/spec/lib/prometheus/pid_provider_old_spec.rb
@@ -2,7 +2,7 @@
 
 require 'fast_spec_helper'
 
-describe Prometheus::PidProvider do
+describe Prometheus::PidProviderOld do
   describe '.worker_id' do
     subject { described_class.worker_id }
 
@@ -25,28 +25,56 @@ describe Prometheus::PidProvider do
       before do
         stub_const('Unicorn::Worker', Class.new)
         hide_const('Puma')
+
+        expect(described_class).to receive(:process_name).at_least(:once).and_return(process_name)
       end
 
       context 'when unicorn master is specified in process name' do
-        before do
-          expect(described_class).to receive(:process_name).at_least(:once).and_return('unicorn_rails master')
+        context 'when in production env' do
+          context 'before the process was renamed' do
+            let(:process_name) { "/opt/gitlab/embedded/bin/unicorn"}
+
+            it { is_expected.to eq 'unicorn_master' }
+          end
+
+          context 'after the process was renamed' do
+            let(:process_name) { "unicorn master -D -E production -c /var/opt/gitlab/gitlab-rails/etc/unicorn.rb /opt/gitlab/embedded/service/gitlab-rails/config.ru" }
+
+            it { is_expected.to eq 'unicorn_master' }
+          end
         end
 
-        it { is_expected.to eq 'unicorn_master' }
+        context 'when in development env' do
+          context 'before the process was renamed' do
+            let(:process_name) { "path_to_bindir/bin/unicorn_rails"}
+
+            it { is_expected.to eq 'unicorn_master' }
+          end
+
+          context 'after the process was renamed' do
+            let(:process_name) { "unicorn_rails master -c /gitlab_dir/config/unicorn.rb -E development" }
+
+            it { is_expected.to eq 'unicorn_master' }
+          end
+        end
       end
 
       context 'when unicorn worker id is specified in process name' do
-        before do
-          expect(described_class).to receive(:process_name).at_least(:once).and_return('unicorn_rails worker[1]')
+        context 'when in production env' do
+          let(:process_name) { "unicorn worker[1] -D -E production -c /var/opt/gitlab/gitlab-rails/etc/unicorn.rb /opt/gitlab/embedded/service/gitlab-rails/config.ru" }
+
+          it { is_expected.to eq 'unicorn_1' }
         end
 
-        it { is_expected.to eq 'unicorn_1' }
+        context 'when in development env' do
+          let(:process_name) { "unicorn_rails worker[1] -c gitlab_dir/config/unicorn.rb -E development" }
+
+          it { is_expected.to eq 'unicorn_1' }
+        end
       end
 
       context 'when no specified unicorn master or worker id in process name' do
-        before do
-          expect(described_class).to receive(:process_name).at_least(:once).and_return('bin/unicorn_rails')
-        end
+        let(:process_name) { "bin/unknown_unicorn_instance"}
 
         it { is_expected.to eq "process_#{Process.pid}" }
       end