Add a log lines for datadir, conf file and options #204

anipaul2 · 2023-03-11T03:34:16Z

fixes: #202

Added the log of default data dir path and where the config file is located
Added the log for non-default options
All tests run successfully
Checked the code with cargo fmt and cargo clippy

void-ness · 2023-03-12T11:52:31Z

teos/src/main.rs

+    let cloned_conf_file_path = conf_file_path.clone();
+
+    // Log datadir path
+    log::info!("Using data directory: {:?}", &path);


As mentioned by @sr-gi in the conversation below, the data directory information is stored in path_network variable instead of the path variable. Kindly look into it.

reference: #202 (comment)

Indeed. Given the tower can be run in multiple networks using the same root dir, the data dir needs to include the network path (check in the issue how bitcoind reports this, we're trying to follow the exact same pattern).

As mentioned by @sr-gi in the conversation below, the data directory information is stored in path_network variable instead of the path variable. Kindly look into it.

reference: #202 (comment)

It's the default data directory

teos/src/main.rs

void-ness · 2023-03-12T11:59:08Z

teos/src/main.rs

    let is_default = conf.is_default();
    conf.patch_with_options(opt);
    conf.verify().unwrap_or_else(|e| {
        eprintln!("{e}");
        std::process::exit(1);
    });

+    // Log config file path and any non-default options
+    log::info!("Using configuration file: {:?}", &cloned_conf_file_path);


Missing items as per the requirements of issue #202 -

Logging the default data directory

Missing items as per the requirements of issue #202 -

Logging the default data directory

Added the default data dir now and working on the network dir that we are currently using.

sr-gi

Building on top of @void-ness review. Check comments inline.

Also, this is not logging a single new line, even though you've added some logging. This is because the logs are called before the logger is initialized, which happens here: https://github.com/talaia-labs/rust-teos/blob/master/teos/src/main.rs#L88-L104. Please do test functionality locally before submitting a PR.

teos/src/main.rs

sr-gi · 2023-03-13T11:19:10Z

teos/src/main.rs

+    let cloned_conf_file_path = conf_file_path.clone();
+
+    // Log datadir path
+    log::info!("Using data directory: {:?}", &path);


Indeed. Given the tower can be run in multiple networks using the same root dir, the data dir needs to include the network path (check in the issue how bitcoind reports this, we're trying to follow the exact same pattern).

sr-gi · 2023-03-13T11:20:14Z

teos/src/main.rs

+    let conf_file_path = path.join("teos.toml");
+
+    // Clone the conf_file_path variable before passing it to the from_file function
+    let cloned_conf_file_path = conf_file_path.clone();


Cloning this is not necessary. Instead, you can make config::from_file receive &Pathbuf instead of Pathbuf

Cloning this is not necessary. Instead, you can make config::from_file receive &Pathbuf instead of Pathbuf

I tried with &Pathbuf but it says expected struct Pathbuf, found &Pathbuf

Cloning this is not necessary. Instead, you can make config::from_file receive &Pathbuf instead of Pathbuf

I did reference it but shows error

`let mut conf = config::from_file::(&conf_file_path);
let is_default = conf.is_default();
conf.patch_with_options(opt);
conf.verify().unwrap_or_else(|e| {
eprintln!("{e}");
std::process::exit(1);
});

// log default data dir log::info!("Using default data directory: {:?}", &path); // Log config file path and any non-default options log::info!("Using configuration file: {:?}", &conf_file_path);`

Cloning this is not necessary. Instead, you can make config::from_file receive &Pathbuf instead of Pathbuf

If I log the config file path before config::from_file, it is not showing error. So, I did this first log::info!("Using configuration file: {:?}", conf_file_path); then let mut conf = config::from_file::<Config>(conf_file_path);. It is working now.

teos/src/main.rs

anipaul2 · 2023-03-13T16:13:57Z

Sorry. I'll change it to the requirements and make a pr again.

sr-gi · 2023-03-13T20:48:04Z

Sorry. I'll change it to the requirements and make a pr again.

No worries. You don't need to create a new PR btw, just append the fixes to this one.

anipaul2 · 2023-03-14T15:27:52Z

Sorry. I'll change it to the requirements and make a pr again.

No worries. You don't need to create a new PR btw, just append the fixes to this one.

Yes I'll do that only. My midsems exams are ending tomorrow so after that I'll be free. Will give my full time from tomorrow.

anipaul2 · 2023-03-19T21:21:58Z

@sr-gi please check the updated commit. I updated the requested changes and tested with cargo test and cargo fmt. Also, for non-default options, specifically for this, I referenced it with btc_network when it's not zero.

sr-gi · 2023-03-20T09:28:20Z

teos/src/main.rs

+    let conf_file_path = path.join("teos.toml");
+
+    // Log config file path
+    log::info!("Using configuration file: {:?}", conf_file_path);


This is still being logged before the logger is initialized, hence it would never be shown

This is still being logged before the logger is initialized, hence it would never be shown

Tried to log it after logger but shows error move occurs because conf_file_path has type pathbuf , which does implement the copy trait

This is still being logged before the logger is initialized, hence it would never be shown

Tried to log it after logger but shows error move occurs because conf_file_path has type pathbuf , which does implement the copy trait

So, I did logged before the config::from_file

Make it so Config:: from_file borrows a Pathbuf instead of taking ownership of it, then you'll be able to just pass a reference to conf_file_path instead of moving it.

I referenced it with let mut conf = config::from_file::<Config>(&conf_file_path); but still shows error and states consider removing the borrow.

I referenced it with let mut conf = config::from_file::<Config>(&conf_file_path); but still shows error and states consider removing the borrow.

Like this:-

`async fn main() {
let opt = Opt::from_args();
let path = config::data_dir_absolute_path(opt.data_dir.clone());
let conf_file_path = path.join("teos.toml");
// Create data dir if it does not exist
fs::create_dir_all(&path).unwrap_or_else(|e| {
eprintln!("Cannot create data dir: {e:?}");
std::process::exit(1);
});

// Load conf (from file or defaults) and patch it with the command line parameters received (if any) let mut conf = config::from_file::<Config>(&conf_file_path); let is_default = conf.is_default(); conf.patch_with_options(opt); conf.verify().unwrap_or_else(|e| { eprintln!("{e}"); std::process::exit(1); });`

I referenced it with let mut conf = config::from_file::<Config>(&conf_file_path); but still shows error and states consider removing the borrow.

Like this:-

`async fn main() { let opt = Opt::from_args(); let path = config::data_dir_absolute_path(opt.data_dir.clone()); let conf_file_path = path.join("teos.toml"); // Create data dir if it does not exist fs::create_dir_all(&path).unwrap_or_else(|e| { eprintln!("Cannot create data dir: {e:?}"); std::process::exit(1); });

// Load conf (from file or defaults) and patch it with the command line parameters received (if any) let mut conf = config::from_file::<Config>(&conf_file_path); let is_default = conf.is_default(); conf.patch_with_options(opt); conf.verify().unwrap_or_else(|e| { eprintln!("{e}"); std::process::exit(1); });`

Then after log I have added this:-

`
// Set log level
SimpleLogger::new()
.with_level(if conf.deps_debug {
LevelFilter::Debug
} else {
LevelFilter::Warn
})
.with_module_level(
"teos",
if conf.debug {
LevelFilter::Debug
} else {
LevelFilter::Info
},
)
.init()
.unwrap();

// log default data dir log::info!("Using default data directory: {:?}", &path); // Log config file path log::info!("Using configuration file: {:?}", &conf_file_path);`

I referenced it with let mut conf = config::from_file::<Config>(&conf_file_path); but still shows error and states consider removing the borrow.

That doesn't make sense. Have you updated the method signature? Just tested it and it works.

diff --git a/teos/src/cli.rs b/teos/src/cli.rs index 5c20f00..eba032b 100644 --- a/teos/src/cli.rs +++ b/teos/src/cli.rs @@ -27,7 +27,7 @@ async fn main() { let command = opt.command.clone(); // Load conf (from file or defaults) and patch it with the command line parameters received (if any) - let mut conf = config::from_file::<Config>(path.join("teos.toml")); + let mut conf = config::from_file::<Config>(&path.join("teos.toml")); conf.patch_with_options(opt); let key = fs::read(&path.join("client-key.pem")) diff --git a/teos/src/config.rs b/teos/src/config.rs index 1669b25..50fb8a3 100644 --- a/teos/src/config.rs +++ b/teos/src/config.rs @@ -16,7 +16,7 @@ pub fn data_dir_absolute_path(data_dir: String) -> PathBuf { } } -pub fn from_file<T: Default + serde::de::DeserializeOwned>(path: PathBuf) -> T { +pub fn from_file<T: Default + serde::de::DeserializeOwned>(path: &PathBuf) -> T { match std::fs::read(path) { Ok(file_content) => toml::from_slice::<T>(&file_content).map_or_else( |e| { diff --git a/teos/src/main.rs b/teos/src/main.rs index e7b8f37..1ab249e 100644 --- a/teos/src/main.rs +++ b/teos/src/main.rs @@ -71,9 +71,6 @@ async fn main() { let path = config::data_dir_absolute_path(opt.data_dir.clone()); let conf_file_path = path.join("teos.toml"); - // Log config file path - log::info!("Using configuration file: {:?}", conf_file_path); - // Create data dir if it does not exist fs::create_dir_all(&path).unwrap_or_else(|e| { eprintln!("Cannot create data dir: {e:?}"); @@ -81,7 +78,7 @@ async fn main() { }); // Load conf (from file or defaults) and patch it with the command line parameters received (if any) - let mut conf = config::from_file::<Config>(conf_file_path); + let mut conf = config::from_file::<Config>(&conf_file_path); let is_default = conf.is_default(); conf.patch_with_options(opt); conf.verify().unwrap_or_else(|e| { @@ -89,9 +86,6 @@ async fn main() { std::process::exit(1); }); - // log default data dir - log::info!("Using default data directory: {:?}", &path); - // log the non-default options that the daemon is using if !is_default { if conf.api_bind != "localhost" { @@ -165,6 +159,12 @@ async fn main() { log::info!("Loading configuration from file") } + // Log config file path + log::info!("Using configuration file: {:?}", conf_file_path); + + // log default data dir + log::info!("Using default data directory: {:?}", &path); + // Create network dir let path_network = path.join(conf.btc_network.clone()); fs::create_dir_all(&path_network).unwrap_or_else(|e| {

I referenced it with let mut conf = config::from_file::<Config>(&conf_file_path); but still shows error and states consider removing the borrow.

That doesn't make sense. Have you updated the method signature? Just tested it and it works.

diff --git a/teos/src/cli.rs b/teos/src/cli.rs index 5c20f00..eba032b 100644 --- a/teos/src/cli.rs +++ b/teos/src/cli.rs @@ -27,7 +27,7 @@ async fn main() { let command = opt.command.clone(); // Load conf (from file or defaults) and patch it with the command line parameters received (if any) - let mut conf = config::from_file::<Config>(path.join("teos.toml")); + let mut conf = config::from_file::<Config>(&path.join("teos.toml")); conf.patch_with_options(opt); let key = fs::read(&path.join("client-key.pem")) diff --git a/teos/src/config.rs b/teos/src/config.rs index 1669b25..50fb8a3 100644 --- a/teos/src/config.rs +++ b/teos/src/config.rs @@ -16,7 +16,7 @@ pub fn data_dir_absolute_path(data_dir: String) -> PathBuf { } } -pub fn from_file<T: Default + serde::de::DeserializeOwned>(path: PathBuf) -> T { +pub fn from_file<T: Default + serde::de::DeserializeOwned>(path: &PathBuf) -> T { match std::fs::read(path) { Ok(file_content) => toml::from_slice::<T>(&file_content).map_or_else( |e| { diff --git a/teos/src/main.rs b/teos/src/main.rs index e7b8f37..1ab249e 100644 --- a/teos/src/main.rs +++ b/teos/src/main.rs @@ -71,9 +71,6 @@ async fn main() { let path = config::data_dir_absolute_path(opt.data_dir.clone()); let conf_file_path = path.join("teos.toml"); - // Log config file path - log::info!("Using configuration file: {:?}", conf_file_path); - // Create data dir if it does not exist fs::create_dir_all(&path).unwrap_or_else(|e| { eprintln!("Cannot create data dir: {e:?}"); @@ -81,7 +78,7 @@ async fn main() { }); // Load conf (from file or defaults) and patch it with the command line parameters received (if any) - let mut conf = config::from_file::<Config>(conf_file_path); + let mut conf = config::from_file::<Config>(&conf_file_path); let is_default = conf.is_default(); conf.patch_with_options(opt); conf.verify().unwrap_or_else(|e| { @@ -89,9 +86,6 @@ async fn main() { std::process::exit(1); }); - // log default data dir - log::info!("Using default data directory: {:?}", &path); - // log the non-default options that the daemon is using if !is_default { if conf.api_bind != "localhost" { @@ -165,6 +159,12 @@ async fn main() { log::info!("Loading configuration from file") } + // Log config file path + log::info!("Using configuration file: {:?}", conf_file_path); + + // log default data dir + log::info!("Using default data directory: {:?}", &path); + // Create network dir let path_network = path.join(conf.btc_network.clone()); fs::create_dir_all(&path_network).unwrap_or_else(|e| {

Yes, working now. Forgot to update the borrow of cli.rs file. Thank you!

sr-gi · 2023-03-20T09:30:37Z

teos/src/main.rs

+    if !is_default {
+        if conf.api_bind != "localhost" {
+            log::info!("Using non-default API bind address: {}", conf.api_bind);
+        }
+        if conf.api_port != 9814 {
+            log::info!("Using non-default API port: {}", conf.api_port);
+        }
+        if conf.rpc_bind != "localhost" {
+            log::info!("Using non-default RPC bind address: {}", conf.rpc_bind);
+        }
+        if conf.rpc_port != 8814 {
+            log::info!("Using non-default RPC port: {}", conf.rpc_port);
+        }
+        if conf.btc_network != "mainnet" {
+            log::info!("Using non-default Bitcoin network: {}", conf.btc_network);
+        }
+        if conf.btc_rpc_user != "user" {
+            log::info!("Using non-default Bitcoin RPC user: {}", conf.btc_rpc_user);
+        }
+        if conf.btc_rpc_password != "passwd" {
+            log::info!(
+                "Using non-default Bitcoin RPC password: {}",
+                conf.btc_rpc_password
+            );
+        }
+        if conf.btc_rpc_connect != "localhost" {
+            log::info!(
+                "Using non-default Bitcoin RPC connect address: {}",
+                conf.btc_rpc_connect
+            );
+        }
+        if conf.btc_rpc_port != 0 {
+            log::info!("Using non-default Bitcoin RPC port: {}", conf.btc_network);
+        }
+        if conf.tor_control_port != 9051 {
+            println!(
+                "Using non-default Tor control port: {}",
+                conf.tor_control_port
+            );
+        }
+        if conf.onion_hidden_service_port != 9814 {
+            println!(
+                "Using non-default onion hidden service port: {}",
+                conf.onion_hidden_service_port
+            );
+        }
+    }
+


This is not the right way to handle this. We don't want to add all this boilerplate to main. Also, most of this info is already in Config. The right way to handle this will most likely involve the Default and Eq traits.

This is not the right way to handle this. We don't want to add all this boilerplate to main. Also, most of this info is already in Config. The right way to handle this will most likely involve the Default and Eq traits.

So, I have to log the non-default options and Eq traits and remove the ones who has default value?

This is not the right way to handle this. We don't want to add all this boilerplate to main. Also, most of this info is already in Config. The right way to handle this will most likely involve the Default and Eq traits.

Please check the latest commit and let me know if it's correct or not.

Please check the latest commit and let me know if it's correct or not.

if conf.btc_rpc_port != 0 { log::info!("Using non-default Bitcoin RPC port: {}", conf.btc_network); } if conf.btc_rpc_user != "user" { log::info!("Using non-default Bitcoin RPC user: {}", conf.btc_rpc_user); } if conf.btc_rpc_password != "passwd" { log::info!("Using non-default Bitcoin RPC password: {}", conf.btc_rpc_password); } if conf.btc_network != "mainnet" { log::info!("Using non-default Bitcoin network: {}", conf.btc_network); } if conf.btc_rpc_connect != "localhost" { log::info!("Using non-default Bitcoin RPC connect address: {}", conf.btc_rpc_connect); }

You are still using hard coded default values ("mainnet", "localhost", etc...) but you could create a default config struct and compare it against conf instead.

Please check the latest commit and let me know if it's correct or not.

if conf.btc_rpc_port != 0 { log::info!("Using non-default Bitcoin RPC port: {}", conf.btc_network); } if conf.btc_rpc_user != "user" { log::info!("Using non-default Bitcoin RPC user: {}", conf.btc_rpc_user); } if conf.btc_rpc_password != "passwd" { log::info!("Using non-default Bitcoin RPC password: {}", conf.btc_rpc_password); } if conf.btc_network != "mainnet" { log::info!("Using non-default Bitcoin network: {}", conf.btc_network); } if conf.btc_rpc_connect != "localhost" { log::info!("Using non-default Bitcoin RPC connect address: {}", conf.btc_rpc_connect); }

You are still using hard coded default values ("mainnet", "localhost", etc...) but you could create a default config struct and compare it against conf instead.

So, I should create a default create config struct in config.rs and match it main using conf?

So, I should create a default config struct

Yeah. But it's already there. Config already implements std::Default.

and match it main using conf?

Try to keep such checks off of main.rs so not to bloat it.

So, I should create a default config struct

Yeah. But it's already there. Config already implements std::Default

changed now. please let me know if it's ok now.

You are still bloating main with the comparisons. Just implement a function in config that deals with the diff between the given Config and the default one. It could be something along the lines of a method that returns a HashMap where keys are the config names, and values are a tuple of the expected value and the received value. Then, you can simply iterate through the map in main and log accordingly.

anipaul2 · 2023-03-25T06:42:34Z

@sr-gi please check the latest commit.

teos/src/config.rs

teos/src/main.rs

anipaul2 · 2023-03-27T13:50:27Z

@sr-gi tested my latest commit and it works fine. Please check once and let me know if I'm still missing something.

sr-gi

All commits need to be squashed.

Just a couple more changes, my bad on suggesting something that feels a bit weird ui-wise :(

teos/src/main.rs

sr-gi · 2023-03-28T15:26:39Z

teos/src/main.rs

@@ -119,6 +135,9 @@ async fn main() {
        DBM::new(path_network.join("teos_db.sql3")).unwrap(),
    ));

+    // Log datadir path


Let's move this some lines up some it is before the options, so right after log::info!("Config file: {:?}", &conf_file_path);

Let's move this some lines up some it is before the options, so right after log::info!("Config file: {:?}", &conf_file_path);

If I move this some lines up it shows error as path_network is defined below.

Define path_network first then? I don't really see the issue.

There's also the option of moving everything else down.

Define path_network first then? I don't really see the issue.

There's also the option of moving everything else down.

okay

Define path_network first then? I don't really see the issue.

There's also the option of moving everything else down.

Done. Can I squash all the commits now as there is no issue left to solve?

teos/src/main.rs

sr-gi · 2023-03-28T15:36:31Z

Something else to have in mind (I just realized about this while testing it) but that can be left for a follow-up: right now we'll be logging something along the lines of:

Custom config arg: key=value

For each arg that has been changed. We should note what made the change, so a follow-up PR should transform that log into either:

Config file arg: key=value

or

Command-line arg: key=value

For context, this is exactly how bitcoind handles it.

teos/src/config.rs

anipaul2 · 2023-03-28T17:32:21Z

Something else to have in mind (I just realized about this while testing it) but that can be left for a follow-up: right now we'll be logging something along the lines of:
Custom config arg: key=value
For each arg that has been changed. We should note what made the change, so a follow-up PR should transform that log into either:
Config file arg: key=value
or
Command-line arg: key=value
For context, this is exactly how bitcoind handles it.

Do I need to modify the get_non_default_options again and take an additional paramater to indicate the source of change (config file or command line), and then include this information in the log output?

anipaul2 · 2023-03-28T20:06:11Z

@sr-gi Can I squash all commits now?

sr-gi · 2023-03-29T10:21:31Z

@sr-gi Can I squash all commits now?

Yes, once every comment has been addressed. It should all be a single commit.

…aia-labs#202 Following up from talaia-labs#204. This commit enhances the logging output to provide clearer information about the data directory and configuration file details. By providing this information, it improves the user experience and make it easier for users to find and understand the relevant paths and settings. Suggestions added

void-ness reviewed Mar 12, 2023

View reviewed changes

sr-gi requested changes Mar 13, 2023

View reviewed changes

sr-gi requested changes Mar 20, 2023

View reviewed changes

sr-gi requested changes Mar 27, 2023

View reviewed changes

teos/src/config.rs Outdated Show resolved Hide resolved

teos/src/main.rs Outdated Show resolved Hide resolved

sr-gi requested changes Mar 28, 2023

View reviewed changes

sr-gi reviewed Mar 28, 2023

View reviewed changes

teos/src/config.rs Outdated Show resolved Hide resolved

sr-gi reviewed Mar 28, 2023

View reviewed changes

teos/src/config.rs Outdated Show resolved Hide resolved

sr-gi reviewed Mar 28, 2023

View reviewed changes

teos/src/config.rs Outdated Show resolved Hide resolved

anipaul2 closed this Mar 30, 2023

anipaul2 force-pushed the master branch from 495ca65 to 886e0ff Compare March 30, 2023 05:30

anipaul2 mentioned this pull request Mar 30, 2023

Add logging for data directory and config file paths #210

Closed

sr-gi mentioned this pull request May 23, 2023

Disallow overwrite_key and force_update in Config File #218

Open

Add a log lines for datadir, conf file and options #204

Add a log lines for datadir, conf file and options #204

Conversation

anipaul2 commented Mar 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sr-gi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anipaul2 commented Mar 13, 2023

sr-gi commented Mar 13, 2023

anipaul2 commented Mar 14, 2023

anipaul2 commented Mar 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariocynicys Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anipaul2 commented Mar 25, 2023 • edited Loading

anipaul2 commented Mar 27, 2023

sr-gi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sr-gi commented Mar 28, 2023

anipaul2 commented Mar 28, 2023 • edited Loading

anipaul2 commented Mar 28, 2023

sr-gi commented Mar 29, 2023

mariocynicys Mar 21, 2023 •

edited

Loading

anipaul2 commented Mar 25, 2023 •

edited

Loading

anipaul2 commented Mar 28, 2023 •

edited

Loading