borgmatic-collective/borgmatic
borgmatic-collective
/
borgmatic
14
58
Fork 41
Code Issues 54 Pull Requests 2 Actions Packages Releases 122 Activity

Add option to disable syslog output #675

Merged
witten merged 11 commits from Soumik_Dutta/borgmatic:feat484-option-to-disable-syslog into main 2023-05-30 20:03:57 +00:00
Conversation 14 Commits 11 Files Changed 8 +19 -11
5 changed files with 19 additions and 11 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit f512d1e460 - Show all commits

16
borgmatic/commands/arguments.py
View File

@ -152,30 +152,30 @@ def make_parsers():
'-v',
'--verbosity',
type=int,
choices=range(-1, 3),
choices=range(-2, 3),
default=0,
help='Display verbose progress to the console (from only errors to very verbose: -1, 0, 1, or 2)',
help='Display verbose progress to the console (from disabled, errors to very verbose: -2, -1, 0, 1, or 2)',
Soumik_Dutta marked this conversation as resolved Outdated
witten commented 2023-05-04 03:15:54 +00:00
Outdated
Review
Copy Link

At this point, it might be clearer to change this help to something like:

Display verbose progress to the console (disabled, errors only, default, some, or lots: -2, -1, 0, 1, or 2)

Or even something like:

Display verbose progress to the console at the requested level: -2 (disabled), -1 (errors only), 0 (default), 1 (some), or 2 (lots)

At this point, it might be clearer to change this help to something like: `Display verbose progress to the console (disabled, errors only, default, some, or lots: -2, -1, 0, 1, or 2)` Or even something like: `Display verbose progress to the console at the requested level: -2 (disabled), -1 (errors only), 0 (default), 1 (some), or 2 (lots)`
witten commented 2023-05-26 18:57:24 +00:00
Outdated
Review
Copy Link

Maybe remove "from" here and in the other helps, as it's no longer "from"/"to".

Maybe remove "from" here and in the other helps, as it's no longer "from"/"to".
)
global_group.add_argument(
'--syslog-verbosity',
type=int,
choices=range(-1, 3),
choices=range(-2, 3),
Soumik_Dutta marked this conversation as resolved Outdated
witten commented 2023-04-13 05:07:23 +00:00
Outdated
Review
Copy Link

For consistency, it would be good to add -2 support to the other verbosity flags as well. (Let me know if you disagree.)

For consistency, it would be good to add `-2` support to the other verbosity flags as well. (Let me know if you disagree.)
Soumik_Dutta commented 2023-04-13 06:58:57 +00:00
Outdated
Review
Copy Link

I was not sure initially about the scope of adding another global verbosity level so I tried to minimize the affected code.

Though I agree that it will be good for consistency and I am working on it. Otherwise we will have to document around this.

I was not sure initially about the scope of adding another global verbosity level so I tried to minimize the affected code. Though I agree that it will be good for consistency and I am working on it. Otherwise we will have to document around this.
witten commented 2023-04-13 16:03:30 +00:00
Outdated
Review
Copy Link

I'll admit I haven't thought through the scope implications, so if it grows too large, please let me know!

I'll admit I haven't thought through the scope implications, so if it grows too large, please let me know!
default=0,
help='Log verbose progress to syslog (from only errors to very verbose: -1, 0, 1, or 2). Ignored when console is interactive or --log-file is given',
help='Log verbose progress to syslog (from disabled, errors to very verbose: -2, -1, 0, 1, or 2). Ignored when console is interactive or --log-file is given',
)
global_group.add_argument(
'--log-file-verbosity',
type=int,
choices=range(-1, 3),
choices=range(-2, 3),
default=0,
help='Log verbose progress to log file (from only errors to very verbose: -1, 0, 1, or 2). Only used when --log-file is given',
help='Log verbose progress to log file (from disabled, errors to very verbose: -2, -1, 0, 1, or 2). Only used when --log-file is given',
)
global_group.add_argument(
'--monitoring-verbosity',
type=int,
choices=range(-1, 3),
choices=range(-2, 3),
default=0,
help='Log verbose progress to monitoring integrations that support logging (from only errors to very verbose: -1, 0, 1, or 2)',
help='Log verbose progress to monitoring integrations that support logging (from disabled, errors to very verbose: -2, -1, 0, 1, or 2)',
)
global_group.add_argument(
'--log-file',

8
borgmatic/logger.py
View File

@ -141,6 +141,7 @@ def add_logging_level(level_name, level_number):
ANSWER = logging.WARN - 5
DISABLED = logging.DEBUG - 5
Soumik_Dutta marked this conversation as resolved Outdated
witten commented 2023-04-13 05:09:49 +00:00
Outdated
Review
Copy Link

Ah, yes. This makes sense to me.

Ah, yes. This makes sense to me.
witten commented 2023-05-04 06:10:05 +00:00
Outdated
Review
Copy Link

Makes sense!

Makes sense!
def add_custom_log_levels(): # pragma: no cover
@ -148,6 +149,7 @@ def add_custom_log_levels(): # pragma: no cover
Add a custom log level between WARN and INFO for user-requested answers.
'''
add_logging_level('ANSWER', ANSWER)
add_logging_level('DISABLED', DISABLED)
def configure_logging(
@ -175,10 +177,12 @@ def configure_logging(
# Log certain log levels to console stderr and others to stdout. This supports use cases like
# grepping (non-error) output.
console_disabled = logging.NullHandler()
console_error_handler = logging.StreamHandler(sys.stderr)
console_standard_handler = logging.StreamHandler(sys.stdout)
console_handler = Multi_stream_handler(
{
logging.DISABLED: console_disabled,
logging.CRITICAL: console_error_handler,
logging.ERROR: console_error_handler,
logging.WARN: console_error_handler,
@ -191,7 +195,7 @@ def configure_logging(
console_handler.setLevel(console_log_level)
Soumik_Dutta marked this conversation as resolved Outdated
witten commented 2023-04-13 05:18:32 +00:00
Outdated
Review
Copy Link

Would it be possible to add a test for this logic? See tests/unit/test_logger.py for the existing tests for this function.

Would it be possible to add a test for this logic? See `tests/unit/test_logger.py` for the existing tests for this function.
👍 1
syslog_path = None
if log_file is None:
if log_file is None and syslog_log_level != logging.DISABLED:
Soumik_Dutta marked this conversation as resolved Outdated
witten commented 2023-05-04 06:12:46 +00:00
Outdated
Review
Copy Link

Based on these changes, the tests in tests/unit/test_logger.py should probably be updated / added to as well.

Based on these changes, the tests in `tests/unit/test_logger.py` should probably be updated / added to as well.
if os.path.exists('/dev/log'):
syslog_path = '/dev/log'
elif os.path.exists('/var/run/syslog'):
@ -206,7 +210,7 @@ def configure_logging(
)
syslog_handler.setLevel(syslog_log_level)
handlers = (console_handler, syslog_handler)
elif log_file:
elif log_file and log_file_log_level != logging.DISABLED:
file_handler = logging.handlers.WatchedFileHandler(log_file)
file_handler.setFormatter(
logging.Formatter(

2
borgmatic/verbosity.py
View File

@ -2,6 +2,7 @@ import logging
import borgmatic.logger
VERBOSITY_DISABLED = -2
VERBOSITY_ERROR = -1
VERBOSITY_ANSWER = 0
VERBOSITY_SOME = 1
@ -15,6 +16,7 @@ def verbosity_to_log_level(verbosity):
borgmatic.logger.add_custom_log_levels()
return {
VERBOSITY_DISABLED: logging.DISABLED,
VERBOSITY_ERROR: logging.ERROR,
VERBOSITY_ANSWER: logging.ANSWER,
VERBOSITY_SOME: logging.INFO,

2
sample/systemd/borgmatic.service
View File

@ -61,4 +61,4 @@ LogRateLimitIntervalSec=0
# Delay start to prevent backups running during boot. Note that systemd-inhibit requires dbus and
# dbus-user-session to be installed.
ExecStartPre=sleep 1m
ExecStart=systemd-inhibit --who="borgmatic" --what="sleep:shutdown" --why="Prevent interrupting scheduled backup" /root/.local/bin/borgmatic --verbosity -1 --syslog-verbosity 1
ExecStart=systemd-inhibit --who="borgmatic" --what="sleep:shutdown" --why="Prevent interrupting scheduled backup" /root/.local/bin/borgmatic --verbosity -2 --syslog-verbosity 1
witten commented 2023-04-13 05:17:20 +00:00
Outdated
Review
Copy Link

I'd actually recommend swapping this default: Set --verbosity to -2 (which would require adding support for that), and --syslog-verbosity to 1. My rationale is that --verbosity/stdout doesn't log the log level, which --syslog-verbosity does. See the "Actual behavior" in #484 for examples of each.

I'd actually recommend swapping this default: Set `--verbosity` to `-2` (which would require adding support for that), and `--syslog-verbosity` to 1. My rationale is that `--verbosity`/stdout doesn't log the log level, which `--syslog-verbosity` does. See the "Actual behavior" in [#484](https://projects.torsion.org/borgmatic-collective/borgmatic/issues/484) for examples of each.
Soumik_Dutta commented 2023-04-13 07:08:07 +00:00
Outdated
Review
Copy Link

Yes I agree, adding verbosity level -2 will be better. It is better to show log level (as done by syslog) in output instead of indicating the log level by its colour (from console_logs) to stdout.

Yes I agree, adding verbosity level `-2` will be better. It is better to show log level (as done by `syslog`) in output instead of indicating the log level by its colour (from `console_logs`) to `stdout`.
👍 1

2
tests/unit/test_verbosity.py
View File

@ -17,11 +17,13 @@ def insert_logging_mock(log_level):
def test_verbosity_to_log_level_maps_known_verbosity_to_log_level():
flexmock(module.borgmatic.logger).should_receive('add_custom_log_levels')
flexmock(module.logging).ANSWER = module.borgmatic.logger.ANSWER
flexmock(module.logging).DISABLED = module.borgmatic.logger.DISABLED
assert module.verbosity_to_log_level(module.VERBOSITY_ERROR) == logging.ERROR
assert module.verbosity_to_log_level(module.VERBOSITY_ANSWER) == module.borgmatic.logger.ANSWER
assert module.verbosity_to_log_level(module.VERBOSITY_SOME) == logging.INFO
assert module.verbosity_to_log_level(module.VERBOSITY_LOTS) == logging.DEBUG
assert module.verbosity_to_log_level(module.VERBOSITY_DISABLED) == logging.DISABLED
def test_verbosity_to_log_level_maps_unknown_verbosity_to_warning_level():