-
Notifications
You must be signed in to change notification settings - Fork 3
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
Use ActiveSupport::Logger instead of Ruby std Logger for the Rails apps #309
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch on the issue, I've made a couple of suggestions
CHANGELOG.md
Outdated
@@ -1,3 +1,7 @@ | |||
# 9.0.4 | |||
|
|||
* Fix an issue of using Production like logs in development environment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't really tell users much about the change or the issue. Would suggest something like: Fix Rails logger not being a descendent of ActiveSupport::Logger and thus lacking expected methods.
It'd also be good to follow the convention of linking to the PR that introduced the changelog (some are gems are very consistent with this but this one seems to have become a bit loose)
@@ -26,6 +26,7 @@ def self.configure | |||
"#{hash.to_json}\n" | |||
}, | |||
) | |||
Rails.logger = ActiveSupport::TaggedLogging.new(logger) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit of a surprise to me that we use ActiveSupport::TaggedLogging here since the commit only talks about ActiveSupport::Logger. I would be more expecting that the change was: Rails.logger = ActiveSupport::Logger.new(
Couple of things I wonder:
a) do we actually need the tagged logger? if so can we include the reasons for that in the commit, if not shall we simplify to just ActiveSupport::Logger?
b) if someone was to actually set a tag logger.tag("test") { logger.warn("hi") }
might that actually break the JSON output or does it just prefix the message (hopefully the latter, just determining if tagging is dangerous)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah totally agree, we ended up using ActiveSupport::TaggedLoggging
because ActiveSupport::Logger.new()
didn't immediately work as expected. I had another playaround with it and realized that the formatter needs to be set separately instead of part of the keyword arguments in order for it to work. The new version uses ActiveSupport::Logger
instead of TaggedLogging.
…plication logger Rails libraries expect the `Rails.logger` instance to inherit from ActiveSupport::Logger. The standard library logger for example does not implement methods like `silence` which is used by some of the Rails internals. This change fixes the inheritance issue. Co-authored-by: [email protected]
ff2ac16
to
cc9fdbd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks - LGTM.
Have you given it a try in an app just to confirm it behaves as expected prior to releasing the gem?
yeah tested the latest changes as well, all good 👍 |
Rails libraries expect the
Rails.logger
instance to inherit from ActiveSupport::Logger. The ruby standard library logger for example does not implement methods likesilence
which is used by some of the Rails internals.This change fixes the inheritance issue.
Co-authored-by: [email protected]
This fixes the below error:
The error appears in development environment when Production like logging is enabled.
Tested with happy results: