-
Notifications
You must be signed in to change notification settings - Fork 599
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
Rails 7.1: Record only one copy of a broadcasted log message #2252
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
# 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 'active_support_broadcast_logger/instrumentation' | ||
require_relative 'active_support_broadcast_logger/chain' | ||
require_relative 'active_support_broadcast_logger/prepend' | ||
|
||
DependencyDetection.defer do | ||
named :'active_support_broadcast_logger' | ||
|
||
depends_on { defined?(ActiveSupport::BroadcastLogger) } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should prevent the BroadcastLogger instrumentation from being installed on Rails versions below 7.1. |
||
|
||
executes do | ||
NewRelic::Agent.logger.info('Installing ActiveSupport::BroadcastLogger instrumentation') | ||
|
||
if use_prepend? | ||
prepend_instrument ActiveSupport::BroadcastLogger, NewRelic::Agent::Instrumentation::ActiveSupportBroadcastLogger::Prepend | ||
else | ||
chain_instrument NewRelic::Agent::Instrumentation::ActiveSupportBroadcastLogger::Chain | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
# 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 | ||
|
||
module NewRelic::Agent::Instrumentation | ||
module ActiveSupportBroadcastLogger::Chain | ||
def self.instrument! | ||
::ActiveSupportBroadcastLogger.class_eval do | ||
include NewRelic::Agent::Instrumentation::ActiveSupportBroadcastLogger | ||
|
||
alias_method(:add_without_new_relic, :add) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In both chain and prepend, I tried to use iteration and example prepend attempt: LOG_RECORD_METHODS = %w[add debug info warn error fatal unknown]
LOG_RECORD_METHODS.each do |method|
define_method "#{method}" do |*args|
skip_broadcasts_with_new_relic(*args) { super }
end
end There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For others who might come across this with curiosity...
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for clarifying, @fallwith! |
||
|
||
def add(*args, &task) | ||
record_one_broadcast_with_new_relic(*args) do | ||
add_without_new_relic(*args, &traced_task) | ||
end | ||
end | ||
|
||
alias_method(:debug_without_new_relic, :debug) | ||
|
||
def debug(*args, &task) | ||
record_one_broadcast_with_new_relic(*args) do | ||
debug_without_new_relic(*args, &traced_task) | ||
end | ||
end | ||
|
||
alias_method(:info_without_new_relic, :info) | ||
|
||
def info(*args, &task) | ||
record_one_broadcast_with_new_relic(*args) do | ||
info_without_new_relic(*args, &traced_task) | ||
end | ||
end | ||
|
||
alias_method(:warn_without_new_relic, :warn) | ||
|
||
def warn(*args, &task) | ||
record_one_broadcast_with_new_relic(*args) do | ||
warn_without_new_relic(*args, &traced_task) | ||
end | ||
end | ||
|
||
alias_method(:error_without_new_relic, :error) | ||
|
||
def error(*args, &task) | ||
record_one_broadcast_with_new_relic(*args) do | ||
error_without_new_relic(*args, &traced_task) | ||
end | ||
end | ||
|
||
alias_method(:fatal_without_new_relic, :fatal) | ||
|
||
def fatal(*args, &task) | ||
record_one_broadcast_with_new_relic(*args) do | ||
fatal_without_new_relic(*args, &traced_task) | ||
end | ||
end | ||
end | ||
|
||
alias_method(:unknown_without_new_relic, :unknown) | ||
|
||
def unknown(*args, &task) | ||
record_one_broadcast_with_new_relic(*args) do | ||
unknown_without_new_relic(*args, &traced_task) | ||
end | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
# 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 | ||
|
||
module NewRelic::Agent::Instrumentation | ||
module ActiveSupportBroadcastLogger | ||
def record_one_broadcast_with_new_relic(*args) | ||
broadcasts[1..-1].each { |broadcasted_logger| broadcasted_logger.instance_variable_set(:@skip_instrumenting, true) } | ||
yield | ||
broadcasts.each { |broadcasted_logger| broadcasted_logger.instance_variable_set(:@skip_instrumenting, false) } | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
# 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 | ||
|
||
module NewRelic::Agent::Instrumentation | ||
module ActiveSupportBroadcastLogger::Prepend | ||
include NewRelic::Agent::Instrumentation::ActiveSupportBroadcastLogger | ||
|
||
def add(*args) | ||
record_one_broadcast_with_new_relic(*args) { super } | ||
end | ||
|
||
def debug(*args) | ||
record_one_broadcast_with_new_relic(*args) { super } | ||
end | ||
|
||
def info(*args) | ||
record_one_broadcast_with_new_relic(*args) { super } | ||
end | ||
|
||
def warn(*args) | ||
record_one_broadcast_with_new_relic(*args) { super } | ||
end | ||
|
||
def error(*args) | ||
record_one_broadcast_with_new_relic(*args) { super } | ||
end | ||
|
||
def fatal(*args) | ||
record_one_broadcast_with_new_relic(*args) { super } | ||
end | ||
|
||
def unknown(*args) | ||
record_one_broadcast_with_new_relic(*args) { super } | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,13 +10,7 @@ | |
named :active_support_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') | ||
defined?(ActiveSupport::Logger) && defined?(ActiveSupport::Logger.broadcast) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
end | ||
|
||
executes do | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
# 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 | ||
|
||
instrumentation_methods :chain, :prepend | ||
|
||
# ActiveSupport::BroadcastLogger introduced in Rails 7.1. | ||
# Rails 7.1 is the latest version at the time of writing. | ||
ACTIVE_SUPPORT_VERSIONS = [ | ||
[nil, 2.7] | ||
] | ||
|
||
unshift_rails_edge(ACTIVE_SUPPORT_VERSIONS) | ||
|
||
def gem_list(activesupport_version = nil) | ||
<<-RB | ||
gem 'activesupport'#{activesupport_version} | ||
RB | ||
end | ||
|
||
create_gemfiles(ACTIVE_SUPPORT_VERSIONS) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
# 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 'active_support' | ||
|
||
class ActiveSupportBroadcastLoggerTest < Minitest::Test | ||
include MultiverseHelpers | ||
|
||
MESSAGE = 'Can you hear me, Major Tom?' | ||
|
||
def setup | ||
NewRelic::Agent.manual_start | ||
|
||
@output = StringIO.new | ||
@io_logger = Logger.new(@output) | ||
@output2 = StringIO.new | ||
@io_logger2 = Logger.new(@output2) | ||
@broadcast = ActiveSupport::BroadcastLogger.new(@io_logger, @io_logger2) | ||
@aggregator = NewRelic::Agent.agent.log_event_aggregator | ||
|
||
@aggregator.reset! | ||
end | ||
|
||
def teardown | ||
NewRelic::Agent.shutdown | ||
end | ||
|
||
def test_broadcasted_logger_sends_one_log_event_per_add_call | ||
@broadcast.add(Logger::DEBUG, MESSAGE) | ||
|
||
assert_log_broadcasted_to_both_outputs | ||
assert_log_seen_once_by_new_relic('DEBUG') | ||
end | ||
|
||
def test_broadcasted_logger_sends_one_log_event_per_unknown_call | ||
@broadcast.unknown(MESSAGE) | ||
|
||
assert_log_broadcasted_to_both_outputs | ||
assert_log_seen_once_by_new_relic('ANY') | ||
end | ||
|
||
%w[debug info warn error fatal].each do |method| | ||
define_method("test_broadcasted_logger_sends_one_log_event_per_#{method}_call") do | ||
@broadcast.send(method.to_sym, MESSAGE) | ||
|
||
assert_log_broadcasted_to_both_outputs | ||
assert_log_seen_once_by_new_relic(method.upcase) | ||
end | ||
end | ||
|
||
private | ||
|
||
def assert_log_broadcasted_to_both_outputs | ||
assert_includes(@output.string, MESSAGE) | ||
assert_includes(@output2.string, MESSAGE) | ||
end | ||
|
||
def assert_log_seen_once_by_new_relic(severity) | ||
assert_equal(1, @aggregator.instance_variable_get(:@seen)) | ||
assert_equal({severity => 1}, @aggregator.instance_variable_get(:@seen_by_severity)) | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
common: &default_settings | ||
license_key: 'bd0e1d52adade840f7ca727d29a86249e89a6f1c' | ||
ca_bundle_path: ../../../config/test.cert.crt | ||
host: localhost | ||
api_host: localhost | ||
port: <%= $collector && $collector.port %> | ||
app_name: Rails multiverse test app | ||
enabled: true | ||
apdex_t: 1.0 | ||
capture_params: true | ||
transaction_tracer: | ||
enabled: true | ||
transaction_threshold: apdex_f | ||
record_sql: obfuscated | ||
stack_trace_threshold: 0.500 | ||
error_collector: | ||
enabled: true | ||
ignore_classes: NewRelic::TestHelpers::Exceptions::IgnoredError | ||
instrumentation: | ||
active_support_broadcast_logger <%= $instrumentation_method %> | ||
|
||
development: | ||
<<: *default_settings | ||
|
||
test: | ||
<<: *default_settings | ||
|
||
production: | ||
<<: *default_settings | ||
|
||
staging: | ||
<<: *default_settings |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
# This file is distributed under New Relic's license terms. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I realized our |
||
# See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details. | ||
# frozen_string_literal: true | ||
|
||
instrumentation_methods :chain, :prepend | ||
|
||
ACTIVE_SUPPORT_VERSIONS = [ | ||
['7.0.4', 2.7], | ||
['6.1.7', 2.5], | ||
['6.0.6', 2.5, 2.7], | ||
['5.2.8', 2.4, 2.7], | ||
['5.1.7', 2.4, 2.7], | ||
['5.0.7', 2.4, 2.7], | ||
['4.2.11', 2.4, 2.4] | ||
] | ||
|
||
def gem_list(activesupport_version = nil) | ||
<<-RB | ||
gem 'activesupport'#{activesupport_version} | ||
RB | ||
end | ||
|
||
create_gemfiles(ACTIVE_SUPPORT_VERSIONS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd love to have suggestions on how to improve this and the statement below, especially if we want to turn it into a more general statement about the agent supporting Rails 7.1.
I also noticed the
active_support_logger
instrumentation wasn't specifically called out in the changelog when we released logs functionality, so what we reveal here may not need to be so specific.