PoC: Use django-tasks for sending notifications
Scope and purpose
Might eventually close #166, #359, #400.
Use django-tasks to send notifications instead of a background process. Currently uses django signals to enqueue tasks on Event create. With the database backend we could use a trigger to add a task on database save of an event!
Has one task to check whether to send notifications and another to actually send the notification.
Test by for instance having a filter that triggers on argus as source 24/7 and sends an email. Turn notifications on (SEND_NOTIFIACTIONS=True in settings or use the environment variable SEND_NOTIFICATIONS=yes) and use a dummy email backend (EMAIL_BACKEND = "django.core.mail.backends.console.EmailBackend" in settings).
This pull request
- adds/changes/removes a dependency
Contributor Checklist
Every pull request should have this checklist filled out, no matter how small it is. More information about contributing to Argus can be found in the Development docs.
- [ ] Added a changelog fragment for towncrier
- [ ] Added/amended tests for new/changed code
- [ ] Added/changed documentation, including updates to the user manual if feature flow or UI is considerably changed
- [x] Linted/formatted the code with ruff and djLint, easiest by using pre-commit
- [x] The first line of the commit message continues the sentence "If applied, this commit will ...", starts with a capital letter, does not end with punctuation and is 50 characters or less long. See our how-to
- [ ] If applicable: Created new issues if this PR does not fix the issue completely/there is further work to be done
- ~[ ] If this results in changes in the UI: Added screenshots of the before and after~
- [ ] If this results in changes to the database model: Updated the ER diagram
Test results
4 files 196 suites 2m 13s ⏱️ 201 tests 167 ✅ 1 💤 0 ❌ 33 🔥 804 runs 668 ✅ 4 💤 0 ❌ 132 🔥
For more details on these errors, see this check.
Results for commit 0251848f.
:recycle: This comment has been updated with latest results.
So, in short, this appears to refactor the existing in-process notification handler by reframing it as a django-tasks task with the
ImmediateBackend.Lots of questions:
* I assume the end goal is to replace the `ImmediateBackend` with `DatabaseBackend` and a `db_worker` management command? (Or perhaps our own custom backend?)
Yes. We ought to document using DatabaseBackend.
* Why is it a good idea to split 'check for notifications' and 'send notifications' into separate tasks?
Because
- we will be adding more to do on the check-step (checking for planned maintenance), making it increasingly expensive, and I want to save the incident ASAP
- actually sending the notification in a separate step makes it "easy" to delay sending notifications, for instance for SMSes. We could have a queue with notifications that needs to be bunched up. We have issues for that already..
- it looks sooo clean...
* Have you figured out how to transition from `django-tasks` to Django 6?
Eight-ball says: too soon to say.
https://github.com/RealOrangeOne/django-tasks/discussions/204
* You mentioned something in IRL that I interpreted as "the db_worker uses database polling". This is why I became averse to the django-q2-based PoC. Could we add NOTIFY/LISTEN support to the db_worker or would we have to write our own? I do realize that a NOTIFY/LISTEN-based mechanism would be PostgreSQL specific - but then again, at what point are we not tied to PostgreSQL already?
I thought your main beef with django-q2 was that it used pickle? django-tasks slings around json-blobs, to the despair of people who very much prefer pickle.
https://github.com/RealOrangeOne/django-tasks/discussions/85
I don't see a problem with poll, it works very well if argus sees as much traffic as I fear it might in the future. How well would NOTIFY/SUBSCRIBE handle the mist spam, lots of incidents at the same second?
Nevertheless, the refactor so far looks quite neat and clean, and I'm glad to see we can eventually accomplish this without the need for a bunch of 3rd party libraries or components :)
https://www.enterprisedb.com/blog/listening-postgres-how-listen-and-notify-syntax-promote-high-availability-application-layer
There’s still a kind of flaw with relying on NOTIFY events from Postgres: they’re ephemeral. If we stopped the listener app for any reason, nobody would receive notifications at all. If we later started the app again, notifications would resume, but any missed notifications would be lost forever. There’s also a small stampede problem if we want multiple iterations of the notification app to run simultaneously. We understand that post notifications are hardly critical data, but we can do better.
Goes on to describe polling with "FOR UPDATE SKIP LOCKED", which is exactly what django-tasks' database backend does...
Quality Gate passed
Issues
0 New issues
0 Accepted issues
Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code