From 9032816f2e217ccaf2d806a2bc1af4e56b729117 Mon Sep 17 00:00:00 2001
From: Chance Feick <chance@librato.com>
Date: Tue, 24 May 2016 11:02:52 -0700
Subject: [PATCH 1/5] add instrument :all syntax

---
 CHANGELOG.md                                  |  3 +++
 lib/librato/rails/subscribers.rb              |  6 +++++-
 lib/librato/rails/subscribers/action.rb       |  2 +-
 test/dummy/app/controllers/home_controller.rb | 16 +++++++++-------
 test/integration/instrument_action_test.rb    | 12 ++++++++++++
 5 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 2c2b657..273bbf8 100644
--- a/CHANGELOG.md
+++ b/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
 
diff --git a/lib/librato/rails/subscribers.rb b/lib/librato/rails/subscribers.rb
index c7b11df..a86f116 100644
--- a/lib/librato/rails/subscribers.rb
+++ b/lib/librato/rails/subscribers.rb
@@ -13,7 +13,11 @@ def self.collector
 
       def self.watch_controller_action(controller, action)
         @watches ||= []
-        @watches << "#{controller}##{action}".freeze
+        if action == :all
+          @watches << "#{controller}".freeze
+        else
+          @watches << "#{controller}##{action}".freeze
+        end
       end
     end
   end
diff --git a/lib/librato/rails/subscribers/action.rb b/lib/librato/rails/subscribers/action.rb
index 3c3c9d3..0063d6b 100644
--- a/lib/librato/rails/subscribers/action.rb
+++ b/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|
 
diff --git a/test/dummy/app/controllers/home_controller.rb b/test/dummy/app/controllers/home_controller.rb
index 9f1fed1..5913669 100644
--- a/test/dummy/app/controllers/home_controller.rb
+++ b/test/dummy/app/controllers/home_controller.rb
@@ -1,31 +1,33 @@
 class HomeController < ApplicationController
+  instrument_action :all
+
   def index
   end
-  
+
   def custom
     # controller helpers
     Librato.group 'custom' do |g|
       g.increment 'visits'
       g.increment 'events', 3
-    
+
       g.timing 'timing', 3
       g.timing 'timing', 9
     end
-    
+
     # test class-level helpers for models
     User.do_custom_events
-    
+
     # test instance-level helpers for models
     user = User.new
     user.touch
-    
+
     render :nothing => true
   end
-  
+
   def boom
     raise 'test exception!'
   end
-  
+
   def slow
     sleep 0.3
     render :nothing => true
diff --git a/test/integration/instrument_action_test.rb b/test/integration/instrument_action_test.rb
index c3a80fa..146ffc8 100644
--- a/test/integration/instrument_action_test.rb
+++ b/test/integration/instrument_action_test.rb
@@ -19,4 +19,16 @@ class InstrumentActionTest < ActiveSupport::IntegrationCase
     assert_equal 1, counters.fetch("#{base}.total", source: source)
   end
 
+  test 'instrument all controller actions' do
+    2.times { visit custom_path }
+    visit slow_path
+
+    metric   = 'rails.action.request.time'
+    action_1 = 'custom'
+    action_2 = 'slow'
+
+    assert_equal 2, aggregate.fetch(metric, source: "HomeController.#{action_1}.html")[:count]
+    assert_equal 1, aggregate.fetch(metric, source: "HomeController.#{action_2}.html")[:count]
+  end
+
 end

From 9f69d4415242ec8088ea039ad3e2890648e77fbf Mon Sep 17 00:00:00 2001
From: Chance Feick <chance@librato.com>
Date: Mon, 30 May 2016 21:38:34 -0700
Subject: [PATCH 2/5] add controller inheritance support

---
 lib/librato/rails/railtie.rb                  |  4 ++-
 lib/librato/rails/subscribers.rb              | 30 ++++++++++++++---
 lib/librato/rails/subscribers/action.rb       |  2 +-
 .../app/controllers/application_controller.rb |  4 +--
 test/dummy/app/controllers/base_controller.rb | 11 +++++++
 .../app/controllers/derived_controller.rb     |  9 ++++++
 test/dummy/app/controllers/home_controller.rb | 16 ++++------
 .../instrument_action_controller.rb           |  2 +-
 .../controllers/intermediate_controller.rb    |  5 +++
 test/dummy/config/routes.rb                   |  6 ++++
 test/integration/instrument_action_test.rb    | 32 +++++++++++++++----
 test/unit/subscribers_test.rb                 | 20 ++++++++++++
 12 files changed, 116 insertions(+), 25 deletions(-)
 create mode 100644 test/dummy/app/controllers/base_controller.rb
 create mode 100644 test/dummy/app/controllers/derived_controller.rb
 create mode 100644 test/dummy/app/controllers/intermediate_controller.rb
 create mode 100644 test/unit/subscribers_test.rb

diff --git a/lib/librato/rails/railtie.rb b/lib/librato/rails/railtie.rb
index d92dfa7..8d9a7a9 100644
--- a/lib/librato/rails/railtie.rb
+++ b/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
 
@@ -15,6 +15,8 @@ class Railtie < ::Rails::Railtie
         Librato.register_tracker(tracker)
       end
 
+      config.after_initialize { Librato::Rails::Subscribers.track_controller_descendants }
+
       initializer 'librato_rails.setup' do |app|
 
         ActiveSupport.on_load :action_controller do
diff --git a/lib/librato/rails/subscribers.rb b/lib/librato/rails/subscribers.rb
index a86f116..33dc8a5 100644
--- a/lib/librato/rails/subscribers.rb
+++ b/lib/librato/rails/subscribers.rb
@@ -13,11 +13,33 @@ def self.collector
 
       def self.watch_controller_action(controller, action)
         @watches ||= []
-        if action == :all
-          @watches << "#{controller}".freeze
-        else
-          @watches << "#{controller}##{action}".freeze
+
+        watch =
+          if action == :all
+            "#{controller}".freeze
+          else
+            "#{controller}##{action}".freeze
+          end
+
+        return @watches if @watches.include?(watch)
+        @watches << watch
+      end
+
+      def self.watch_controller_descendants_for(controller)
+        klass = controller.is_a?(String) ? controller.constantize : controller
+        return @watches if klass.descendants.empty? # base case
+
+        klass.descendants.each do |descendant|
+          Subscribers.watch_controller_action(descendant, :all)
+          Subscribers.watch_controller_descendants_for(descendant)
         end
+
+        @watches
+      end
+
+      def self.track_controller_descendants
+        controllers = @watches.reject { |c| c.include?('#') } # specific controller actions do not have descendants
+        controllers.each { |c| Subscribers.watch_controller_descendants_for(c) }
       end
     end
   end
diff --git a/lib/librato/rails/subscribers/action.rb b/lib/librato/rails/subscribers/action.rb
index 0063d6b..380e9a8 100644
--- a/lib/librato/rails/subscribers/action.rb
+++ b/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}") || @watches.index("#{controller}##{action}"))
+        if @watches && (@watches.index(controller) || @watches.index("#{controller}##{action}"))
           source = "#{controller}.#{action}.#{format}"
           collector.group 'rails.action.request' do |r|
 
diff --git a/test/dummy/app/controllers/application_controller.rb b/test/dummy/app/controllers/application_controller.rb
index 245f328..508b77a 100644
--- a/test/dummy/app/controllers/application_controller.rb
+++ b/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
diff --git a/test/dummy/app/controllers/base_controller.rb b/test/dummy/app/controllers/base_controller.rb
new file mode 100644
index 0000000..9dbd0c5
--- /dev/null
+++ b/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
diff --git a/test/dummy/app/controllers/derived_controller.rb b/test/dummy/app/controllers/derived_controller.rb
new file mode 100644
index 0000000..1a63ea7
--- /dev/null
+++ b/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
diff --git a/test/dummy/app/controllers/home_controller.rb b/test/dummy/app/controllers/home_controller.rb
index 5913669..9f1fed1 100644
--- a/test/dummy/app/controllers/home_controller.rb
+++ b/test/dummy/app/controllers/home_controller.rb
@@ -1,33 +1,31 @@
 class HomeController < ApplicationController
-  instrument_action :all
-
   def index
   end
-
+  
   def custom
     # controller helpers
     Librato.group 'custom' do |g|
       g.increment 'visits'
       g.increment 'events', 3
-
+    
       g.timing 'timing', 3
       g.timing 'timing', 9
     end
-
+    
     # test class-level helpers for models
     User.do_custom_events
-
+    
     # test instance-level helpers for models
     user = User.new
     user.touch
-
+    
     render :nothing => true
   end
-
+  
   def boom
     raise 'test exception!'
   end
-
+  
   def slow
     sleep 0.3
     render :nothing => true
diff --git a/test/dummy/app/controllers/instrument_action_controller.rb b/test/dummy/app/controllers/instrument_action_controller.rb
index d29b640..aaf8ead 100644
--- a/test/dummy/app/controllers/instrument_action_controller.rb
+++ b/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
 
diff --git a/test/dummy/app/controllers/intermediate_controller.rb b/test/dummy/app/controllers/intermediate_controller.rb
new file mode 100644
index 0000000..9da04ba
--- /dev/null
+++ b/test/dummy/app/controllers/intermediate_controller.rb
@@ -0,0 +1,5 @@
+class IntermediateController < BaseController
+  def action_1
+    render nothing: true
+  end
+end
diff --git a/test/dummy/config/routes.rb b/test/dummy/config/routes.rb
index 6c7801a..27dbfb2 100644
--- a/test/dummy/config/routes.rb
+++ b/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.
 
diff --git a/test/integration/instrument_action_test.rb b/test/integration/instrument_action_test.rb
index 146ffc8..9ed4450 100644
--- a/test/integration/instrument_action_test.rb
+++ b/test/integration/instrument_action_test.rb
@@ -20,15 +20,33 @@ class InstrumentActionTest < ActiveSupport::IntegrationCase
   end
 
   test 'instrument all controller actions' do
-    2.times { visit custom_path }
-    visit slow_path
+    visit base_action_1_path
+    visit base_action_2_path
 
-    metric   = 'rails.action.request.time'
-    action_1 = 'custom'
-    action_2 = 'slow'
+    metric = 'rails.action.request.time'
 
-    assert_equal 2, aggregate.fetch(metric, source: "HomeController.#{action_1}.html")[:count]
-    assert_equal 1, aggregate.fetch(metric, source: "HomeController.#{action_2}.html")[:count]
+    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
diff --git a/test/unit/subscribers_test.rb b/test/unit/subscribers_test.rb
new file mode 100644
index 0000000..fa632ac
--- /dev/null
+++ b/test/unit/subscribers_test.rb
@@ -0,0 +1,20 @@
+require 'test_helper'
+
+module Librato
+  module Rails
+    class SubscribersTest < MiniTest::Unit::TestCase
+
+      def test_watch_controller_descendants
+        expected = %w(BaseController IntermediateController DerivedController)
+        actual = Subscribers.watch_controller_descendants_for('BaseController')
+
+        assert expected.to_set.subset?(actual.to_set)
+      end
+
+      def test_track_descendants
+        assert Subscribers.track_controller_descendants.exclude?('#')
+      end
+
+    end
+  end
+end

From dd52142b118961c09986767724822d3facefbf9e Mon Sep 17 00:00:00 2001
From: Chance Feick <chance@librato.com>
Date: Thu, 2 Jun 2016 14:55:59 -0700
Subject: [PATCH 3/5] update README

---
 README.md | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/README.md b/README.md
index 55271a4..43859d0 100644
--- a/README.md
+++ b/README.md
@@ -255,7 +255,6 @@ Symbols can be used interchangably with strings for metric names.
 `librato-rails` also has special helpers which are available inside your controllers:
 
 #### instrument_action
-_experimental_, this interface may change:
 
 Use when you want to profile execution time or request volume for a specific controller action:
 
@@ -269,6 +268,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)

From 85080e192ecc8c25bce376000802bc748bba1ea4 Mon Sep 17 00:00:00 2001
From: Brian DeHamer <brian@dehamer.com>
Date: Mon, 20 Jun 2016 21:06:19 -0700
Subject: [PATCH 4/5] alt implementation for controller inheritance

---
 lib/librato/rails/helpers/controller.rb |  7 ++++++-
 lib/librato/rails/railtie.rb            |  2 --
 lib/librato/rails/subscribers.rb        | 19 +++----------------
 test/unit/subscribers_test.rb           | 20 --------------------
 4 files changed, 9 insertions(+), 39 deletions(-)
 delete mode 100644 test/unit/subscribers_test.rb

diff --git a/lib/librato/rails/helpers/controller.rb b/lib/librato/rails/helpers/controller.rb
index 31844ac..88ad38d 100644
--- a/lib/librato/rails/helpers/controller.rb
+++ b/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
\ No newline at end of file
+end
diff --git a/lib/librato/rails/railtie.rb b/lib/librato/rails/railtie.rb
index 8d9a7a9..19aace1 100644
--- a/lib/librato/rails/railtie.rb
+++ b/lib/librato/rails/railtie.rb
@@ -15,8 +15,6 @@ class Railtie < ::Rails::Railtie
         Librato.register_tracker(tracker)
       end
 
-      config.after_initialize { Librato::Rails::Subscribers.track_controller_descendants }
-
       initializer 'librato_rails.setup' do |app|
 
         ActiveSupport.on_load :action_controller do
diff --git a/lib/librato/rails/subscribers.rb b/lib/librato/rails/subscribers.rb
index 33dc8a5..12c2fa8 100644
--- a/lib/librato/rails/subscribers.rb
+++ b/lib/librato/rails/subscribers.rb
@@ -21,25 +21,12 @@ def self.watch_controller_action(controller, action)
             "#{controller}##{action}".freeze
           end
 
-        return @watches if @watches.include?(watch)
         @watches << watch
       end
 
-      def self.watch_controller_descendants_for(controller)
-        klass = controller.is_a?(String) ? controller.constantize : controller
-        return @watches if klass.descendants.empty? # base case
-
-        klass.descendants.each do |descendant|
-          Subscribers.watch_controller_action(descendant, :all)
-          Subscribers.watch_controller_descendants_for(descendant)
-        end
-
-        @watches
-      end
-
-      def self.track_controller_descendants
-        controllers = @watches.reject { |c| c.include?('#') } # specific controller actions do not have descendants
-        controllers.each { |c| Subscribers.watch_controller_descendants_for(c) }
+      def self.inherit_watches(base, descendant)
+        @watches ||= []
+        @watches << descendant.freeze if @watches.include?(base)
       end
     end
   end
diff --git a/test/unit/subscribers_test.rb b/test/unit/subscribers_test.rb
deleted file mode 100644
index fa632ac..0000000
--- a/test/unit/subscribers_test.rb
+++ /dev/null
@@ -1,20 +0,0 @@
-require 'test_helper'
-
-module Librato
-  module Rails
-    class SubscribersTest < MiniTest::Unit::TestCase
-
-      def test_watch_controller_descendants
-        expected = %w(BaseController IntermediateController DerivedController)
-        actual = Subscribers.watch_controller_descendants_for('BaseController')
-
-        assert expected.to_set.subset?(actual.to_set)
-      end
-
-      def test_track_descendants
-        assert Subscribers.track_controller_descendants.exclude?('#')
-      end
-
-    end
-  end
-end

From 1a0e2423f84818ac4371ad090e2a27f6f1edcf85 Mon Sep 17 00:00:00 2001
From: Chance Feick <chance@librato.com>
Date: Tue, 21 Jun 2016 10:00:19 -0700
Subject: [PATCH 5/5] re-add experimental inteface warning

---
 README.md | 1 +
 1 file changed, 1 insertion(+)

diff --git a/README.md b/README.md
index 43859d0..4633595 100644
--- a/README.md
+++ b/README.md
@@ -255,6 +255,7 @@ Symbols can be used interchangably with strings for metric names.
 `librato-rails` also has special helpers which are available inside your controllers:
 
 #### instrument_action
+_experimental_, this interface may change:
 
 Use when you want to profile execution time or request volume for a specific controller action: