From 160320f1e5b35a52ad455eb905704921bf86878a Mon Sep 17 00:00:00 2001 From: Noemi <45180344+unflxw@users.noreply.github.com> Date: Thu, 19 Dec 2024 13:35:25 +0100 Subject: [PATCH] Implement logger broadcasting Implement broadcasting logs to other loggers, following the same interface as `#broadcast_to`. Unlike `#broadcast_to`, however, instead of forwarding each method call one-for-one, we rely on the `#add` method which is expected of the logger interface. As such, we should not run into the kinds of bugs that affect `ActiveSupport::BroadcastLogger` when used alongside additional logger features such as tagged logging, such as rails/rails#49745. This enables customers to use both broadcast logging and tagged logging with the AppSignal logger without running into application-breaking bugs. As a trade-off, unlike `ActiveSupport::BroadcastLogger`, our broadcast implementation is sensitive to the order in which those additional features are applied, expecting them to be applied on top of the broadcast logger, and not on the underlying loggers: ```ruby # Incorrect example some_logger = ActiveSupport::TaggedLogging.new( Logger.new("/etc/app.log") ) appsignal_logger = Appsignal::Logger.new("app") # Even if `some_logger` is a tagging-enabled logger, # this logger will not behave as a tagging-enabled logger. Rails.logger = appsignal_logger.broadcast_to(some_logger) ``` ```ruby # Correct example some_logger = Logger.new("/etc/app.log") appsignal_logger = Appsignal::Logger.new("app") appsignal_logger.broadcast_to(some_logger) # As the tagged logging is applied on the broadcast logger, # both logs sent to AppSignal and to `some_logger` will include # given tags, and `Rails.logger` will be a tagging-enabled logger. Rails.logger = ActiveSupport::TaggedLogging.new(appsignal_logger) ``` --- .changesets/implement-logger-broadcasting.md | 14 ++++++ lib/appsignal/logger.rb | 11 +++++ spec/lib/appsignal/logger_spec.rb | 47 ++++++++++++++++++++ 3 files changed, 72 insertions(+) create mode 100644 .changesets/implement-logger-broadcasting.md diff --git a/.changesets/implement-logger-broadcasting.md b/.changesets/implement-logger-broadcasting.md new file mode 100644 index 00000000..bb34c7b0 --- /dev/null +++ b/.changesets/implement-logger-broadcasting.md @@ -0,0 +1,14 @@ +--- +bump: patch +type: add +--- + +Add logger broadcasting. This change implements an alternative within `Appsignal::Logger` to `ActiveSupport::BroadcastLogger`, following the same interface. This enables a proper workaround to the issues with `ActiveSupport::BroadcastLogger` (([#49745](https://github.com/rails/rails/issues/49745), [#51883](https://github.com/rails/rails/issues/51883))) when used alongside tagged logging. + +For example, to use tagged logging both in logs emitted by the default `Rails.logger` and in logs sent to AppSignal, replace the `Rails.logger` with an AppSignal logger that broadcasts to the default `Rails.logger`: + +```ruby +appsignal_logger = Appsignal::Logger.new("app") +appsignal_logger.broadcast_to(Rails.logger) +Rails.logger = ActiveSupport::TaggedLogging.new(appsignal_logger) +``` diff --git a/lib/appsignal/logger.rb b/lib/appsignal/logger.rb index 6cc9a3c5..5644e261 100644 --- a/lib/appsignal/logger.rb +++ b/lib/appsignal/logger.rb @@ -38,6 +38,7 @@ def initialize(group, level: INFO, format: PLAINTEXT, attributes: {}) @mutex = Mutex.new @default_attributes = attributes @appsignal_attributes = {} + @loggers = [] end # We support the various methods in the Ruby @@ -60,6 +61,12 @@ def add(severity, message = nil, group = nil) message = formatter.call(severity, Time.now, group, message) if formatter + @loggers.each do |logger| + logger.add(severity, message, group) + rescue + nil + end + unless message.is_a?(String) Appsignal.internal_logger.warn( "Logger message was ignored, because it was not a String: #{message.inspect}" @@ -158,6 +165,10 @@ def silence(_severity = ERROR, &block) block.call end + def broadcast_to(logger) + @loggers << logger + end + private attr_reader :default_attributes, :appsignal_attributes diff --git a/spec/lib/appsignal/logger_spec.rb b/spec/lib/appsignal/logger_spec.rb index ac80dfe5..4c3a4a56 100644 --- a/spec/lib/appsignal/logger_spec.rb +++ b/spec/lib/appsignal/logger_spec.rb @@ -338,6 +338,53 @@ end end + describe "#broadcast_to" do + it "broadcasts the message to the given logger" do + other_device = StringIO.new + other_logger = ::Logger.new(other_device) + + logger.broadcast_to(other_logger) + + expect(Appsignal::Extension).to receive(:log) + .with("group", 3, 0, "Log message", instance_of(Appsignal::Extension::Data)) + + logger.info("Log message") + + expect(other_device.string).to include("INFO -- group: Log message") + end + + if DependencyHelper.rails_present? + describe "wrapped in ActiveSupport::TaggedLogging" do + let(:other_stream) { StringIO.new } + let(:other_logger) { ::Logger.new(other_stream) } + + let(:logger) do + appsignal_logger = Appsignal::Logger.new("group") + appsignal_logger.broadcast_to(other_logger) + ActiveSupport::TaggedLogging.new(appsignal_logger) + end + + it "broadcasts a tagged message to the given logger" do + expect(Appsignal::Extension).to receive(:log) + .with( + "group", + 3, + 0, + a_string_starting_with("[My tag] [My other tag] Some message"), + Appsignal::Utils::Data.generate({}) + ) + + logger.tagged("My tag", "My other tag") do + logger.info("Some message") + end + + expect(other_stream.string) + .to include("INFO -- group: [My tag] [My other tag] Some message") + end + end + end + end + [ ["debug", 2, ::Logger::INFO], ["info", 3, ::Logger::WARN],