Out of band crate evaluation for 2017-07-06: env_logger

[sebasmagri]

Crate evaluation for 2017-07-06: env_logger

For additional contribution opportunities, see the main libz blitz thread.

This post is a wiki. Feel free to edit it.

Links

• https://crates.io/crates/env_logger
• https://github.com/sebasmagri/env_logger
• https://docs.rs/env_logger

Needs your help!

Anything that is not checked off still needs your help! There is no need to sign up or ask for permission - follow any of these links and leave your thoughts:

Guidelines checklist

Legend

• [y] = guideline is adhered to, no work needed.
• [n] = guideline may need work, see comments nearby
• [/] = guideline not applicable to this crate

Checklist

Guidelines checklist

This document is collaboratively editable. Pick a few of the guidelines, compare the env_logger crate against them, and fill in the checklist with [y] if the crate conforms to the guideline, [n] if the crate does not conform, and [/] if the guideline does not apply to this crate. Each guideline is explained in $

  - [n] Crate name is a palindrome (C-PALINDROME)
   - error-chain backwards is niahc-rorre which is not the same as error-chain

Cookbook recipes

Cookbook example ideas

Come up with ideas for nice introductory examples of using env_logger, possibly in combination with other crates, that would be good to show in the Rust Cookbook. Please leave a comment in that issue with your ideas! You don’t necessarily have to write the example code yourself but PRs are always welcome.

API guideline updates

What lessons can we learn from env_logger that will be broadly applicable to other crates? Please leave a comment in that issue with your ideas!

Discussion topics

(List concerns that would be valuable to discuss in the libs team meeting)

Crate issues

(List potential issues to file after the review)

How are we tracking?

Pre-review checklist

Do these before the libs team review.

• [x] Create evaluation thread based on this template
• [ ] Work with author and issue tracker to identify known blockers
• [ ] Compare crate to guidelines by filling in checklist
• [ ] Record other questions and notes about crate
• [ ] Draft several use case statements to serve as cookbook examples
• [ ] Record recommendations for updated guidelines

Post-review checklist

Do these after the libs team review.

• [ ] Publish libs team meeting video
• [ ] Create new issues and tracking issue on crate’s issue tracker
• [ ] Solicit use cases for cookbook examples related to the crate
• [ ] File issues to implement cookbook examples
• [ ] File issues to update guidelines
• [ ] Post all approachable issues to TWiR call for participation thread
• [ ] Update links in coordination thread

[brson]

Thanks a bunch for leading this one @sebasmagri, and for offering to take on maintenance of env_logger. Hopefully this one will be short and sweet since it got some attention during the previous log evaluation. But it was not the focus of that review, so I’m hopeful there’s some more polish to be stirred up. I’ll try to pitch in with the review a bit next week.

[KodrAus]

I’ve started filling in the guidelines checklist. I’ll do some more work on it later today. I think it’s mostly missing docs and a few common trait impls.

[budziq]

My biggest gripes are that:

• There is no way to either specify a file Target or provide some custom impl.
• The Builder::format having to return a String is quite restrictive.

Arguably the most valuable part of the crate is the env parsing. I’d love to have it exposed in some way as a building block for a more advanced logger.

[KodrAus]

We could make the Target some kind of trait that’s implemented for stderr and stdout, and could also be implemented for File too.

What do we expect from env_logger? If it’s meant to be a no-fuss sink for emitting logs, then its current scope seems reasonable to me. Unfortunately there’s a pretty big conceptual jump between env_logger and something like slog, so if env_logger doesn’t do a simple thing that someone expects then having to move to slog could be seen as busy-work.

[jethrogb]

There is no way to either specify a file Target or provide some custom impl.

Arguably the most valuable part of the crate is the env parsing. I’d love to have it exposed in some way as a building block for a more advanced logger.

There’s a PR for this: https://github.com/rust-lang-nursery/log/pull/203

[budziq]

if env_logger doesn’t do a simple thing that someone expects then having to move to slog could be seen as busy-work.

Your concerns are perfectly valid! And I’m not suggesting to add any bells and whistles to the crate itself but possibly make it just slightly more configurable and reusable as a building block for other loggers.

I venture that large part of users would be satisfied with the simplest possible logger (instead of going for more advanced stuff like slog, fern or log4rs). And env_logger is “oh so close” to this sweet spot but still does not wins the cake.

For instance:

• I would like to log to file - and this would be a oneliner if env_logger has just slightly different API
• Would like to have an env_logger env var compliant logger with colored output like pretty_env_logger to support colors in cross platform way - but its impossible due to either:
  • Builder::Format having to return a String
  • env_logger parsing and filtering being essentially not reusable

It’s not like I feel super strongly about this but I think that this might offer some perspective.

We could make the Target some kind of trait that’s implemented for stderr and stdout, and could also be implemented for File too.

How about just a trait bound on std::io::Write?

[budziq]

I guess that I would prefer to have Directive and parse_spec exposed and made easily consumable by an API than having to instantiate a separate dummy Silent logger just to achieve a boolean matches function.

[sebasmagri]

So long, seems like the biggest issues are:

• Having a better API for Target (I like the std::io::Write approach)
• Making Builder::Format return a higher level type to allow more advanced use cases
• Revamp parsing and filtering to make it extensible/reusable

Any comments on this? I will create some issues in the new repo after confirmation.

[KodrAus]

@sebasmagri I’ve finished going through the checklist and left a few points you may or may not think are worthwhile. I think those points you’ve posted are a good set of API improvements.

[sebasmagri]

There’s something I’d like to define before.

How much of this do you think should/has to be implemented in the log base API instead? Should it be convenient to make env_logger a complete alternative to featureful logging crates such as slog or log4rs? Or should it be just an env controlled log with a smaller feature set while improving log?

[sebasmagri]

@KodrAus thanks for the checklist update! Let’s try to discuss it a little bit more to start adding actions/issues and to plan the roadmap of the next updates.

[KodrAus]

How much of this do you think should/has to be implemented in the log base API instead?

This is a good question. We should include env_logger in the discussion about how other loggers fit with log.

In my opinion, there’s no reason env_logger shouldn’t have its scope expanded if log is going to focus just on exposing APIs for libraries to write logs and provide a simple out-of-the-box Logger so they always go somewhere.

What’s the goal of env_logger given there are more sophisticated loggers out there already? I think catering to a common case of wanting to log somewhere with minimal fuss is a good value proposition that’s compatible with the loggers already out there.

[budziq]

I think catering to a common case of wanting to log somewhere with minimal fuss is a good value proposition

I’d certainly opt for env_logger to cater to the most common and trivial cases with minimal LOC.

So that people coming with a simple needs will be able to handle these trivial problems with trivial API via env_logger or in their own “scratch my itch” logger reusing part of env_logger’s impl. Avoiding the situations which leads users to switch loggers due to some minor detail.

On the other people looking for sophistication and advanced features will know to look elsewhere and will be well equipped with the massive logger framework crates already on the market.

[sanxiyn]

I personally think env_logger is a terrible name and it would be nice if it had better name. Not sure what that should be though.

[brson]

So long, seems like the biggest issues are:

Having a better API for Target (I like the std::io::Write approach) Making Builder::Format return a higher level type to allow more advanced use cases Revamp parsing and filtering to make it extensible/reusable

Any comments on this? I will create some issues in the new repo after confirmation.

It seems like we need to get opinions from @sfackler and @alexcrichton about expanding the scope of env_logger.

If you can’t get their feedback I’d suggest formulating the issues as “consider blah …” and waiting for more feedback.

I went over the checklist and everything looked fine. @sebasmagri before you file the issues do you mind writing them up in checklist format like they will appear in the tracking issue and post them here for review? For reference here’s what I did for error-chain.

The matter with moving env_logger out of the log repo needs to be resolved soon, because the situation is pretty unclear right now. The two code-bases are presumably already diverging.

I’d suggest filing a PR against log to remove env_logger to politely force the issue.

[alexcrichton]

I think it’d be fine to make env_logger much more flexible than it currently is, it’s sort of just been stagnant for a long time and it could definitely use an update!

[sebasmagri]

What do you think could be the process to make the split from log?

From the issues mentioned before in the thread, what do you think would be the best improvements to make it more flexible?

[sebasmagri]

I’ve added a tracking issue with clear action items from the first round of evaluation to the env_logger repo:

https://github.com/sebasmagri/env_logger/issues/5

Please feel free to work on any of them.

We also have a PR open that could use some additional comments.

[system]

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.