borgmatic-collective/borgmatic
borgmatic-collective
/
borgmatic
14
58
Fork 42
Code Issues 58 Pull Requests 3 Actions Packages Releases 122 Activity

Add support for healthchecks "log" feature #628 #645

Merged
witten merged 8 commits from :feat628-add-healtchecks-logs-support into master 2023-03-07 22:21:31 +00:00
Conversation 10 Commits 8 Files Changed 11 +28 -19
5 changed files with 28 additions and 19 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 45256ae33f - Show all commits

21
borgmatic/commands/borgmatic.py
View File

@ -153,16 +153,17 @@ def run_configuration(config_filename, config, arguments):
error_repository = repository_path
try:
# send logs irrespective of error
dispatch.call_hooks(
'ping_monitor',
hooks,
config_filename,
monitor.MONITOR_HOOK_NAMES,
monitor.State.LOG,
monitoring_log_level,
global_arguments.dry_run,
)
if using_primary_action:
# send logs irrespective of error
dispatch.call_hooks(
'ping_monitor',
hooks,
config_filename,
monitor.MONITOR_HOOK_NAMES,
monitor.State.LOG,
monitoring_log_level,
global_arguments.dry_run,
)
except (OSError, CalledProcessError) as error:
if command.considered_soft_failure(config_filename, error):
return

1
borgmatic/hooks/cronhub.py
View File

@ -10,7 +10,6 @@ MONITOR_STATE_TO_CRONHUB = {
monitor.State.START: 'start',
monitor.State.FINISH: 'finish',
monitor.State.FAIL: 'fail',
monitor.State.LOG: 'log',
}
Soumik_Dutta marked this conversation as resolved
witten commented 2023-03-05 18:19:21 +00:00
Review
Copy Link

I might be missing something, but without any other changes to the Cronhub hook's ping_monitor() function, won't it try to ping Cronhub with a /log/[uuid] URL, which doesn't exist?

I might be missing something, but without any other changes to the Cronhub hook's `ping_monitor()` function, won't it try to ping Cronhub with a `/log/[uuid]` URL, which [doesn't exist](https://docs.cronhub.io/how-to-ping.html#ping-api)?
Soumik_Dutta commented 2023-03-05 20:00:07 +00:00
Review
Copy Link

Oops, I overlooked that issue. I will remove that from the dict.

Oops, I overlooked that issue. I will remove that from the dict.
👍 1

1
borgmatic/hooks/cronitor.py
View File

@ -10,7 +10,6 @@ MONITOR_STATE_TO_CRONITOR = {
monitor.State.START: 'run',
monitor.State.FINISH: 'complete',
monitor.State.FAIL: 'fail',
monitor.State.LOG: 'ok',
}
Soumik_Dutta marked this conversation as resolved
witten commented 2023-03-05 18:27:01 +00:00
Review
Copy Link

I'm not sure this one makes sense for Cronitor. If I'm reading the docs correctly, ok is meant to "manually reset the monitor to a passing state". But the borgmatic "log" state is supposed to be independent of passing or failing, right?

I do agree though that the PagerDuty hook already handles this. With these other monitoring hook integrations though, my recommendation would be just to just return early from ping_monitor() if an unsupported log state is requested similar to how the PagerDuty hook already does. That might mean taking the LOG state back out of the MONITOR_STATE_TO_... dict for hooks that don't support it.

I'm not sure this one makes sense for Cronitor. If I'm reading [the docs](https://cronitor.io/docs/telemetry-api) correctly, `ok` is meant to "manually reset the monitor to a passing state". But the borgmatic "log" state is supposed to be independent of passing or failing, right? I do agree though that the PagerDuty hook already handles this. With these other monitoring hook integrations though, my recommendation would be just to just return early from `ping_monitor()` if an unsupported log state is requested similar to how the PagerDuty hook already does. That might mean taking the `LOG` state back out of the `MONITOR_STATE_TO_...` dict for hooks that don't support it.
Soumik_Dutta commented 2023-03-05 19:58:37 +00:00
Review
Copy Link

Yes. I was unsure about what it meant as "manually reset the monitor to a passing state" ... okay yeah, I remove the log state from the dict.

Yes. I was unsure about what it meant as "manually reset the monitor to a passing state" ... okay yeah, I remove the log state from the dict.
👍 1

7
borgmatic/hooks/ntfy.py
View File

@ -6,13 +6,6 @@ from borgmatic.hooks import monitor
logger = logging.getLogger(__name__)
MONITOR_STATE_TO_NTFY = {
monitor.State.START: None,
monitor.State.FINISH: None,
monitor.State.FAIL: None,
monitor.State.LOG: None,
}
def initialize_monitor(
ping_url, config_filename, monitoring_log_level, dry_run

17
tests/unit/hooks/test_healthchecks.py
View File

@ -184,6 +184,23 @@ def test_ping_monitor_hits_ping_url_for_fail_state():
)
def test_ping_monitor_hits_ping_url_for_log_state():
hook_config = {'ping_url': 'https://example.com'}
payload = 'data'
flexmock(module).should_receive('format_buffered_logs_for_payload').and_return(payload)
flexmock(module.requests).should_receive('post').with_args(
'https://example.com/log', data=payload.encode('utf'), verify=True
).and_return(flexmock(ok=True))
module.ping_monitor(
hook_config,
'config.yaml',
state=module.monitor.State.LOG,
monitoring_log_level=1,
dry_run=False,
)
Soumik_Dutta marked this conversation as resolved
witten commented 2023-03-07 06:34:16 +00:00
Review
Copy Link

Looks good!

Looks good!
def test_ping_monitor_with_ping_uuid_hits_corresponding_url():
hook_config = {'ping_url': 'abcd-efgh-ijkl-mnop'}
payload = 'data'