-
Notifications
You must be signed in to change notification settings - Fork 21.7k
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
TaggedLogging not broadcasting tags #44668
Comments
Fixed by #44695 |
Edouard-chin
added a commit
to Edouard-chin/rails
that referenced
this issue
Oct 25, 2023
- ### 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.
Edouard-chin
added a commit
to Edouard-chin/rails
that referenced
this issue
Jan 17, 2024
- ### 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.
Edouard-chin
added a commit
to Edouard-chin/rails
that referenced
this issue
Oct 28, 2024
- ### 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.
Edouard-chin
added a commit
to Edouard-chin/rails
that referenced
this issue
Oct 28, 2024
- ### 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
This is similar to #43291, but I am calling the method with a block.
When not using a block it works fine, demonstrated by the test mentioned in the issue:
rails/activesupport/test/tagged_logging_test.rb
Line 221 in f95c0b7
Steps to reproduce
Expected behavior
Both loggers should log
"[OMG] Broadcasting...\n"
Actual behavior
One of them logs
"[OMG] Broadcasting...\n"
, the other logsBroadcasting...\n
instead.Result of the test script:
System configuration
Rails version: main
Ruby version: 3.1.1
The text was updated successfully, but these errors were encountered: