From d3a7bdda986949ca76df3c4932ee8c973437a743 Mon Sep 17 00:00:00 2001 From: Jan Provaznik Date: Wed, 5 Jun 2019 19:20:37 +0200 Subject: Moved RackTimeout observer to a different location Because there will be similar observer for PumaWorkerKiller, it makes sense to keep both on better place. --- config/initializers/rack_timeout.rb | 2 +- lib/gitlab/cluster/rack_timeout_observer.rb | 48 ++++++++++++++++++ lib/gitlab/rack_timeout_observer.rb | 46 ----------------- .../gitlab/cluster/rack_timeout_observer_spec.rb | 58 ++++++++++++++++++++++ spec/lib/gitlab/rack_timeout_observer_spec.rb | 58 ---------------------- 5 files changed, 107 insertions(+), 105 deletions(-) create mode 100644 lib/gitlab/cluster/rack_timeout_observer.rb delete mode 100644 lib/gitlab/rack_timeout_observer.rb create mode 100644 spec/lib/gitlab/cluster/rack_timeout_observer_spec.rb delete mode 100644 spec/lib/gitlab/rack_timeout_observer_spec.rb diff --git a/config/initializers/rack_timeout.rb b/config/initializers/rack_timeout.rb index 5c4f2dd708c..58f46b55725 100644 --- a/config/initializers/rack_timeout.rb +++ b/config/initializers/rack_timeout.rb @@ -18,6 +18,6 @@ if defined?(::Puma) && !Rails.env.test? wait_timeout: 90) end - observer = Gitlab::RackTimeoutObserver.new + observer = Gitlab::Cluster::RackTimeoutObserver.new Rack::Timeout.register_state_change_observer(:gitlab_rack_timeout, &observer.callback) end diff --git a/lib/gitlab/cluster/rack_timeout_observer.rb b/lib/gitlab/cluster/rack_timeout_observer.rb new file mode 100644 index 00000000000..2bc006b8011 --- /dev/null +++ b/lib/gitlab/cluster/rack_timeout_observer.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +module Gitlab + module Cluster + class RackTimeoutObserver + def initialize + @counter = Gitlab::Metrics.counter(:rack_state_total, 'Number of requests in a given rack state') + end + + # returns the Proc to be used as the observer callback block + def callback + method(:log_timeout_exception) + end + + private + + def log_timeout_exception(env) + info = env[::Rack::Timeout::ENV_INFO_KEY] + return unless info + + @counter.increment(labels(info, env)) + end + + def labels(info, env) + params = controller_params(env) || grape_params(env) || {} + + { + controller: params['controller'], + action: params['action'], + route: params['route'], + state: info.state + } + end + + def controller_params(env) + env['action_dispatch.request.parameters'] + end + + def grape_params(env) + endpoint = env[Grape::Env::API_ENDPOINT] + route = endpoint&.route&.pattern&.origin + return unless route + + { 'route' => route } + end + end + end +end diff --git a/lib/gitlab/rack_timeout_observer.rb b/lib/gitlab/rack_timeout_observer.rb deleted file mode 100644 index 80d3f7dea60..00000000000 --- a/lib/gitlab/rack_timeout_observer.rb +++ /dev/null @@ -1,46 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - class RackTimeoutObserver - def initialize - @counter = Gitlab::Metrics.counter(:rack_state_total, 'Number of requests in a given rack state') - end - - # returns the Proc to be used as the observer callback block - def callback - method(:log_timeout_exception) - end - - private - - def log_timeout_exception(env) - info = env[::Rack::Timeout::ENV_INFO_KEY] - return unless info - - @counter.increment(labels(info, env)) - end - - def labels(info, env) - params = controller_params(env) || grape_params(env) || {} - - { - controller: params['controller'], - action: params['action'], - route: params['route'], - state: info.state - } - end - - def controller_params(env) - env['action_dispatch.request.parameters'] - end - - def grape_params(env) - endpoint = env[Grape::Env::API_ENDPOINT] - route = endpoint&.route&.pattern&.origin - return unless route - - { 'route' => route } - end - end -end diff --git a/spec/lib/gitlab/cluster/rack_timeout_observer_spec.rb b/spec/lib/gitlab/cluster/rack_timeout_observer_spec.rb new file mode 100644 index 00000000000..2adb1d7cc71 --- /dev/null +++ b/spec/lib/gitlab/cluster/rack_timeout_observer_spec.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Cluster::RackTimeoutObserver do + let(:counter) { Gitlab::Metrics::NullMetric.instance } + + before do + allow(Gitlab::Metrics).to receive(:counter) + .with(any_args) + .and_return(counter) + end + + describe '#callback' do + context 'when request times out' do + let(:env) do + { + ::Rack::Timeout::ENV_INFO_KEY => double(state: :timed_out), + 'action_dispatch.request.parameters' => { + 'controller' => 'foo', + 'action' => 'bar' + } + } + end + + subject { described_class.new } + + it 'increments timeout counter' do + expect(counter) + .to receive(:increment) + .with({ controller: 'foo', action: 'bar', route: nil, state: :timed_out }) + + subject.callback.call(env) + end + end + + context 'when request expires' do + let(:endpoint) { double } + let(:env) do + { + ::Rack::Timeout::ENV_INFO_KEY => double(state: :expired), + Grape::Env::API_ENDPOINT => endpoint + } + end + + subject { described_class.new } + + it 'increments timeout counter' do + allow(endpoint).to receive_message_chain('route.pattern.origin') { 'foobar' } + expect(counter) + .to receive(:increment) + .with({ controller: nil, action: nil, route: 'foobar', state: :expired }) + + subject.callback.call(env) + end + end + end +end diff --git a/spec/lib/gitlab/rack_timeout_observer_spec.rb b/spec/lib/gitlab/rack_timeout_observer_spec.rb deleted file mode 100644 index 3dc1a8b68fb..00000000000 --- a/spec/lib/gitlab/rack_timeout_observer_spec.rb +++ /dev/null @@ -1,58 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe Gitlab::RackTimeoutObserver do - let(:counter) { Gitlab::Metrics::NullMetric.instance } - - before do - allow(Gitlab::Metrics).to receive(:counter) - .with(any_args) - .and_return(counter) - end - - describe '#callback' do - context 'when request times out' do - let(:env) do - { - ::Rack::Timeout::ENV_INFO_KEY => double(state: :timed_out), - 'action_dispatch.request.parameters' => { - 'controller' => 'foo', - 'action' => 'bar' - } - } - end - - subject { described_class.new } - - it 'increments timeout counter' do - expect(counter) - .to receive(:increment) - .with({ controller: 'foo', action: 'bar', route: nil, state: :timed_out }) - - subject.callback.call(env) - end - end - - context 'when request expires' do - let(:endpoint) { double } - let(:env) do - { - ::Rack::Timeout::ENV_INFO_KEY => double(state: :expired), - Grape::Env::API_ENDPOINT => endpoint - } - end - - subject { described_class.new } - - it 'increments timeout counter' do - allow(endpoint).to receive_message_chain('route.pattern.origin') { 'foobar' } - expect(counter) - .to receive(:increment) - .with({ controller: nil, action: nil, route: 'foobar', state: :expired }) - - subject.callback.call(env) - end - end - end -end -- cgit v1.2.1 From 13364c00d57ce3150a50c23ea3eb0e6af4271d92 Mon Sep 17 00:00:00 2001 From: Jan Provaznik Date: Wed, 12 Jun 2019 12:33:34 +0200 Subject: Monitor only final states There is no reason to monitor transition states so we ignore ready and active states. We can get ratio of completed vs failed requests from final states. --- lib/gitlab/cluster/rack_timeout_observer.rb | 5 ++++- .../gitlab/cluster/rack_timeout_observer_spec.rb | 23 ++++++++++++++++++++-- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/lib/gitlab/cluster/rack_timeout_observer.rb b/lib/gitlab/cluster/rack_timeout_observer.rb index 2bc006b8011..5182b2be148 100644 --- a/lib/gitlab/cluster/rack_timeout_observer.rb +++ b/lib/gitlab/cluster/rack_timeout_observer.rb @@ -3,8 +3,10 @@ module Gitlab module Cluster class RackTimeoutObserver + TRANSITION_STATES = %i(ready active).freeze + def initialize - @counter = Gitlab::Metrics.counter(:rack_state_total, 'Number of requests in a given rack state') + @counter = Gitlab::Metrics.counter(:rack_requests_total, 'Number of requests in a given rack state') end # returns the Proc to be used as the observer callback block @@ -17,6 +19,7 @@ module Gitlab def log_timeout_exception(env) info = env[::Rack::Timeout::ENV_INFO_KEY] return unless info + return if TRANSITION_STATES.include?(info.state) @counter.increment(labels(info, env)) end diff --git a/spec/lib/gitlab/cluster/rack_timeout_observer_spec.rb b/spec/lib/gitlab/cluster/rack_timeout_observer_spec.rb index 2adb1d7cc71..68e5435450c 100644 --- a/spec/lib/gitlab/cluster/rack_timeout_observer_spec.rb +++ b/spec/lib/gitlab/cluster/rack_timeout_observer_spec.rb @@ -25,7 +25,7 @@ describe Gitlab::Cluster::RackTimeoutObserver do subject { described_class.new } - it 'increments timeout counter' do + it 'increments counter' do expect(counter) .to receive(:increment) .with({ controller: 'foo', action: 'bar', route: nil, state: :timed_out }) @@ -45,7 +45,7 @@ describe Gitlab::Cluster::RackTimeoutObserver do subject { described_class.new } - it 'increments timeout counter' do + it 'increments counter' do allow(endpoint).to receive_message_chain('route.pattern.origin') { 'foobar' } expect(counter) .to receive(:increment) @@ -54,5 +54,24 @@ describe Gitlab::Cluster::RackTimeoutObserver do subject.callback.call(env) end end + + context 'when request is being processed' do + let(:endpoint) { double } + let(:env) do + { + ::Rack::Timeout::ENV_INFO_KEY => double(state: :active), + Grape::Env::API_ENDPOINT => endpoint + } + end + + subject { described_class.new } + + it 'does not increment counter' do + allow(endpoint).to receive_message_chain('route.pattern.origin') { 'foobar' } + expect(counter).not_to receive(:increment) + + subject.callback.call(env) + end + end end end -- cgit v1.2.1