Skip to content
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

feat(timber): prettier logs #263

Merged
merged 1 commit into from
Feb 22, 2021
Merged

Conversation

EverlastingBugstopper
Copy link
Contributor

@EverlastingBugstopper EverlastingBugstopper commented Feb 11, 2021

this is a crack at #220 that gives us some prettier logs and also separates logging concerns from user-facing output concerns.

log usage is still the same, folks still pass --log <log-level>, and instead of tracing::info for user-facing output, we directly use eprintln.

log formatting is also a bit different and a bit more helpful for debugging than our previous formatting:

   reqwest::async_impl::client: response '200 OK' for https://graphql.api.apollographql.com/api/graphql, , , , 
    at /home/avery/.cargo/registry/src/github.com-1ecc6299db9ec823/reqwest-0.11.0/src/async_impl/client.rs:1477

   rover_client::blocking::client: response_status: 200, response_headers: {"x-powered-by": "Express", "vary": "Origin", "access-control-allow-credentials": "true", "access-control-expose-headers": "logged-in,identity", "strict-transport-security": "max-age=31536000; includeSubDomains", "logged-in": "true", "identity": "user:gh.EverlastingBugstopper", "content-type": "application/json; charset=utf-8", "content-length": "290", "etag": "W/\"122-ywmnv7flrpbtgXc02AmStpRls80\"", "date": "Fri, 19 Feb 2021 20:53:05 GMT", "via": "1.1 google", "alt-svc": "clear"}
    at crates/rover-client/src/blocking/client.rs:51

   rover_client::blocking::client: {"data":{"frontendUrlRoot":"https://studio.apollographql.com","service":{"implementingServices":{"__typename":"FederatedImplementingServices","services":[{"name":"people","url":"","updatedAt":"2020-12-18T18:25:44.844Z"},{"name":"films","url":"","updatedAt":"2020-12-18T18:26:10.961Z"}]}}}}

@EverlastingBugstopper
Copy link
Contributor Author

EverlastingBugstopper commented Feb 11, 2021

comment out of date, see updated top level comment

perHAPS. perHAPS. We could just get around all of this by adding some sneaky eprintln!(" ") before our println statements in RoverStdout. This way output wouldn't get mangled if it was passed to another process AND the spacing of our output would align properly with the default info level logs, making things look much nicer.

@EverlastingBugstopper
Copy link
Contributor Author

EverlastingBugstopper commented Feb 16, 2021

comment out of date, see updated top level comment

still not loving this. after some thought - maybe a better approach is more like this:

by default we do not print any tracing logs to stderr, and we convert all of our app-level user messages (that are currently tracing::info!) to eprintln! statements, and we persist all debug/trace level logs to the users filesystem as outlined in #239

if user is TTY and they pass -l debug or -l info, we print those level logs to stderr and not persist the logs on the file system.

I would assume then that it's OK to just leave user-facing eprintln level logs out of the actual logs altogether.

@lrlna
Copy link
Member

lrlna commented Feb 17, 2021

If I remember correctly we were talking about timber eventually taking care of both logging with tracing crate and printing to stdout. I think your second comment saying that we should only print println/eprintln to the user makes a lot of sense if we end up going with that route in the future. If users are passing in --log=debug, we can just print the logs the way you have them formatted here (I think I'd even keep the source_location). And at that point, I don't think it matters too much that they are indented a bit to the right.

I wanted to also say that tracing_subscriber has a visitor that does all the padding (in case you haven't seen this already. If we really wanted to unify the padding situation, we can also write a small visitor of our own. I do think though if we are going to be using (even for just right now) only printlns to print out to the user including everything that's currently done by tracing::info, we can live with logs being a tad indented. Would be happy to hear your thoughts on this though!

.without_time()
.with_target(false)
.pretty()
.with_source_location(false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we (the rover team) are debugging-debugging, I think the source_location is pretty useful. Let's keep it!

@EverlastingBugstopper EverlastingBugstopper force-pushed the avery/TTTIIIMMMBBBEEERRR branch 2 times, most recently from 35a0eb5 to 2bb6c1c Compare February 17, 2021 19:51
@lrlna
Copy link
Member

lrlna commented Feb 19, 2021

By the way, I think this is more of stab at #220!

out.out Show resolved Hide resolved
@JakeDawkins JakeDawkins added feature 🎉 new commands, flags, functionality, and improved error messages and removed changelog - feature labels Feb 22, 2021
progress messages are now eprintln! and not tracing::info!
by default, no logs are printed to the user

if a log level is specified, they are formatted using tracings
pretty subscriber
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature 🎉 new commands, flags, functionality, and improved error messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants