ActiveSupport::LogSubscriber restore compatibility with SemanticLogger #49621
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fix: rails#49563 The semantic_logger gems doesn't behave exactly like stdlib logger in that `SemanticLogger#level` returns a Symbol while stdlib `Logger#level` returns an Integer. Because of this we can't simply compare integers, we have to use the various `#{level}?` methods.
byroot
added a commit
that referenced
this pull request
Oct 13, 2023
…-compatibility ActiveSupport::LogSubscriber restore compatibility with SemanticLogger
This was referenced Oct 18, 2023
|
@byroot Facing the same issue related to log level. Which version can we expect this fix to be available? |
|
You can fix it right now by pointing your Gemfile at |
|
@byroot I started facing this issue when I tried to upgrade from 7.0.8 to 7.1.1. Should I be pointing to Will this change be released in a new version called |
|
@preethi29 I think he meant |
|
Yeah that's what I meant, sorry. |
Edouard-chin
added a commit
to Edouard-chin/rails
that referenced
this pull request
Dec 12, 2023
Fix `config.log_level` being ignored when using a Broadcast Logger: - ### Problem If an application has configured a Broadcast Logger, setting the `config.log_level` to any value has no effect: ```ruby # config/application.rb config.logger = ActiveSupport::BroadcastLogger.new(Logger.new(STDOUT)) config.log_level = :warn puts Rails.logger.broadcasts.map(&:level) # => [Logger::DEBUG] ``` This is a side effect of rails#49621 which tried to fix the `log_level` default value overriding the whole broadcasts. ### Context The `log_level` default's value shouldn't apply to a Broadcast Logger, as otherwise it overrides whatever the application previously configured. While this behaviour is the same with a vanilla logger, at least we can workaround it: ```ruby # When using a vanilla logger config.logger = Logger.new(STDOUT, level: LOGGER::WARN) # Once the app boots, the level is overriden to DEBUG. We need to add the following line. config.log_level = :warn # When using a broadcast logger stdout = Logger.new(STDOUT, level: Logger::INFO) stderr = Logger.new(STDERR, level: Logger::ERROR) config.logger = ActiveSupport::BroadcastLogger(stdout, stderr) # Once the app boots the whole broadcast level is overriden to DEBUG. # There is no way to workaround this as you can't fine grain the level of each # loggers with `config.log_level=`. ``` ### Solution This PR ignores the default `log_level` value when using a Broadcast Logger, but ensure it gets used if an application manually sets it. Fix rails#50324
Edouard-chin
added a commit
to Edouard-chin/rails
that referenced
this pull request
Dec 12, 2023
- ### Problem If an application has configured a Broadcast Logger, setting the `config.log_level` to any value has no effect: ```ruby # config/application.rb config.logger = ActiveSupport::BroadcastLogger.new(Logger.new(STDOUT)) config.log_level = :warn puts Rails.logger.broadcasts.map(&:level) # => [Logger::DEBUG] ``` This is a side effect of rails#49621 which tried to fix the `log_level` default value overriding the whole broadcasts. ### Context The `log_level` default's value shouldn't apply to a Broadcast Logger, as otherwise it overrides whatever the application previously configured. While this behaviour is the same with a vanilla logger, at least we can workaround it: ```ruby # When using a vanilla logger config.logger = Logger.new(STDOUT, level: LOGGER::WARN) # Once the app boots, the level is overriden to DEBUG. We need to add the following line. config.log_level = :warn # When using a broadcast logger stdout = Logger.new(STDOUT, level: Logger::INFO) stderr = Logger.new(STDERR, level: Logger::ERROR) config.logger = ActiveSupport::BroadcastLogger(stdout, stderr) # Once the app boots the whole broadcast level is overriden to DEBUG. # There is no way to workaround this as you can't fine grain the level of each # loggers with `config.log_level=`. ``` ### Solution This PR ignores the default `log_level` value when using a Broadcast Logger, but ensure it gets used if an application manually sets it. Fix rails#50324
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix: #49563
The semantic_logger gems doesn't behave exactly like stdlib logger in that
SemanticLogger#levelreturns a Symbol while stdlibLogger#levelreturns an Integer.Because of this we can't simply compare integers, we have to use the various
#{level}?methods.