ActiveSupport::BroadcastLogger does not respect config.log_level in Rails 7.1
#50324
Comments
|
👋 I think the real issue is the In Rails 7, if you had configured a broadcast with different logger levels, this would have had to be done inside an initializer after Rails' I agree with you that
|
|
Thanks for thinking it through with me @Edouard-chin I'm not sure about this:
If you don't set a My opinion is we just need this:
Although as I write that, I realise that is at odds with the issue described by #49721 🤔 (And I think even if we ignored the Proposing we take #49721 off |
The default # config/application.rb
stdout = Logger.new(STDOUT, level: Logger::INFO)
stderr = Logger.new(STDERR, level: Logger::ERROR)
config.logger = ActiveSupport::BroadcastLogger.new(stdout, stderr)
# After the app boots
puts Rails.logger.broadcasts.map(&:level) # => [Logger::DEBUG, Logger::DEBUG]
That's true, but if an application sets a broadcast logger with various levels and then explicitly calls I believe #49721 was originally opened to fix the issue in my first snippet, with the side effect that it's now completely ignoring Rails' I don't think it's worth reverting #49721. I can open a fix today (hopefully my rationale makes sense 😅). |
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
- ### 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
|
Thanks for your help, makes total sense to me. |
I am working on upgrading an app from Rails 7 to 7.1 and onto edge. I've come across an issue that I think is a bug, but could also be configuration not being used as intended.
Rails 7 config:
With this config,
Rails.logger.debugdoes nothing in production. 🟢The Rails 7 config relied on the private API described in #48615, so we have to refactor it for 7.1. 7.1/Edge Rails config:
With this config,
Rails.logger.debuglogs in production. 🔴I think the issue comes from #49721, since the
log_levelis no longer being assigned to the broadcast loggers. I would expectconfig.log_levelto work for whatever you assignconfig.loggertoo, and if that's changing it seems like a breaking change to me.Is the expectation that if you use
BroadcastLogger, you set thelevelmanually on loggers?cc @Edouard-chin @andrewn617 @rafaelfranca
System configuration
Rails version:
mainRuby version: 3.2
The text was updated successfully, but these errors were encountered: