borgmatic-collective/borgmatic
borgmatic-collective
/
borgmatic
15
57
Fork 40
Code Issues 54 Pull Requests 1 Actions Packages Releases 121 Activity

By default only brief logging output at verbosity 1 #283

Merged
witten merged 5 commits from palto42/borgmatic:list-files into master 2020-01-22 20:36:59 +00:00
Conversation 3 Commits 5 Files Changed 8 +57 -25
palto42 commented 2020-01-18 14:13:38 +00:00
Contributor
Copy Link

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:

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
witten reviewed 2020-01-21 18:13:42 +00:00
witten left a comment
Owner
Copy Link

Lookin' good! Thanks for the PR.

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))
witten commented 2020-01-21 17:42:15 +00:00
Owner
Copy Link

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 ```
palto42 commented 2020-01-22 18:16:40 +00:00
Author
Contributor
Copy Link

Agree, will change as suggested.

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:
witten commented 2020-01-21 17:43:40 +00:00
Owner
Copy Link

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.
palto42 commented 2020-01-22 18:35:12 +00:00
Author
Contributor
Copy Link

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.
witten commented 2020-01-22 20:27:28 +00:00
Owner
Copy Link

You are of course correct. Carry on!

You are of course correct. Carry on!
witten commented 2020-01-22 20:47:11 +00:00
Owner
Copy Link

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
witten commented 2020-01-21 18:02:42 +00:00
Owner
Copy Link

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 ())
witten commented 2020-01-21 18:06:10 +00:00
Owner
Copy Link

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`?
palto42 commented 2020-01-22 18:56:10 +00:00
Author
Contributor
Copy Link

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 ()) ```
witten commented 2020-01-22 20:29:31 +00:00
Owner
Copy Link

Looks good, thanks!

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,
witten commented 2020-01-21 18:11:43 +00:00
Owner
Copy Link

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.
palto42 commented 2020-01-22 18:38:31 +00:00
Author
Contributor
Copy Link

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?
palto42 commented 2020-01-22 18:41:05 +00:00
Author
Contributor
Copy Link

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'
witten commented 2020-01-21 18:12:43 +00:00
Owner
Copy Link

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

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
Author
Contributor
Copy Link

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 closed this pull request 2020-01-22 20:36:59 +00:00
witten commented 2020-01-22 20:37:13 +00:00
Owner
Copy Link

Thanks again for all your hard work on this!

Thanks again for all your hard work on this!
witten commented 2020-01-22 23:36:08 +00:00
Owner
Copy Link

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.
Reviewers
No reviewers
Labels
Clear labels   
bug

   
data loss

   
design finalized

   
good first issue

   
new feature area

   
question / support

   
security

   
waiting for response
No Label
bug
data loss
design finalized
good first issue
new feature area
question / support
security
waiting for response
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: borgmatic-collective/borgmatic#283
No description provided.
Delete Branch "palto42/borgmatic:list-files"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?