-
-
Notifications
You must be signed in to change notification settings - Fork 2k
Switch to stdlib log #5426
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
base: master
Are you sure you want to change the base?
Switch to stdlib log #5426
Conversation
2f1c8ee
to
a9b370c
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.
Behemoth PRs like this are a bit hard to review 😅 Do you think you can split it into smaller ones?
More specifically I think the module changes mentioned here:
Some functions were encapsulated into modules (YoutubeJSONParser, Parser, I18n, and others) to keep the source (from where the Log function is being called) consistent for some functions.
Can be split off into smaller PRs that can be merged after this main migration. Having it all in one adds a bunch of noise to the primary change of switching to the stdlib logger.
In my opinion for a large-scale code migration like this having some minor issues during the midst of it like with how logs are represented isn't too big of a deal.
) | ||
# Log only the path | ||
requested_url = context.request.path | ||
formatter = ::Log::Formatter.new do |entry, io| |
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.
Just a comment
If we want to squeeze some extra performance calling a function is slightly faster (1.03x on my machine) than a proc so subclassing Log::Formatter
or Log::StaticFormatter
will result in slightly faster performance than using the proc constructor.
I don't know if such a small gain will be worth it though
Done, that should be better ;) |
|
||
class Config | ||
include YAML::Serializable | ||
CLog = ::Log.for(self) |
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.
For consistency can you also set this as Log
? A namespace collision can be prevented by changing the Log::Severity
below to ::Log::Severity
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.
I did that because I didn't know how to fix this error 😅:
...
> 151 | # Sort types to avoid parsing nulls and numbers as strings
> 152 |
> 153 | [Log::Severity].each do |ivar_type|
> 154 | if !success
> 155 | begin
> 156 | config.log_level = ivar_type.from_yaml(env_value)
> 157 | success = true
> 158 | rescue
> 159 | # nop
> 160 | end
> 161 | end
> 162 | end
...
Error: undefined constant Log::Severity
It should be ::Log::Severity
but is a passed as Log::Severity
in the macro.
message = entry.message | ||
severity = entry.severity | ||
data = entry.data | ||
source = entry.source | ||
timestamp = entry.timestamp | ||
|
||
io << (use_color ? timestamp.colorize(:dark_gray) : timestamp) << " " | ||
io << (use_color ? colorize_severity(severity) : severity.label) << " " | ||
io << (use_color ? source.colorize(:dark_gray) : source) << ": " if !source.empty? | ||
io << message | ||
if !data.empty? | ||
io << " " | ||
data.each do |dat| | ||
io << (use_color ? dat[0].to_s.colorize(:light_cyan) : dat[0].to_s) | ||
io << "=" | ||
io << dat[1].to_s | ||
end | ||
end |
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.
I'd suggest to use the IO when using Colorize so that no intermediary string is created. The code is a bit convoluted due to the way Colorize
is implemented in the standard library.
The use_color
condition can also be removed, as Colorize
keeps track of the enabled state (see lines 7/8 of this same file).
Finally, .each
does nothing when the array is empty, thus making the if
redundant. Note that in my suggestion the key=value
pairs are separated by a single space, but a newline or another character might be more appropriate.
message = entry.message | |
severity = entry.severity | |
data = entry.data | |
source = entry.source | |
timestamp = entry.timestamp | |
io << (use_color ? timestamp.colorize(:dark_gray) : timestamp) << " " | |
io << (use_color ? colorize_severity(severity) : severity.label) << " " | |
io << (use_color ? source.colorize(:dark_gray) : source) << ": " if !source.empty? | |
io << message | |
if !data.empty? | |
io << " " | |
data.each do |dat| | |
io << (use_color ? dat[0].to_s.colorize(:light_cyan) : dat[0].to_s) | |
io << "=" | |
io << dat[1].to_s | |
end | |
end | |
entry.timestamp.colorize.dark_gray.to_s(io) | |
io << " " | |
colorize_severity(entry.severity).to_s(io) | |
io << " " | |
if !entry.source.empty? | |
entry.source.colorize.dark_gray.to_s(io) | |
io << ": " | |
end | |
io << entry.message | |
data.each do |dat| | |
io << " " | |
dat[0].colorize.light_cyan.to_s(io) | |
io << "=" << dat[1] | |
end |
Closes #5413
CONFIG.output
works again (it didn't seem to work when I tested it)Some functions were encapsulated into modules (YoutubeJSONParser
,Parser
,I18n
, and others) to keep thesource
(from where the Log function is being called) consistent for some functions.A macro::Log.forf
was added to add the function name to the source so we can prevent usingLog.info { "fetch_channel: something" }
,Log.info { "fetch_channel: something2" }
, etc.How it looks and behaves: https://asciinema.org/a/WQXsdnxcV1Hu7fOufJeMEaovZ
It's slower (~1.06x times slower) than using the old
Invidious::LogHandler
, but this is mostly because of heavy use of colorization. Without colorization,Log
is 1.12x times faster.With the
Invidious::Logger.formatter
added in this PR, it's possible to useLog.info &.emit("msg", error: "someerror", debuginfo: response.data)
or something like that which may be useful for better context in the logs.