diff options
author | James Edwards-Jones <jedwardsjones@gitlab.com> | 2017-07-21 12:31:08 +0100 |
---|---|---|
committer | James Edwards-Jones <jedwardsjones@gitlab.com> | 2017-07-21 12:31:08 +0100 |
commit | bf4a1306ca7fa765f51c15514dc73b15feccffe7 (patch) | |
tree | c03208d4be9861285335cb4de318530399b2e536 | |
parent | b639ed9f07174c06424586a08db0ed0cde654e54 (diff) | |
download | gitlab-ce-bf4a1306ca7fa765f51c15514dc73b15feccffe7.tar.gz |
Revert "Merge branch 'bjk/metric_names' into 'master'"9-4-stable-prepare-rc6
This reverts commit b639ed9f07174c06424586a08db0ed0cde654e54.
-rw-r--r-- | app/controllers/sessions_controller.rb | 2 | ||||
-rw-r--r-- | config/initializers/8_metrics.rb | 2 | ||||
-rw-r--r-- | doc/administration/monitoring/prometheus/gitlab_metrics.md | 33 | ||||
-rw-r--r-- | lib/gitlab/health_checks/fs_shards_check.rb | 6 | ||||
-rw-r--r-- | lib/gitlab/health_checks/simple_abstract_check.rb | 2 | ||||
-rw-r--r-- | lib/gitlab/metrics/connection_rack_middleware.rb | 45 | ||||
-rw-r--r-- | lib/gitlab/metrics/requests_rack_middleware.rb | 40 | ||||
-rw-r--r-- | spec/controllers/metrics_controller_spec.rb | 34 | ||||
-rw-r--r-- | spec/lib/gitlab/health_checks/fs_shards_check_spec.rb | 18 | ||||
-rw-r--r-- | spec/lib/gitlab/health_checks/simple_check_shared.rb | 6 | ||||
-rw-r--r-- | spec/lib/gitlab/metrics/connection_rack_middleware_spec.rb (renamed from spec/lib/gitlab/metrics/requests_rack_middleware_spec.rb) | 35 |
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 |