diff --git a/.changesets/allow--logger-tagged--to-be-called-without-a-block.md b/.changesets/allow--logger-tagged--to-be-called-without-a-block.md deleted file mode 100644 index 5e9d79a4..00000000 --- a/.changesets/allow--logger-tagged--to-be-called-without-a-block.md +++ /dev/null @@ -1,11 +0,0 @@ ---- -bump: patch -type: fix ---- - -Allow `Appsignal::Logger#tagged` to be called without a block, in the same way as `ActiveSupport::TaggedLogging`: - -```ruby -Appsignal::Logger.new("rails").tagged("some tag").info("message") -# => logs "[some tag] message" -``` diff --git a/.changesets/fix--appsignal--logger-tagged--with-several-tags.md b/.changesets/fix--appsignal--logger-tagged--with-several-tags.md deleted file mode 100644 index 88e7a0d6..00000000 --- a/.changesets/fix--appsignal--logger-tagged--with-several-tags.md +++ /dev/null @@ -1,6 +0,0 @@ ---- -bump: patch -type: fix ---- - -Fix an issue when calling `Appsignal::Logger#tagged` directly with several tags. This does not affect users of `Appsignal::Logger` who also use `ActiveSupport::TaggedLogging` to wrap the logger. diff --git a/.changesets/remove-tagged-logging-support.md b/.changesets/remove-tagged-logging-support.md new file mode 100644 index 00000000..a50a98ae --- /dev/null +++ b/.changesets/remove-tagged-logging-support.md @@ -0,0 +1,45 @@ +--- +bump: patch +type: remove +--- + +Remove tagged logging support from `Appsignal::Logger`. + +Tagged logging is still supported by wrapping an instance of `Appsignal::Logger` with `ActiveSupport::TaggedLogging`: + +```ruby +appsignal_logger = Appsignal::Logger.new("rails") +tagged_logger = ActiveSupport::TaggedLogging.new(appsignal_logger) +Rails.logger = tagged_logger +``` + +Removing this functionality allows for a workaround to issues within Rails ([#49745](https://github.com/rails/rails/issues/49745), [#51883](https://github.com/rails/rails/issues/51883)), where using the broadcast logger to log to more than one tagged logger results in incorrect behaviour of the tagged logging methods, resulting in breakage throughout Rails' internals: + +```ruby +# We use the built-in request ID middleware as an example that triggers +# the issue: +Rails.config.log_tags = [:request_id] + +appsignal_logger = Appsignal::Logger.new("rails") +tagged_logger = ActiveSupport::TaggedLogging.new(appsignal_logger) + +# This does not work correctly, because the default `Rails.logger` is a +# broadcast logger that is already broadcasting to a tagged logger. +# When asked to broadcast to a second tagged logger, the return value of +# `Rails.logger.tagged { ... }` will be incorrect, in turn causing the +# `RequestID` middleware, which uses it internally, to return broken +# Rack responses. +Rails.logger.broadcast_to(tagged_logger) +``` + +By reverting the changes to our logger so that it is no longer a tagged logger, we enable a workaround to this issue: + +```ruby +Rails.config.log_tags = [:request_id] + +appsignal_logger = Appsignal::Logger.new("rails") + +# This works correctly, because `appsignal_logger` is not a tagged logger. +# Note that `appsignal_logger` will not have the `request_id` tags. +Rails.logger.broadcast_to(appsignal_logger) +``` diff --git a/lib/appsignal/logger.rb b/lib/appsignal/logger.rb index 5ba59ddf..6cc9a3c5 100644 --- a/lib/appsignal/logger.rb +++ b/lib/appsignal/logger.rb @@ -29,7 +29,7 @@ class Logger < ::Logger # @param format Format to use to parse log line attributes. # @param attributes Default attributes for all log lines. # @return [void] - def initialize(group, level: INFO, format: PLAINTEXT, attributes: {}, tags: []) + def initialize(group, level: INFO, format: PLAINTEXT, attributes: {}) raise TypeError, "group must be a string" unless group.is_a? String @group = group @@ -38,7 +38,6 @@ def initialize(group, level: INFO, format: PLAINTEXT, attributes: {}, tags: []) @mutex = Mutex.new @default_attributes = attributes @appsignal_attributes = {} - @tags = tags end # We support the various methods in the Ruby @@ -59,11 +58,6 @@ def add(severity, message = nil, group = nil) end return if message.nil? - if @tags.any? - formatted_tags = @tags.map { |tag| "[#{tag}]" } - message = "#{formatted_tags.join(" ")} #{message}" - end - message = formatter.call(severity, Time.now, group, message) if formatter unless message.is_a?(String) @@ -150,46 +144,6 @@ def fatal(message = nil, attributes = {}) add_with_attributes(FATAL, message, @group, attributes) end - # Listen to ActiveSupport tagged logging tags set with `Rails.logger.tagged`. - def tagged(*tags) - # Depending on the Rails version, the tags can be passed as an array or - # as separate arguments. Flatten the tags argument array to deal with them - # indistinctly. - tags = tags.flatten - - # If called without a block, return a new logger that always logs with the - # given set of tags. - return with_tags(tags) unless block_given? - - # If called with a block, modify the current logger to log with the given - # set of tags for the duration of the block. - @tags.append(*tags) - begin - yield self - ensure - @tags.pop(tags.length) - end - end - - # Listen to ActiveSupport tagged logging tags set with `Rails.config.log_tags`. - def push_tags(*tags) - # Depending on the Rails version, the tags can be passed as an array or - # as separate arguments. Flatten the tags argument array to deal with them - # indistinctly. - tags = tags.flatten - @tags.append(*tags) - end - - # Remove a set of ActiveSupport tagged logging tags set with `Rails.config.log_tags`. - def pop_tags(count = 1) - @tags.pop(count) - end - - # Remove all ActiveSupport tagged logging tags set with `Rails.config.log_tags`. - def clear_tags! - @tags.clear - end - # When using ActiveSupport::TaggedLogging without the broadcast feature, # the passed logger is required to respond to the `silence` method. # In our case it behaves as the broadcast feature of the Rails logger, but @@ -206,16 +160,6 @@ def silence(_severity = ERROR, &block) private - def with_tags(tags) - Logger.new( - @group, - :level => @level, - :format => @format, - :attributes => @default_attributes, - :tags => @tags + tags - ) - end - attr_reader :default_attributes, :appsignal_attributes def add_with_attributes(severity, message, group, attributes) diff --git a/spec/lib/appsignal/logger_spec.rb b/spec/lib/appsignal/logger_spec.rb index bce891de..ac80dfe5 100644 --- a/spec/lib/appsignal/logger_spec.rb +++ b/spec/lib/appsignal/logger_spec.rb @@ -466,8 +466,6 @@ end end - it_behaves_like "tagged logging" - if DependencyHelper.rails_present? describe "wrapped in ActiveSupport::TaggedLogging" do let(:logger) do