Tag log messages with originating stream#903
Conversation
| Roast::Log.logger.warn { "#{format_path(event)} ❯❯ #{event[:stderr]}" } | ||
| Roast::Log.logger.warn(Roast::Log::Message.new( | ||
| stream: :stderr, | ||
| text: "#{format_path(event)} ❯❯ #{event[:stderr]}", | ||
| )) |
There was a problem hiding this comment.
This is really clever. I like the approach of passing through a richer context object than just a string, so we can include additional parameters like the originating stream.
A couple minor optimizations I'd suggest:
The style of the existing logger invocation that might seem somewhat odd at first glance, where the message argument is passed inside a block passed to the logger.warn method instead of just passing the message directly as a string serves a useful purpose. The block is not evaluated at all if the relevant log level is disabled, saving the cost of the string formatting that happens inside it (and any additional work, which might in some cases be more substantial, like computing a full stack trace, etc.). I'd like to preserve that style of invocation, so please look into returning the Roast::Log::Message from a block instead of passing it directly (and because it now includes object instantiation, not just string formatting, there is actually a bit more perf overhead to save when the log won't actually be emitted).
I'd also suggest you consider renaming the stream parameter to something more generic, like type. Right now, special colouring rules are only used for stdout and stderr, but it might be the case that in the future we want additional special colours (for instance, we might want them for block output for prompts and/or responses of chat and agent cogs), so making that param a little more generic makes it more future-proof for this kind of thing
2 participants
No description provided.