Merge pull request #104 from librato/feature/instrument_action_all

Instrument all controller actions

CHANGELOG.md

@@ -1,3 +1,6 @@
+### master
+* Add support to instrument all controller actions, e.g., `instrument_action :all`.
+
 ### Version 1.3.0
 * Add support for configurable metric suites
 

README.md

@@ -269,6 +269,22 @@ class CommentController < ApplicationController
 end
 ```
 
+Optionally, you can instrument all controller actions:
+
+```ruby
+class ArticlesController < ApplicationController
+  instrument_action :all
+
+  def create
+    # ...
+  end
+
+  def show
+    # ...
+  end
+end
+```
+
 Once you instrument an action, `librato-rails` will start reporting a set of metrics specific to that action including:
 
 * rails.action.request.total     (# of requests)

lib/librato/rails/helpers/controller.rb

@@ -10,7 +10,12 @@ def instrument_action(*actions)
           end
         end
 
+        def inherited(other)
+          super
+          Subscribers.inherit_watches(self.to_s, other.to_s)
+        end
+
       end
     end
   end
-end
+end

lib/librato/rails/railtie.rb

@@ -5,7 +5,7 @@ class Railtie < ::Rails::Railtie
       # don't have any custom http vars anymore, check if hostname is UUID
       on_heroku = Socket.gethostname =~ /[a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12}/i
 
-      config.before_configuration do 
+      config.before_configuration do
         # make configuration proxy for config inside Rails
         config.librato_rails = Configuration.new
 

lib/librato/rails/subscribers.rb

@@ -13,7 +13,20 @@ def self.collector
 
       def self.watch_controller_action(controller, action)
         @watches ||= []
-        @watches << "#{controller}##{action}".freeze
+
+        watch =
+          if action == :all
+            "#{controller}".freeze
+          else
+            "#{controller}##{action}".freeze
+          end
+
+        @watches << watch
+      end
+
+      def self.inherit_watches(base, descendant)
+        @watches ||= []
+        @watches << descendant.freeze if @watches.include?(base)
       end
     end
   end

lib/librato/rails/subscribers/action.rb

@@ -14,7 +14,7 @@ module Subscribers
         format = "all" if format == "*/*"
         exception = event.payload[:exception]
 
-        if @watches && @watches.index("#{controller}##{action}")
+        if @watches && (@watches.index(controller) || @watches.index("#{controller}##{action}"))
           source = "#{controller}.#{action}.#{format}"
           collector.group 'rails.action.request' do |r|
 

test/dummy/app/controllers/application_controller.rb

@@ -1,8 +1,8 @@
 class ApplicationController < ActionController::Base
   protect_from_forgery
-
+  instrument_action :all
   #after_filter :flush_metrics_rails
-  
+
   # manually flush per request
   def flush_metrics_rails
     Librato::Rails.flush

test/dummy/app/controllers/base_controller.rb

@@ -0,0 +1,11 @@
+class BaseController < ApplicationController
+  instrument_action :all
+
+  def action_1
+    render nothing: true
+  end
+
+  def action_2
+    render nothing: true
+  end
+end

test/dummy/app/controllers/derived_controller.rb

@@ -0,0 +1,9 @@
+class DerivedController < IntermediateController
+  def action_1
+    render nothing: true
+  end
+
+  def action_2
+    render nothing: true
+  end
+end

test/dummy/app/controllers/instrument_action_controller.rb

@@ -10,7 +10,7 @@ def inst
     end
   end
 
-  def not_instrumented
+  def not
     render nothing: true
   end
 

test/dummy/app/controllers/intermediate_controller.rb

@@ -0,0 +1,5 @@
+class IntermediateController < BaseController
+  def action_1
+    render nothing: true
+  end
+end

test/dummy/config/routes.rb

@@ -20,6 +20,12 @@
   get 'instrument/inst' => 'instrument_action#inst', :as => :instrument_action
   get 'instrument/not'  => 'instrument_action#not',  :as => :not_instrumented
 
+  get 'base/action_1'         => 'base#action_1',         :as => :base_action_1
+  get 'base/action_2'         => 'base#action_2',         :as => :base_action_2
+  get 'intermediate/action_1' => 'intermediate#action_1', :as => :intermediate_action_1
+  get 'derived/action_1'      => 'derived#action_1',      :as => :derived_action_1
+  get 'derived/action_2'      => 'derived#action_2',      :as => :derived_action_2
+
   # The priority is based upon order of creation:
   # first created -> highest priority.
 

test/integration/instrument_action_test.rb

@@ -19,4 +19,34 @@ class InstrumentActionTest < ActiveSupport::IntegrationCase
     assert_equal 1, counters.fetch("#{base}.total", source: source)
   end
 
+  test 'instrument all controller actions' do
+    visit base_action_1_path
+    visit base_action_2_path
+
+    metric = 'rails.action.request.time'
+
+    assert_equal 1, aggregate.fetch(metric, source: 'BaseController.action_1.html')[:count]
+    assert_equal 1, aggregate.fetch(metric, source: 'BaseController.action_2.html')[:count]
+  end
+
+  test 'instrument all controller actions for inherited controllers' do
+    visit intermediate_action_1_path
+    visit derived_action_1_path
+    visit derived_action_2_path
+
+    metric = 'rails.action.request.time'
+
+    assert_equal 1, aggregate.fetch(metric, source: 'IntermediateController.action_1.html')[:count]
+    assert_equal 1, aggregate.fetch(metric, source: 'DerivedController.action_1.html')[:count]
+    assert_equal 1, aggregate.fetch(metric, source: 'DerivedController.action_2.html')[:count]
+  end
+
+  test 'instrument all controller actions for all controllers' do
+    visit not_instrumented_path
+
+    metric = 'rails.action.request.time'
+
+    assert_equal 1, aggregate.fetch(metric, source: 'InstrumentActionController.not.html')[:count]
+  end
+
 end