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

An ability to make a combined logger #8

Open
dvtomas opened this issue Jan 8, 2018 · 4 comments
Open

An ability to make a combined logger #8

dvtomas opened this issue Jan 8, 2018 · 4 comments

Comments

@dvtomas
Copy link
Contributor

dvtomas commented Jan 8, 2018

I'd like sloggers to be able to configure and instantiate a logger that logs both to terminal_logger and file_logger.

An example of expected configuration file:

[terminal_logger]

format = "compact" # full/compact
source_location = "none" # none/module_and_line
timezone = "local" # local/utc
destination = "stdout" # stdout or stderr
level = "info" # trace/debug/info/warning/error/critical

[file_logger]

format = "full" # full/compact
source_location = "none" # none/module_and_line
timezone = "local" # local/utc
level = "info" # debug/info/warning/error/critical
path = "railprofiles.log"
truncate = false # true/false, delete file before

Luckily, it is already possible to do it with this code (just stub shown):

use sloggers::file::FileLoggerConfig;
use sloggers::terminal::TerminalLoggerConfig;

#[derive(Deserialize)]
struct LoggersConfig {
    file_logger: FileLoggerConfig,
    terminal_logger: TerminalLoggerConfig,
}

pub fn logger_from_config_string(config_string: &str) -> Result<Logger, Error> {
    let config: LoggersConfig = toml::from_str(config_string).context("Couldn't parse logger config")?;

    let file_builder = config.file_logger.try_to_builder().context("Couldn't create file log builder from config")?;
    let file_logger = file_builder.build().context("Couldn't build file logger from builder")?;

    let terminal_builder = config.terminal_logger.try_to_builder().context("Couldn't create terminal log builder from config")?;
    let terminal_logger: Logger = terminal_builder.build().context("Couldn't build terminal logger from builder")?;

    let logger = Logger::root(Duplicate::new(file_logger, terminal_logger).fuse(), o!());
    Ok(logger)
}

This approach works for me so far, so this is more like a "what if" issue for me.

The interesting questions are

  • should this be part of sloggers?
  • does this approach scale? That said, I must mention that I like the approach of slog-config, where an arbitrary number of drain factories can be registered and used to process the configuration. Maybe sloggers could benefit from this approach sometime in the future as well...
@sbant
Copy link

sbant commented Jan 11, 2018

Another question,

Both file_logger and terminal_logger contains one Async, thus opened two threads.

Can we avoid that, something like only create one root Async(thread) for all child Drains.

@dpc
Copy link
Contributor

dpc commented Jan 11, 2018

Part of the reason why core slog is a bunch of modular crates is that there is an infinite number of ways to glue together non-trivial logging setups. That's also why there is a dozen of similar, but slightly different logging libraries based on log - each of them having a different subset of everything else, wired slightly different. There is just no "one, best/correct way".

I am the author of slog, not sloggers. I consider sloggers a "newbie friendly", "on ramp" to slog, so that new users don't have to spent too much time just getting the benefits of slog. But when I see proposals to add more complex setup to sloggers, I'm afraid it is losing its main appeal.

If you require non-trivial logging setup requirements for your apps, maybe you should just revert back to slog, and compose such setup "manually", driving it the exact way your app needs.

Note that you can still use sloggers. It returns Logger: https://docs.rs/sloggers/0.2.6/sloggers/terminal/struct.TerminalLoggerBuilder.html#impl-Build , and slog::Logger implements slog::Drain so you can further compose it with other Drains if you need.

Anyway, it's just my quick opinion, nothing that I feel strong about. Also the exact line, where there is "too much complexity" so new users get confused would be entirely for the authors of sloggers to draw.

@sile
Copy link
Owner

sile commented Jan 12, 2018

I consider sloggers a "newbie friendly", "on ramp" to slog, so that new users don't have to spent too much time just getting the benefits of slog. But when I see proposals to add more complex setup to sloggers, I'm afraid it is losing its main appeal.
If you require non-trivial logging setup requirements for your apps, maybe you should just revert back to slog, and compose such setup "manually", driving it the exact way your app needs.

I agree with this opinion.
I think that this feature is worthwhile, but if it introduces complexity, it may be better to forgo supporting it directly in sloggers (until a good idea comes up).

FYI, it is possible to construct combined loggers by a scalable (or generic) way as follows:

 extern crate serde;
 #[macro_use]
 extern crate serde_derive;
 #[macro_use]
 extern crate slog;
 extern crate sloggers;
 extern crate toml;

 use slog::{Drain, Duplicate, Logger};
 use sloggers::{Config, LoggerConfig};

 #[derive(Deserialize)]
 struct CombinedLoggerConfig {
     loggers: Vec<LoggerConfig>,
 }

 fn main() {
     let conf = r#"
 [[loggers]]
 type = "terminal"
 format = "compact"
 source_location = "none"
 timezone = "local"
 destination = "stdout"
 level = "info"

 [[loggers]]
 type = "file"
 format = "full"
 source_location = "none"
 timezone = "local"
 level = "info"
 path = "railprofiles.log"
 truncate = false
 "#;

     let combined_logger_config: CombinedLoggerConfig = toml::from_str(conf).unwrap();
     let mut combined_logger = None;
     for logger_config in combined_logger_config.loggers {
         let logger = logger_config.build_logger().unwrap();
         if let Some(pred) = combined_logger.take() {
             combined_logger = Some(Logger::root(Duplicate::new(logger, pred).fuse(), o!()));
         } else {
             combined_logger = Some(logger);
         }
     }

     let combined_logger = combined_logger.take().unwrap();
     info!(combined_logger, "Hello World!");
 }

@sile
Copy link
Owner

sile commented Jan 12, 2018

Another question,
Both file_logger and terminal_logger contains one Async, thus opened two threads.
Can we avoid that, something like only create one root Async(thread) for all child Drains.

Using the current version of sloggers, it is impossible to achieve this.

If this is necessary, it may be a good means to add a flag to sloggers to control whether or not to use Async drain.
By disabling internal use of it, you can manually set one Async drain on top of a combined logger.

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

No branches or pull requests

4 participants