Add option to disable syslog
output #675
No reviewers
Labels
No Label
bug
data loss
design finalized
good first issue
new feature area
question / support
security
waiting for response
No Milestone
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: borgmatic-collective/borgmatic#675
Loading…
Reference in New Issue
No description provided.
Delete Branch "Soumik_Dutta/borgmatic:feat484-option-to-disable-syslog"
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?
Fixes #484
This patch allows users to disable syslog output to by setting its value to -2.
i.e. by
--syslog-verbosity -2
This patch adds:
DISABLED
with level60
60
by settingsyslog_path
toNone
-2
Signed-off-by: Soumik Dutta shalearkane@gmail.com
Add option to disable `syslog` outputto WIP: Add option to disable `syslog` output793146fb7e
tob9e9f5db10
This general approach looks good to me! Thanks for taking on this ticket.
@ -160,3 +160,3 @@
'--syslog-verbosity',
type=int,
choices=range(-1, 3),
choices=range(-2, 3),
For consistency, it would be good to add
-2
support to the other verbosity flags as well. (Let me know if you disagree.)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'll admit I haven't thought through the scope implications, so if it grows too large, please let me know!
@ -141,6 +141,7 @@ def add_logging_level(level_name, level_number):
ANSWER = logging.WARN - 5
DISABLED = logging.CRITICAL + 10
Ah, yes. This makes sense to me.
@ -192,3 +194,3 @@
syslog_path = None
if log_file is None:
if syslog_log_level != logging.DISABLED and log_file is None:
Would it be possible to add a test for this logic? See
tests/unit/test_logger.py
for the existing tests for this function.@ -62,3 +62,3 @@
# 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 -1 --syslog-verbosity -2
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.Yes I agree, adding verbosity level
-2
will be better. It is better to show log level (as done bysyslog
) in output instead of indicating the log level by its colour (fromconsole_logs
) tostdout
.b9e9f5db10
to8a8b0f6f91
8a8b0f6f91
to08843d51d9
@witten For verbosity level for monitoring services, I see that for all the definitions of
ping_monitor
:and for most of the definitions of
initialize_monitor
(except Healthchecks):the
monitoring_log_level
variable is actually unused. Is this an intended choicemonitoring_log_level
isn't actually get used?or
If it is the first case, will it be prudent to just add a condition to the top of the
ping_monitor
function like thisOr if it is the second case, will I have to make sure that if indeed logs are being sent (and not just the backup state), it will send none? i.e. effectively disabling sending logs but keep sending backup state (start/finish/fail) in the case for Healthchecks?
1f9eb6df8f
tod72d15455f
I have set the level of logging.DISABLED to be higher than CRITICAL so that there is no logs which have higher level than DISABLED, effectively not logging anything.
While writing unit tests for
Healthchecks
intests/unit/hooks/test_healthchecks.py
, I noticed thatmonitoring_log_level
is set to1
whilemonitoring_log_level
should actually correspond to values10
,20
, ..., etc as provided byverbosity_to_log_level()
function which converts1
,2
, etc to the aforementioned values. Should I open another PR fixing that or is that not actually necessary?I think it's probably number 1.
Note that there is at least one ticket for sending logs to another monitoring service (#409). But that ticket, as with Healthchecks, probably would only need to make use of
monitoring_log_level
at monitoring initialization time.I think that makes a lot of sense! An alternative would be to do that check before calling
ping_monitor()
to begin with. I think either approach could work.Cool!
Tests (especially unit tests) often make use of nonsense values and/or mock objects if the values are not relevant to the test in question. Looking at the tests you're referring to, I don't think that a value of 1 or anything else should impact the results—unless you're adding new logic to the unit under test like
if monitoring_log_level == logging.DISABLED
that does hinge on the actual value. If that's the case, then yes, please feel free to change the values if you need that for your tests to work.This is all looking good to me!
@ -156,2 +155,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)',
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)
@ -141,6 +141,7 @@ def add_logging_level(level_name, level_number):
ANSWER = logging.WARN - 5
DISABLED = logging.CRITICAL + 10
Makes sense!
@ -192,3 +196,3 @@
syslog_path = None
if log_file is None:
if log_file is None and syslog_log_level != logging.DISABLED:
Based on these changes, the tests in
tests/unit/test_logger.py
should probably be updated / added to as well.2fc14f5f45
to35dca82f82
@witten I have updated the tests. However I faced one issue. It seems to me that in the tests in the file
tests/unit/commands/test_borgmatic.py
, for the line where we set themonitoring_verbosity
,changing the value does not yield any effect. Shouldn't this line produce error if the logging level is determined incorrectly?
Yes, assuming you're referring to one of the tests for
run_configuration()
, that looks like it should fail. Can I have a look at the full test in question? Thanks!It's the
test_run_configuration_bails_for_monitor_log_soft_failure
test intests/unit/commands/test_borgmatic.py
, line 119Ah, so my previous statement was incorrect. What I think is going on here is that because this test is mocking both sides of this interaction (the
arguments
value being passed in and the result afterverbosity_to_log_level()
is called with thosearguments
), there's no error. Put another way, for this test,verbosity_to_log_level()
isn't the unit under test, and therefore changes that might affect it in "real life" don't affect it in this test.. It's completely mocked out.I hope that makes sense!
And with that hopefully resolved, let me know if this is ready for review again!
963f86e4e8
to0283f9ae2a
In
tests/unit/test_logger.py
Line 316shouldn't this be
since
log_file
is notNone
?Good catch! I think you are correct there.
I think I have completed most of the tests. So please review. 😄
Awesome, I will when I get the chance! If you think this is ready to merge though (after review of course), feel free to remove the WIP prefix.
These tests look great, and the PR is ready to merge after the minor changes noted. Feel free to take off the WIP tag whenever you're ready.
@ -156,2 +155,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 only, default, some, or lots: -2, -1, 0, 1, or 2)',
Maybe remove "from" here and in the other helps, as it's no longer "from"/"to".
@ -172,0 +175,4 @@
flexmock(module.dispatch).should_receive('call_hooks').never()
flexmock(module).should_receive('run_actions').and_return([])
flexmock(module.dispatch).should_receive('call_hooks').never()
This line is duplicative of the earlier call two lines up. The first one is sufficient to assert that
call_hooks()
is never called.WIP: Add option to disable `syslog` outputto Add option to disable `syslog` outputThanks so much for sticking with this. I really appreciate your work here, and I think borgmatic users will too!