Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use django-q2 for notifications #630

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Conversation

hmpf
Copy link
Contributor

@hmpf hmpf commented Apr 25, 2023

Closes #400

Note that django-q2 won't even install on python 3.7.

Dependencies

There's a new one, so remember to install django-q2 in the venv.

Database

Depending on django_q2 adds new database tables, so migrations must be run:

python manage.py migrate

Redis

It currently uses redis, the same redis that is configured for websockets, so set the environment-variable ARGUS_REDIS_SERVER. This is used to make the redis-settings in src/argus/site/settings/base.py, setting Q_CLUSTER. Override in local settings if necessary.

Running django-q2

Start the brokers with

python manage.py qcluster

It does not run in the background and can be turned off with Ctrl-C. Note: if any changes are made to the code, qcluster must be restarted, it does not automatically discover changes.

Check if a task worked or failed in Django Admin, the "Django Q2 Queue"-section.

Test sending notifications

Test queuing notifications by

  1. Disabling profiles sending to non-email destinations
  2. Ensuring that EMAIL_BACKEND is 'django.core.mail.backends.console.EmailBackend', this pumps the contents of the email to the console/sys.stdout where qcluster is running.
  3. python manage.py create_fake_incident

In the terminal where you do step 3, the last log-line will contain something like:
"Notification: will be sending notification for "'Incident start':..".

In the terminal where qcluster is running, you'll get a log line starting with "Notification: sending event "'Incident start':..". The header and body will be dumped as plaintext after this line. After the email there will be a log line starting with "Notification: sent event "'Incident start':..".

Docker

The postgres image has been updated to version 13, this means the volume should be deleted and remade. Clean everything away with:

docker compose down --volumes

@github-actions
Copy link

github-actions bot commented Apr 25, 2023

Test results

       7 files     574 suites   21m 39s ⏱️
   462 tests    461 ✔️ 1 💤 0
3 234 runs  3 227 ✔️ 7 💤 0

Results for commit 4fe2960.

♻️ This comment has been updated with latest results.

@hmpf hmpf requested review from lunkwill42, johannaengland and stveit and removed request for lunkwill42 April 25, 2023 07:24
@hmpf
Copy link
Contributor Author

hmpf commented Apr 25, 2023

Missing:

  • docs (how to)
  • work with bulk-fix
  • update docker this-and-that

@sonarcloud
Copy link

sonarcloud bot commented Apr 27, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@hmpf hmpf force-pushed the django-q2 branch 2 times, most recently from bbd742a to 2b41824 Compare September 20, 2023 06:33
@codecov-commenter
Copy link

codecov-commenter commented Sep 20, 2023

Codecov Report

Attention: Patch coverage is 73.07692% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 84.70%. Comparing base (6488afa) to head (4fe2960).

Files Patch % Lines
src/argus/incident/signals.py 56.25% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #630      +/-   ##
==========================================
+ Coverage   84.61%   84.70%   +0.09%     
==========================================
  Files          75       75              
  Lines        3750     3754       +4     
==========================================
+ Hits         3173     3180       +7     
+ Misses        577      574       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@lunkwill42 lunkwill42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like you're on the right track. I got this to work with only slight modifications.

I added an extra qcluster service to docker-compose.yml, which basically is a copy of api, but which executes the manage.py qcluster command rather than daphne, and can get it to dispatch notifications according to my profile when I inject fake ones.

src/argus/site/settings/base.py Outdated Show resolved Hide resolved
@lunkwill42
Copy link
Member

I added an extra qcluster service to docker-compose.yml, which basically is a copy of api, but which executes the manage.py qcluster command rather than daphne, and can get it to dispatch notifications according to my profile when I inject fake ones.

i.e. it's usable, but I don't like the redundant environment definitions in docker-compose.yml - I'm sure there some way we can template those out, though.

@hmpf
Copy link
Contributor Author

hmpf commented Sep 21, 2023

I've looked at bulk-changes (Incident.objects.create_events()).

Should we make one notification per change (easy, by putting the new events on the queue one at a time) or should we make a single notification for a "bulk-change"-event? The problem with the latter is matching it with filters: a filter might hit one of the events in the bulk, but not all of them.

@lunkwill42
Copy link
Member

I've looked at bulk-changes (Incident.objects.create_events()).

Should we make one notification per change (easy, by putting the new events on the queue one at a time) or should we make a single notification for a "bulk-change"-event? The problem with the latter is matching it with filters: a filter might hit one of the events in the bulk, but not all of them.

I like the easy and less complicated route of one queued job per notification. It shouldn't become a bottleneck, but if it does, we can work it out later.

@sonarcloud
Copy link

sonarcloud bot commented Sep 25, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@hmpf hmpf self-assigned this Jan 29, 2024
@hmpf hmpf force-pushed the django-q2 branch 4 times, most recently from eb7fa00 to 9a0fd3c Compare February 16, 2024 07:19
Copy link

sonarcloud bot commented Feb 16, 2024

Quality Gate Passed Quality Gate passed

Issues
3 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@hmpf hmpf force-pushed the django-q2 branch 4 times, most recently from 4206b6a to f59ae53 Compare April 16, 2024 09:04
hmpf and others added 5 commits April 23, 2024 13:10
This sets up the API web server and the Django Q2 qcluster command as
separate Docker Compose services, based on the same image and config.

The only difference between the two containers is which command they
run.  The image defaults to the API entrypoint, while docker-compose.yml
will override the command of the qcluster service.

In an attempt to make it DRY, the two nearly-identical services are
templated out using YAML anchors and X properties, as suggested by the
Docker Compose docs.
Copy link

sonarcloud bot commented Apr 23, 2024

Quality Gate Passed Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@hmpf hmpf added the paused Assignee is busy with things of higher priority label Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
paused Assignee is busy with things of higher priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Try using django-q for sending notifications in the background
3 participants