-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
BroadcastLogger#tagged without a block returns an array instead of a logger #49757
Comments
looks like a similar issue of #49745. Need to investigate further more. |
Similar yes. Basically |
@Edouard-chin Do you have any thoughts on this issue? |
Thanks for the ping, I didn't see this issue and David's one. I have a few changes stashed that I originally wrote it to fix an unrelated issue with Broadcasting + Tagged logging, and this should also fix those two issues. Will open a PR ! |
- ### Context The Tagged Logging functionality has been a source of a few issues over the years, especially when combined with the broadcasting feature. Initializating a Tagged Logger wasn't very intuitive: ```ruby logger = Logger.new(STDOUT) tagged_logger = ActiveSupport::TaggedLogging.new(logger) # Would expect the `tagged_logger` to be an instance of `AS::TaggedLogging` # but it's a clone of the `logger`. tagged_logger.formatter = ->(_, _, _, message) do { message: message } end # Modifies the formatter to output JSON formatted logs. # This breaks tagged logging. ``` I believe the main reason of those issues is because tagged logging is implemented at the wrong level. ### Solution I made a proposal on the Ruby logger upstream in ruby/logger#90 to help solve this but it hasn't been reviewed yet. So I thought about adding it here for now. The TL;DR is to decouple formatting and adding extra information to logs (which is what tagged logging does). ### Deprecation Since TaggedLogging will no longer access the formatter, there is a few things I'd like to deprecate (such as setting a default formatter https://github.com/rails/rails/blob/d68e43922bc11829c52ad9f736ad5549fc97631b/activesupport/lib/active_support/tagged_logging.rb#L124) but doing so in this PR would increase the size of the diff significantly and would maybe distract for PR reviews. Another thing that I believe should be deprecated is `ActiveSupport::TaggedLogging.new`. Adding tagging functionality to a logger should be done using a more ruby approach such as `logger.extend(AS::TaggedLogging)`. Fix rails#49757 Fix rails#49745 Fix rails#46084 Fix rails#44668 I made a propose on the Ruby logger upstream in ruby/logger#90, but it hasn't been reviewed it.
- ### Context The Tagged Logging functionality has been a source of a few issues over the years, especially when combined with the broadcasting feature. Initializating a Tagged Logger wasn't very intuitive: ```ruby logger = Logger.new(STDOUT) tagged_logger = ActiveSupport::TaggedLogging.new(logger) # Would expect the `tagged_logger` to be an instance of `AS::TaggedLogging` # but it's a clone of the `logger`. tagged_logger.formatter = ->(_, _, _, message) do { message: message } end # Modifies the formatter to output JSON formatted logs. # This breaks tagged logging. ``` I believe the main reason of those issues is because tagged logging is implemented at the wrong level. ### Solution I made a proposal on the Ruby logger upstream in ruby/logger#90 to help solve this but it hasn't been reviewed yet. So I thought about adding it here for now. The TL;DR is to decouple formatting and adding extra information to logs (which is what tagged logging does). ### Deprecation Since TaggedLogging will no longer access the formatter, there is a few things I'd like to deprecate (such as setting a default formatter https://github.com/rails/rails/blob/d68e43922bc11829c52ad9f736ad5549fc97631b/activesupport/lib/active_support/tagged_logging.rb#L124) but doing so in this PR would increase the size of the diff significantly and would maybe distract for PR reviews. Another thing that I believe should be deprecated is `ActiveSupport::TaggedLogging.new`. Adding tagging functionality to a logger should be done using a more ruby approach such as `logger.extend(AS::TaggedLogging)`. Fix rails#49757 Fix rails#49745 Fix rails#46084 Fix rails#44668 I made a propose on the Ruby logger upstream in ruby/logger#90, but it hasn't been reviewed it.
Steps to reproduce
BroadcastLogger#broadcast_to(logger)
to add a new loggerBroadcastLogger#tagged("MyTag")
without a block to return a new tagged loggerExpected behavior
Using
ActiveSupport::BroadcastLogger#tagged
without a block returns a tagged logger that can be used for logging as normal.Actual behavior
When broadcasting to more than one logger that supports tagging, an
Array
is returned rather than an object that you can log to directly.The specific use case where this came up: in some tests, I want to capture logs so I can test them. Previously I was replacing
Rails.logger
entirely, but now I tried adding and removing a broadcast target and I noticed this didn't work.This appears to be caused by the
method_missing
implementation:rails/activesupport/lib/active_support/broadcast_logger.rb
Line 242 in ca5132b
System configuration
Rails version: 7.1.1
Ruby version: 3.2.2
The text was updated successfully, but these errors were encountered: