subscriber: add with_binary_name and with_process_id options. #2655
Conversation
|
I was going back and forth on whether to put the binary name and pid before or after the log level on the line. I think I'm leaning towards before (so right after the timestamp) currently, but what do you think? |
nicolasavru
force-pushed
the
format-binary-name-pid
branch
3 times, most recently
from
ebdcbcc
to
5340233
Compare
|
I'd love to see this merged. Can I help promote this somehow? |
|
I think it's a useful feature to have |
|
Is this planned to be merged anytime soon? I'd like to switch to tracing but we also merge logs of multiple applications and need to see the application name as part of the log. |
There was a problem hiding this comment.
Looks quite good! About the output order, currently it looks like this:
2024-07-09T15:16:26.616426Z cool PID(456304) INFO ThreadId(01) fmt: yak shaving completed. all_yaks_shaved=false
IMO it looks better with all the "os-stuff" before the level ("INFO"). So maybe we should move ThreadId back as well? @hawkw what do you think?
| /// [argv\[0\]]: std::env::args | ||
| pub fn with_binary_name(self, | ||
| display_binary_name: bool, | ||
| binary_name: Option<&str>, |
There was a problem hiding this comment.
This is not formatted correctly. Please run cargo fmt.
There was a problem hiding this comment.
cargo fmt was missing this subdir, I'm guessing due to rust-lang/rustfmt#3253. Had to run rustfmt manually.
| /// otherwise the spcified string will be used. | ||
| /// | ||
| /// [argv\[0\]]: std::env::args | ||
| pub fn with_binary_name( |
There was a problem hiding this comment.
To be honest I would lean more towards application_name instead of binary_name what do you think?
There was a problem hiding this comment.
To me, an application is the whole software package, so to speak, and could consist of multiple binaries working together. How about executable_name?
There was a problem hiding this comment.
That should work as well, yes!
| pub fn with_binary_name( | ||
| self, | ||
| display_binary_name: bool, | ||
| binary_name: Option<&str>, |
There was a problem hiding this comment.
To make the api more inclusive (and maybe avoid an additional allocation), we could use Option<impl Into<String>> here.
| ) -> Format<F, T> { | ||
| Format { | ||
| display_binary_name, | ||
| binary_name: binary_name.map(str::to_string), |
There was a problem hiding this comment.
This here could then be .map(Into::into).
|
No strong opinion on the order, but if you are showing multiple application logs in one terminal IMHO it's nicer if the order is |
|
That's actually a good point @gz! Let's do it like that then. |
nicolasavru
force-pushed
the
format-binary-name-pid
branch
2 times, most recently
from
a338a1a
to
12f69b5
Compare
|
Addressed comments, went back to |
nicolasavru
force-pushed
the
format-binary-name-pid
branch
2 times, most recently
from
35e6ec2
to
92e90a7
Compare
These options are useful when multiple processes are logging to the same destination (stderr, in the common case). This can happen when a process launches a child process or when a user simply launches multiple processes in the same terminal. Fixes tokio-rs#2447. Implement these two options.
nicolasavru
force-pushed
the
format-binary-name-pid
branch
from
92e90a7
to
ff0e343
Compare
Motivation
These options are useful when multiple processes are logging to the same destination (stderr, in the common case). This can happen when a process launches a child process or when a user simply launches multiple processes in the same terminal.
Fixes #2447.
Solution
Implement these two options.