Merge branch 'rack-timeout-rename' into 'master'

Moved RackTimeout observer to a different location and ignore transition states

See merge request gitlab-org/gitlab-ce!29214

4 files changed, 74 insertions, 50 deletions

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..5182b2be148
--- /dev/null
+++ b/lib/gitlab/cluster/rack_timeout_observer.rb
@@ -0,0 +1,51 @@
+# frozen_string_literal: true
+
+module Gitlab
+  module Cluster
+    class RackTimeoutObserver
+      TRANSITION_STATES = %i(ready active).freeze
+
+      def initialize
+        @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
+      def callback
+        method(:log_timeout_exception)
+      end
+
+      private
+
+      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
+
+      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/rack_timeout_observer_spec.rb b/spec/lib/gitlab/cluster/rack_timeout_observer_spec.rb
index 3dc1a8b68fb..68e5435450c 100644
--- a/spec/lib/gitlab/rack_timeout_observer_spec.rb
+++ b/spec/lib/gitlab/cluster/rack_timeout_observer_spec.rb
@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-describe Gitlab::RackTimeoutObserver do
+describe Gitlab::Cluster::RackTimeoutObserver do
   let(:counter) { Gitlab::Metrics::NullMetric.instance }
 
   before do
@@ -25,7 +25,7 @@ describe Gitlab::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::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::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