add a hook for sending push notifications via ntfy.sh #543

Merged
witten merged 1 commits from g-a-c/borgmatic:feat/ntfy into master 3 weeks ago
g-a-c commented 3 weeks ago

Out of the current monitoring options (Healthchecks.io, Cronhub, Cronitor, and PagerDuty) I didn't see any free options which would provide real-time push notifications. Healthchecks.io allows 5 SMS or WhatsApp messages per month but this is kind of limiting.

So I decided since I had a use case, and felt like a little bit of hacking on something, I'd add a hook for Ntfy that I would personally find useful, and hopefully others might too.

It can be configured as simply as providing a topic, which will be enough to publish a message on failure, or there are more (documented) options to allow more configuration.

Out of the current monitoring options (Healthchecks.io, Cronhub, Cronitor, and PagerDuty) I didn't see any free options which would provide real-time push notifications. Healthchecks.io allows 5 SMS or WhatsApp messages per month but this is kind of limiting. So I decided since I had a use case, and felt like a little bit of hacking on something, I'd add a hook for [Ntfy](https://ntfy.sh) that I would personally find useful, and hopefully others might too. It can be configured as simply as providing a `topic`, which will be enough to publish a message on failure, or there are more (documented) options to allow more configuration.
g-a-c added 1 commit 3 weeks ago
g-a-c force-pushed feat/ntfy from 506907e6b6 to 0ff5c6192c 3 weeks ago
Owner

OMG this is great. And documentation, too! Thanks so much for taking the time to add this functionality. The only thing I see potentially missing is tests for ping_monitor(). Do you want to take a stab at that, or would you prefer I do them? You can grep for test_ping_monitor if you want inspiration.

OMG this is great. And documentation, too! Thanks so much for taking the time to add this functionality. The only thing I see potentially missing is tests for `ping_monitor()`. Do you want to take a stab at that, or would you prefer I do them? You can grep for `test_ping_monitor` if you want inspiration.
Poster

I'm happy to take a look and finish the job, somehow I totally missed seeing the tests/unit/hooks directory and assumed that only the core Borgmatic functionality was tested and not external services. I'll do that this evening and push another commit

I'm happy to take a look and finish the job, somehow I totally missed seeing the `tests/unit/hooks` directory and assumed that only the core Borgmatic functionality was tested and not external services. I'll do that this evening and push another commit
g-a-c added 2 commits 3 weeks ago
g-a-c force-pushed feat/ntfy from e218aec8ce to 4a91f6a741 3 weeks ago
g-a-c force-pushed feat/ntfy from 4a91f6a741 to a7a99799fc 3 weeks ago
g-a-c reviewed 3 weeks ago
logger = logging.getLogger(__name__)
MONITOR_STATE_TO_NTFY = {
g-a-c commented 3 weeks ago
Poster

This is not really necessary, but had to be done to keep flake8 happy because otherwise borgmatic.hooks.monitor was imported due to being required by the tests, even though it was not referenced within this hook directly.

This is not really necessary, but had to be done to keep `flake8` happy because otherwise `borgmatic.hooks.monitor` was imported due to being required by the tests, even though it was not referenced within this hook directly.
witten commented 3 weeks ago
Owner

That seems unnecessary to me, given that it's never used...? Your tests can always import directly from borgmatic.hooks.monitor rather than relying on module.monitor. That's just a little convention in borgmatic tests to make sure you're using the exact same import that the code under test is importing. But no need to bend over backwards to do it!

That seems unnecessary to me, given that it's never used...? Your tests can always import directly from `borgmatic.hooks.monitor` rather than relying on `module.monitor`. That's just a little convention in borgmatic tests to make sure you're using the exact same import that the code under test is importing. But no need to bend over backwards to do it!
g-a-c force-pushed feat/ntfy from a7a99799fc to d0bb740171 3 weeks ago
Poster

Too much multi-tasking with my day job and a couple of force-pushed later, and this should be reviewable...the tests don't exhaustively cover every possible scenario but they do cover:

  • minimal config (topic set only)
  • self-hosted config (topic and server set)
  • custom fail messages (topic and fail set)
  • custom states with the default message (topic and states set)
  • dry run (topic set, with dryrun set on the test)
Too much multi-tasking with my day job and a couple of force-pushed later, and this should be reviewable...the tests don't exhaustively cover every possible scenario but they do cover: - minimal config (`topic` set only) - self-hosted config (`topic` and `server` set) - custom fail messages (`topic` and `fail` set) - custom states with the default message (`topic` and `states` set) - dry run (`topic` set, with `dryrun` set on the test)
g-a-c added 1 commit 3 weeks ago
Poster

So I realised I was missing one last test to get 100% code coverage, which is the "connection error" test. I stole the one from one of the other tests as this seemed a relatively simple one, but as it is in the most recent commit, it fails, and it seems to be not necessarily related to my hook but related to logging:

self = <Logger borgmatic.hooks.ntfy (WARNING)>, record = <LogRecord: borgmatic.hooks.ntfy, 30, /Users/g-a-c/Development/borgmatic/borgmatic/hooks/ntfy.py, 70, "config.yaml: Ntfy error: ">

    def callHandlers(self, record):
        """
        Pass a record to all relevant handlers.

        Loop through all handlers for this logger and its parents in the
        logger hierarchy. If no handler was found, output a one-off error
        message to sys.stderr. Stop searching up the hierarchy whenever a
        logger with the "propagate" attribute set to zero is found - that
        will be the last logger whose handlers are called.
        """
        c = self
        found = 0
        while c:
            for hdlr in c.handlers:
                found = found + 1
>               if record.levelno >= hdlr.level:
E               AttributeError: 'NoneType' object has no attribute 'level'

/usr/local/Cellar/python@3.9/3.9.13_1/Frameworks/Python.framework/Versions/3.9/lib/python3.9/logging/__init__.py:1660: AttributeError

So this raises a couple of questions I had:

  • what is the purpose of the logging.getLogger('urllib3').setLevel() call? I don't see anywhere that a logger called urllib3 is either created or used (unless this happens transparently through Requests?) it is transparently used by Requests, and urllib3 as the underlying library - answered my own question there...
  • why does the Cronitor hook change the log level of that logger, when the except block is going to output through the __name__ logger anyway?
  • why does test_cronitor.py (for example) pass this same test when the logger object is instantiated identically in both cronitor.py and ntfy.py?
So I realised I was missing one last test to get 100% code coverage, which is the "connection error" test. I stole the one from one of the other tests as this seemed a relatively simple one, but as it is in the most recent commit, it fails, and it seems to be not necessarily related to my hook but related to logging: ``` self = <Logger borgmatic.hooks.ntfy (WARNING)>, record = <LogRecord: borgmatic.hooks.ntfy, 30, /Users/g-a-c/Development/borgmatic/borgmatic/hooks/ntfy.py, 70, "config.yaml: Ntfy error: "> def callHandlers(self, record): """ Pass a record to all relevant handlers. Loop through all handlers for this logger and its parents in the logger hierarchy. If no handler was found, output a one-off error message to sys.stderr. Stop searching up the hierarchy whenever a logger with the "propagate" attribute set to zero is found - that will be the last logger whose handlers are called. """ c = self found = 0 while c: for hdlr in c.handlers: found = found + 1 > if record.levelno >= hdlr.level: E AttributeError: 'NoneType' object has no attribute 'level' /usr/local/Cellar/python@3.9/3.9.13_1/Frameworks/Python.framework/Versions/3.9/lib/python3.9/logging/__init__.py:1660: AttributeError ``` So this raises a couple of questions I had: - ~~what is the purpose of the `logging.getLogger('urllib3').setLevel()` call? I don't see anywhere that a logger called `urllib3` is either created or used (unless this happens transparently through Requests?)~~ it is transparently used by Requests, and `urllib3` as the underlying library - answered my own question there... - why does the Cronitor hook change the log level of **that** logger, when the `except` block is going to output through the `__name__` logger anyway? - why does `test_cronitor.py` (for example) pass this same test when the `logger` object is instantiated identically in both `cronitor.py` and `ntfy.py`?
Owner
  • what is the purpose of the logging.getLogger('urllib3').setLevel() call? I don't see anywhere that a logger called urllib3 is either created or used (unless this happens transparently through Requests?) it is transparently used by Requests, and urllib3 as the underlying library - answered my own question there...
  • why does the Cronitor hook change the log level of that logger, when the except block is going to output through the name logger anyway?

Yeah, IIRC, requests' use of urllib3 had some pretty verbose logging by default—and it uses its own urllib3 logger rather than the global logger—so this setLevel() call is just to suppress (most of) its logging.

  • why does test_cronitor.py (for example) pass this same test when the logger object is instantiated identically in both cronitor.py and ntfy.py?

So, about that. This one is pretty bad, but I was able to track it down. I ended up having to hack Python logging's addHandler() and raising when hdlr was None so I could see where a None handler was getting set. Turned out, it was in the tests for Healthchecks! Apparently those tests were (accidentally, implicitly) adding a None handler and then not cleaning it up, thereby impacting downstream tests. Your Ntfy tests were lucky enough to be after the Healthchecks tests, while the Cronitor tests came before both. I think the fix is to make the Healthchecks tests not change any global state—a pretty clear no-no in any testing.

I'll take on this task in borgmatic master, and let you know when that's fixed!

> * what is the purpose of the logging.getLogger('urllib3').setLevel() call? I don't see anywhere that a logger called urllib3 is either created or used (unless this happens transparently through Requests?) it is transparently used by Requests, and urllib3 as the underlying library - answered my own question there... > * why does the Cronitor hook change the log level of that logger, when the except block is going to output through the __name__ logger anyway? Yeah, IIRC, requests' use of urllib3 had some pretty verbose logging by default—and it uses its own urllib3 logger rather than the global logger—so this `setLevel()` call is just to suppress (most of) its logging. > * why does test_cronitor.py (for example) pass this same test when the logger object is instantiated identically in both cronitor.py and ntfy.py? So, about that. This one is pretty bad, but I was able to track it down. I ended up having to hack Python logging's `addHandler()` and raising when `hdlr` was None so I could see where a None handler was getting set. Turned out, it was in the tests for Healthchecks! Apparently those tests were (accidentally, implicitly) adding a None handler and then *not cleaning it up*, thereby impacting downstream tests. Your Ntfy tests were lucky enough to be *after* the Healthchecks tests, while the Cronitor tests came before both. I think the fix is to make the Healthchecks tests not change any global state—a pretty clear no-no in any testing. I'll take on this task in borgmatic master, and let you know when that's fixed!
Owner

Okay, this is fixed in master! Feel free to pull that changeset in. Sorry you had to discover this the hard way.

Okay, this is [fixed in master](https://projects.torsion.org/borgmatic-collective/borgmatic/commit/3561c93d74e7bafb3308c83788684761338775e3)! Feel free to pull that changeset in. Sorry you had to discover this the hard way.
witten reviewed 3 weeks ago
witten left a comment
Owner

Lookin' really good. As far as I'm concerned, this is ready to go once tests are passing!

Lookin' really good. As far as I'm concerned, this is ready to go once tests are passing!
run_states = hook_config.get('states', ['fail'])
if state.name.lower() in run_states:
witten commented 3 weeks ago
Owner

Might not need to .lower() this, since states in configuration is an enum? Meaning there's no way to make those values uppercase..

Similar below.

Might not need to `.lower()` this, since `states` in configuration is an `enum`? Meaning there's no way to make those values uppercase.. Similar below.
g-a-c commented 3 weeks ago
Poster

It is an enum, but my understanding (which appears to be backed up by my testing) is that when it's retrieved, because the enum is capitalised on the left hand side the .name is uppercase, and it feels clunky making the user fill out this state list in the config in uppercase when nothing else is?

It is an `enum`, but my understanding (which _appears_ to be backed up by my testing) is that when it's retrieved, because the `enum` is capitalised on the left hand side the `.name` is uppercase, and it feels clunky making the user fill out this state list in the config in uppercase when nothing else is?
witten commented 3 weeks ago
Owner

Right you are! My bad.. I didn't remember that the state passed into ping_monitor() didn't come from the configuration schema enum.

Right you are! My bad.. I didn't remember that the `state` passed into `ping_monitor()` didn't come from the configuration schema enum.
witten marked this conversation as resolved
state_config = hook_config.get(
state.name.lower(),
{
'title': 'A Borgmatic event happened',
witten commented 3 weeks ago
Owner

You could interpolate the state name into the title/message here. (Do not feel strongly.)

You *could* interpolate the state name into the title/message here. (Do not feel strongly.)
base_url = hook_config.get('server', 'https://ntfy.sh')
topic = hook_config.get('topic')
logger.info('{}: Pinging ntfy topic {}{}'.format(config_filename, topic, dry_run_label))
witten commented 3 weeks ago
Owner

You could use a Python f-string here and elsewhere. (Do not feel strongly.)

You could use a Python f-string here and elsewhere. (Do not feel strongly.)
g-a-c commented 3 weeks ago
Poster

Yeah, I actually wasn't aware of the f-string until I was poking around in other modules to try and work out that test issue, but I do find that cleaner to read so I think I'll refactor to use those.

Yeah, I actually wasn't aware of the f-string until I was poking around in other modules to try and work out that test issue, but I do find that cleaner to read so I think I'll refactor to use those.
fail:
title: A Borgmatic backup failed
message: You should probably fix it
tags: borgmatic,-1,skull
witten commented 3 weeks ago
Owner

Love it. (The skull.)

Love it. (The skull.)
def test_ping_monitor_minimal_config_does_not_hit_hosted_ntfy_on_start():
hook_config = {
'topic': topic,
}
witten commented 3 weeks ago
Owner

Could all be on one line. (Do not feel strongly.)

Similar elsewhere.

Could all be on one line. (Do not feel strongly.) Similar elsewhere.
g-a-c commented 3 weeks ago
Poster

I think one of the auto-formatters may have done this, as I don't remember doing it for these simple single-element dicts.

I'll clean it up and see if it comes back when black or flake8 get run.

I _think_ one of the auto-formatters may have done this, as I don't remember doing it for these simple single-element `dict`s. I'll clean it up and see if it comes back when `black` or `flake8` get run.
module.monitor.State.FAIL,
monitoring_log_level=1,
dry_run=False,
)
witten commented 3 weeks ago
Owner

These tests look great! I appreciate you taking the time to add them. IMO not every possible permutation needs testing as long as most of the code paths are hit. (Which they are here.)

These tests look great! I appreciate you taking the time to add them. IMO not every possible permutation needs testing as long as most of the code paths are hit. (Which they are here.)
Poster

Sorry you had to discover this the hard way.

💩 happens! Sounds like it was an experience for both of us.

That does make a ton of sense, because when I ran just my ntfy test to get it to go a little quicker, it passed in isolation. I guess if I'd done some more messing around I might have worked out that when I ran all of the hook tests together the problem would have come back and isolated it that way - but I don't think I'd have come any closer to tracking down the root cause myself

> Sorry you had to discover this the hard way. :poop: happens! Sounds like it was an experience for both of us. That does make a ton of sense, because when I ran **just** my `ntfy` test to get it to go a little quicker, it passed in isolation. I guess if I'd done some more messing around I might have worked out that when I ran all of the `hook` tests together the problem would have come back and isolated it that way - but I don't think I'd have come any closer to tracking down the root cause myself
g-a-c force-pushed feat/ntfy from e0be85cbcb to a8b8d507b6 3 weeks ago
Poster

I've squashed everything down to a single commit instead of my chopping and hacking around, 100% code coverage, black, isort and flake8 are all happy so I think this is ready for reviewing again...

I've squashed everything down to a single commit instead of my chopping and hacking around, 100% code coverage, `black`, `isort` and `flake8` are all happy so I think this is ready for reviewing again...
Owner

Awesome! Thanks again for doing this—and slogging through the tests. This will be part of the next release.

Awesome! Thanks again for doing this—and slogging through the tests. This will be part of the next release.
witten merged commit 7648bcff39 into master 3 weeks ago
g-a-c deleted branch feat/ntfy 3 weeks ago
Owner

Released in borgmatic 1.6.3!

Released in borgmatic 1.6.3!
The pull request has been merged as 7648bcff39.
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.