systemd / casync

Content-Addressable Data Synchronization Tool
1.5k stars 117 forks source link

Support multiple verbosity levels, and show "Acquiring..." only for verbose > 1 #185

Closed elboulangero closed 5 years ago

elboulangero commented 5 years ago

I use the -v flag so that casync displays statistics at the end of an operation.

However, when running casync extract with a remote store, casync will display a log Acquiring ... for each chunk that is downloaded from the remote store, effectively flooding the console.

It's easy to workaround with casync -v extract ... | grep -v Acquiring.

I would like to improve that a bit. The easy way is just to remove this log.

The hard way could be to allow multiple levels of verbosity, for example like rsync (and many others do): -v for level 1, -vv for more verbose, and so on. In the code, arg_verbose would become an integer instead of a bool, and pretty much nothing else would change. Except for the line regarding the Acquiring ... log, which would go from if (arg_verbose) ... to if (arg_verbose > 1) ....

Do you like the idea, should I implement this change?

poettering commented 5 years ago

hmm, our logging framework is mostly stolen from systemd's. There we don't distuingish verbosity levels but just the usual levels syslog knows, i.e. LOG_INFO, LOG_ERR, LOG_WARN, LOG_NOTICE and LOG_DEBUG.

I'd kinda opt for the same logic here, i.e. make use of log_debug() for debug logging more often. And people can then enable debug logging by setting $CASYNC_LOG_LEVEL. (we could also add an option exposing that as cmdline arg, if needed).

i.e. I'd suggest we simply make more use log log_debug(), wherever it makes sense

elboulangero commented 5 years ago

(we could also add an option exposing that as cmdline arg, if needed)

I looked into that a bit, and in particular I looked into systemd implementation. So I could import the code from systemd "as is", which means for example importing some macros like DEFINE_STRING_TABLE_LOOKUP_WITH_FALLBACK, in order to implement log_level_from_string:

https://github.com/systemd/systemd/blob/56ee4d7001a162a7a526a10482c3007a7115539f/src/basic/string-table.h#L90

Or you might think it's overkill, and instead I could implement the function like log_level_from_string manually.

Or you might just want to avoid growing casync's code, and stick with the environment variable CASYNC_LOG_LEVEL for now.

Any preference?

poettering commented 5 years ago

a minimal version of that stuff is already included in the tree:

https://github.com/systemd/casync/blob/master/src/log.c#L13

i figure that code could be slightly reworked so that it is also available for use for the cmdline parser

elboulangero commented 5 years ago

a minimal version of that stuff is already included in the tree:

Right, I missed that, thanks! Patch submitted.