-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add logging in client package to log usage of QuoteProvider or Device(IOCTLs) to fetch raw attestation quote. #35
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logging should be off by default. You can introduce a verbose flag which can control the logging e.g. 0 (default): No logs, 1: Only warnings 2: Warnings and info
@@ -130,8 +130,9 @@ func parseBytes(name string, in io.Reader, inform string, byteSize int) ([]byte, | |||
} | |||
} | |||
func main() { | |||
logger.Init("", *verbose, false, os.Stderr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect location to init logger since verbose flag is never parsed so this was always ignored.
flag.Parse() | ||
logger.Init("", *verbose, false, os.Stderr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
verbose argument is used to write info and warning logs to the log file i.e. STDERR in this case. It uses default logger created using Init call.
Whereas verbosity level defines the detail level upto which we want logs. It is as per the value assigned while calling INFO, WARN, ERROR etc. logs.
quiet = flag.Bool("quiet", false, "If true, writes nothing the stdout or stderr. Success is exit code 0, failure exit code 1.") | ||
verbose = flag.Int("verbosity", 0, "The output verbosity. Higher number means more verbose output") | ||
quiet = flag.Bool("quiet", false, "If true, writes nothing the stdout or stderr. Success is exit code 0, failure exit code 1.") | ||
verbosity = flag.Int("verbosity", 0, "The output verbosity. Higher number means more verbose output") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct argument name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the readme. I see in attest we have verbose and verbosity whereas here we have quiet and verbosity. Just for consistency it might be better to add verbose over here also. It can be used to set the quiet variable so rest of the logic will be governed by a single variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verbose and quiet have different utilities. One is to stop output of even FATAL logs to stdout or stderr, whereas verbose is used to include INFO and WARN logs to stdout. I think we should not have a single variable to govern both.
tools/attest/README.md
Outdated
|
||
### `verbose` | ||
|
||
If set, then logger appends INFO and WARNING logs to Stdout. Default logger has verbosity set to `0`, so verbosity option should be set to appropriate value to append INFO and WARN logs at variable verbosity levels to Stdout. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you replace the first sentence with below:
If set, then the logger can append INFO and WARNING logs to stdout as per the verbosity level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
|
||
Used to set the verbosity of logger, where higher number means more verbose output. | ||
|
||
Default value is `0`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you describe the meaning of each verbosity level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verbosity level is not associated with INFO, WARN, ERROR, FATAL type but instead to the amount of detailed logs we want. E.g.
logger.V(0).Info("detail 1")
logger.V(1).Info("detail 2")
logger.V(2).Info("detail 3")
Verbosity level 1 would show details 1 & 2; level 2 would show details 1, 2 & 3.
Added logging in client package to log usage of QuoteProvider or Device(IOCTLs) when fetching raw attestation quote. Verbosity argument added to attest CLI to set logging level.