Merge pull request #2061 from newrelic/mlt_controller_without_method

Controller methods not in controller class don't warn

CHANGELOG.md

@@ -2,7 +2,7 @@
 
 ## dev
 
-Version <dev> of the agent adds the ability to filter logs by level and expands instrumentation for Action Cable.
+Version <dev> of the agent adds the ability to filter logs by level, expands instrumentation for Action Cable, and provides a bugfix for Code-Level Metrics.
 
 - **Feature: Filter forwarded logs based on level**
 
@@ -20,6 +20,10 @@ Version <dev> of the agent adds the ability to filter logs by level and expands
     * `transmit_subscription_confirmation.action_cable`
     * `transmit_subscription_rejection.action_cable`
 
+- **Bugfix: Report Code-Level Metrics for Rails controller methods**
+
+  Controllers in Rails automatically render views with names that correspond to valid routes. This means that a controller method may not have a corresponding method in the controller class. Code-Level Metrics now report on these methods and don't log false warnings. Thanks to [@jcrisp](https://github.com/jcrisp) for reporting this issue. [PR#2061](https://github.com/newrelic/newrelic-ruby-agent/pull/2061)
+
 ## v9.2.2
 
   Version 9.2.2 of the agent fixes a bug with the `Transaction#finished?` method.

lib/new_relic/agent/method_tracer_helpers.rb

@@ -46,13 +46,15 @@ def code_information(object, method_name)
         cache_key = "#{object.object_id}#{method_name}".freeze
         return @code_information[cache_key] if @code_information.key?(cache_key)
 
-        namespace, location, is_class_method = namespace_and_location(object, method_name.to_sym)
+        info = namespace_and_location(object, method_name.to_sym)
+        return ::NewRelic::EMPTY_HASH if info.empty?
 
+        namespace, location, is_class_method = info
         @code_information[cache_key] = {filepath: location.first,
                                         lineno: location.last,
                                         function: "#{'self.' if is_class_method}#{method_name}",
                                         namespace: namespace}.freeze
-      rescue => e
+      rescue StandardError => e
         ::NewRelic::Agent.logger.warn("Unable to determine source code info for '#{object}', " \
                                         "method '#{method_name}' - #{e.class}: #{e.message}")
         ::NewRelic::Agent.increment_metric(SOURCE_CODE_INFORMATION_FAILURE_METRIC, 1)
@@ -101,6 +103,9 @@ def namespace_and_location(object, method_name)
         klass = object.singleton_class? ? klassify_singleton(object) : object
         name = klass.name || '(Anonymous)'
         is_class_method = false
+
+        return controller_info(klass, name, is_class_method) if controller_without_method?(klass, method_name)
+
         method = if (klass.instance_methods + klass.private_instance_methods).include?(method_name)
           klass.instance_method(method_name)
         else
@@ -109,6 +114,22 @@ def namespace_and_location(object, method_name)
         end
         [name, method.source_location, is_class_method]
       end
+
+      # Rails controllers can be a special case because by default, controllers in Rails
+      # automatically render views with names that correspond to valid routes. This means
+      # that a controller method may not have a corresponding method in the controller class.
+      def controller_without_method?(klass, method_name)
+        defined?(Rails) &&
+          defined?(ApplicationController) &&
+          klass < ApplicationController &&
+          !klass.method_defined?(method_name)
+      end
+
+      def controller_info(klass, name, is_class_method)
+        path = Rails.root.join("app/controllers/#{klass.name.underscore}.rb")
+
+        File.exist?(path) ? [name, [path.to_s, 1], is_class_method] : []
+      end
     end
   end
 end

test/README.md

@@ -33,7 +33,7 @@ Also, if you set it to the full path instead of relative, then you can run it fr
 Then you'll be able to run the tests super easy like
 
     bert # run all unit tests
-    bere 61 # run env tests for rails 6.1    
+    bere 61 # run env tests for rails 6.1
     berm rake # run all rake multiverse suites
     bermq rake # run multiverse rake env=0 method=prepend
     ber -h # explains all the args for each option

test/environments/rails70/app/controllers/application_controller.rb

@@ -0,0 +1,6 @@
+# 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
+
+class ApplicationController < ActionController::Base
+end

test/environments/rails70/app/controllers/no_method_controller.rb

@@ -0,0 +1,7 @@
+# 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 'application_controller'
+
+class NoMethodController < ApplicationController; end

test/new_relic/agent/method_tracer_helpers_test.rb

@@ -147,4 +147,44 @@ def test_clm_memoization_hash_uses_frozen_keys_and_values
       assert_predicate memoized.values.first, :frozen?
     end
   end
+
+  if defined?(::Rails::VERSION::MAJOR) && ::Rails::VERSION::MAJOR >= 7
+    require_relative '../../environments/rails70/app/controllers/no_method_controller'
+
+    def test_provides_info_for_no_method_on_controller
+      skip_unless_minitest5_or_above
+
+      with_config(:'code_level_metrics.enabled' => true) do
+        info = NewRelic::Agent::MethodTracerHelpers.code_information(NoMethodController, :a_method)
+
+        assert_equal({filepath: Rails.root.join('app/controllers/no_method_controller.rb').to_s,
+          lineno: 1,
+          function: 'a_method',
+          namespace: 'NoMethodController'},
+          info)
+      end
+    end
+
+    def test_controller_info_no_filepath
+      skip_unless_minitest5_or_above
+
+      with_config(:'code_level_metrics.enabled' => true) do
+        info = NewRelic::Agent::MethodTracerHelpers.send(:controller_info, Object, :a_method, false)
+
+        assert_equal NewRelic::EMPTY_ARRAY, info
+      end
+    end
+
+    def test_code_information_returns_empty_hash_when_no_info_is_available
+      with_config(:'code_level_metrics.enabled' => true) do
+        object = String
+        method_name = :a_method
+        NewRelic::Agent::MethodTracerHelpers.stub(:namespace_and_location, [], [object, method_name]) do
+          info = NewRelic::Agent::MethodTracerHelpers.code_information(object, method_name)
+
+          assert_equal NewRelic::EMPTY_HASH, info
+        end
+      end
+    end
+  end
 end