-
Notifications
You must be signed in to change notification settings - Fork 268
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.
I like this a lot! What do you think about changing those unwraps to an "expect" just to give more context?
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 like it. We'll have decent logs now!
(now for some thoughts on future logging features that aren't really relevant right now but I want to get them out there)
Looking forward, the benefit of regex coloring may quickly degrade if we don't have conventions for log prefixes (like "error/net"), because we will get lazy and put whatever we want into the log messages, or make typos, so the patterns will deviate from the regex rules. It might be nice (in the future) to have an enum for all the different kinds of internal log messages, so we're forced to pick one when writing a log statement (or add another variant for a new type of message). Barring that, we should just be careful about it.
We could take enums a step further and let the variants have structure, so we're not limited to logging just strings. Each variant can know how to serialize itself. We can also apply coloring rules directly to those variants, in addition to regex matches of what's inside. There can be a generic variant for normal debug messages with a From<&str> impl so there's no boilerplate for simple log strings.
|
||
[logger] | ||
type = "debug" | ||
[[logger.rules.rules]] |
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.
awkward but ok :)
container_api/src/logger.rs
Outdated
// renders a log message, using the id color if no color specified for the message. | ||
pub fn render(msg: LogMessage) { | ||
let id_color = pick_color(&msg.id); | ||
let msg_color = match msg.color == "" { |
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.
Why not Option<String>
for color?
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.
fixed
pub logger_type: String, | ||
#[serde(default)] | ||
pub rules: LogRules, | ||
// pub file: Option<String>, |
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.
remove?
How about these standard logging levels: |
@zippy I see inconsistencies between "err" and "error". Might be easier with an enum? |
This PR adds: