From 257aeb30f24b3362c11fd6e4392fb83e3e735f2b Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Mon, 9 Oct 2023 14:36:13 -0700 Subject: [PATCH] Skip Rails 7.1 ActiveSupportLogger instrumentation The private method ActiveSupport::Logger.broadcast has been removed in Rails 7.1. This throws an error in our tests. It also prevents the @skip_instrumenting instance variable from being set on broadcasted loggers. This may cause users of Rails 7.1 to send duplicate log events to New Relic. This change stops ActiveSupport::Logger instrumentation from being installed on Rails 7.1 or above. This is intended to be a temporary measure until a solution can be found to stop sending duplicate logs in Rails 7.1. --- .../agent/instrumentation/active_support_logger.rb | 10 +++++++++- .../suites/rails/active_support_logger_test.rb | 14 +++++++++++--- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/lib/new_relic/agent/instrumentation/active_support_logger.rb b/lib/new_relic/agent/instrumentation/active_support_logger.rb index 475d22145a..bdcd7515f5 100644 --- a/lib/new_relic/agent/instrumentation/active_support_logger.rb +++ b/lib/new_relic/agent/instrumentation/active_support_logger.rb @@ -9,7 +9,15 @@ DependencyDetection.defer do named :active_support_logger - depends_on { defined?(ActiveSupport::Logger) } + depends_on do + defined?(ActiveSupport::Logger) && + # TODO: Rails 7.1 - ActiveSupport::Logger#broadcast method removed + # APM logs-in-context automatic forwarding still works, but sends + # log events for each broadcasted logger, often causing duplicates + # Issue #2245 + defined?(Rails::VERSION::STRING) && + Gem::Version.new(Rails::VERSION::STRING) < Gem::Version.new('7.1.0') + end executes do NewRelic::Agent.logger.info('Installing ActiveSupport::Logger instrumentation') diff --git a/test/multiverse/suites/rails/active_support_logger_test.rb b/test/multiverse/suites/rails/active_support_logger_test.rb index d0abc9d5dd..34996faa4f 100644 --- a/test/multiverse/suites/rails/active_support_logger_test.rb +++ b/test/multiverse/suites/rails/active_support_logger_test.rb @@ -9,23 +9,31 @@ class ActiveSupportLoggerTest < Minitest::Test include MultiverseHelpers setup_and_teardown_agent + def rails_7_1? + Gem::Version.new(::Rails::VERSION::STRING) >= Gem::Version.new('7.1.0') + end + def setup @output = StringIO.new @logger = Logger.new(@output) @broadcasted_output = StringIO.new @broadcasted_logger = ActiveSupport::Logger.new(@broadcasted_output) - @logger.extend(ActiveSupport::Logger.broadcast(@broadcasted_logger)) + @logger.extend(ActiveSupport::Logger.broadcast(@broadcasted_logger)) unless rails_7_1? @aggregator = NewRelic::Agent.agent.log_event_aggregator @aggregator.reset! end def test_broadcasted_logger_marked_skip_instrumenting - assert @broadcasted_logger.instance_variable_get(:@skip_instrumenting) - assert_nil @logger.instance_variable_get(:@skip_instrumenting) + skip 'Rails 7.1. Active Support Logger instrumentation broken, see #2245' if rails_7_1? + + assert @broadcasted_logger.instance_variable_get(:@skip_instrumenting), 'Broadcasted logger not set with @skip_instrumenting' + assert_nil @logger.instance_variable_get(:@skip_instrumenting), 'Logger has @skip_instrumenting defined' end def test_logs_not_forwarded_by_broadcasted_logger + skip 'Rails 7.1. Active Support Logger instrumentation broken, see #2245' if rails_7_1? + message = 'Can you hear me, Major Tom?' @logger.add(Logger::DEBUG, message)