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

Add logging #370

Closed
Closed

Conversation

mwilliammyers
Copy link

@mwilliammyers mwilliammyers commented Jun 11, 2019

As it stands right now, this is an MVP of logging, with just the bare minimum I thought useful (at least at for my work).

Some questions need to be answered:

  • Should we treat logging from the perspective of Juniper users or from Juniper's perspective?
    • e.g. should validation errors be warn/error or simply debug and reserve warn and error for recoverable errors and unrecoverable errors?
    • In general, how much logging should we do?
  • Right now, I just logged the execute function. Maybe we should go 1+ levels deeper, and for example, log in the execute_validated_query function so that users can be more granular and enable/disable juniper::executor etc.
  • We might want to add a warning to the docs about logging sensitive information? We probably shouldn't log requests in case they contain passwords?
    • I am thinking about creating a ClearTextPassword scalar type in my code and making the Display/Debug impl be something like [password]. Perhaps we add something to that effect to the book?

I didn't and probably won't (yet) do much logging of the actual parser because #138...

I am happy to keep adding commits (and squashing?) to this PR and/or do this in chunks and open smaller PRs as I add more logging.

Closes #48.

@LegNeato
Copy link
Member

Thanks for the patch! I need to think about this for a little bit.

@Empty2k12
Copy link
Contributor

I think this should rather be using https://github.com/tokio-rs/tracing where the user can choose how something is logged. Additionally, traces can be used for much more than just logging, for example users could send them to Zipkin or some trace viewer and visually check how the application is performing.

Copy link
Member

@LegNeato LegNeato left a comment

Choose a reason for hiding this comment

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

Thanks again for the PR! After reading up I think using https://github.com/tokio-rs/tracing/tree/master/tracing is the better choice and will also be easier to reason about once we go async (soon™).

@mwilliammyers
Copy link
Author

mwilliammyers commented Jul 20, 2019

I will need to look at tracing more, but unless I am mistaken, tracing is for application level logging/tracing. Juniper is a library. So, users could use the tracing crate in their application which uses the Juniper library...

This PR uses a logging facade via the log crate for that purpose—so users can decide which logging front end to use (e.g. tracing or env-logger etc). Regardless, the log crate is pretty much the standard/official (it is maintained by the Rust lang team and has 13+ million downloads) way to implement logging in libraries.

Also, tokio uses the log crate...

@theduke
Copy link
Member

theduke commented Aug 30, 2019

Thanks for the PR @mwilliammyers , but I'm closing this for now.

We will want to use the tracing crate, especially with async support, but even without it.
It gives us the possibility for fine-grained resolver based tracing.

@theduke theduke closed this Aug 30, 2019
@mwilliammyers
Copy link
Author

@theduke I am happy to open a new PR using tracing (or update this one), but if we want fine-grained tracing events (e.g. span etc.) and async support; then why not use async_log? It is built by the Rust async team and builds on the standard log crate, and will probably be the de facto async logging crate.

There is also tracing_log which users could use if they wanted to use tracing:

use tracing_log::LogTracer;
use log;

LogTracer::init()?;

// will be available for Subscribers as a tracing Event
log::trace!("an example trace log");

I would really prefer to not force users to use a specific logging adapter and let them choose the crate.

@Empty2k12
Copy link
Contributor

You can fine-grain tracing! The idea is, that there are different implementations on subscribers which can indicate interest in a specific trace or event. These subscribers could - for example - only listen to events emitted by Tokio and Juniper, or just Tokio, or just events from a specific thread, or ... What the subscriber does with the events is also not set in stone. There's existing logging subscribers or timing subscribers, but only your imagination can set the limits here. I, for example, are tinkering with a subscriber that exports events in the OpenZipkin format, so traces can be visualized. One could also think about exporting to Honeycomb.

@theduke
Copy link
Member

theduke commented Aug 30, 2019

Open to all ideas. Let's discuss in #423.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Leverage Slog And Or Log For Debugging Output
4 participants