Skip to content

Commit

Permalink
Remove tagged logging support
Browse files Browse the repository at this point in the history
This commit removes tagged logging support from `Appsignal::Logger`,
essentially reverting most of the changes in #1344, #1350, #1351 and

The change to use instance variables instead of thread variables is
maintained, as well as the tests that assert the expected behaviour
when used alongside `ActiveSupport::TaggedLogging`, which pass
notwithstanding the reverted functionality. This makes explicit in
our test suite the way in which tagged logging is supported.
  • Loading branch information
unflxw committed Dec 19, 2024
1 parent 5f987a4 commit c061aa4
Show file tree
Hide file tree
Showing 5 changed files with 46 additions and 76 deletions.
11 changes: 0 additions & 11 deletions .changesets/allow--logger-tagged--to-be-called-without-a-block.md

This file was deleted.

This file was deleted.

45 changes: 45 additions & 0 deletions .changesets/remove-tagged-logging-support.md
Original file line number Diff line number Diff line change
@@ -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)
```
58 changes: 1 addition & 57 deletions lib/appsignal/logger.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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)
Expand Down Expand Up @@ -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
Expand All @@ -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)
Expand Down
2 changes: 0 additions & 2 deletions spec/lib/appsignal/logger_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -466,8 +466,6 @@
end
end

it_behaves_like "tagged logging"

if DependencyHelper.rails_present?
describe "wrapped in ActiveSupport::TaggedLogging" do
let(:logger) do
Expand Down

0 comments on commit c061aa4

Please sign in to comment.