Revert "Merge branch 'bjk/metric_names' into 'master'"9-4-stable-prepare-rc6

This reverts commit b639ed9f07174c06424586a08db0ed0cde654e54.

11 files changed, 109 insertions, 114 deletions

diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb
index 6e52d42d161..f39441a281e 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_total, 'User sign in 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/config/initializers/8_metrics.rb b/config/initializers/8_metrics.rb
index 25630b298ce..c80d28746d6 100644
--- a/config/initializers/8_metrics.rb
+++ b/config/initializers/8_metrics.rb
@@ -123,7 +123,7 @@ Gitlab::Metrics::UnicornSampler.initialize_instance(Settings.monitoring.unicorn_
 
 Gitlab::Application.configure do |config|
   # 0 should be Sentry to catch errors in this middleware
-  config.middleware.insert(1, Gitlab::Metrics::RequestsRackMiddleware)
+  config.middleware.insert(1, Gitlab::Metrics::ConnectionRackMiddleware)
 end
 
 if Gitlab::Metrics.enabled?
diff --git a/doc/administration/monitoring/prometheus/gitlab_metrics.md b/doc/administration/monitoring/prometheus/gitlab_metrics.md
index e3684263208..7b5ee3dfe05 100644
--- a/doc/administration/monitoring/prometheus/gitlab_metrics.md
+++ b/doc/administration/monitoring/prometheus/gitlab_metrics.md
@@ -26,24 +26,21 @@ server, because the embedded server configuration is overwritten once every
 
 In this experimental phase, only a few metrics are available:
 
-| Metric                            | Type      | Description |
-| --------------------------------- | --------- | ----------- |
-| db_ping_timeout                   | Gauge     | Whether or not the last database ping timed out |
-| db_ping_success                   | Gauge     | Whether or not the last database ping succeeded |
-| db_ping_latency_seconds           | Gauge     | Round trip time of the database ping |
-| filesystem_access_latency_seconds | Gauge     | Latency in accessing a specific filesystem |
-| filesystem_accessible             | Gauge     | Whether or not a specific filesystem is accessible |
-| filesystem_write_latency_seconds  | Gauge     | Write latency of a specific filesystem |
-| filesystem_writable               | Gauge     | Whether or not the filesystem is writable |
-| filesystem_read_latency_seconds   | Gauge     | Read latency of a specific filesystem |
-| filesystem_readable               | Gauge     | Whether or not the filesystem is readable |
-| http_requests_total               | Counter   | Rack request count |
-| http_request_duration_seconds     | Histogram | HTTP response time from rack middleware |
-| rack_uncaught_errors_total        | Counter   | Rack connections handling uncaught errors count |
-| redis_ping_timeout                | Gauge     | Whether or not the last redis ping timed out |
-| redis_ping_success                | Gauge     | Whether or not the last redis ping succeeded |
-| redis_ping_latency_seconds        | Gauge     | Round trip time of the redis ping |
-| user_session_logins_total         | Counter   | Counter of how many users have logged in |
+| Metric | Type | Description |
+| ------ | ---- | ----------- |
+| db_ping_timeout | Gauge | Whether or not the last database ping timed out |
+| db_ping_success | Gauge | Whether or not the last database ping succeeded |
+| db_ping_latency | Gauge | Round trip time of the database ping |
+| redis_ping_timeout | Gauge | Whether or not the last redis ping timed out |
+| redis_ping_success | Gauge | Whether or not the last redis ping succeeded |
+| redis_ping_latency | Gauge | Round trip time of the redis ping |
+| filesystem_access_latency | gauge | Latency in accessing a specific filesystem |
+| filesystem_accessible | gauge | Whether or not a specific filesystem is accessible |
+| filesystem_write_latency | gauge | Write latency of a specific filesystem |
+| filesystem_writable | gauge | Whether or not the filesystem is writable |
+| filesystem_read_latency | gauge | Read latency of a specific filesystem |
+| filesystem_readable | gauge | Whether or not the filesystem is readable |
+| user_sessions_logins | Counter | Counter of how many users have logged in | 
 
 ## Metrics shared directory
 
diff --git a/lib/gitlab/health_checks/fs_shards_check.rb b/lib/gitlab/health_checks/fs_shards_check.rb
index bebde857b16..70da4080cae 100644
--- a/lib/gitlab/health_checks/fs_shards_check.rb
+++ b/lib/gitlab/health_checks/fs_shards_check.rb
@@ -35,9 +35,9 @@ module Gitlab
           repository_storages.flat_map do |storage_name|
             tmp_file_path = tmp_file_path(storage_name)
             [
-              operation_metrics(:filesystem_accessible, :filesystem_access_latency_seconds, -> { storage_stat_test(storage_name) }, shard: storage_name),
-              operation_metrics(:filesystem_writable, :filesystem_write_latency_seconds, -> { storage_write_test(tmp_file_path) }, shard: storage_name),
-              operation_metrics(:filesystem_readable, :filesystem_read_latency_seconds, -> { storage_read_test(tmp_file_path) }, shard: storage_name)
+              operation_metrics(:filesystem_accessible, :filesystem_access_latency, -> { storage_stat_test(storage_name) }, shard: storage_name),
+              operation_metrics(:filesystem_writable, :filesystem_write_latency, -> { storage_write_test(tmp_file_path) }, shard: storage_name),
+              operation_metrics(:filesystem_readable, :filesystem_read_latency, -> { storage_read_test(tmp_file_path) }, shard: storage_name)
             ].flatten
           end
         end
diff --git a/lib/gitlab/health_checks/simple_abstract_check.rb b/lib/gitlab/health_checks/simple_abstract_check.rb
index 3dcb28a193c..fbe1645c1b1 100644
--- a/lib/gitlab/health_checks/simple_abstract_check.rb
+++ b/lib/gitlab/health_checks/simple_abstract_check.rb
@@ -20,7 +20,7 @@ module Gitlab
           [
             metric("#{metric_prefix}_timeout", result.is_a?(Timeout::Error) ? 1 : 0),
             metric("#{metric_prefix}_success", is_successful?(result) ? 1 : 0),
-            metric("#{metric_prefix}_latency_seconds", elapsed)
+            metric("#{metric_prefix}_latency", elapsed)
           ]
         end
       end
diff --git a/lib/gitlab/metrics/connection_rack_middleware.rb b/lib/gitlab/metrics/connection_rack_middleware.rb
new file mode 100644
index 00000000000..b3da360be8f
--- /dev/null
+++ b/lib/gitlab/metrics/connection_rack_middleware.rb
@@ -0,0 +1,45 @@
+module Gitlab
+  module Metrics
+    class ConnectionRackMiddleware
+      def initialize(app)
+        @app = app
+      end
+
+      def self.rack_request_count
+        @rack_request_count ||= Gitlab::Metrics.counter(:rack_request, 'Rack request count')
+      end
+
+      def self.rack_response_count
+        @rack_response_count ||= Gitlab::Metrics.counter(:rack_response, 'Rack response count')
+      end
+
+      def self.rack_uncaught_errors_count
+        @rack_uncaught_errors_count ||= Gitlab::Metrics.counter(:rack_uncaught_errors, 'Rack connections handling uncaught errors count')
+      end
+
+      def self.rack_execution_time
+        @rack_execution_time ||= Gitlab::Metrics.histogram(:rack_execution_time, 'Rack connection handling execution time',
+                                                           {}, [0.05, 0.1, 0.25, 0.5, 0.7, 1, 1.5, 2, 2.5, 3, 5, 7, 10])
+      end
+
+      def call(env)
+        method = env['REQUEST_METHOD'].downcase
+        started = Time.now.to_f
+        begin
+          ConnectionRackMiddleware.rack_request_count.increment(method: method)
+
+          status, headers, body = @app.call(env)
+
+          ConnectionRackMiddleware.rack_response_count.increment(method: method, status: status)
+          [status, headers, body]
+        rescue
+          ConnectionRackMiddleware.rack_uncaught_errors_count.increment
+          raise
+        ensure
+          elapsed = Time.now.to_f - started
+          ConnectionRackMiddleware.rack_execution_time.observe({}, elapsed)
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/metrics/requests_rack_middleware.rb b/lib/gitlab/metrics/requests_rack_middleware.rb
deleted file mode 100644
index 0dc19f31d03..00000000000
--- a/lib/gitlab/metrics/requests_rack_middleware.rb
+++ /dev/null
@@ -1,40 +0,0 @@
-module Gitlab
-  module Metrics
-    class RequestsRackMiddleware
-      def initialize(app)
-        @app = app
-      end
-
-      def self.http_request_total
-        @http_request_total ||= Gitlab::Metrics.counter(:http_requests_total, 'Request count')
-      end
-
-      def self.rack_uncaught_errors_count
-        @rack_uncaught_errors_count ||= Gitlab::Metrics.counter(:rack_uncaught_errors_total, 'Request handling uncaught errors count')
-      end
-
-      def self.http_request_duration_seconds
-        @http_request_duration_seconds ||= Gitlab::Metrics.histogram(:http_request_duration_seconds, 'Request handling execution time',
-                                                           {}, [0.05, 0.1, 0.25, 0.5, 0.7, 1, 2.5, 5, 10, 25])
-      end
-
-      def call(env)
-        method = env['REQUEST_METHOD'].downcase
-        started = Time.now.to_f
-        begin
-          RequestsRackMiddleware.http_request_total.increment(method: method)
-
-          status, headers, body = @app.call(env)
-
-          elapsed = Time.now.to_f - started
-          RequestsRackMiddleware.http_request_duration_seconds.observe({ method: method, status: status }, elapsed)
-
-          [status, headers, body]
-        rescue
-          RequestsRackMiddleware.rack_uncaught_errors_count.increment
-          raise
-        end
-      end
-    end
-  end
-end
diff --git a/spec/controllers/metrics_controller_spec.rb b/spec/controllers/metrics_controller_spec.rb
index 7b0976e3e67..265efb84cf2 100644
--- a/spec/controllers/metrics_controller_spec.rb
+++ b/spec/controllers/metrics_controller_spec.rb
@@ -24,7 +24,7 @@ describe MetricsController do
 
         expect(response.body).to match(/^db_ping_timeout 0$/)
         expect(response.body).to match(/^db_ping_success 1$/)
-        expect(response.body).to match(/^db_ping_latency_seconds [0-9\.]+$/)
+        expect(response.body).to match(/^db_ping_latency [0-9\.]+$/)
       end
 
       it 'returns Redis ping metrics' do
@@ -32,41 +32,17 @@ describe MetricsController do
 
         expect(response.body).to match(/^redis_ping_timeout 0$/)
         expect(response.body).to match(/^redis_ping_success 1$/)
-        expect(response.body).to match(/^redis_ping_latency_seconds [0-9\.]+$/)
-      end
-
-      it 'returns Caching ping metrics' do
-        get :index
-
-        expect(response.body).to match(/^redis_cache_ping_timeout 0$/)
-        expect(response.body).to match(/^redis_cache_ping_success 1$/)
-        expect(response.body).to match(/^redis_cache_ping_latency_seconds [0-9\.]+$/)
-      end
-
-      it 'returns Queues ping metrics' do
-        get :index
-
-        expect(response.body).to match(/^redis_queues_ping_timeout 0$/)
-        expect(response.body).to match(/^redis_queues_ping_success 1$/)
-        expect(response.body).to match(/^redis_queues_ping_latency_seconds [0-9\.]+$/)
-      end
-
-      it 'returns SharedState ping metrics' do
-        get :index
-
-        expect(response.body).to match(/^redis_shared_state_ping_timeout 0$/)
-        expect(response.body).to match(/^redis_shared_state_ping_success 1$/)
-        expect(response.body).to match(/^redis_shared_state_ping_latency_seconds [0-9\.]+$/)
+        expect(response.body).to match(/^redis_ping_latency [0-9\.]+$/)
       end
 
       it 'returns file system check metrics' do
         get :index
 
-        expect(response.body).to match(/^filesystem_access_latency_seconds{shard="default"} [0-9\.]+$/)
+        expect(response.body).to match(/^filesystem_access_latency{shard="default"} [0-9\.]+$/)
         expect(response.body).to match(/^filesystem_accessible{shard="default"} 1$/)
-        expect(response.body).to match(/^filesystem_write_latency_seconds{shard="default"} [0-9\.]+$/)
+        expect(response.body).to match(/^filesystem_write_latency{shard="default"} [0-9\.]+$/)
         expect(response.body).to match(/^filesystem_writable{shard="default"} 1$/)
-        expect(response.body).to match(/^filesystem_read_latency_seconds{shard="default"} [0-9\.]+$/)
+        expect(response.body).to match(/^filesystem_read_latency{shard="default"} [0-9\.]+$/)
         expect(response.body).to match(/^filesystem_readable{shard="default"} 1$/)
       end
 
diff --git a/spec/lib/gitlab/health_checks/fs_shards_check_spec.rb b/spec/lib/gitlab/health_checks/fs_shards_check_spec.rb
index 3de73a9ff65..b333e162909 100644
--- a/spec/lib/gitlab/health_checks/fs_shards_check_spec.rb
+++ b/spec/lib/gitlab/health_checks/fs_shards_check_spec.rb
@@ -109,9 +109,9 @@ describe Gitlab::HealthChecks::FsShardsCheck do
           expect(subject).to include(an_object_having_attributes(name: :filesystem_readable, value: 0))
           expect(subject).to include(an_object_having_attributes(name: :filesystem_writable, value: 0))
 
-          expect(subject).to include(an_object_having_attributes(name: :filesystem_access_latency_seconds, value: be >= 0))
-          expect(subject).to include(an_object_having_attributes(name: :filesystem_read_latency_seconds, value: be >= 0))
-          expect(subject).to include(an_object_having_attributes(name: :filesystem_write_latency_seconds, value: be >= 0))
+          expect(subject).to include(an_object_having_attributes(name: :filesystem_access_latency, value: be >= 0))
+          expect(subject).to include(an_object_having_attributes(name: :filesystem_read_latency, value: be >= 0))
+          expect(subject).to include(an_object_having_attributes(name: :filesystem_write_latency, value: be >= 0))
         end
       end
 
@@ -127,9 +127,9 @@ describe Gitlab::HealthChecks::FsShardsCheck do
           expect(subject).to include(an_object_having_attributes(name: :filesystem_readable, value: 1))
           expect(subject).to include(an_object_having_attributes(name: :filesystem_writable, value: 1))
 
-          expect(subject).to include(an_object_having_attributes(name: :filesystem_access_latency_seconds, value: be >= 0))
-          expect(subject).to include(an_object_having_attributes(name: :filesystem_read_latency_seconds, value: be >= 0))
-          expect(subject).to include(an_object_having_attributes(name: :filesystem_write_latency_seconds, value: be >= 0))
+          expect(subject).to include(an_object_having_attributes(name: :filesystem_access_latency, value: be >= 0))
+          expect(subject).to include(an_object_having_attributes(name: :filesystem_read_latency, value: be >= 0))
+          expect(subject).to include(an_object_having_attributes(name: :filesystem_write_latency, value: be >= 0))
         end
       end
     end
@@ -159,9 +159,9 @@ describe Gitlab::HealthChecks::FsShardsCheck do
         expect(subject).to include(an_object_having_attributes(name: :filesystem_readable, value: 0))
         expect(subject).to include(an_object_having_attributes(name: :filesystem_writable, value: 0))
 
-        expect(subject).to include(an_object_having_attributes(name: :filesystem_access_latency_seconds, value: be >= 0))
-        expect(subject).to include(an_object_having_attributes(name: :filesystem_read_latency_seconds, value: be >= 0))
-        expect(subject).to include(an_object_having_attributes(name: :filesystem_write_latency_seconds, value: be >= 0))
+        expect(subject).to include(an_object_having_attributes(name: :filesystem_access_latency, value: be >= 0))
+        expect(subject).to include(an_object_having_attributes(name: :filesystem_read_latency, value: be >= 0))
+        expect(subject).to include(an_object_having_attributes(name: :filesystem_write_latency, value: be >= 0))
       end
     end
   end
diff --git a/spec/lib/gitlab/health_checks/simple_check_shared.rb b/spec/lib/gitlab/health_checks/simple_check_shared.rb
index 67b19f2cefa..3f871d66034 100644
--- a/spec/lib/gitlab/health_checks/simple_check_shared.rb
+++ b/spec/lib/gitlab/health_checks/simple_check_shared.rb
@@ -8,7 +8,7 @@ shared_context 'simple_check' do |metrics_prefix, check_name, success_result|
 
       it { is_expected.to include(have_attributes(name: "#{metrics_prefix}_success", value: 1)) }
       it { is_expected.to include(have_attributes(name: "#{metrics_prefix}_timeout", value: 0)) }
-      it { is_expected.to include(have_attributes(name: "#{metrics_prefix}_latency_seconds", value: be >= 0)) }
+      it { is_expected.to include(have_attributes(name: "#{metrics_prefix}_latency", value: be >= 0)) }
     end
 
     context 'Check is misbehaving' do
@@ -18,7 +18,7 @@ shared_context 'simple_check' do |metrics_prefix, check_name, success_result|
 
       it { is_expected.to include(have_attributes(name: "#{metrics_prefix}_success", value: 0)) }
       it { is_expected.to include(have_attributes(name: "#{metrics_prefix}_timeout", value: 0)) }
-      it { is_expected.to include(have_attributes(name: "#{metrics_prefix}_latency_seconds", value: be >= 0)) }
+      it { is_expected.to include(have_attributes(name: "#{metrics_prefix}_latency", value: be >= 0)) }
     end
 
     context 'Check is timeouting' do
@@ -28,7 +28,7 @@ shared_context 'simple_check' do |metrics_prefix, check_name, success_result|
 
       it { is_expected.to include(have_attributes(name: "#{metrics_prefix}_success", value: 0)) }
       it { is_expected.to include(have_attributes(name: "#{metrics_prefix}_timeout", value: 1)) }
-      it { is_expected.to include(have_attributes(name: "#{metrics_prefix}_latency_seconds", value: be >= 0)) }
+      it { is_expected.to include(have_attributes(name: "#{metrics_prefix}_latency", value: be >= 0)) }
     end
   end
 
diff --git a/spec/lib/gitlab/metrics/requests_rack_middleware_spec.rb b/spec/lib/gitlab/metrics/connection_rack_middleware_spec.rb
index 461b1e4182a..94251af305f 100644
--- a/spec/lib/gitlab/metrics/requests_rack_middleware_spec.rb
+++ b/spec/lib/gitlab/metrics/connection_rack_middleware_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe Gitlab::Metrics::RequestsRackMiddleware do
+describe Gitlab::Metrics::ConnectionRackMiddleware do
   let(:app) { double('app') }
   subject { described_class.new(app) }
 
@@ -22,8 +22,14 @@ describe Gitlab::Metrics::RequestsRackMiddleware do
         allow(app).to receive(:call).and_return([200, nil, nil])
       end
 
+      it 'increments response count with status label' do
+        expect(described_class).to receive_message_chain(:rack_response_count, :increment).with(include(status: 200, method: 'get'))
+
+        subject.call(env)
+      end
+
       it 'increments requests count' do
-        expect(described_class).to receive_message_chain(:http_request_total, :increment).with(method: 'get')
+        expect(described_class).to receive_message_chain(:rack_request_count, :increment).with(method: 'get')
 
         subject.call(env)
       end
@@ -32,21 +38,20 @@ describe Gitlab::Metrics::RequestsRackMiddleware do
         execution_time = 10
         allow(app).to receive(:call) do |*args|
           Timecop.freeze(execution_time.seconds)
-          [200, nil, nil]
         end
 
-        expect(described_class).to receive_message_chain(:http_request_duration_seconds, :observe).with({ status: 200, method: 'get' }, execution_time)
+        expect(described_class).to receive_message_chain(:rack_execution_time, :observe).with({}, execution_time)
 
         subject.call(env)
       end
     end
 
     context '@app.call throws exception' do
-      let(:http_request_duration_seconds) { double('http_request_duration_seconds') }
+      let(:rack_response_count) { double('rack_response_count') }
 
       before do
         allow(app).to receive(:call).and_raise(StandardError)
-        allow(described_class).to receive(:http_request_duration_seconds).and_return(http_request_duration_seconds)
+        allow(described_class).to receive(:rack_response_count).and_return(rack_response_count)
       end
 
       it 'increments exceptions count' do
@@ -56,13 +61,25 @@ describe Gitlab::Metrics::RequestsRackMiddleware do
       end
 
       it 'increments requests count' do
-        expect(described_class).to receive_message_chain(:http_request_total, :increment).with(method: 'get')
+        expect(described_class).to receive_message_chain(:rack_request_count, :increment).with(method: 'get')
+
+        expect { subject.call(env) }.to raise_error(StandardError)
+      end
+
+      it "does't increment response count" do
+        expect(described_class.rack_response_count).not_to receive(:increment)
 
         expect { subject.call(env) }.to raise_error(StandardError)
       end
 
-      it "does't measure request execution time" do
-        expect(described_class.http_request_duration_seconds).not_to receive(:increment)
+      it 'measures execution time' do
+        execution_time = 10
+        allow(app).to receive(:call) do |*args|
+          Timecop.freeze(execution_time.seconds)
+          raise StandardError
+        end
+
+        expect(described_class).to receive_message_chain(:rack_execution_time, :observe).with({}, execution_time)
 
         expect { subject.call(env) }.to raise_error(StandardError)
       end