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

Document and Match the Rails logger interface #24218

Merged
merged 1 commit into from Mar 17, 2016

Conversation

Projects
None yet
3 participants
@schneems
Member

schneems commented Mar 16, 2016

The logger interface to get all Rails features is not obvious. This change adds documentation on how to assign a logger that will use all Rails features.

We are also matching the stdout logging interface to the default logger in bootstrap

logger = ActiveSupport::Logger.new f
logger.formatter = config.log_formatter
logger = ActiveSupport::TaggedLogging.new(logger)
logger
.

Document and Match the Rails logger interface
The logger interface to get all Rails features is not obvious. This change adds documentation on how to assign a logger that will use all Rails features.

We are also matching the stdout logging interface to the default logger in bootstrap https://github.com/rails/rails/blob/f5a5988352b165143f0f9d622707c351c1470882/railties/lib/rails/application/bootstrap.rb#L42-L45.
@schneems

This comment has been minimized.

Member

schneems commented Mar 16, 2016

I think we could make this interface way better. Here's some alternatives to simply documenting.

Have config.log_formatter= set the default formatter of TaggedLogging so that you don't have to manually assign yourself. It seems weird that we have a config value that does nothing if you're also setting config.logger. In addition to this change we could also:

  • Have the TaggedLogging module also include the modules needed to silence logs. This would let you use the default Logger class so you could do this instead.
config.logger = ActiveSupport::TaggedLogging.new(::Logger.new(STDOUT))
  • Have ActiveSupport::Logger which already implements the silence behavior extend ActiveSupport::TaggedLogging by default. Then this code could look like this:
config.logger = ActiveSupport::Logger.new(STDOUT)

We could go a step further and auto wrap logs with ActiveSupport::TaggedLogging if they aren't already wrapped, but that might be a bit too magical.

At bare minimum we should document the current interface, but we could make it easier to those who want to provide a custom logger.

@vipulnsward

This comment has been minimized.

Member

vipulnsward commented Mar 16, 2016

"We could go a step further and auto wrap logs with ActiveSupport::TaggedLogging if they aren't already wrapped, but that might be a bit too magical."
👍 for this, but yes this is too magical, and might get confusing/hard to debug.

schneems added a commit that referenced this pull request Mar 17, 2016

Merge pull request #24218 from schneems/schneems/match-logger
Document and Match the Rails logger interface

@schneems schneems merged commit 47c0a39 into rails:master Mar 17, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment