diff --git a/CHANGELOG.md b/CHANGELOG.md index 8141b06cda..f2c57ec993 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ The upcoming release of the agent introduces additional Ruby on Rails instrument The agent now newly supports or has updated support for the following libraries: + - Action Controller [PR#1744](https://github.com/newrelic/newrelic-ruby-agent/pull/1744/) - Action Dispatch (for middleware) [PR#1745](https://github.com/newrelic/newrelic-ruby-agent/pull/1745) - Action Mailbox (for sending mail) [PR#1740](https://github.com/newrelic/newrelic-ruby-agent/pull/1740) - Action Mailer (for routing mail) [PR#1740](https://github.com/newrelic/newrelic-ruby-agent/pull/1740) @@ -19,6 +20,7 @@ The upcoming release of the agent introduces additional Ruby on Rails instrument | Configuration name | Default | Behavior | | ----- | ----- | ----- | + | `disable_action_controller` | `false` | If `true`, disables Action Controller instrumentation. | | `disable_action_dispatch` | `false` | If `true`, disables Action Dispatch instrumentation. | | `disable_action_mailbox` | `false` | If `true`, disables Action Mailbox instrumentation. | | `disable_action_mailer` | `false` | If `true`, disables Action Mailer instrumentation. | diff --git a/lib/new_relic/agent/configuration/default_source.rb b/lib/new_relic/agent/configuration/default_source.rb index d2ec679f02..182af4fd4d 100644 --- a/lib/new_relic/agent/configuration/default_source.rb +++ b/lib/new_relic/agent/configuration/default_source.rb @@ -1147,6 +1147,13 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :allowed_from_server => false, :description => 'If `true`, disables Action Dispatch instrumentation.' }, + :disable_action_controller => { + :default => false, + :public => true, + :type => Boolean, + :allowed_from_server => false, + :description => 'If `true`, disables Action Controller instrumentation.' + }, :disable_action_mailbox => { :default => false, :public => true, diff --git a/lib/new_relic/agent/instrumentation/action_controller_other_subscriber.rb b/lib/new_relic/agent/instrumentation/action_controller_other_subscriber.rb new file mode 100644 index 0000000000..5b9ea78216 --- /dev/null +++ b/lib/new_relic/agent/instrumentation/action_controller_other_subscriber.rb @@ -0,0 +1,39 @@ +# 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 add_segment_params(segment, payload) + 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 diff --git a/lib/new_relic/agent/instrumentation/action_mailbox_subscriber.rb b/lib/new_relic/agent/instrumentation/action_mailbox_subscriber.rb index 6a68c5e370..56b5edf647 100644 --- a/lib/new_relic/agent/instrumentation/action_mailbox_subscriber.rb +++ b/lib/new_relic/agent/instrumentation/action_mailbox_subscriber.rb @@ -8,36 +8,6 @@ module NewRelic module Agent module Instrumentation class ActionMailboxSubscriber < 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)) - push_segment(id, segment) - end - - def finish_segment(id, payload) - if segment = pop_segment(id) - if exception = exception_object(payload) - segment.notice_error(exception) - end - segment.finish - end - end - def metric_name(name, payload) mailbox = payload[:mailbox].class.name method = method_from_name(name) @@ -45,13 +15,12 @@ def metric_name(name, payload) end PATTERN = /\A([^\.]*)\.action_mailbox\z/ - UNKNOWN = "unknown".freeze METHOD_NAME_MAPPING = Hash.new do |h, k| if PATTERN =~ k h[k] = $1 else - h[k] = UNKNOWN + h[k] = NewRelic::UNKNOWN end end diff --git a/lib/new_relic/agent/instrumentation/action_mailer_subscriber.rb b/lib/new_relic/agent/instrumentation/action_mailer_subscriber.rb index 0901b026ec..fc6ed03c44 100644 --- a/lib/new_relic/agent/instrumentation/action_mailer_subscriber.rb +++ b/lib/new_relic/agent/instrumentation/action_mailer_subscriber.rb @@ -15,14 +15,13 @@ class ActionMailerSubscriber < NotificationsSubscriber BASE_NAME = 'Ruby/ActionMailer' PAYLOAD_KEYS = %i[action data key mailer message_id perform_deliveries subject] PATTERN = /\A([^\.]+)\.action_mailer\z/ - UNKNOWN = 'unknown' UNKNOWN_MAILER = %r{^#{BASE_NAME}/#{UNKNOWN}/} METHOD_NAME_MAPPING = Hash.new do |h, k| if PATTERN =~ k h[k] = $1 else - h[k] = UNKNOWN + h[k] = NewRelic::UNKNOWN end end @@ -54,7 +53,7 @@ def finish_segment(id, payload) return unless segment if segment.name.match?(UNKNOWN_MAILER) && payload.key?(:mailer) - segment.name.sub!(UNKNOWN_MAILER, "#{BASE_NAME}/#{payload[:mailer]}/") + segment.name = segment.name.sub(UNKNOWN_MAILER, "#{BASE_NAME}/#{payload[:mailer]}/") end PAYLOAD_KEYS.each do |key| diff --git a/lib/new_relic/agent/instrumentation/action_view_subscriber.rb b/lib/new_relic/agent/instrumentation/action_view_subscriber.rb index b95f869481..6aa9747fdc 100644 --- a/lib/new_relic/agent/instrumentation/action_view_subscriber.rb +++ b/lib/new_relic/agent/instrumentation/action_view_subscriber.rb @@ -10,28 +10,25 @@ module NewRelic module Agent module Instrumentation class ActionViewSubscriber < NotificationsSubscriber - def start(name, id, payload) # THREAD_LOCAL_ACCESS + def start_segment(name, id, payload) parent = segment_stack[id].last metric_name = format_metric_name(name, payload, parent) event = ActionViewEvent.new(metric_name, payload[:identifier]) - if state.is_execution_traced? && recordable?(name, metric_name) + + if recordable?(name, metric_name) event.finishable = Tracer.start_segment(name: metric_name) end push_segment(id, event) - rescue => e - log_notification_error(e, name, 'start') end - def finish(name, id, payload) + def finish_segment(id, payload) if segment = pop_segment(id) if exception = exception_object(payload) segment.notice_error(exception) end segment.finish end - rescue => e - log_notification_error(e, name, 'finish') end def format_metric_name(event_name, payload, parent) @@ -61,12 +58,15 @@ def recordable?(event_name, metric_name) RENDER_TEMPLATE_EVENT_NAME = 'render_template.action_view'.freeze RENDER_PARTIAL_EVENT_NAME = 'render_partial.action_view'.freeze RENDER_COLLECTION_EVENT_NAME = 'render_collection.action_view'.freeze + RENDER_LAYOUT_EVENT_NAME = 'render_layout.action_view'.freeze def metric_action(name) case name when /#{RENDER_TEMPLATE_EVENT_NAME}$/o then 'Rendering' when RENDER_PARTIAL_EVENT_NAME then 'Partial' when RENDER_COLLECTION_EVENT_NAME then 'Partial' + when RENDER_LAYOUT_EVENT_NAME then 'Layout' + else NewRelic::UNKNOWN end end diff --git a/lib/new_relic/agent/instrumentation/active_storage_subscriber.rb b/lib/new_relic/agent/instrumentation/active_storage_subscriber.rb index e3d254024a..670d011447 100644 --- a/lib/new_relic/agent/instrumentation/active_storage_subscriber.rb +++ b/lib/new_relic/agent/instrumentation/active_storage_subscriber.rb @@ -8,36 +8,9 @@ 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)) + def add_segment_params(segment, payload) segment.params[:key] = payload[:key] segment.params[:exist] = payload[:exist] if payload.key?(:exist) - push_segment(id, segment) - end - - def finish_segment(id, payload) - if segment = pop_segment(id) - if exception = exception_object(payload) - segment.notice_error(exception) - end - segment.finish - end end def metric_name(name, payload) @@ -47,13 +20,12 @@ def metric_name(name, payload) end PATTERN = /\Aservice_([^\.]*)\.active_storage\z/ - UNKNOWN = "unknown".freeze METHOD_NAME_MAPPING = Hash.new do |h, k| if PATTERN =~ k h[k] = $1 else - h[k] = UNKNOWN + h[k] = NewRelic::UNKNOWN end end diff --git a/lib/new_relic/agent/instrumentation/active_support_subscriber.rb b/lib/new_relic/agent/instrumentation/active_support_subscriber.rb index 756a8c2593..326efd4b16 100644 --- a/lib/new_relic/agent/instrumentation/active_support_subscriber.rb +++ b/lib/new_relic/agent/instrumentation/active_support_subscriber.rb @@ -8,29 +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)) - - add_segment_params(segment, payload) - push_segment(id, segment) - end - def add_segment_params(segment, payload) segment.params[:key] = payload[:key] segment.params[:store] = payload[:store] @@ -39,15 +16,6 @@ def add_segment_params(segment, payload) segment end - def finish_segment(id, payload) - if segment = pop_segment(id) - if exception = exception_object(payload) - segment.notice_error(exception) - end - segment.finish - end - end - def metric_name(name, payload) store = payload[:store] method = method_from_name(name) @@ -55,13 +23,12 @@ def metric_name(name, payload) end PATTERN = /\Acache_([^\.]*)\.active_support\z/ - UNKNOWN = "unknown".freeze METHOD_NAME_MAPPING = Hash.new do |h, k| if PATTERN =~ k h[k] = $1 else - h[k] = UNKNOWN + h[k] = NewRelic::UNKNOWN end end diff --git a/lib/new_relic/agent/instrumentation/notifications_subscriber.rb b/lib/new_relic/agent/instrumentation/notifications_subscriber.rb index 6846cadd26..3db9809621 100644 --- a/lib/new_relic/agent/instrumentation/notifications_subscriber.rb +++ b/lib/new_relic/agent/instrumentation/notifications_subscriber.rb @@ -47,6 +47,47 @@ 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) + segment = Tracer.start_segment(name: metric_name(name, payload)) + add_segment_params(segment, payload) + push_segment(id, segment) + end + + def finish_segment(id, payload) + if segment = pop_segment(id) + if exception = exception_object(payload) + segment.notice_error(exception) + end + segment.finish + end + end + + # for subclasses + def add_segment_params(segment, payload) + # no op + end + + # for subclasses + def metric_name(name, payload) + "Ruby/#{name}" + 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. diff --git a/lib/new_relic/agent/instrumentation/rails_notifications/action_controller.rb b/lib/new_relic/agent/instrumentation/rails_notifications/action_controller.rb index 726bdd0ecc..aaf1b90960 100644 --- a/lib/new_relic/agent/instrumentation/rails_notifications/action_controller.rb +++ b/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 @@ -13,7 +14,8 @@ end depends_on do - defined?(ActionController) && (defined?(ActionController::Base) || defined?(ActionController::API)) + !NewRelic::Agent.config[:disable_action_controller] && + defined?(ActionController) && (defined?(ActionController::Base) || defined?(ActionController::API)) end executes do @@ -29,5 +31,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 diff --git a/test/environments/lib/environments/runner.rb b/test/environments/lib/environments/runner.rb index f0149e6bdb..3949a983bf 100644 --- a/test/environments/lib/environments/runner.rb +++ b/test/environments/lib/environments/runner.rb @@ -99,7 +99,7 @@ def bundle(dir) def run(dir) puts "Starting tests for dir '#{dir}'..." - cmd = "cd #{dir} && bundle exec rake" + cmd = String.new("cd #{dir} && bundle exec rake") cmd << " file=#{ENV['file']}" if ENV["file"] IO.popen(cmd) do |io| diff --git a/test/multiverse/suites/rails/action_controller_other_test.rb b/test/multiverse/suites/rails/action_controller_other_test.rb new file mode 100644 index 0000000000..378d310ea2 --- /dev/null +++ b/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 diff --git a/test/new_relic/agent/instrumentation/action_mailer_subscriber_test.rb b/test/new_relic/agent/instrumentation/action_mailer_subscriber_test.rb index c6bd4368a0..7ef665043c 100644 --- a/test/new_relic/agent/instrumentation/action_mailer_subscriber_test.rb +++ b/test/new_relic/agent/instrumentation/action_mailer_subscriber_test.rb @@ -8,7 +8,7 @@ if defined?(ActiveSupport) && defined?(ActionMailer) && ActionMailer.respond_to?(:gem_version) && - ActionMailer.gem_version >= Gem::Version.new('5.0') + ActionMailer.gem_version >= Gem::Version.new('5.0') && RUBY_VERSION > '2.4.0' require_relative 'rails/action_mailer_subscriber' else puts "Skipping tests in #{__FILE__} because ActionMailer is unavailable" diff --git a/test/new_relic/agent/instrumentation/notification_subscriber_test.rb b/test/new_relic/agent/instrumentation/notification_subscriber_test.rb new file mode 100644 index 0000000000..27cdb927fa --- /dev/null +++ b/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 diff --git a/test/new_relic/agent/instrumentation/rails/action_mailbox_subscriber.rb b/test/new_relic/agent/instrumentation/rails/action_mailbox_subscriber.rb index d73309923d..288ba3752c 100644 --- a/test/new_relic/agent/instrumentation/rails/action_mailbox_subscriber.rb +++ b/test/new_relic/agent/instrumentation/rails/action_mailbox_subscriber.rb @@ -65,7 +65,7 @@ def test_start_with_exception_raised end def test_segment_naming_with_unknown_method - assert_equal "Ruby/ActionMailbox/#{MAILBOX.class.name}/unknown", + assert_equal "Ruby/ActionMailbox/#{MAILBOX.class.name}/Unknown", SUBSCRIBER.send(:metric_name, 'indecipherable', {mailbox: MAILBOX}) end diff --git a/test/new_relic/agent/instrumentation/rails/action_mailer_subscriber.rb b/test/new_relic/agent/instrumentation/rails/action_mailer_subscriber.rb index 8e751501f2..a71ec6b644 100644 --- a/test/new_relic/agent/instrumentation/rails/action_mailer_subscriber.rb +++ b/test/new_relic/agent/instrumentation/rails/action_mailer_subscriber.rb @@ -78,7 +78,7 @@ def test_start_with_exception_raised end def test_segment_naming_with_unknown_method - assert_equal 'Ruby/ActionMailer/mailer/unknown', + assert_equal 'Ruby/ActionMailer/mailer/Unknown', SUBSCRIBER.send(:metric_name, 'indecipherable', {mailer: 'mailer'}) end @@ -167,7 +167,7 @@ def test_an_actual_mail_delivery assert_equal "Ruby/ActionMailer/#{TestMailer.name}/deliver", segment.name assert_equal TestMailer.name, params[:mailer] assert_equal TestMailer::SUBJECT, params[:subject] - assert params[:perform_deliveries] + assert params[:perform_deliveries] if Gem::Version.new(Rails::VERSION::STRING) >= Gem::Version.new('6.0.0') # only exists in rails 6+ end end end diff --git a/test/new_relic/agent/instrumentation/rails/action_view_subscriber.rb b/test/new_relic/agent/instrumentation/rails/action_view_subscriber.rb index 8e1f5deaf1..1d2c7d8171 100644 --- a/test/new_relic/agent/instrumentation/rails/action_view_subscriber.rb +++ b/test/new_relic/agent/instrumentation/rails/action_view_subscriber.rb @@ -110,6 +110,23 @@ def test_records_metrics_for_simple_collection assert_metrics_recorded('View/model/_user.html.erb/Partial' => expected) end + def test_records_metrics_for_simple_layout + params = {:identifier => '/root/app/views/model/_layout.html.erb'} + nr_freeze_process_time + in_transaction do + @subscriber.start('render_layout.action_view', :id, params) + @subscriber.start('!render_layout.action_view', :id, + :virtual_path => 'model/_layout') + advance_process_time(2.0) + @subscriber.finish('!render_layout.action_view', :id, + :virtual_path => 'model/_layout') + @subscriber.finish('render_layout.action_view', :id, params) + end + expected = {:call_count => 1, :total_call_time => 2.0} + + assert_metrics_recorded('View/model/_layout.html.erb/Layout' => expected) + end + def test_records_metrics_for_layout nr_freeze_process_time in_transaction do @@ -263,4 +280,18 @@ def test_metric_path_cannot_identify_empty_collection_render_event def test_metric_path_index_html_erb assert_equal('model/index.html.erb', @subscriber.metric_path('render_template.action_view', 'model/index.html.erb')) end + + def test_finish_segment_when_no_segment + @subscriber.stub :pop_segment, nil do + assert_nil @subscriber.send(:finish_segment, :id, {}) + end + end + + def test_metric_action_nil_name + assert_equal 'Unknown', @subscriber.metric_action(nil) + end + + def test_metric_action_unknown_name + assert_equal 'Unknown', @subscriber.metric_action('cheez-it') + end end diff --git a/test/new_relic/agent/instrumentation/rails/active_storage_subscriber.rb b/test/new_relic/agent/instrumentation/rails/active_storage_subscriber.rb index 584666c54d..fee73202f2 100644 --- a/test/new_relic/agent/instrumentation/rails/active_storage_subscriber.rb +++ b/test/new_relic/agent/instrumentation/rails/active_storage_subscriber.rb @@ -55,7 +55,7 @@ def test_failsafe_if_event_does_not_match_expected_pattern generate_event('wat?') end - assert_metrics_recorded 'Ruby/ActiveStorage/DiskService/unknown' + assert_metrics_recorded 'Ruby/ActiveStorage/DiskService/Unknown' end def test_key_recorded_as_attribute_on_traces diff --git a/test/new_relic/agent/instrumentation/rails/active_support_subscriber.rb b/test/new_relic/agent/instrumentation/rails/active_support_subscriber.rb index 3ce9cf28e0..6ec574afe2 100644 --- a/test/new_relic/agent/instrumentation/rails/active_support_subscriber.rb +++ b/test/new_relic/agent/instrumentation/rails/active_support_subscriber.rb @@ -79,7 +79,7 @@ def test_failsafe_if_event_does_not_match_expected_pattern generate_event('charcuterie_build_a_board_workshop') end - assert_metrics_recorded "#{METRIC_PREFIX}#{DEFAULT_STORE}/unknown" + assert_metrics_recorded "#{METRIC_PREFIX}#{DEFAULT_STORE}/Unknown" end def test_key_recorded_as_attribute_on_traces @@ -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+' diff --git a/test/new_relic/agent/instrumentation/rails/notifications_subscriber.rb b/test/new_relic/agent/instrumentation/rails/notifications_subscriber.rb new file mode 100644 index 0000000000..b623becb4a --- /dev/null +++ b/test/new_relic/agent/instrumentation/rails/notifications_subscriber.rb @@ -0,0 +1,77 @@ +# 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 + + def test_segment_created_notsub + NewRelic::Agent.instance.adaptive_sampler.stubs(:sampled?).returns(true) + + in_transaction do |txn| + @subscriber.start(DEFAULT_EVENT, @id, {}) + + @subscriber.finish(DEFAULT_EVENT, @id, {}) + end + + name = "Ruby/#{DEFAULT_EVENT}" + spans = harvest_span_events! + + refute_empty(spans[1].select { |s| s[0]["name"] == name }) + end + end + end + end +end