Subscribe to additional Action controller topics

lib/new_relic/agent/instrumentation/action_controller_other_subscriber.rb

@@ -0,0 +1,46 @@
+# This file is distributed under New Relic's license terms.
+# See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details.
+# frozen_string_literal: true
+
+require 'new_relic/agent/instrumentation/notifications_subscriber'
+require 'new_relic/agent/instrumentation/ignore_actions'
+require 'new_relic/agent/parameter_filtering'
+
+module NewRelic
+  module Agent
+    module Instrumentation
+      class ActionControllerOtherSubscriber < NotificationsSubscriber
+        def start_segment(name, id, payload)
+          segment = Tracer.start_segment(name: metric_name(name, payload))
+
+          add_segment_params(segment, payload, name)
+          push_segment(id, segment)
+        end
+
+        def add_segment_params(segment, payload, name)
+          segment.params[:filter] = payload[:filter] if payload[:filter]
+          segment.params[:keys] = payload[:keys] if payload[:keys]
+          segment.params[:original_path] = payload[:request].original_fullpath if payload[:request]
+
+          if payload[:context]
+            segment.params[:action] = payload[:context][:action]
+            segment.params[:controller] = payload[:context][:controller]
+          end
+        end
+
+        def metric_name(name, payload)
+          controller_name = controller_name_for_metric(payload)
+          "Ruby/ActionController#{"/#{controller_name}" if controller_name}/#{name.gsub(/\.action_controller/, '')}"
+        end
+
+        def controller_name_for_metric(payload)
+          # redirect_to
+          return payload[:request].controller_class.controller_path if payload[:request] && payload[:request].controller_class
+
+          # unpermitted_parameters
+          ::NewRelic::LanguageSupport.constantize(payload[:context][:controller]).controller_path if payload[:context] && payload[:context][:controller]
+        end
+      end
+    end
+  end
+end

lib/new_relic/agent/instrumentation/active_storage_subscriber.rb

@@ -8,22 +8,6 @@ module NewRelic
   module Agent
     module Instrumentation
       class ActiveStorageSubscriber < NotificationsSubscriber
-        def start(name, id, payload)
-          return unless state.is_execution_traced?
-
-          start_segment(name, id, payload)
-        rescue => e
-          log_notification_error(e, name, 'start')
-        end
-
-        def finish(name, id, payload)
-          return unless state.is_execution_traced?
-
-          finish_segment(id, payload)
-        rescue => e
-          log_notification_error(e, name, 'finish')
-        end
-
         def start_segment(name, id, payload)
           segment = Tracer.start_segment(name: metric_name(name, payload))
           segment.params[:key] = payload[:key]

lib/new_relic/agent/instrumentation/active_support_subscriber.rb

@@ -8,22 +8,6 @@ module NewRelic
   module Agent
     module Instrumentation
       class ActiveSupportSubscriber < NotificationsSubscriber
-        def start(name, id, payload)
-          return unless state.is_execution_traced?
-
-          start_segment(name, id, payload)
-        rescue => e
-          log_notification_error(e, name, 'start')
-        end
-
-        def finish(name, id, payload)
-          return unless state.is_execution_traced?
-
-          finish_segment(id, payload)
-        rescue => e
-          log_notification_error(e, name, 'finish')
-        end
-
         def start_segment(name, id, payload)
           segment = Tracer.start_segment(name: metric_name(name, payload))
 

lib/new_relic/agent/instrumentation/notifications_subscriber.rb

@@ -47,6 +47,30 @@ def self.subscribe(pattern)
           end
         end
 
+        def start(name, id, payload)
+          return unless state.is_execution_traced?
+
+          start_segment(name, id, payload)
+        rescue => e
+          log_notification_error(e, name, 'start')
+        end
+
+        def finish(name, id, payload)
+          return unless state.is_execution_traced?
+
+          finish_segment(id, payload)
+        rescue => e
+          log_notification_error(e, name, 'finish')
+        end
+
+        def start_segment(name, id, payload)
+          raise NotImplementedError
+        end
+
+        def finish_segment(name, id, payload)
+          raise NotImplementedError
+        end
+
         def log_notification_error(error, name, event_type)
           # These are important enough failures that we want the backtraces
           # logged at error level, hence the explicit log_exception call.

lib/new_relic/agent/instrumentation/rails_notifications/action_controller.rb

@@ -3,6 +3,7 @@
 # frozen_string_literal: true
 
 require 'new_relic/agent/instrumentation/action_controller_subscriber'
+require 'new_relic/agent/instrumentation/action_controller_other_subscriber'
 require 'new_relic/agent/prepend_supportability'
 
 DependencyDetection.defer do
@@ -29,5 +30,15 @@
 
     NewRelic::Agent::Instrumentation::ActionControllerSubscriber \
       .subscribe(/^process_action.action_controller$/)
+
+    subs = %w[send_file
+      send_data
+      redirect_to
+      halted_callback
+      unpermitted_parameters]
+
+    # have to double escape period because its going from string -> regex
+    NewRelic::Agent::Instrumentation::ActionControllerOtherSubscriber \
+      .subscribe(Regexp.new("^(#{subs.join('|')})\\.action_controller$"))
   end
 end

test/multiverse/suites/rails/action_controller_other_test.rb

@@ -0,0 +1,112 @@
+# This file is distributed under New Relic's license terms.
+# See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details.
+# frozen_string_literal: true
+
+require './app'
+
+if defined?(ActionController::Live)
+
+  class DataController < ApplicationController
+    # send_file
+    def send_test_file
+      send_file(Rails.root + "../../../../README.md")
+    end
+
+    # send_data
+    def send_test_data
+      send_data("wow its a adata")
+    end
+
+    # halted_callback
+    before_action :do_a_redirect, only: :halt_my_callback
+    def halt_my_callback; end
+
+    # redirect_to
+    def do_a_redirect
+      redirect_to("http://foo.bar/")
+    end
+
+    # unpermitted_parameters
+    def not_allowed
+      params.permit(:only_this)
+    end
+  end
+
+  class ActionControllerDataTest < ActionDispatch::IntegrationTest
+    include MultiverseHelpers
+
+    def teardown
+      NewRelic::Agent.drop_buffered_data
+    end
+
+    def test_send_file
+      get('/data/send_test_file')
+
+      assert_metrics_recorded(['Controller/data/send_test_file', 'Ruby/ActionController/send_file'])
+    end
+
+    def test_send_data
+      get('/data/send_test_data')
+
+      assert_metrics_recorded(['Controller/data/send_test_data', 'Ruby/ActionController/send_data'])
+    end
+
+    def test_halted_callback
+      get('/data/halt_my_callback')
+
+      trace = last_transaction_trace
+      tt_node = find_node_with_name(trace, 'Ruby/ActionController/halted_callback')
+
+      # depending on the rails version, the value will be either
+      # "do_a_redirect" OR :do_a_redirect OR ":do_a_redirect"
+      assert_includes(tt_node.params[:filter].to_s, "do_a_redirect")
+      assert_metrics_recorded(['Controller/data/halt_my_callback', 'Ruby/ActionController/halted_callback'])
+    end
+
+    def test_redirect_to
+      get('/data/do_a_redirect')
+
+      # payload does not include the request in rails < 6.1
+      rails61 = Gem::Version.new(Rails::VERSION::STRING) >= Gem::Version.new('6.1.0')
+
+      segment_name = if rails61
+        'Ruby/ActionController/data/redirect_to'
+      else
+        'Ruby/ActionController/redirect_to'
+      end
+
+      trace = last_transaction_trace
+      tt_node = find_node_with_name(trace, segment_name)
+
+      assert_equal('/data/do_a_redirect', tt_node.params[:original_path]) if rails61
+
+      assert_metrics_recorded(['Controller/data/do_a_redirect', segment_name])
+    end
+
+    def test_unpermitted_parameters
+      skip if Gem::Version.new(Rails::VERSION::STRING) < Gem::Version.new('6.0.0')
+      # unpermitted parameters is only available in rails 6.0+
+
+      get('/data/not_allowed', params: {this_is_a_param: 1})
+
+      # in Rails < 7 the context key is not present in this payload, so it changes the params and name
+      # because we're using context info to create the name
+      rails7 = Gem::Version.new(Rails::VERSION::STRING) >= Gem::Version.new('7.0.0')
+
+      segment_name = if rails7
+        'Ruby/ActionController/data/unpermitted_parameters'
+      else
+        'Ruby/ActionController/unpermitted_parameters'
+      end
+
+      trace = last_transaction_trace
+      tt_node = find_node_with_name(trace, segment_name)
+
+      assert_equal(['this_is_a_param'], tt_node.params[:keys])
+      assert_equal('not_allowed', tt_node.params[:action]) if rails7
+      assert_equal('DataController', tt_node.params[:controller]) if rails7
+
+      assert_metrics_recorded(['Controller/data/not_allowed', segment_name])
+    end
+  end
+end

test/new_relic/agent/instrumentation/notification_subscriber_test.rb

@@ -0,0 +1,12 @@
+# This file is distributed under New Relic's license terms.
+# See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details.
+# frozen_string_literal: true
+
+require_relative '../../../test_helper'
+require 'new_relic/agent/instrumentation/notifications_subscriber'
+
+if defined?(ActiveSupport)
+  require_relative 'rails/notifications_subscriber'
+else
+  puts "Skipping tests in #{__FILE__} because Active Support is unavailable"
+end

test/new_relic/agent/instrumentation/rails/active_support_subscriber.rb

@@ -102,8 +102,6 @@ def test_hit_recorded_as_attribute_on_traces
           trace = last_transaction_trace
           tt_node = find_node_with_name(trace, "#{METRIC_PREFIX}#{DEFAULT_STORE}/read")
 
-          puts txn.segments.last.inspect
-
           assert tt_node.params.key?(:hit)
           refute tt_node.params[:hit]
         end
@@ -116,8 +114,6 @@ def test_super_operation_recorded_as_attribute_on_traces
           trace = last_transaction_trace
           tt_node = find_node_with_name(trace, "#{METRIC_PREFIX}#{DEFAULT_STORE}/read")
 
-          puts txn.segments.last.inspect
-
           assert tt_node.params.key?(:super_operation)
           refute tt_node.params[:super_operation]
         end
@@ -163,42 +159,6 @@ def test_pop_segment_returns_false
           end
         end
 
-        def test_start_logs_notification_error
-          logger = MiniTest::Mock.new
-
-          NewRelic::Agent.stub :logger, logger do
-            logger.expect :error, nil, [/Error during .* callback/]
-            logger.expect :log_exception, nil, [:error, ArgumentError]
-
-            in_transaction do |txn|
-              @subscriber.stub :start_segment, -> { raise 'kaboom' } do
-                @subscriber.start(DEFAULT_EVENT, @id, {})
-              end
-
-              assert_equal 1, txn.segments.size
-            end
-          end
-          logger.verify
-        end
-
-        def test_finish_logs_notification_error
-          logger = MiniTest::Mock.new
-
-          NewRelic::Agent.stub :logger, logger do
-            logger.expect :error, nil, [/Error during .* callback/]
-            logger.expect :log_exception, nil, [:error, ArgumentError]
-
-            in_transaction do |txn|
-              @subscriber.stub :finish_segment, -> { raise 'kaboom' } do
-                @subscriber.finish(DEFAULT_EVENT, @id, {})
-              end
-
-              assert_equal 1, txn.segments.size
-            end
-          end
-          logger.verify
-        end
-
         def test_an_actual_active_storage_cache_write
           unless defined?(ActiveSupport::VERSION::MAJOR) && ActiveSupport::VERSION::MAJOR >= 5
             skip 'Test restricted to Active Support v5+'

test/new_relic/agent/instrumentation/rails/notifications_subscriber.rb

@@ -0,0 +1,62 @@
+# This file is distributed under New Relic's license terms.
+# See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details.
+# frozen_string_literal: true
+
+require_relative '../../../../test_helper'
+
+module NewRelic
+  module Agent
+    module Instrumentation
+      class NotificationsSubscriberTest < Minitest::Test
+        DEFAULT_EVENT = "any.event"
+
+        def setup
+          nr_freeze_process_time
+          @subscriber = NotificationsSubscriber.new
+          NewRelic::Agent.drop_buffered_data
+          @id = fake_guid(32)
+        end
+
+        def teardown
+          NewRelic::Agent.drop_buffered_data
+        end
+
+        def test_start_logs_notification_error
+          logger = MiniTest::Mock.new
+
+          NewRelic::Agent.stub :logger, logger do
+            logger.expect :error, nil, [/Error during .* callback/]
+            logger.expect :log_exception, nil, [:error, ArgumentError]
+
+            in_transaction do |txn|
+              @subscriber.stub :start_segment, -> { raise 'kaboom' } do
+                @subscriber.start(DEFAULT_EVENT, @id, {})
+              end
+
+              assert_equal 1, txn.segments.size
+            end
+          end
+          logger.verify
+        end
+
+        def test_finish_logs_notification_error
+          logger = MiniTest::Mock.new
+
+          NewRelic::Agent.stub :logger, logger do
+            logger.expect :error, nil, [/Error during .* callback/]
+            logger.expect :log_exception, nil, [:error, ArgumentError]
+
+            in_transaction do |txn|
+              @subscriber.stub :finish_segment, -> { raise 'kaboom' } do
+                @subscriber.finish(DEFAULT_EVENT, @id, {})
+              end
+
+              assert_equal 1, txn.segments.size
+            end
+          end
+          logger.verify
+        end
+      end
+    end
+  end
+end