Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Controller methods not in controller class don't warn #2061

Merged
merged 5 commits into from
Jun 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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**

Expand All @@ -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.
Expand Down
25 changes: 23 additions & 2 deletions lib/new_relic/agent/method_tracer_helpers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
hannahramadan marked this conversation as resolved.
Show resolved Hide resolved
::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)
Expand Down Expand Up @@ -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
Expand All @@ -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")
hannahramadan marked this conversation as resolved.
Show resolved Hide resolved

File.exist?(path) ? [name, [path.to_s, 1], is_class_method] : []
end
end
end
end
2 changes: 1 addition & 1 deletion test/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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
40 changes: 40 additions & 0 deletions test/new_relic/agent/method_tracer_helpers_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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