env_logger: Add config to Logger to allow changing log format etc

[nicksanders]

env_logger: Add config to Logger to allow changing the environmental variable, the default log level and the log format

#[macro_use]
extern crate log;
extern crate env_logger;
extern crate time;

use log::{LogRecord, LogLevelFilter};
use env_logger::LogConfig;

fn main() {
    let formatter = |record: &LogRecord| {
        let t = time::now();
        format!("{},{:03} - {} - {}",
            time::strftime("%Y-%m-%d %H:%M:%S", &t).unwrap(),
            t.tm_nsec / 1000_000,
            record.level(),
            record.args()
        )
    };

    let config = LogConfig {
        env_var: "RUST_LOGGER",
        format: Box::new(formatter),
        level: LogLevelFilter::Info,
    };

    env_logger::init_with_config(config).unwrap();

    error!("error message");
    info!("info message");
}

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[alexcrichton]

Thanks for the PR! I think if we're going to expose any form of configuration, however, it shouldn't touch environment variables and should instead accept a pre-parsed form. I'm not sure how much we want to expose from this library just yet, though, as it's currently quite conservative in its exposed API and we'd likely look to stabilize it one day.

[nicksanders]

I only added the environment variable as I saw it in an open issue so I'm happy to remove that.

I do think there should be a way to change the default log format and the default log level though.

[alexcrichton]

Yeah let's remove the environment variable aspect, and I think that this should also try to minimize API additions where possible. Along those lines, how about having Config as a builder-style API which exposes all the options for configuration? There'd be a final init() method (or something like that) which actually performs the initialization, and then the global init method would just delegate to a Config after parsing RUST_LOG.

[nicksanders]

I'm not sure I understand what you meant about the global init method but the builder style API is a lot cleaner :)

[alexcrichton]

Looking good! I think the init method on the builder is still doing a bit too much logic, however, as it shouldn't in theory be touching the environment. Taking another look at this, though, I'm not quite sure how this configuration is beneficial for this crate?

The main task for this crate is taking the RUST_LOG environment variable and parsing it into a configuration which is used to filter log messages, but if you want fine-grained configuration it seems like you should just not use this crate at all? I'm not sure what the major benefit is from using this crate while still customizing the behavior. Could you help clarify the use case?

[nicksanders]

I have a server that I need to log timestamped logs at a default log level of Info.

I would like to be able to use env_logger so that I don't have to recompile to change the log level to debug issues.

I'll see if there's another way to use the environment variable parsing with a custom logger.

[alexcrichton]

Hm ok, so in that sense I think that the current incarnation makes sense, I think the env::var logic should just be refactored out. The build probably wants a method that looks like:

pub fn filter(&mut self, module: &str, level: LogLevel) -> &mut Self;
pub fn parse(&mut self, filters: &str) -> &mut Self;

That way if you're customizing you can just call parse, and the top level would call parse with env::var("RUST_LOG").

[alexcrichton]

To make this a little more ergonomic, I'd recommend writing this as:

pub fn format<F>(&mut self, format: F) -> &mut Self
    where F: ...
{
    self.format = Box::new(format);
    self
}

[nicksanders]

I removed LogConfig because I didn't think it made sense anymore?

I added filter but wasn't sure if it was needed, couldn't we just use parse?

Would it make sense to convert the tests to use parse?

[alexcrichton]

I removed LogConfig because I didn't think it made sense anymore?

I think we may want to keep the extra structure around to emphasize that it's a builder and not a usable instance itself. I'd recommend renaming it to LogBuilder, though, for a name.

I added filter but wasn't sure if it was needed, couldn't we just use parse?

I think there's two use cases here:

• If a configuration is known ahead of time, or a different syntax is desired, it makes more sense to allow direct configuration rather than serializing into the parsable syntax we're expecting.
• If the same syntax is coming from a different location (e.g. not RUST_LOG), then we still want to provide the same parsing method.

As a result I think there's good use cases for both, so I figured we'd have both.

Would it make sense to convert the tests to use parse?

Nah I think it's fine to leave them as-is.

[alexcrichton]

Could you be sure to add documentation for all these new methods as well?

[alexcrichton]

Oh dear, sorry this fell by the wayside @nicksanders! Feel free to ping the PR whenever you update it as unfortunately github doesn't send out notifications otherwise.

r? @sfackler, this looks good to me, but curious on your thoughts as well

[softprops]

+1

[Draghtnod]

+1 I'm really missing some sort of date in the logs.

[nicksanders]

I have rebased the pull request #48 so the changes are easier to see.

Below is an updated example of how to add date/time to the log format.

#[macro_use]
extern crate log;
extern crate env_logger;
extern crate time;

use std::env;
use log::{LogRecord, LogLevelFilter};
use env_logger::LogBuilder;

fn main() {
    let format = |record: &LogRecord| {
        let t = time::now();
        format!("{},{:03} - {} - {}",
            time::strftime("%Y-%m-%d %H:%M:%S", &t).unwrap(),
            t.tm_nsec / 1000_000,
            record.level(),
            record.args()
        )
    };

    let mut builder = LogBuilder::new().format(format).filter(None, LogLevelFilter::Info);

    if env::var("RUST_LOG").is_ok() {
       builder = builder.parse(&env::var("RUST_LOG").unwrap());
    }

    builder.init().unwrap();

    error!("error message");
    info!("info message");
}

[alexcrichton]

Thanks again @nicksanders!

[softprops]

will this be published in a new version soon?

[alexcrichton]

Sure, I just published 0.3.2

[softprops]

Thanks you guys are the best!