Add debug flag #465

unknowndevQwQ · 2021-11-10T08:19:49Z

utam0k · 2021-11-10T12:52:42Z

crates/youki/src/logger.rs

    let formatter = match log_format.as_deref() {
        None | Some(LOG_FORMAT_TEXT) => text_write,
        Some(LOG_FORMAT_JSON) => json_write,
        Some(unknown) => bail!("unknown log format: {}", unknown),
    };
+    let loglevel = if log_debug_flag { "debug" } else { "warn" };


Do you use loglevel anywhere? I think you need to use this flag, perhaps around line 41?

Do you use loglevel anywhere? I think you need to use this flag, perhaps around line 41?

I don't know what I should do after this, can you give me some tips?

You need to pass the log level to the logger builder. It has a filter_level method that should do the trick. You should set the log level to debug if the debug flag is specified, otherwise you need to check the youki log environment variable. If both are not set, you should fall back to the default log level.

You need to pass the log level to the logger builder. It has a filter_level method that should do the trick. You should set the log level to debug if the debug flag is specified, otherwise you need to check the youki log environment variable. If both are not set, you should fall back to the default log level.

like this? Or should I make some other changes?

Suggested change

let loglevel = if log_debug_flag { "debug" } else { "warn" };

let loglevel = if log_debug_flag {

"debug"

} else {

DEFAULT_LOG_LEVEL

};

and set a variable loglevel instead of DEFAULT_LOG_LEVEL at L41.
How about this?

and set a variable loglevel instead of DEFAULT_LOG_LEVEL at L41. How about this?

I want the flag to have a higher priority than the environment variable

yihuaf

First of all, there is a DEFAULT_LOG_LEVEL variable you should be using for default. Second, in addition to --debug flag, you will have to handle YOUKI_LOG_LEVEL from environment variable. We use env_logger, and you should read the documentation here: https://docs.rs/env_logger/0.9.0/env_logger

crates/youki/src/main.rs

crates/youki/src/logger.rs

unknowndevQwQ · 2021-11-14T16:03:34Z

@utam0k Seems to be ready to go

utam0k · 2021-11-15T12:10:16Z

@unknowndevQwQ Sorry, this pr seems to be conflicting.

unknowndevQwQ · 2021-11-15T15:58:14Z

@unknowndevQwQ Sorry, this pr seems to be conflicting.

fixed

utam0k · 2021-11-16T05:52:59Z

crates/youki/src/main.rs

+    // I don't know how to get the log level when the --debug flag is not set (I want to show some default values on the help page when the options are not set)
+    // Example: '--debug     change log level to debug. (default: "warn")'
+    /// change log level to debug.


These comments in this section are ./youki -h will be output.
When I thought about it, I felt that the first line was unnecessary.

These codes may be helpful for you.
https://github.com/containers/youki/blob/ba08bb323165f4d0b98a0321fb990725bb32de69/crates/youki/src/commands/create.rs#L10-L27

These codes may be helpful for you.

https://github.com/containers/youki/blob/ba08bb323165f4d0b98a0321fb990725bb32de69/crates/youki/src/commands/create.rs#L10-L27

The default value in some options is dynamic, for example, --root

@unknowndevQwQ
My point is that the comments in this section are not for developers, but for users. I would like you to take that into account when making your comments.

@unknowndevQwQ My point is that the comments in this section are not for developers, but for users. I would like you to take that into account when making your comments.

Should I delete L41-L42?

or like this?

utam0k · 2021-11-16T06:03:57Z

crates/youki/src/logger.rs

+    env_logger::Builder::new()
+        .parse_env(env_logger::Env::new().filter("YOUKI_LOG_LEVEL"))
+        .filter_level(FromStr::from_str(log_level).unwrap())
+        .format(formatter)
+        .target(target)
+        .init();


This doesn't give a clear idea of which level has the highest priority.
So it is better to decide which log level you want in lines 33-37 and just set the value in the builder and do not overwrite it.

This thing is starting to get tricky for me

What areas became tricky?

Get the value from the environment variable and convert it to the appropriate type

Get the value from the environment variable and convert it to the appropriate type

It didn't seem like it would be a very complex code, but what are your concerns?

Doesn't this code overwrite the default level even if YOUKI_LOG_LEVEL is set?

Get the value from the environment variable and convert it to the appropriate type

It didn't seem like it would be a very complex code, but what are your concerns?

Get the value from the environment variable and convert it to &str, output a friendly error if it fails

let log_level = if log_debug_flag { "debug" } else { match env::var("YOUKI_LOG_LEVEL") { Ok(val) => Err(_e) => DEFAULT_LOG_LEVEL } };

for youki-dev#454

I didn't find a proper error handling in L39 In L51, `bail!()` does not exit after the output, but continues to execute, and I can't do anything about it

codecov-commenter · 2021-11-16T14:30:11Z

Codecov Report

Merging #465 (5725f52) into main (ba08bb3) will not change coverage.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##             main     #465   +/-   ##
=======================================
  Coverage   60.44%   60.44%           
=======================================
  Files          82       82           
  Lines       12150    12150           
=======================================
  Hits         7344     7344           
  Misses       4806     4806

Furisto · 2021-11-16T22:27:30Z

crates/youki/src/logger.rs

+    let log_level_from_env = match env::var("YOUKI_LOG_LEVEL") {
+        Ok(val) => val,
+        Err(_) => String::from("i don't know"),
+    };
+    let log_level = if log_debug_flag {
+        "debug"
+    } else if log_level_from_env != "i don't know" {
+        &log_level_from_env
+    } else {
+        DEFAULT_LOG_LEVEL
+    };
+    let log_level = match LevelFilter::from_str(log_level) {
+        Ok(val) => val,
+        Err(err) => {
+            bail!("error: '{}', val from env: '{}'", err, log_level);
+            exit(1)
+        },
+    };


How about this?

Suggested change

let log_level_from_env = match env::var("YOUKI_LOG_LEVEL") {

Ok(val) => val,

Err(_) => String::from("i don't know"),

};

let log_level = if log_debug_flag {

"debug"

} else if log_level_from_env != "i don't know" {

&log_level_from_env

} else {

DEFAULT_LOG_LEVEL

};

let log_level = match LevelFilter::from_str(log_level) {

Ok(val) => val,

Err(err) => {

bail!("error: '{}', val from env: '{}'", err, log_level);

exit(1)

},

};

let filter: Cow<str> = if debug_flag {

"debug".into()

} else if let Ok(level) = std::env::var("YOUKI_LOG_LEVEL") {

level.into()

} else {

DEFAULT_LOG_LEVEL.into()

};

env_logger::Builder::new()

.filter_level(LevelFilter::from_str(filter.as_ref()).context("failed to parse log level")?)

.format(formatter)

.target(target)

.init();

YOUKI_LOG_LEVEL=o sudo -E ./youki/target/x86_64-unknown-linux-gnu/release/youki state test

log init failed: failed to parse log level Caused by: attempted to convert a string that doesn't match an existing log level { "ociVersion": "v1.0.2", "id": "test", "status": "stopped", "pid": 114514, "bundle": "test", "annotations": {}, "created": "2021-11-11T11:11:11.111111111Z", "creator": 0, "useSystemd": false }

Is it normal to continue doing other things after a loglevel failure?

Furisto · 2021-11-17T19:33:00Z

Yes, a faulty log level should not prevent execution of the command.

Already addressed

Furisto · 2021-11-17T19:35:08Z

lgtm

unknowndevQwQ · 2021-11-18T02:09:43Z

lgtm

Can we compress the redundant commits?

utam0k · 2021-11-18T02:17:46Z

lgtm

Can we compress the redundant commits?

I wish We could have, but I don't have to be so obsessed.

unknowndevQwQ · 2021-11-18T02:23:39Z

lgtm

Can we compress the redundant commits?

I wish We could have, but I don't have to be so obsessed.

Then let nature take its course.

utam0k reviewed Nov 10, 2021

View reviewed changes

yihuaf previously requested changes Nov 11, 2021

View reviewed changes

crates/youki/src/main.rs Show resolved Hide resolved

unknowndevQwQ commented Nov 12, 2021

View reviewed changes

crates/youki/src/logger.rs Outdated Show resolved Hide resolved

unknowndevQwQ mentioned this pull request Nov 13, 2021

style: adjusting the position of parameters and flag declarations #474

Merged

Furisto marked this pull request as ready for review November 15, 2021 08:11

unknowndevQwQ force-pushed the add_debug_flag branch from 41f4739 to 059a8e9 Compare November 15, 2021 08:46

unknowndevQwQ force-pushed the add_debug_flag branch from 059a8e9 to e3b9062 Compare November 15, 2021 15:53

unknowndevQwQ requested a review from yihuaf November 15, 2021 15:59

unknowndevQwQ force-pushed the add_debug_flag branch from e3b9062 to f908d88 Compare November 16, 2021 05:34

utam0k reviewed Nov 16, 2021

View reviewed changes

unknowndevQwQ force-pushed the add_debug_flag branch from f908d88 to c50d8c7 Compare November 16, 2021 06:02

utam0k reviewed Nov 16, 2021

View reviewed changes

feat: add debug flag

93bbaba

for youki-dev#454

unknowndevQwQ force-pushed the add_debug_flag branch from c50d8c7 to 93bbaba Compare November 16, 2021 12:36

why?

475a2bf

I didn't find a proper error handling in L39 In L51, `bail!()` does not exit after the output, but continues to execute, and I can't do anything about it

Furisto reviewed Nov 16, 2021

View reviewed changes

the end?

5725f52

Furisto approved these changes Nov 17, 2021

View reviewed changes

Furisto merged commit acf6e31 into youki-dev:main Nov 17, 2021

unknowndevQwQ deleted the add_debug_flag branch November 18, 2021 02:24

Add debug flag #465

Add debug flag #465

Conversation

unknowndevQwQ commented Nov 10, 2021

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

unknowndevQwQ Nov 12, 2021 • edited Loading

Choose a reason for hiding this comment

yihuaf left a comment

Choose a reason for hiding this comment

unknowndevQwQ commented Nov 14, 2021

utam0k commented Nov 15, 2021

unknowndevQwQ commented Nov 15, 2021

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

unknowndevQwQ Nov 16, 2021 • edited Loading

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

codecov-commenter commented Nov 16, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Furisto commented Nov 17, 2021

Furisto commented Nov 17, 2021

unknowndevQwQ commented Nov 18, 2021

utam0k commented Nov 18, 2021

unknowndevQwQ commented Nov 18, 2021

unknowndevQwQ Nov 12, 2021 •

edited

Loading

unknowndevQwQ Nov 16, 2021 •

edited

Loading

codecov-commenter commented Nov 16, 2021 •

edited

Loading