From 192b719b1e9366510b8415310ed7fe63c86f4388 Mon Sep 17 00:00:00 2001 From: Lilit0x Date: Fri, 1 Dec 2023 12:00:38 +0100 Subject: [PATCH 1/4] feat: added --logs subcmd to local command --- .gitignore | 3 +++ crates/glaredb/src/args/local.rs | 4 ++++ crates/glaredb/src/bin/main.rs | 6 ++++-- crates/logutil/src/lib.rs | 17 ++++++++++++++--- crates/testing/src/slt/cli.rs | 2 +- 5 files changed, 26 insertions(+), 6 deletions(-) diff --git a/.gitignore b/.gitignore index 4f2e978b3..f651b8fcf 100644 --- a/.gitignore +++ b/.gitignore @@ -28,3 +28,6 @@ node_modules # Benchmark data bench_data/ + +# Logs +*.log \ No newline at end of file diff --git a/crates/glaredb/src/args/local.rs b/crates/glaredb/src/args/local.rs index dfd70972c..d5a83ff24 100644 --- a/crates/glaredb/src/args/local.rs +++ b/crates/glaredb/src/args/local.rs @@ -14,6 +14,10 @@ pub struct LocalArgs { /// Execute an SQL file. pub file: Option, + + /// File for logs to be written to, defaults to debug.log + #[clap(long, value_parser, default_value = "debug.log")] + pub logs: PathBuf } #[derive(Debug, Clone, Parser)] diff --git a/crates/glaredb/src/bin/main.rs b/crates/glaredb/src/bin/main.rs index 33450c1be..8c72f627d 100644 --- a/crates/glaredb/src/bin/main.rs +++ b/crates/glaredb/src/bin/main.rs @@ -54,8 +54,10 @@ fn main() -> Result<()> { // Disable logging when running locally since it'll clobber the repl // _unless_ the user specified a logging related option. match (&command, cli.log_mode, cli.verbose) { - (Commands::Local { .. }, None, 0) => (), - _ => logutil::init(cli.verbose, cli.log_mode.unwrap_or_default().into()), + (Commands::Local(args), None, 0) => { + logutil::init(1, LoggingMode::Full.into(), Some(&args.logs)) + } + _ => logutil::init(cli.verbose, cli.log_mode.unwrap_or_default().into(), None), } command.run() diff --git a/crates/logutil/src/lib.rs b/crates/logutil/src/lib.rs index 8a3c9cd89..c97bb09cf 100644 --- a/crates/logutil/src/lib.rs +++ b/crates/logutil/src/lib.rs @@ -1,4 +1,6 @@ //! Utilities for logging and tracing. +use std::path::PathBuf; + use tracing::{subscriber, trace, Level}; use tracing_subscriber::{ filter::EnvFilter, @@ -71,7 +73,7 @@ pub fn init_test() { /// Initialize a trace subsriber printing to the console using the given /// verbosity count. -pub fn init(verbosity: impl Into, mode: LoggingMode) { +pub fn init(verbosity: impl Into, mode: LoggingMode, log_file: Option<&PathBuf>) { let verbosity: Verbosity = verbosity.into(); let level: Level = verbosity.into(); @@ -84,8 +86,17 @@ pub fn init(verbosity: impl Into, mode: LoggingMode) { subscriber::set_global_default(subscriber) } LoggingMode::Full => { - let subscriber = full_fmt(level).with_env_filter(env_filter).finish(); - subscriber::set_global_default(subscriber) + let subscriber = full_fmt(level).with_env_filter(env_filter); + + if let Some(file) = log_file { + let debug_log = { + let file = std::fs::File::create(file).expect("Failed to create log file"); + std::sync::Arc::new(file) + }; + subscriber::set_global_default(subscriber.with_writer(debug_log).finish()) + } else { + subscriber::set_global_default(subscriber.finish()) + } } LoggingMode::Compact => { let subscriber = compact_fmt(level).with_env_filter(env_filter).finish(); diff --git a/crates/testing/src/slt/cli.rs b/crates/testing/src/slt/cli.rs index 6e27b35fc..dc2761da4 100644 --- a/crates/testing/src/slt/cli.rs +++ b/crates/testing/src/slt/cli.rs @@ -107,7 +107,7 @@ impl Cli { Verbosity::Debug => LoggingMode::Full, Verbosity::Trace => LoggingMode::Full, }; - logutil::init(cli.verbose, log_mode); + logutil::init(cli.verbose, log_mode, None); // Abort the program on panic. This will ensure that slt tests will // never pass if there's a panic somewhere. From e765e9c4121a4f25337f12d488bcac732d890efc Mon Sep 17 00:00:00 2001 From: Lilit0x Date: Fri, 1 Dec 2023 12:49:20 +0100 Subject: [PATCH 2/4] test: added tests for log file --- crates/glaredb/src/args/local.rs | 2 +- crates/glaredb/tests/log_file_test.rs | 33 +++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) create mode 100644 crates/glaredb/tests/log_file_test.rs diff --git a/crates/glaredb/src/args/local.rs b/crates/glaredb/src/args/local.rs index d5a83ff24..ebe8a62bb 100644 --- a/crates/glaredb/src/args/local.rs +++ b/crates/glaredb/src/args/local.rs @@ -17,7 +17,7 @@ pub struct LocalArgs { /// File for logs to be written to, defaults to debug.log #[clap(long, value_parser, default_value = "debug.log")] - pub logs: PathBuf + pub logs: PathBuf, } #[derive(Debug, Clone, Parser)] diff --git a/crates/glaredb/tests/log_file_test.rs b/crates/glaredb/tests/log_file_test.rs new file mode 100644 index 000000000..a0fbecaab --- /dev/null +++ b/crates/glaredb/tests/log_file_test.rs @@ -0,0 +1,33 @@ +mod setup; + +use crate::setup::{make_cli, DEFAULT_TIMEOUT}; + +#[test] +fn test_default_log_file() { + let mut cmd = make_cli(); + cmd.timeout(DEFAULT_TIMEOUT) + .arg("-q") + .arg("select 1;") + .assert() + .success(); + let mut log_file = std::env::current_dir().expect("failed to retrieve current dir"); + log_file.push("debug.log"); + assert!(log_file.exists()); +} + +#[test] +fn test_named_log_file() { + let mut cmd = make_cli(); + let log_file_name = "test.log"; + cmd.timeout(DEFAULT_TIMEOUT) + .arg("-q") + .arg("select 1;") + .arg("--logs") + .arg(log_file_name) + .assert() + .success(); + + let mut log_file = std::env::current_dir().expect("failed to retrieve current dir"); + log_file.push(log_file_name); + assert!(log_file.exists()); +} From 1200263b0ac5b7a4b43565b06766190ce9e962d5 Mon Sep 17 00:00:00 2001 From: Lilit0x Date: Fri, 1 Dec 2023 18:56:52 +0100 Subject: [PATCH 3/4] fix: removed default logging --- crates/glaredb/src/args/local.rs | 6 +++--- crates/glaredb/src/bin/main.rs | 4 +++- crates/glaredb/tests/log_file_test.rs | 15 +-------------- crates/logutil/src/lib.rs | 6 +++--- 4 files changed, 10 insertions(+), 21 deletions(-) diff --git a/crates/glaredb/src/args/local.rs b/crates/glaredb/src/args/local.rs index ebe8a62bb..00d882636 100644 --- a/crates/glaredb/src/args/local.rs +++ b/crates/glaredb/src/args/local.rs @@ -15,9 +15,9 @@ pub struct LocalArgs { /// Execute an SQL file. pub file: Option, - /// File for logs to be written to, defaults to debug.log - #[clap(long, value_parser, default_value = "debug.log")] - pub logs: PathBuf, + /// File for logs to be written to + #[clap(long, value_parser)] + pub log_file: Option, } #[derive(Debug, Clone, Parser)] diff --git a/crates/glaredb/src/bin/main.rs b/crates/glaredb/src/bin/main.rs index 8c72f627d..26c6b5dc8 100644 --- a/crates/glaredb/src/bin/main.rs +++ b/crates/glaredb/src/bin/main.rs @@ -55,7 +55,9 @@ fn main() -> Result<()> { // _unless_ the user specified a logging related option. match (&command, cli.log_mode, cli.verbose) { (Commands::Local(args), None, 0) => { - logutil::init(1, LoggingMode::Full.into(), Some(&args.logs)) + if let Some(file) = &args.log_file { + logutil::init(1, LoggingMode::Full.into(), Some(file)) + } } _ => logutil::init(cli.verbose, cli.log_mode.unwrap_or_default().into(), None), } diff --git a/crates/glaredb/tests/log_file_test.rs b/crates/glaredb/tests/log_file_test.rs index a0fbecaab..ed0bad9d0 100644 --- a/crates/glaredb/tests/log_file_test.rs +++ b/crates/glaredb/tests/log_file_test.rs @@ -2,19 +2,6 @@ mod setup; use crate::setup::{make_cli, DEFAULT_TIMEOUT}; -#[test] -fn test_default_log_file() { - let mut cmd = make_cli(); - cmd.timeout(DEFAULT_TIMEOUT) - .arg("-q") - .arg("select 1;") - .assert() - .success(); - let mut log_file = std::env::current_dir().expect("failed to retrieve current dir"); - log_file.push("debug.log"); - assert!(log_file.exists()); -} - #[test] fn test_named_log_file() { let mut cmd = make_cli(); @@ -22,7 +9,7 @@ fn test_named_log_file() { cmd.timeout(DEFAULT_TIMEOUT) .arg("-q") .arg("select 1;") - .arg("--logs") + .arg("--log-file") .arg(log_file_name) .assert() .success(); diff --git a/crates/logutil/src/lib.rs b/crates/logutil/src/lib.rs index c97bb09cf..1711568b3 100644 --- a/crates/logutil/src/lib.rs +++ b/crates/logutil/src/lib.rs @@ -1,5 +1,5 @@ //! Utilities for logging and tracing. -use std::path::PathBuf; +use std::{fs::File, path::PathBuf, sync::Arc}; use tracing::{subscriber, trace, Level}; use tracing_subscriber::{ @@ -90,8 +90,8 @@ pub fn init(verbosity: impl Into, mode: LoggingMode, log_file: Option if let Some(file) = log_file { let debug_log = { - let file = std::fs::File::create(file).expect("Failed to create log file"); - std::sync::Arc::new(file) + let file = File::create(file).expect("Failed to create log file"); + Arc::new(file) }; subscriber::set_global_default(subscriber.with_writer(debug_log).finish()) } else { From f26a18e9baa56720a1b06fdd00f582c68095ebc2 Mon Sep 17 00:00:00 2001 From: Lilit0x Date: Wed, 6 Dec 2023 20:46:52 +0100 Subject: [PATCH 4/4] chore: removed logging restrictions on local --- crates/glaredb/src/bin/main.rs | 12 +++-- crates/glaredb/tests/log_file_test.rs | 69 ++++++++++++++++++++++++++- crates/logutil/src/lib.rs | 43 ++++++++++++++--- 3 files changed, 111 insertions(+), 13 deletions(-) diff --git a/crates/glaredb/src/bin/main.rs b/crates/glaredb/src/bin/main.rs index 26c6b5dc8..388d031e3 100644 --- a/crates/glaredb/src/bin/main.rs +++ b/crates/glaredb/src/bin/main.rs @@ -51,12 +51,14 @@ fn main() -> Result<()> { None => Commands::Local(cli.local_args), }; - // Disable logging when running locally since it'll clobber the repl - // _unless_ the user specified a logging related option. - match (&command, cli.log_mode, cli.verbose) { - (Commands::Local(args), None, 0) => { + match &command { + Commands::Local(args) => { if let Some(file) = &args.log_file { - logutil::init(1, LoggingMode::Full.into(), Some(file)) + logutil::init( + cli.verbose, + cli.log_mode.unwrap_or_default().into(), + Some(file), + ) } } _ => logutil::init(cli.verbose, cli.log_mode.unwrap_or_default().into(), None), diff --git a/crates/glaredb/tests/log_file_test.rs b/crates/glaredb/tests/log_file_test.rs index ed0bad9d0..d54a510c5 100644 --- a/crates/glaredb/tests/log_file_test.rs +++ b/crates/glaredb/tests/log_file_test.rs @@ -1,12 +1,50 @@ mod setup; +use std::{ + fs::{remove_file, OpenOptions}, + io::Read, +}; + use crate::setup::{make_cli, DEFAULT_TIMEOUT}; #[test] -fn test_named_log_file() { +fn test_log_file() -> Result<(), Box> { + let mut cmd = make_cli(); + let log_file_name = "test.log"; + cmd.timeout(DEFAULT_TIMEOUT) + .arg("-q") + .arg("select 1;") + .arg("--log-file") + .arg(log_file_name) + .assert() + .success(); + + let mut log_file = std::env::current_dir().expect("failed to retrieve current dir"); + log_file.push(log_file_name); + assert!(log_file.exists()); + + let mut file = OpenOptions::new().read(true).open(&log_file)?; + assert!(file.metadata()?.len() > 0); + + let mut content = String::new(); + file.read_to_string(&mut content)?; + + assert!(content.contains("INFO")); + assert!(!content.contains("DEBUG")); + assert!(!content.contains("TRACE")); + + assert!(content.contains("Starting in-memory metastore")); + + remove_file(log_file)?; + Ok(()) +} + +#[test] +fn test_log_file_verbosity_level() -> Result<(), Box> { let mut cmd = make_cli(); let log_file_name = "test.log"; cmd.timeout(DEFAULT_TIMEOUT) + .arg("-v") .arg("-q") .arg("select 1;") .arg("--log-file") @@ -17,4 +55,33 @@ fn test_named_log_file() { let mut log_file = std::env::current_dir().expect("failed to retrieve current dir"); log_file.push(log_file_name); assert!(log_file.exists()); + + let mut file = OpenOptions::new().read(true).open(&log_file)?; + assert!(file.metadata()?.len() > 0); + + let mut content = String::new(); + file.read_to_string(&mut content)?; + + assert!(content.contains("DEBUG")); + assert!(!content.contains("TRACE")); + + remove_file(log_file)?; + Ok(()) +} + +#[test] +fn test_skipping_logs() { + let mut cmd = make_cli(); + let log_file_name = "/usr/bin/debug.log"; + cmd.timeout(DEFAULT_TIMEOUT) + .arg("-q") + .arg("select 1;") + .arg("--log-file") + .arg(log_file_name) + .assert() + .success(); + + let mut log_file = std::env::current_dir().expect("failed to retrieve current dir"); + log_file.push(log_file_name); + assert!(!log_file.exists()); } diff --git a/crates/logutil/src/lib.rs b/crates/logutil/src/lib.rs index 1711568b3..472af673d 100644 --- a/crates/logutil/src/lib.rs +++ b/crates/logutil/src/lib.rs @@ -82,25 +82,54 @@ pub fn init(verbosity: impl Into, mode: LoggingMode, log_file: Option let env_filter = env_filter(level); match mode { LoggingMode::Json => { - let subscriber = json_fmt(level).with_env_filter(env_filter).finish(); - subscriber::set_global_default(subscriber) + let subscriber = json_fmt(level).with_env_filter(env_filter); + + if let Some(file) = log_file { + let debug_log = match File::create(file) { + Ok(file) => Arc::new(file), + Err(_) => { + eprintln!("Failed to create file: {:#?}", file); + return subscriber::set_global_default(subscriber.finish()).unwrap(); + } + }; + + subscriber::set_global_default(subscriber.with_writer(debug_log).finish()) + } else { + subscriber::set_global_default(subscriber.finish()) + } } LoggingMode::Full => { let subscriber = full_fmt(level).with_env_filter(env_filter); if let Some(file) = log_file { - let debug_log = { - let file = File::create(file).expect("Failed to create log file"); - Arc::new(file) + let debug_log = match File::create(file) { + Ok(file) => Arc::new(file), + Err(_) => { + eprintln!("Failed to create file: {:#?}", file); + return subscriber::set_global_default(subscriber.finish()).unwrap(); + } }; + subscriber::set_global_default(subscriber.with_writer(debug_log).finish()) } else { subscriber::set_global_default(subscriber.finish()) } } LoggingMode::Compact => { - let subscriber = compact_fmt(level).with_env_filter(env_filter).finish(); - subscriber::set_global_default(subscriber) + let subscriber = compact_fmt(level).with_env_filter(env_filter); + if let Some(file) = log_file { + let debug_log = match File::create(file) { + Ok(file) => Arc::new(file), + Err(_) => { + eprintln!("Failed to create file: {:#?}", file); + return subscriber::set_global_default(subscriber.finish()).unwrap(); + } + }; + + subscriber::set_global_default(subscriber.with_writer(debug_log).finish()) + } else { + subscriber::set_global_default(subscriber.finish()) + } } } .unwrap();