Move Prometheus presentation logic to PrometheusText

+ Use NullMetrics to mock metrics when unused
+ Use method_missing in NullMetrics mocking
+ Update prometheus gem to version that correctly uses transitive dependencies
+ Ensure correct folders are used in Multiprocess prometheus client tests.
+ rename Sessions controller's metric

13 files changed, 87 insertions, 48 deletions

diff --git a/Gemfile b/Gemfile
index 8ec2c94a2e0..0c18007d447 100644
--- a/Gemfile
+++ b/Gemfile
@@ -270,8 +270,7 @@ group :metrics do
   gem 'influxdb', '~> 0.2', require: false
 
   # Prometheus
-  gem 'mmap2', '~> 2.2.6'
-  gem 'prometheus-client-mmap', '~>0.7.0.beta3'
+  gem 'prometheus-client-mmap', '~>0.7.0.beta5'
 end
 
 group :development do
diff --git a/Gemfile.lock b/Gemfile.lock
index bb574ae1834..12520570dfe 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -561,8 +561,8 @@ GEM
     premailer-rails (1.9.2)
       actionmailer (>= 3, < 6)
       premailer (~> 1.7, >= 1.7.9)
-    prometheus-client-mmap (0.7.0.beta3)
-      quantile (~> 0.2.0)
+    prometheus-client-mmap (0.7.0.beta5)
+      mmap2 (~> 2.2.6)
     pry (0.10.4)
       coderay (~> 1.1.0)
       method_source (~> 0.8.1)
@@ -573,7 +573,6 @@ GEM
     pry-rails (0.3.5)
       pry (>= 0.9.10)
     pyu-ruby-sasl (0.0.3.3)
-    quantile (0.2.0)
     rack (1.6.5)
     rack-accept (0.4.5)
       rack (>= 0.4)
@@ -972,7 +971,6 @@ DEPENDENCIES
   mail_room (~> 0.9.1)
   method_source (~> 0.8)
   minitest (~> 5.7.0)
-  mmap2 (~> 2.2.6)
   mousetrap-rails (~> 1.4.6)
   mysql2 (~> 0.3.16)
   net-ssh (~> 3.0.1)
@@ -1000,7 +998,7 @@ DEPENDENCIES
   pg (~> 0.18.2)
   poltergeist (~> 1.9.0)
   premailer-rails (~> 1.9.0)
-  prometheus-client-mmap (~> 0.7.0.beta3)
+  prometheus-client-mmap (~> 0.7.0.beta5)
   pry-byebug (~> 3.4.1)
   pry-rails (~> 0.3.4)
   rack-attack (~> 4.4.1)
diff --git a/app/controllers/metrics_controller.rb b/app/controllers/metrics_controller.rb
index 9bcd6f96b34..0e9a19c0b6f 100644
--- a/app/controllers/metrics_controller.rb
+++ b/app/controllers/metrics_controller.rb
@@ -5,10 +5,8 @@ class MetricsController < ActionController::Base
 
   before_action :validate_prometheus_metrics
 
-  def metrics
-    response = "#{metrics_service.health_metrics_text}\n#{metrics_service.prometheus_metrics_text}"
-
-    render text: response, content_type: 'text/plain; version=0.0.4'
+  def index
+    render text: metrics_service.metrics_text, content_type: 'text/plain; verssion=0.0.4'
   end
 
   private
diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb
index fc9de30f256..ab52b676e01 100644
--- a/app/controllers/sessions_controller.rb
+++ b/app/controllers/sessions_controller.rb
@@ -48,7 +48,7 @@ class SessionsController < Devise::SessionsController
   private
 
   def login_counter
-    @login_counter ||= Gitlab::Metrics.counter(:user_session_logins, 'User logins count')
+    @login_counter ||= Gitlab::Metrics.counter(:user_session_logins, 'User sign in count')
   end
 
   # Handle an "initial setup" state, where there's only one user, it's an admin,
diff --git a/app/services/metrics_service.rb b/app/services/metrics_service.rb
index 350c3639e92..0faa86a228b 100644
--- a/app/services/metrics_service.rb
+++ b/app/services/metrics_service.rb
@@ -12,18 +12,23 @@ class MetricsService
   end
 
   def health_metrics_text
-    results = CHECKS.flat_map(&:metrics)
+    metrics = CHECKS.flat_map(&:metrics)
 
-    types = results.map(&:name).uniq.map { |metric_name| "# TYPE #{metric_name} gauge" }
-    metrics = results.map(&method(:metric_to_prom_line))
+    formatter.marshal(metrics)
+  end
 
-    types.concat(metrics).join("\n")
+  def metrics_text
+    "#{health_metrics_text}\n#{prometheus_metrics_text}"
   end
 
   private
 
+  def formatter
+    @formatter ||= PrometheusText.new
+  end
+
   def multiprocess_metrics_path
-    Rails.root.join(ENV['prometheus_multiproc_dir'])
+    @multiprocess_metrics_path ||= Rails.root.join(ENV['prometheus_multiproc_dir'])
   end
 
   def metric_to_prom_line(metric)
diff --git a/config.ru b/config.ru
index 2614c9aaf74..89aba462f19 100644
--- a/config.ru
+++ b/config.ru
@@ -16,7 +16,7 @@ if defined?(Unicorn)
 end
 
 # set default directory for multiproces metrics gathering
-ENV['prometheus_multiproc_dir'] ||= 'tmp/prometheus_data_dir'
+ENV['prometheus_multiproc_dir'] ||= 'tmp/prometheus_multiproc_dir'
 
 require ::File.expand_path('../config/environment',  __FILE__)
 
diff --git a/lib/gitlab/health_checks/prometheus_text.rb b/lib/gitlab/health_checks/prometheus_text.rb
new file mode 100644
index 00000000000..a01e6b2be1f
--- /dev/null
+++ b/lib/gitlab/health_checks/prometheus_text.rb
@@ -0,0 +1,38 @@
+module Gitlab::HealthChecks
+  class PrometheusText
+    def marshal(metrics)
+      metrics_with_type_declarations(metrics).join("\n")
+    end
+
+    private
+
+    def metrics_with_type_declarations(metrics)
+      type_declaration_added = {}
+
+      metrics.flat_map do |metric|
+        metric_lines = []
+
+        unless type_declaration_added.has_key?(metric.name)
+          type_declaration_added[metric.name] = true
+          metric_lines << metric_type_declaration(metric)
+        end
+
+        metric_lines << metric_text(metric)
+      end
+    end
+
+    def metric_type_declaration(metric)
+      "# TYPE #{metric.name} gauge"
+    end
+
+    def metric_text(metric)
+      labels = metric.labels&.map { |key, value| "#{key}=\"#{value}\"" }&.join(',') || ''
+
+      if labels.empty?
+        "#{metric.name} #{metric.value}"
+      else
+        "#{metric.name}{#{labels}} #{metric.value}"
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/metrics.rb b/lib/gitlab/metrics.rb
index a41cbd214a1..34f6b32f7da 100644
--- a/lib/gitlab/metrics.rb
+++ b/lib/gitlab/metrics.rb
@@ -73,7 +73,7 @@ module Gitlab
       if prometheus_metrics_enabled?
         registry.get(name)
       else
-        DummyMetric.new
+        NullMetric.new
       end
     end
 
diff --git a/lib/gitlab/metrics/dummy_metric.rb b/lib/gitlab/metrics/null_metric.rb
index d27bb83854a..1501cd38676 100644
--- a/lib/gitlab/metrics/dummy_metric.rb
+++ b/lib/gitlab/metrics/null_metric.rb
@@ -1,7 +1,12 @@
 module Gitlab
   module Metrics
     # Mocks ::Prometheus::Client::Metric and all derived metrics
-    class DummyMetric
+    class NullMetric
+      def method_missing(name, *args, &block)
+        nil
+      end
+
+      # these methods shouldn't be called when metrics are disabled
       def get(*args)
         raise NotImplementedError
       end
@@ -9,21 +14,6 @@ module Gitlab
       def values(*args)
         raise NotImplementedError
       end
-
-      # counter
-      def increment(*args)
-        # noop
-      end
-
-      # gauge
-      def set(*args)
-        # noop
-      end
-
-      # histogram / summary
-      def observe(*args)
-        # noop
-      end
     end
   end
 end
diff --git a/spec/controllers/metrics_controller_spec.rb b/spec/controllers/metrics_controller_spec.rb
index cd31f750ffd..7baf7d1bef9 100644
--- a/spec/controllers/metrics_controller_spec.rb
+++ b/spec/controllers/metrics_controller_spec.rb
@@ -6,10 +6,10 @@ describe MetricsController do
   let(:token) { current_application_settings.health_check_access_token }
   let(:json_response) { JSON.parse(response.body) }
 
-  around do |examples|
+  around do |example|
     Dir.mktmpdir do |tmp_dir|
       @metrics_multiproc_dir = tmp_dir
-      examples.run
+      example.run
     end
   end
 
diff --git a/spec/lib/gitlab/metrics_spec.rb b/spec/lib/gitlab/metrics_spec.rb
index 863868e1576..87c9f4ebda4 100644
--- a/spec/lib/gitlab/metrics_spec.rb
+++ b/spec/lib/gitlab/metrics_spec.rb
@@ -1,6 +1,8 @@
 require 'spec_helper'
 
 describe Gitlab::Metrics do
+  include StubENV
+
   describe '.settings' do
     it 'returns a Hash' do
       expect(described_class.settings).to be_an_instance_of(Hash)
@@ -247,45 +249,53 @@ describe Gitlab::Metrics do
 
     it_behaves_like 'prometheus metrics API'
 
-    describe '#dummy_metric' do
+    describe '#null_metric' do
       subject { described_class.provide_metric(:test) }
 
-      it { is_expected.to be_a(Gitlab::Metrics::DummyMetric) }
+      it { is_expected.to be_a(Gitlab::Metrics::NullMetric) }
     end
 
     describe '#counter' do
       subject { described_class.counter(:counter, 'doc') }
 
-      it { is_expected.to be_a(Gitlab::Metrics::DummyMetric) }
+      it { is_expected.to be_a(Gitlab::Metrics::NullMetric) }
     end
 
     describe '#summary' do
       subject { described_class.summary(:summary, 'doc') }
 
-      it { is_expected.to be_a(Gitlab::Metrics::DummyMetric) }
+      it { is_expected.to be_a(Gitlab::Metrics::NullMetric) }
     end
 
     describe '#gauge' do
       subject { described_class.gauge(:gauge, 'doc') }
 
-      it { is_expected.to be_a(Gitlab::Metrics::DummyMetric) }
+      it { is_expected.to be_a(Gitlab::Metrics::NullMetric) }
     end
 
     describe '#histogram' do
       subject { described_class.histogram(:histogram, 'doc') }
 
-      it { is_expected.to be_a(Gitlab::Metrics::DummyMetric) }
+      it { is_expected.to be_a(Gitlab::Metrics::NullMetric) }
     end
   end
 
   context 'prometheus metrics enabled' do
+    around do |example|
+      Dir.mktmpdir do |tmp_dir|
+        @metrics_multiproc_dir = tmp_dir
+        example.run
+      end
+    end
+
     before do
+      stub_const('Prometheus::Client::Multiprocdir', @metrics_multiproc_dir)
       allow(described_class).to receive(:prometheus_metrics_enabled?).and_return(true)
     end
 
     it_behaves_like 'prometheus metrics API'
 
-    describe '#dummy_metric' do
+    describe '#null_metric' do
       subject { described_class.provide_metric(:test) }
 
       it { is_expected.to be_nil }
@@ -294,25 +304,25 @@ describe Gitlab::Metrics do
     describe '#counter' do
       subject { described_class.counter(:name, 'doc') }
 
-      it { is_expected.not_to be_a(Gitlab::Metrics::DummyMetric) }
+      it { is_expected.not_to be_a(Gitlab::Metrics::NullMetric) }
     end
 
     describe '#summary' do
       subject { described_class.summary(:name, 'doc') }
 
-      it { is_expected.not_to be_a(Gitlab::Metrics::DummyMetric) }
+      it { is_expected.not_to be_a(Gitlab::Metrics::NullMetric) }
     end
 
     describe '#gauge' do
       subject { described_class.gauge(:name, 'doc') }
 
-      it { is_expected.not_to be_a(Gitlab::Metrics::DummyMetric) }
+      it { is_expected.not_to be_a(Gitlab::Metrics::NullMetric) }
     end
 
     describe '#histogram' do
       subject { described_class.histogram(:name, 'doc') }
 
-      it { is_expected.not_to be_a(Gitlab::Metrics::DummyMetric) }
+      it { is_expected.not_to be_a(Gitlab::Metrics::NullMetric) }
     end
   end
 end
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index 994c7dcbb46..f800c5bcb07 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -3,6 +3,7 @@ SimpleCovEnv.start!
 
 ENV["RAILS_ENV"] ||= 'test'
 ENV["IN_MEMORY_APPLICATION_SETTINGS"] = 'true'
+# ENV['prometheus_multiproc_dir'] = 'tmp/prometheus_multiproc_dir_test'
 
 require File.expand_path("../../config/environment", __FILE__)
 require 'rspec/rails'
diff --git a/tmp/prometheus_data_dir/.gitkeep b/tmp/prometheus_multiproc_dir/.gitkeep
index e69de29bb2d..e69de29bb2d 100644
--- a/tmp/prometheus_data_dir/.gitkeep
+++ b/tmp/prometheus_multiproc_dir/.gitkeep