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
Contributor

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:

  • Another verbosity level DISABLED with level 60
  • Disables syslog output at level 60 by setting syslog_path to None
  • Expose the verbosity level as -2
  • Updates test for custom logging levels

Signed-off-by: Soumik Dutta shalearkane@gmail.com

Fixes [#484](https://projects.torsion.org/borgmatic-collective/borgmatic/issues/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: - Another verbosity level `DISABLED` with level `60` - Disables syslog output at level `60` by setting `syslog_path` to `None` - Expose the verbosity level as `-2` - Updates test for custom logging levels Signed-off-by: Soumik Dutta <shalearkane@gmail.com>
Soumik_Dutta changed title from Add option to disable `syslog` output to WIP: Add option to disable `syslog` output 2023-04-12 10:01:31 +00:00
Soumik_Dutta force-pushed feat484-option-to-disable-syslog from 793146fb7e to b9e9f5db10 2023-04-13 04:05:05 +00:00 Compare
witten requested changes 2023-04-13 05:20:50 +00:00
witten left a comment
Owner

This general approach looks good to me! Thanks for taking on this ticket.

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),
Owner

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.)
Author
Contributor

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.
Owner

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!
Soumik_Dutta marked this conversation as resolved
@ -141,6 +141,7 @@ def add_logging_level(level_name, level_number):
ANSWER = logging.WARN - 5
DISABLED = logging.CRITICAL + 10
Owner

Ah, yes. This makes sense to me.

Ah, yes. This makes sense to me.
Soumik_Dutta marked this conversation as resolved
@ -192,3 +194,3 @@
syslog_path = None
if log_file is None:
if syslog_log_level != logging.DISABLED and log_file is None:
Owner

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.
Soumik_Dutta marked this conversation as resolved
@ -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
Owner

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.
Author
Contributor

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`.
Soumik_Dutta force-pushed feat484-option-to-disable-syslog from b9e9f5db10 to 8a8b0f6f91 2023-04-30 20:41:53 +00:00 Compare
Soumik_Dutta force-pushed feat484-option-to-disable-syslog from 8a8b0f6f91 to 08843d51d9 2023-04-30 21:15:11 +00:00 Compare
Author
Contributor

@witten For verbosity level for monitoring services, I see that for all the definitions of ping_monitor :

def ping_monitor(hook_config, config_filename, state, monitoring_log_level, dry_run):
'''
description

and for most of the definitions of initialize_monitor (except Healthchecks):

def initialize_monitor(
    ping_url, config_filename, monitoring_log_level, dry_run
):

the monitoring_log_level variable is actually unused. Is this an intended choice

  1. since most of the monitoring services doesn't actually receive logs (except Healthchecks), monitoring_log_level isn't actually get used?
    or
  2. to send logs/backup state (start/finish/fail) irrespective of error level since that is defined in the config file?

If it is the first case, will it be prudent to just add a condition to the top of the ping_monitor function like this

def ping_monitor(hook_config, config_filename, state, monitoring_log_level, dry_run):
'''description'''
	if monitoring_log_level == logging.DISABLED:
		return

Or 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?

@witten For verbosity level for monitoring services, I see that for all the definitions of `ping_monitor` : ```python def ping_monitor(hook_config, config_filename, state, monitoring_log_level, dry_run): ''' description ``` and for most of the definitions of `initialize_monitor` (except Healthchecks): ```python def initialize_monitor( ping_url, config_filename, monitoring_log_level, dry_run ): ``` the `monitoring_log_level` variable is actually unused. Is this an intended choice 1. since most of the monitoring services doesn't actually receive logs (except Healthchecks), `monitoring_log_level` isn't actually get used? or 2. to send logs/backup state (start/finish/fail) irrespective of error level since that is defined in the config file? If it is the first case, will it be prudent to just add a condition to the top of the `ping_monitor` function like this ``` python def ping_monitor(hook_config, config_filename, state, monitoring_log_level, dry_run): '''description''' if monitoring_log_level == logging.DISABLED: return ``` Or 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?
Soumik_Dutta force-pushed feat484-option-to-disable-syslog from 1f9eb6df8f to d72d15455f 2023-05-01 19:16:27 +00:00 Compare
Author
Contributor

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.

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.
Soumik_Dutta requested review from witten 2023-05-03 14:13:05 +00:00
Author
Contributor

While writing unit tests for Healthchecks in tests/unit/hooks/test_healthchecks.py, I noticed that monitoring_log_level is set to 1 while monitoring_log_level should actually correspond to values 10, 20, ..., etc as provided by verbosity_to_log_level() function which converts 1, 2, etc to the aforementioned values. Should I open another PR fixing that or is that not actually necessary?

While writing unit tests for `Healthchecks` in `tests/unit/hooks/test_healthchecks.py`, I noticed that `monitoring_log_level` is set to `1` while `monitoring_log_level` should actually correspond to values `10`, `20`, ..., etc as provided by `verbosity_to_log_level()` function which converts `1`, `2`, etc to the aforementioned values. Should I open another PR fixing that or is that not actually necessary?
Owner

the monitoring_log_level variable is actually unused. Is this an intended choice

  1. since most of the monitoring services doesn't actually receive logs (except Healthchecks), monitoring_log_level isn't actually get used?
    or
  2. to send logs/backup state (start/finish/fail) irrespective of error level since that is defined in the config file?

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.

If it is the first case, will it be prudent to just add a condition to the top of the ping_monitor function like this

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.

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.

Cool!

While writing unit tests for Healthchecks in tests/unit/hooks/test_healthchecks.py, I noticed that monitoring_log_level is set to 1 while monitoring_log_level should actually correspond to values 10, 20, ..., etc as provided by verbosity_to_log_level() function which converts 1, 2, etc to the aforementioned values. Should I open another PR fixing that or is that not actually necessary?

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.

> the monitoring_log_level variable is actually unused. Is this an intended choice > > 1. since most of the monitoring services doesn't actually receive logs (except Healthchecks), monitoring_log_level isn't actually get used? > or > 2. to send logs/backup state (start/finish/fail) irrespective of error level since that is defined in the config file? 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. > If it is the first case, will it be prudent to just add a condition to the top of the ping_monitor function like this 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. > 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. Cool! > While writing unit tests for Healthchecks in tests/unit/hooks/test_healthchecks.py, I noticed that monitoring_log_level is set to 1 while monitoring_log_level should actually correspond to values 10, 20, ..., etc as provided by verbosity_to_log_level() function which converts 1, 2, etc to the aforementioned values. Should I open another PR fixing that or is that not actually necessary? 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.
witten reviewed 2023-05-04 06:13:00 +00:00
witten left a comment
Owner

This is all looking good to me!

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)',
Owner

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)`
Soumik_Dutta marked this conversation as resolved
@ -141,6 +141,7 @@ def add_logging_level(level_name, level_number):
ANSWER = logging.WARN - 5
DISABLED = logging.CRITICAL + 10
Owner

Makes sense!

Makes sense!
Soumik_Dutta marked this conversation as resolved
@ -192,3 +196,3 @@
syslog_path = None
if log_file is None:
if log_file is None and syslog_log_level != logging.DISABLED:
Owner

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.
Soumik_Dutta marked this conversation as resolved
Soumik_Dutta force-pushed feat484-option-to-disable-syslog from 2fc14f5f45 to 35dca82f82 2023-05-12 13:45:59 +00:00 Compare
Author
Contributor

@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 the monitoring_verbosity,

arguments = {'global': flexmock(monitoring_verbosity=1, dry_run=False), 'create': flexmock()}

changing the value does not yield any effect. Shouldn't this line produce error if the logging level is determined incorrectly?

flexmock(module).should_receive('verbosity_to_log_level').and_return(module.DISABLED)
@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 the `monitoring_verbosity`, ```python arguments = {'global': flexmock(monitoring_verbosity=1, dry_run=False), 'create': flexmock()} ``` changing the value does not yield any effect. Shouldn't this line produce error if the logging level is determined incorrectly? ```python flexmock(module).should_receive('verbosity_to_log_level').and_return(module.DISABLED) ```
Owner

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!

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!
Author
Contributor

It's the test_run_configuration_bails_for_monitor_log_soft_failure test in tests/unit/commands/test_borgmatic.py, line 119

It's the `test_run_configuration_bails_for_monitor_log_soft_failure` test in `tests/unit/commands/test_borgmatic.py`, line 119
Owner

Ah, 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 after verbosity_to_log_level() is called with those arguments), 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!

Ah, 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 after `verbosity_to_log_level()` is called with those `arguments`), 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!
Owner

And with that hopefully resolved, let me know if this is ready for review again!

And with that hopefully resolved, let me know if this is ready for review again!
Soumik_Dutta force-pushed feat484-option-to-disable-syslog from 963f86e4e8 to 0283f9ae2a 2023-05-26 03:28:08 +00:00 Compare
Author
Contributor

In tests/unit/test_logger.py Line 316

flexmock(module.os.path).should_receive('exists').with_args('/dev/log').and_return(True)

shouldn't this be

flexmock(module.os.path).should_receive('exists').never()

since log_file is not None?

In `tests/unit/test_logger.py` Line 316 ```python flexmock(module.os.path).should_receive('exists').with_args('/dev/log').and_return(True) ``` shouldn't this be ```python flexmock(module.os.path).should_receive('exists').never() ``` since `log_file` is *not* `None`?
Soumik_Dutta added 1 commit 2023-05-26 04:29:02 +00:00
3d41ed3a34 add test to check that log_file is disabled
if logging is disabled

Signed-off-by: Soumik Dutta <shalearkane@gmail.com>
Owner

Good catch! I think you are correct there.

Good catch! I think you are correct there.
Author
Contributor

I think I have completed most of the tests. So please review. 😄

I think I have completed most of the tests. So please review. 😄
Owner

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.

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.
witten requested changes 2023-05-26 19:24:07 +00:00
witten left a comment
Owner

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.

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)',
Owner

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".
Soumik_Dutta marked this conversation as resolved
@ -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()
Owner

This line is duplicative of the earlier call two lines up. The first one is sufficient to assert that call_hooks() is never called.

This line is duplicative of the earlier call two lines up. The first one is sufficient to assert that `call_hooks()` is never called.
Soumik_Dutta marked this conversation as resolved
Soumik_Dutta added 1 commit 2023-05-28 19:39:07 +00:00
a7f81d538d nit changes
- help strings in borgmatic commands
- test fixes in test_logger and test_borgmatic

Signed-off-by: Soumik Dutta <shalearkane@gmail.com>
Soumik_Dutta changed title from WIP: Add option to disable `syslog` output to Add option to disable `syslog` output 2023-05-28 19:40:34 +00:00
witten merged commit c0aaba6891 into main 2023-05-30 20:03:57 +00:00
Owner

Thanks so much for sticking with this. I really appreciate your work here, and I think borgmatic users will too!

Thanks so much for sticking with this. I really appreciate your work here, and I think borgmatic users will too!
Sign in to join this conversation.
No reviewers
No Milestone
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#675
No description provided.