By default only brief logging output at verbosity 1 #283

palto42 · 2020-01-18T14:13:38Z

palto42 commented

2020-01-18 14:13:38 +00:00

As discussed in PR #272 I changed the approach for more brief output at v1 level with "file list" and "stats" disabled by default.
The stats can be enabled with existing --stats option and for the file listing a new option --filesis added.

Related tickets:

#277
#282

As discussed in PR #272 I changed the approach for more brief output at v1 level with "file list" and "stats" disabled by default. The stats can be enabled with existing `--stats` option and for the file listing a new option `--files`is added. Related tickets: * #277 * #282

palto42 referenced this pull request

2020-01-18 14:13:53 +00:00

New command option to log information about executed task at verbosity 0 #272

witten reviewed

2020-01-21 18:13:42 +00:00

witten left a comment

Lookin' good! Thanks for the PR.

borgmatic/borg/create.py

					
				@ -181,0 +181,4 @@

				            if logger.isEnabledFor(logging.INFO)

				            and not json

				            and not progress

				            and (files or logger.isEnabledFor(logging.DEBUG))

Potential simplification: You can probably remove the if logger.isEnabledFor(logging.INFO) check, given that Borg's --list --filter works at verbosity 0, and it's now opt-in. Example revision:

if (files or logger.isEnabledFor(logging.DEBUG))
and not json
and not progress

Potential simplification: You can probably remove the `if logger.isEnabledFor(logging.INFO)` check, given that Borg's `--list --filter` works at verbosity 0, and it's now opt-in. Example revision: ``` if (files or logger.isEnabledFor(logging.DEBUG)) and not json and not progress ```

Agree, will change as suggested.

borgmatic/borg/create.py

					
				@ -208,3 +212,3 @@

				    if json:

				        output_log_level = None

				    elif stats:

				    elif stats and logger.getEffectiveLevel() == logging.WARNING:

To avoid that the --stats option always changes the borg output to WARNING level, I also modified the related logic:

Good call!

Based on this, I wonder though: Maybe the --stats option shouldn't influence the log level at all! For instance, --stats works just fine at Borg's equivalent of verbosity 0.

> To avoid that the --stats option always changes the borg output to WARNING level, I also modified the related logic: Good call! Based on this, I wonder though: Maybe the `--stats` option shouldn't influence the log level at all! For instance, `--stats` works just fine at Borg's equivalent of verbosity 0.

If removed, then no stats would be should at v0 since this only logs WARNING and all borg output would be logged as INFO.
This is fine for me since the used can now use v1 without the file details instead.

If removed, then no stats would be should at v0 since this only logs WARNING and all borg output would be logged as INFO. This is fine for me since the used can now use v1 without the file details instead.

You are of course correct. Carry on!

FYI I think I'm going to put this line back. Having tried --stats at -v 0, it's potentially confusing to the user why their requested stats aren't showing up.

FYI I think I'm going to put this line back. Having tried `--stats` at `-v 0`, it's potentially confusing to the user why their requested stats aren't showing up.

borgmatic/borg/prune.py

					
				@ -61,1 +61,3 @@

				        + (('--info', '--list') if logger.getEffectiveLevel() == logging.INFO else ())

				        + (

				            ('--stats',)

				            if not dry_run and logger.getEffectiveLevel() == logging.DEBUG or stats

Would be good to parenthesize the logger.getEffectiveLevel() == logging.DEBUG or stats part for clarity.

Would be good to parenthesize the `logger.getEffectiveLevel() == logging.DEBUG or stats` part for clarity.

borgmatic/borg/prune.py

					
				@ -62,0 +64,4 @@

				            else ()

				        )

				        + (('--info',) if logger.getEffectiveLevel() == logging.INFO else ())

				        + (('--list',) if logger.getEffectiveLevel() == logging.INFO and files else ())

Shouldn't this be an or rather than an and, so --files works at multiple log levels? And for prune, is the idea that the file listing should still be enabled at INFO, rather than demoting it to DEBUG like with create?

Shouldn't this be an `or` rather than an `and`, so `--files` works at multiple log levels? And for `prune`, is the idea that the file listing should still be enabled at `INFO`, rather than demoting it to `DEBUG` like with `create`?

My intention was to only show the file listing at INFO and not at WARNING level. This is related to the old influence of the --stats option to change the logging level of the borg output. Since now --statshas no influence on the borg output level anymore, I could simplify the condition to if files.

To make the code a bit more consistent I moved the --listout of extra DEBUG line similar as in create.py:

    + (('--list',) if files or logger.isEnabledFor(logging.DEBUG) else ())
    + (('--debug', '--show-rc') if logger.isEnabledFor(logging.DEBUG) else ())

My intention was to only show the file listing at INFO and not at WARNING level. This is related to the old influence of the `--stats` option to change the logging level of the borg output. Since now `--stats`has no influence on the borg output level anymore, I could simplify the condition to `if files`. To make the code a bit more consistent I moved the `--list`out of extra DEBUG line similar as in `create.py`: ``` + (('--list',) if files or logger.isEnabledFor(logging.DEBUG) else ()) + (('--debug', '--show-rc') if logger.isEnabledFor(logging.DEBUG) else ()) ```

Looks good, thanks!

borgmatic/borg/prune.py

					
				@ -71,1 +76,3 @@

				        output_log_level=logging.WARNING if stats else logging.INFO,

				        output_log_level=logging.WARNING

				        if (stats and logger.getEffectiveLevel() == logging.WARNING)

				        else logging.INFO,

If you decide that --stats shouldn't influence the log level (see comment above for create), then this could be changed as well.

If you decide that `--stats` shouldn't influence the log level (see comment above for `create`), then this could be changed as well.

Decided to remove the influence on log level.

I saw that in create.py there is a condition based on json which is not in prune.py:

if json:
    output_log_level = None
else:
    output_log_level = logging.INFO

Is this correct?

Decided to remove the influence on log level. I saw that in `create.py` there is a condition based on `json` which is not in `prune.py`: ``` python if json: output_log_level = None else: output_log_level = logging.INFO ``` Is this correct?

Just recognised that prune.pyhas no attribute json, so all good.

Just recognised that `prune.py`has no attribute `json`, so all good.

setup.py Outdated

					
				@ -1,6 +1,6 @@

				from setuptools import find_packages, setup

				VERSION = '1.4.22.dev0'

				VERSION = '1.4.22.dev0-files'

If/when you merge this, it'd be good to remove the branch version tag.

palto42 commented

2020-01-22 19:07:32 +00:00

I updated the code as per comments above and also aligned the impacted test cases for removed influence of --stats on the log level.

I updated the code as per comments above and also aligned the impacted test cases for removed influence of `--stats` on the log level.

witten referenced this pull request from a commit

2020-01-22 20:36:59 +00:00

changes as per comments in PR #283

witten closed this pull request

2020-01-22 20:36:59 +00:00

witten commented

2020-01-22 20:37:13 +00:00

Thanks again for all your hard work on this!

witten referenced this pull request

2020-01-22 23:30:42 +00:00

How to limit output to healthchecks.io? #277

witten commented

2020-01-22 23:36:08 +00:00

FYI: After playing around more with this some more, I decided to disable borg --list and --stats by default at verbosity 2 as well! The rationale is that if a user has multiple different borgmatic log handlers going, e.g. console and syslog, or syslog and Healthchecks, then if any of those handlers are at verbosity 2, then all of them get file details and stats.. because borgmatic only invokes Borg once with a common set of parameters.

So the "solution" I'm using is to make file details and stats 100% opt-in for all verbosity levels. The main downside is if you opt-in at all, you get it everywhere (all log handlers).

Shouldn't affect your use case, but thought you might be interested.

FYI: After playing around more with this some more, I decided to disable `borg --list` and `--stats` by default at verbosity 2 as well! The rationale is that if a user has multiple different borgmatic log handlers going, e.g. console and syslog, or syslog and Healthchecks, then if any of those handlers are at verbosity 2, then *all* of them get file details and stats.. because borgmatic only invokes Borg once with a common set of parameters. So the "solution" I'm using is to make file details and stats 100% opt-in for all verbosity levels. The main downside is if you opt-in at all, you get it everywhere (all log handlers). Shouldn't affect your use case, but thought you might be interested.

Sign in to join this conversation.

No reviewers