-
Notifications
You must be signed in to change notification settings - Fork 128
Adds option to emit json structured logs #245
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: main
Are you sure you want to change the base?
Conversation
ikatson
left a comment
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.
Needs a fix, at least to unbreak the desktop app.
b3ca2e0 to
dd95c17
Compare
|
Thanks for the feedback and pointers, I'm moving this back to a draft for now while I fight with tracing_subscriber |
|
Also please don't change stderr to stdout for logs. If you really need it for some reason, make it a parameter, but keep the current behavior. |
|
I'm misunderstanding where the stdout log stream(s) are coming from, I'm seeing duplicated logs, some with human and some with json. I guess need to revisit all the branches of the logging recorder and see what I missed |
6b9c193 to
2ad7128
Compare
a13f938 to
3583c6e
Compare
| let log_layer: Box<dyn Layer<_> + Send + Sync> = if opts.log_file_json { | ||
| Box::new( | ||
| fmt::layer() | ||
| .with_ansi(false) |
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 makes log file contain ANSI escape codes which makes it hard to grep and use. Those escape codes are intended for terminal use only.
PR LGTM otherwise, I'd merge if not for this
This introduces two new options that affect logging output, one that turns on json-formatted logs for the log file option and one for the stdout stream.
I think I've managed to not change things too much, lmk what you think