Skip to content
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

Remove tagged logging support #1355

Merged
merged 1 commit into from
Dec 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Loading