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 +25 -1
7 changed files with 25 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 69f6695253 - Show all commits

18
borgmatic/commands/borgmatic.py
View File

@ -152,6 +152,24 @@ def run_configuration(config_filename, config, arguments):
encountered_error = error
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,
)
except (OSError, CalledProcessError) as error:
if command.considered_soft_failure(config_filename, error):
return
encountered_error = error
yield from log_error_records('{}: Error pinging monitor'.format(config_filename), error)
if not encountered_error:
try:
if using_primary_action:

1
borgmatic/config/schema.yaml
View File

@ -1199,6 +1199,7 @@ properties:
- start
- finish
- fail
- log
uniqueItems: true
description: |
List of one or more monitoring states to ping for:

1
borgmatic/hooks/cronhub.py
View File

@ -10,6 +10,7 @@ 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,6 +10,7 @@ MONITOR_STATE_TO_CRONITOR = {
monitor.State.START: 'run',
monitor.State.FINISH: 'complete',
monitor.State.FAIL: 'fail',
monitor.State.LOG: 'log',
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
}

3
borgmatic/hooks/healthchecks.py
View File

@ -10,6 +10,7 @@ MONITOR_STATE_TO_HEALTHCHECKS = {
monitor.State.START: 'start',
monitor.State.FINISH: None, # Healthchecks doesn't append to the URL for the finished state.
monitor.State.FAIL: 'fail',
monitor.State.LOG: 'log',
}
PAYLOAD_TRUNCATION_INDICATOR = '...\n'
@ -117,7 +118,7 @@ def ping_monitor(hook_config, config_filename, state, monitoring_log_level, dry_
)
logger.debug('{}: Using Healthchecks ping URL {}'.format(config_filename, ping_url))
if state in (monitor.State.FINISH, monitor.State.FAIL):
if state in (monitor.State.FINISH, monitor.State.FAIL, monitor.State.LOG):
Soumik_Dutta marked this conversation as resolved
witten commented 2023-03-05 18:53:10 +00:00
Review
Copy Link

This looks good! I'm glad to see the same logic for the finish/fail states work for the log state.

This looks good! I'm glad to see the same logic for the finish/fail states work for the log state.
payload = format_buffered_logs_for_payload()
else:
payload = ''

1
borgmatic/hooks/monitor.py
View File

@ -7,3 +7,4 @@ class State(Enum):
START = 1
FINISH = 2
FAIL = 3
LOG = 4

1
borgmatic/hooks/ntfy.py
View File

@ -10,6 +10,7 @@ MONITOR_STATE_TO_NTFY = {
monitor.State.START: None,
monitor.State.FINISH: None,
monitor.State.FAIL: None,
monitor.State.LOG: None,
}
Soumik_Dutta marked this conversation as resolved
witten commented 2023-03-05 18:50:08 +00:00
Review
Copy Link

I would actually recommend removing MONITOR_STATE_TO_NTFY altogether. It looks like it's completely unused!

But I agree that the Ntfy ping_monitor() should already handle ignoring the log state.

I would actually recommend removing `MONITOR_STATE_TO_NTFY` altogether. It looks like it's completely unused! But I agree that the Ntfy `ping_monitor()` should already handle ignoring the `log` state.