-
Notifications
You must be signed in to change notification settings - Fork 262
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
Consider changing the default enabled log levels #147
Comments
Bump for this for the missing offline comments. Also, has build-time stubbing of disabled levels (like slog does) been discussed? Is it in scope here? |
Oops, sorry! You can already statically disable levels actually: https://github.com/rust-lang-nursery/log/blob/master/Cargo.toml#L23-L35. The question here is over defaults. I feel pretty strongly that we shouldn't disable any level of logging by default. The whole point of logging is that you have bunch of diagnostic information available to you when you need it. You normally don't need to look at debug or trace level logging, but when you do it's invaluable! For example, Hyper's trace-level logging helped track down a bug in its chunked request body parser in a running server. If, for example, trace logging were disabled by default, you'd have several problems. If I ran into that Hyper problem above, I'd have to ship a new custom build of the server with trace logging enabled to actually be able to get the information I need. If the issue you're looking into is one of the annoying transient ones that no-one knows how to reproduce but pops up in production every once in a while, it's really important to be able to look very closely at whatever box it shows up on immediately! I could also override the default to enable trace logging by default, but then I potentially have other issues - if people become too used to trace logging being turned off, you may run into issues where those log events are extremely expensive or even broken! Rustc has some of those issues - the old log framework disabled debug logging in prod builds, and didn't evaluate log arguments if the log event wasn't enabled and it turned out that several would panic if actually evaluated. In addition, there is a fast path for logging levels that are entirely disabled at runtime. If you don't have any trace-level logging enabled in your logger configuration, the cost of a |
Thanks for the feedback! It looks like my knowledge around here is wrong/stale, so I'm paging @dpc for some informed comment. |
I understand @sfackler points, and I think they make sense. I also don't feel strongly about default logging level in As a library writer, it is convenient to have a logging level for your disposal, that you don't have to feel guilty about using, and comes handy when you are testing and debugging your library while you are working on it, change it etc.. It is not optimal if
There are some differences between Also, because in slog you do compose how the different pices of your software route their logging messages, it is possible to modify in-flight eg. lowering logging levels of your dependencies. Even if they do overuse trace, or debug, and you turned it on, they still wouldn't get logged and their overhead would be minimal. So in slog, if there's something useful to know in production, no matter how minor it might be from global perspective you can make it an So I don't think |
For reference, I ended up here today because of such discussions happening around TLS libraries and logging: |
I think a concur that all log levels should be compiled in by default. Would be quite surprising for a naive user to deploy an app, go to turn on verbose logs and discover they don't exist. |
Probably about time to decide one way or the other here. |
I'm personally on board with "compile everything in by default" |
Compiling all by default sounds good to me too. Are we happy to close this or would you like to keep it open to solicit some more feedback? |
I think we're good to go. |
Right now I believe all log levels are enabled by default in all builds. @sfackler provided some good rationale for this during the evaluation, @sfackler want to copy that down here as well?
In any case would be worth discussing before a 1.0 release!
The text was updated successfully, but these errors were encountered: