Different health check endpoints for different commands

rsyring commented

2022-04-03 19:41:59 +00:00

What I'm trying to do and why

I have a cronitor health check integration. I'd like the cronitor endpoint to only be used during a create action. Not check, list, etc.

Why: I really only need cronitor to monitor the automated actions...so that I know if/when they succeed (or fail). If I'm running CLI commands manually, I'm there doing it the work, I don't need cronitor to be notified (and it messes up the stats a bit).

Other notes / implementation ideas

Adding configuration options to specify health check URLs for each borgmatic command, instead of using one URL for all commands. In my use case, I could list one URL for archive creation and none for the other commands.

Another option might be to have a CLI flag for disabling health checks. I could use that when running the check/list actions manually.

#### What I'm trying to do and why I have a cronitor health check integration. I'd like the cronitor endpoint to only be used during a create action. Not check, list, etc. Why: I really only need cronitor to monitor the automated actions...so that I know if/when they succeed (or fail). If I'm running CLI commands manually, I'm there doing it the work, I don't need cronitor to be notified (and it messes up the stats a bit). #### Other notes / implementation ideas Adding configuration options to specify health check URLs for each borgmatic command, instead of using one URL for all commands. In my use case, I could list one URL for archive creation and none for the other commands. Another option might be to have a CLI flag for disabling health checks. I could use that when running the check/list actions manually.

witten commented

2022-04-22 02:12:48 +00:00

Owner

Thanks for filing this! The current Cronitor integration actually only fires for particular borgmatic actions, specifically: prune, compact, create, and check. However that still doesn't help with your need of only running on the create action. But instead of adding configuration points for all the different actions, what do you think of changing borgmatic to simply not ping Cronitor when borgmatic is run interactively from the command-line (as opposed to from a cron job, etc). Would that satisfy your use case?

Thanks for filing this! The current Cronitor integration actually only fires for particular borgmatic actions, specifically: `prune`, `compact`, `create`, and `check`. However that still doesn't help with your need of only running on the `create` action. But instead of adding configuration points for all the different actions, what do you think of changing borgmatic to simply *not* ping Cronitor when borgmatic is run interactively from the command-line (as opposed to from a cron job, etc). Would that satisfy your use case?

rsyring commented

2022-04-22 14:01:13 +00:00

Author

Witten, thanks for your reply and work on borg tooling.

Would that satisfy your use case?

That would satisfy a use case, especially if it was a opt-in option instead of opt-out. In the applications I build, when I have commands that work both from the CLI but will also be run automatically (cron, systemd timers, etc.) I've migrated to using CLI flags to turn on the pinging instead. E.g.:

$ marshal harvest sync --ping-cronitor

I did it that way, instead of a flag to turn it off, because:

I have to type less that way. I type commands in the CLI way more than I type putting them into a cron or unit file. And, at least for my workflows, if I'm working in the CLI I almost never want to ping cronitor b/c they are monitoring automated processes...I don't want my CLI work to interfere.
Pinging cronitor is explicit rather than implicit. I'm not accidently triggering the cronitor on the command line by not knowing or not remembering to add --no-cronitor

what do you think of changing borgmatic to simply not ping Cronitor when borgmatic is run interactively from the command-line (as opposed to from a cron job, etc).

I wouldn't take away the functionality completely b/c I do sometimes want to ping cronitor from the CLI, usually when I'm testing to make sure the cronitor monitors got setup correctly.

Additionally, it doesn't really satisify my use case. For example, the borgbase docs request running the check command weekly instead of every time create is ran. In that case, I'd want two monitors for the same repo: one that is making sure the backup runs hourly (or daily, etc.) and that the check command is ran weekly. With only a single URL to specify the monitor, I can't monitor the weekly check command to make sure it's running correctly.

Taking into account all of the above, it seems to me a good solution would be to support different URLs for different actions:


    # Cronitor ping URL to notify when a backup begins, ends, or
    # errors. Create an account at https://cronitor.io if you'd
    # like to use this service. See borgmatic monitoring
    # documentation for details.
    
    # If specified this way, use the URL for all actions.  Keeps backwards compat.
    cronitor: https://cronitor.link/d3x0c1
    
    # Or:
    cronitor:
    	create: https://cronitor.link/aaa111
        check: https://cronitor.link/bbb222
        prune: https://cronitor.link/ccc333
        # compact: isn't set and would not ping cronitor when used

I'm not sure how you approach configuration/options. Is more bad or good? There are a lot of options already. If you really wanted to make things flexible, you could add another option:


    # The option to ping cronitor (or not) can be specified with the --cronitor and 
    # --no-cronitor flags.  If the flag is not set, what is the default: on / off
    # Defaults to on.
    # cronitor_default: on

The above would keep backwards compatability but also make the functionality I'm looking for possible.

If you like this approach, but don't necessarily want to put development time towards it, I could make a PR for your review.

Witten, thanks for your reply and work on borg tooling. > Would that satisfy your use case? That would satisfy _a_ use case, especially if it was a opt-in option instead of opt-out. In the applications I build, when I have commands that work both from the CLI but will also be run automatically (cron, systemd timers, etc.) I've migrated to using CLI flags to _turn on_ the pinging instead. E.g.: ``` $ marshal harvest sync --ping-cronitor ``` I did it that way, instead of a flag to turn it off, because: 1. I have to type less that way. I type commands in the CLI way more than I type putting them into a cron or unit file. And, at least for my workflows, if I'm working in the CLI I almost never want to ping cronitor b/c they are monitoring automated processes...I don't want my CLI work to interfere. 2. Pinging cronitor is explicit rather than implicit. I'm not accidently triggering the cronitor on the command line by not knowing or not remembering to add `--no-cronitor` > what do you think of changing borgmatic to simply not ping Cronitor when borgmatic is run interactively from the command-line (as opposed to from a cron job, etc). I wouldn't take away the functionality completely b/c I do sometimes want to ping cronitor from the CLI, usually when I'm testing to make sure the cronitor monitors got setup correctly. Additionally, it doesn't really satisify my use case. For example, the borgbase docs request [running the check command weekly](https://docs.borgbase.com/faq/#how-often-should-i-run-borg-check) instead of every time create is ran. In that case, I'd want two monitors for the same repo: one that is making sure the backup runs hourly (or daily, etc.) and that the check command is ran weekly. With only a single URL to specify the monitor, I can't monitor the weekly check command to make sure it's running correctly. Taking into account all of the above, it seems to me a good solution would be to support different URLs for different actions: ```yaml # Cronitor ping URL to notify when a backup begins, ends, or # errors. Create an account at https://cronitor.io if you'd # like to use this service. See borgmatic monitoring # documentation for details. # If specified this way, use the URL for all actions. Keeps backwards compat. cronitor: https://cronitor.link/d3x0c1 # Or: cronitor: create: https://cronitor.link/aaa111 check: https://cronitor.link/bbb222 prune: https://cronitor.link/ccc333 # compact: isn't set and would not ping cronitor when used ``` I'm not sure how you approach configuration/options. Is more bad or good? There are a lot of options already. If you really wanted to make things flexible, you could add another option: ```yaml # The option to ping cronitor (or not) can be specified with the --cronitor and # --no-cronitor flags. If the flag is not set, what is the default: on / off # Defaults to on. # cronitor_default: on ``` The above would keep backwards compatability but also make the functionality I'm looking for possible. If you like this approach, but don't necessarily want to put development time towards it, I could make a PR for your review.

witten commented

2022-04-22 16:48:25 +00:00

Owner

Thanks for adding your thoughts here. I do generally try go reduce configuration options as much as possible (so I don't know that we need a cronitor_default), but I see now why a configuration like you mentioned might be necessary:

    cronitor:
        create: https://cronitor.link/aaa111
        check: https://cronitor.link/bbb222
        prune: https://cronitor.link/ccc333

So if you wanted to take a stab at a PR for that, I'd welcome the contribution! It would be nice though if this wasn't a breaking change for folks with an existing Cronitor configuration. And let me know if you get stuck anywhere on the PR.. I'd be happy to help.

Also note that there is a related ticket #366 for the exact same sort of thing with another borgmatic monitor hook (Healthchecks). So at the risk of ballooning scope, it might make sense to make these changes for all monitor hooks. (I'll leave that call up to you.)

Thanks for adding your thoughts here. I do generally try go reduce configuration options as much as possible (so I don't know that we need a `cronitor_default`), but I see now why a configuration like you mentioned might be necessary: ```yaml cronitor: create: https://cronitor.link/aaa111 check: https://cronitor.link/bbb222 prune: https://cronitor.link/ccc333 ``` So if you wanted to take a stab at a PR for that, I'd welcome the contribution! It would be nice though if this wasn't a breaking change for folks with an existing Cronitor configuration. And let me know if you get stuck anywhere on the PR.. I'd be happy to help. Also note that there is a related ticket #366 for the exact same sort of thing with another borgmatic monitor hook (Healthchecks). So at the risk of ballooning scope, it might make sense to make these changes for all monitor hooks. (I'll leave that call up to you.)

witten added the

design finalized

label

2022-04-22 16:48:34 +00:00

witten referenced this issue

2022-04-22 16:49:18 +00:00

Healthchecks integration for independent operations #366

witten added the

waiting for response

label

2022-04-28 17:49:17 +00:00

Sdub76 referenced this issue

2022-05-09 18:06:13 +00:00

Send --stats output to healthchecks #524

witten referenced this issue

2022-10-04 22:31:56 +00:00

add "series" event parameters when pinging cronitor hook #592

witten removed the

waiting for response

label

2022-10-05 06:15:17 +00:00

xLogiiCx referenced this issue

2022-10-06 18:25:17 +00:00

add "series" event parameters when pinging cronitor hook #592

witten referenced this issue

2022-10-27 16:24:45 +00:00

Hook "on_warning" #606

jetchirag commented

2023-03-23 05:22:16 +00:00

Contributor

@witten I want to take a pick at this. @rsyring's schema looks good to me.

We can check if cronitor is string or contains sub config to decide url per command.

Do you have any input before I proceed?

@witten I want to take a pick at this. @rsyring's schema looks good to me. We can check if cronitor is string or contains sub config to decide url per command. Do you have any input before I proceed?

witten commented

2023-03-23 21:29:12 +00:00

Owner

The good news is the example configuration above is already out of date, and the new schema is no longer just a string. Here's what the cronitor configuration looks like now:

hooks:
    cronitor:
        ping_url: https://cronitor.link/d3x0c1

But there still would be a transformation needed though to get that into the proposed per-action form. My recommendation would be to actually normalize the loaded configuration once so that all downstream borgmatic code can assume the "new" schema. Fortunately, there's precedent for doing that! Take a look at normalize() in normalize.py, which is responsible for taking "old" config (if found) and turning it into the new schema.

Let me know if I can answer any questions as you proceed.

The good news is the example configuration above is already out of date, and the new schema is no longer just a string. Here's what the `cronitor` configuration looks like now: ``` hooks: cronitor: ping_url: https://cronitor.link/d3x0c1 ``` But there still would be a transformation needed though to get that into the proposed per-action form. My recommendation would be to actually normalize the loaded configuration once so that all downstream borgmatic code can assume the "new" schema. Fortunately, there's precedent for doing that! Take a look at `normalize()` in [`normalize.py`](https://projects.torsion.org/borgmatic-collective/borgmatic/src/branch/master/borgmatic/config/normalize.py), which is responsible for taking "old" config (if found) and turning it into the new schema. Let me know if I can answer any questions as you proceed.

jetchirag commented

2023-03-24 14:51:13 +00:00

Contributor

@witten I was going for a schema config like

cronitor:
  type: object
  additionalProperties: false
  oneOf:
    - required:
        - ping_url
      properties:
        ping_url:
          type: string
          description: |
            Cronitor ping URL to notify when a backup begins, ends, or errors.
          example: https://cronitor.link/d3x0c1
    - required:
        - create
      properties:
        create:
          type: string
          description: |
            Cronitor ping URL to notify when create action begins, ends, or errors.
          example: https://cronitor.link/d3x0c1
   	...

decide hook url based on action_name and send to hook:

# action_name = list(arguments.keys())[0]
# ping_url based on action_name to allow easy expansion to other monitoring hook
dispatch.call_hooks(
                'ping_monitor',
                hooks,
                config_filename,
                monitor.MONITOR_HOOK_NAMES,
                monitor.State.LOG,
                monitoring_log_level,
                global_arguments.dry_run,
                ping_url
            )

Then normalise ping_url to fill 'create', 'prune', 'compact', 'check' for compatibility. Do you have any suggestion where I should build the logic similar to oneOf to make one of them optional in schema.yaml?

@witten I was going for a schema config like ``` cronitor: type: object additionalProperties: false oneOf: - required: - ping_url properties: ping_url: type: string description: | Cronitor ping URL to notify when a backup begins, ends, or errors. example: https://cronitor.link/d3x0c1 - required: - create properties: create: type: string description: | Cronitor ping URL to notify when create action begins, ends, or errors. example: https://cronitor.link/d3x0c1 ... ``` decide hook url based on action_name and send to hook: ``` # action_name = list(arguments.keys())[0] # ping_url based on action_name to allow easy expansion to other monitoring hook dispatch.call_hooks( 'ping_monitor', hooks, config_filename, monitor.MONITOR_HOOK_NAMES, monitor.State.LOG, monitoring_log_level, global_arguments.dry_run, ping_url ) ``` Then normalise ping_url to fill 'create', 'prune', 'compact', 'check' for compatibility. Do you have any suggestion where I should build the logic similar to oneOf to make one of them optional in schema.yaml?

witten commented

2023-03-24 22:38:25 +00:00

Owner

I actually don't think you need to build any backwards compatibility into the schema at all! That's because normalize() does all of its configuration transformations before the schema validation is applied. So if the schema only supports the new format and normalize() takes care of turning the old format into the new format, then everything should work out.

ping_url based on action_name to allow easy expansion to other monitoring hook

That makes sense as long as the URL construction happens in cronitor.py. (I wasn't sure from your example.)

Then normalise ping_url to fill 'create', 'prune', 'compact', 'check' for compatibility.

This totally makes sense to me!

Do you have any suggestion where I should build the logic similar to oneOf to make one of them optional in schema.yaml?

If the schema only defines the new format, you won't need to do this any longer.

Let me know if I can answer any other questions about this.

I actually don't think you need to build any backwards compatibility into the schema at all! That's because normalize() does all of its configuration transformations *before* the schema validation is applied. So if the schema only supports the new format and normalize() takes care of turning the old format into the new format, then everything should work out. > ping_url based on action_name to allow easy expansion to other monitoring hook That makes sense as long as the URL construction happens in `cronitor.py`. (I wasn't sure from your example.) > Then normalise ping_url to fill 'create', 'prune', 'compact', 'check' for compatibility. This totally makes sense to me! > Do you have any suggestion where I should build the logic similar to oneOf to make one of them optional in schema.yaml? If the schema only defines the new format, you won't need to do this any longer. Let me know if I can answer any other questions about this.

jetchirag commented

2023-03-25 04:23:43 +00:00

Contributor

~~The reason I wanted to allow specifying ping_url was so if someone wants to get notified on every action on same url, they won't have to specify same link 4 times.~~

~~URL construction will happen in cronitor.py, selecting which url (from schema) to send to monitor hook in borgmatic.py. Otherwise we'll have same code in all monitoring hook.~~

~~The reason I wanted to allow specifying ping_url was so if someone wants to get notified on every action on same url, they won't have to specify same link 4 times.~~ ~~URL construction will happen in cronitor.py, selecting which url (from schema) to send to monitor hook in borgmatic.py. Otherwise we'll have same code in all monitoring hook.~~

jetchirag referenced this issue from a pull request that will close it,

2023-03-25 13:38:02 +00:00

WIP: Separate endpoint for each primary action for monitor hooks #660

jetchirag commented

2023-03-25 13:40:51 +00:00

Contributor

I've pushed some basic code to define the schema. Can you verify the method I've used?

witten commented

2023-03-25 16:08:14 +00:00

Owner

Oh, gotcha! That approach and the oneOf approach make sense to me then.

Do you have any suggestion where I should build the logic similar to oneOf to make one of them optional in schema.yaml?

Are you asking where in the code to enforce the schema's oneOf logic? If so, I think the answer is nowhere! Because the existing JSON Schema validation does that for you, and you should get back the resulting config with either ping_url or create, etc.. depending on which one was found in the config file.

Oh, gotcha! That approach and the `oneOf` approach make sense to me then. > Do you have any suggestion where I should build the logic similar to oneOf to make one of them optional in schema.yaml? Are you asking where in the code to enforce the schema's `oneOf` logic? If so, I think the answer is nowhere! Because the existing JSON Schema validation does that for you, and you *should* get back the resulting config with either `ping_url` or `create`, etc.. depending on which one was found in the config file.

witten commented

2023-03-25 16:08:38 +00:00

Owner

I've pushed some basic code to define the schema. Can you verify the method I've used?

I'll have a look when I get a chance!

> I've pushed some basic code to define the schema. Can you verify the method I've used? I'll have a look when I get a chance!

xrisk commented

2023-05-08 20:35:25 +00:00

This would be useful for me for a different reason.

Healthchecks lets you set the maximum time allowed between pings. For example, if you've got a day limit configured, and borgmatic doesn't ping for > 1 day, healthchecks will send an alert.

Now, I've got borgmatic check prune borgmatic verify set as cronjobs with different schedules (once a day and once a week). I can't use the same ping url with both of these.

Imagine if borgmatic verify never fires --- this configuration would not catch it, because healthchecks has no way to distinguish between different kinds of events). The only solution is to use two different "projects" in healthchecks with two different ping urls.

This would be useful for me for a different reason. Healthchecks lets you set the maximum time allowed between pings. For example, if you've got a day limit configured, and borgmatic doesn't ping for > 1 day, healthchecks will send an alert. Now, I've got `borgmatic check prune` `borgmatic verify` set as cronjobs with different schedules (once a day and once a week). I can't use the same ping url with both of these. Imagine if borgmatic verify never fires --- this configuration would not catch it, because healthchecks has no way to distinguish between different kinds of events). The only solution is to use two different "projects" in healthchecks with two different ping urls.

witten commented

2023-05-08 21:58:53 +00:00

Owner

@xrisk That makes sense to me. (I assume by verify you mean check.) Note however that the Healthchecks variant of this ticket is #366, so you may want to follow along there. In practice though, the two tickets might be implemented together.

@xrisk That makes sense to me. (I assume by `verify` you mean `check`.) Note however that the Healthchecks variant of this ticket is #366, so you may want to follow along there. In practice though, the two tickets might be implemented together.

witten referenced this issue

2023-05-22 18:02:41 +00:00

Archive checks are incorrectly skipped if you have multiple config files with different archive name formats #688

witten commented

2023-06-27 22:00:00 +00:00

Owner

WIP PR here: #660

WIP PR here: https://projects.torsion.org/borgmatic-collective/borgmatic/pulls/660

witten referenced this issue

2023-07-07 03:56:59 +00:00

WIP: Separate endpoint for each primary action for monitor hooks #660