From a8d5b16aae2fafe09b52ee415f041c4d27566f6d Mon Sep 17 00:00:00 2001 From: hramadan Date: Thu, 8 Jun 2023 09:44:56 -0700 Subject: [PATCH 1/5] Rails controller methods report CLM Previously, when a new thread was spawned the agent would continue using the current transaction to record data on, even if this transaction had finished already in a different thread. Now the agent will only use the current transaction in the new thread if it is not yet finished. --- lib/new_relic/agent/method_tracer_helpers.rb | 25 ++++++++++- test/README.md | 2 +- .../app/controllers/application_controller.rb | 6 +++ .../app/controllers/test_controller.rb | 7 ++++ .../agent/method_tracer_helpers_test.rb | 42 +++++++++++++++++++ 5 files changed, 79 insertions(+), 3 deletions(-) create mode 100644 test/environments/rails70/app/controllers/application_controller.rb create mode 100644 test/environments/rails70/app/controllers/test_controller.rb diff --git a/lib/new_relic/agent/method_tracer_helpers.rb b/lib/new_relic/agent/method_tracer_helpers.rb index 1ee9bde7a4..6fbd212f71 100644 --- a/lib/new_relic/agent/method_tracer_helpers.rb +++ b/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 diff --git a/test/README.md b/test/README.md index bd81fd22ff..d8ffab2811 100644 --- a/test/README.md +++ b/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 diff --git a/test/environments/rails70/app/controllers/application_controller.rb b/test/environments/rails70/app/controllers/application_controller.rb new file mode 100644 index 0000000000..612314dd94 --- /dev/null +++ b/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 diff --git a/test/environments/rails70/app/controllers/test_controller.rb b/test/environments/rails70/app/controllers/test_controller.rb new file mode 100644 index 0000000000..471eaee01a --- /dev/null +++ b/test/environments/rails70/app/controllers/test_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 TestController < ApplicationController; end diff --git a/test/new_relic/agent/method_tracer_helpers_test.rb b/test/new_relic/agent/method_tracer_helpers_test.rb index b3179f46eb..6df2aac5ca 100644 --- a/test/new_relic/agent/method_tracer_helpers_test.rb +++ b/test/new_relic/agent/method_tracer_helpers_test.rb @@ -147,4 +147,46 @@ 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 + 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(TestController, :a_method) + + assert_equal({filepath: Rails.root.join('app/controllers/test_controller.rb').to_s, + lineno: 1, + function: 'a_method', + namespace: 'TestController'}, + info) + end + end + end + + if defined?(::Rails::VERSION::MAJOR) && ::Rails::VERSION::MAJOR >= 7 + 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 + end + + if defined?(::Rails::VERSION::MAJOR) && ::Rails::VERSION::MAJOR >= 7 + 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 From 50a1239582c22f5a935e42eae0605912d227ca35 Mon Sep 17 00:00:00 2001 From: hramadan Date: Sun, 11 Jun 2023 16:47:56 -0700 Subject: [PATCH 2/5] Add CHANGELOG --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index fe90a95861..14accbb8d8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,10 @@ Version 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. From b6857063efb2ca14444388f9a5ff4a15471bf5e9 Mon Sep 17 00:00:00 2001 From: hramadan Date: Mon, 12 Jun 2023 10:24:04 -0700 Subject: [PATCH 3/5] Test refactor --- .../{test_controller.rb => no_method_controller.rb} | 2 +- test/new_relic/agent/method_tracer_helpers_test.rb | 12 +++++------- 2 files changed, 6 insertions(+), 8 deletions(-) rename test/environments/rails70/app/controllers/{test_controller.rb => no_method_controller.rb} (80%) diff --git a/test/environments/rails70/app/controllers/test_controller.rb b/test/environments/rails70/app/controllers/no_method_controller.rb similarity index 80% rename from test/environments/rails70/app/controllers/test_controller.rb rename to test/environments/rails70/app/controllers/no_method_controller.rb index 471eaee01a..70a729db3f 100644 --- a/test/environments/rails70/app/controllers/test_controller.rb +++ b/test/environments/rails70/app/controllers/no_method_controller.rb @@ -4,4 +4,4 @@ require_relative 'application_controller' -class TestController < ApplicationController; end +class NoMethodController < ApplicationController; end diff --git a/test/new_relic/agent/method_tracer_helpers_test.rb b/test/new_relic/agent/method_tracer_helpers_test.rb index 6df2aac5ca..b11e59a3f3 100644 --- a/test/new_relic/agent/method_tracer_helpers_test.rb +++ b/test/new_relic/agent/method_tracer_helpers_test.rb @@ -149,22 +149,22 @@ def test_clm_memoization_hash_uses_frozen_keys_and_values 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(TestController, :a_method) + info = NewRelic::Agent::MethodTracerHelpers.code_information(NoMethodController, :a_method) - assert_equal({filepath: Rails.root.join('app/controllers/test_controller.rb').to_s, + assert_equal({filepath: Rails.root.join('app/controllers/no_method_controller.rb').to_s, lineno: 1, function: 'a_method', - namespace: 'TestController'}, + namespace: 'NoMethodController'}, info) end end - end - if defined?(::Rails::VERSION::MAJOR) && ::Rails::VERSION::MAJOR >= 7 def test_controller_info_no_filepath skip_unless_minitest5_or_above @@ -174,9 +174,7 @@ def test_controller_info_no_filepath assert_equal NewRelic::EMPTY_ARRAY, info end end - end - if defined?(::Rails::VERSION::MAJOR) && ::Rails::VERSION::MAJOR >= 7 def test_code_information_returns_empty_hash_when_no_info_is_available with_config(:'code_level_metrics.enabled' => true) do object = String From bd3e9cbbac15dba8134cbe46e7f7cb71786b929c Mon Sep 17 00:00:00 2001 From: hramadan Date: Mon, 12 Jun 2023 10:26:45 -0700 Subject: [PATCH 4/5] Update changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 14accbb8d8..76ebba28f7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ## dev -Version of the agent adds the ability to filter logs by level and expands instrumentation for Action Cable. +Version 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** From 2b2280327851c739ad4768253a0a75643837f106 Mon Sep 17 00:00:00 2001 From: Hannah Ramadan <76922290+hannahramadan@users.noreply.github.com> Date: Mon, 12 Jun 2023 10:27:26 -0700 Subject: [PATCH 5/5] Apply suggestions from code review Co-authored-by: Kayla Reopelle (she/her) <87386821+kaylareopelle@users.noreply.github.com> --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 76ebba28f7..697135f47a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,9 +20,9 @@ Version of the agent adds the ability to filter logs by level, expands ins * `transmit_subscription_confirmation.action_cable` * `transmit_subscription_rejection.action_cable` -- **Bugfix: Report Code Level Metrics for Rails controller methods** +- **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) + 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