Debug notification sending

changelog.d/+debug-notification-sending-1.changed.md

@@ -0,0 +1,2 @@
+To improve debugability: Change how sending notifications are logged so that
+there's a log both when sending succeds and when it fails.

changelog.d/+debug-notification-sending-2.changed.md

@@ -0,0 +1 @@
+Return False and log if sms-to-email has trouble with the email server.

src/argus/notificationprofile/media/__init__.py

@@ -60,6 +60,8 @@ def send_notification(destinations: Iterable[DestinationConfig], *events: Iterab
             sent = medium.send(event=event, destinations=destinations)
             if sent:
                 LOG.info('Notification: sent event "%s" to "%s"', event, medium.MEDIA_SLUG)
+            else:
+                LOG.warn('Notification: could not send event "%s" to "%s"', event, medium.MEDIA_SLUG)
 
 
 def background_send_notification(destinations: Iterable[DestinationConfig], *events: Event):

src/argus/notificationprofile/media/base.py

@@ -12,7 +12,7 @@
         from collections.abc import Iterable
 
     from types import NoneType
-    from typing import List, Union
+    from typing import Union, Set
     from django.db.models.query import QuerySet
     from argus.auth.models import User
     from argus.incident.models import Event
@@ -57,8 +57,8 @@ def get_label(destination: DestinationConfig) -> str:
         pass
 
     @classmethod
-    def get_relevant_addresses(cls, destinations: Iterable[DestinationConfig]) -> List[DestinationConfig]:
-        """Returns a list of addresses the message should be sent to"""
+    def get_relevant_addresses(cls, destinations: Iterable[DestinationConfig]) -> Set[DestinationConfig]:
+        """Returns a set of addresses the message should be sent to"""
         pass
 
     @classmethod

src/argus/notificationprofile/media/email.py

@@ -22,7 +22,7 @@
         from collections.abc import Iterable
 
     from types import NoneType
-    from typing import List, Union
+    from typing import Union, Set
     from django.db.models.query import QuerySet
     from argus.auth.models import User
     from ..serializers import RequestDestinationConfigSerializer
@@ -41,7 +41,7 @@ def modelinstance_to_dict(obj):
     return dict_
 
 
-def send_email_safely(function, additional_error=None, *args, **kwargs):
+def send_email_safely(function, additional_error=None, *args, **kwargs) -> int:
     try:
         result = function(*args, **kwargs)
         return result
@@ -143,15 +143,15 @@ def has_duplicate(cls, queryset: QuerySet, settings: dict) -> bool:
         return queryset.filter(settings__email_address=settings["email_address"]).exists()
 
     @classmethod
-    def get_relevant_addresses(cls, destinations: Iterable[DestinationConfig]) -> List[DestinationConfig]:
+    def get_relevant_addresses(cls, destinations: Iterable[DestinationConfig]) -> Set[DestinationConfig]:
         """Returns a list of email addresses the message should be sent to"""
         email_addresses = [
             destination.settings["email_address"]
             for destination in destinations
             if destination.media_id == cls.MEDIA_SLUG
         ]
 
-        return email_addresses
+        return set(email_addresses)
 
     @staticmethod
     def create_message_context(event: Event):
@@ -183,17 +183,31 @@ def send(cls, event: Event, destinations: Iterable[DestinationConfig], **_) -> b
         email_addresses = cls.get_relevant_addresses(destinations=destinations)
         if not email_addresses:
             return False
+        num_emails = len(email_addresses)
 
         subject, message, html_message = cls.create_message_context(event=event)
 
+        failed = set()
         for email_address in email_addresses:
-            send_email_safely(
+            sent = send_email_safely(
                 send_mail,
                 subject=subject,
                 message=message,
                 from_email=None,
                 recipient_list=[email_address],
                 html_message=html_message,
             )
-
+            if not sent:  # 0 for failure otherwise 1
+                failed.add(email_address)
+
+        if failed:
+            if num_emails == len(failed):
+                LOG.error("Email: Failed to send to any addresses")
+                return False
+            LOG.warn(
+                "Email: Failed to send to %i of %i addresses",
+                len(failed),
+                num_emails,
+            )
+            LOG.debug("Email: Failed to send to:", " ".join(failed))
         return True

src/argus/notificationprofile/media/sms_as_email.py

@@ -27,7 +27,7 @@
     else:
         from collections.abc import Iterable
 
-    from typing import List, Union
+    from typing import Union, Set
     from types import NoneType
     from django.db.models.query import QuerySet
     from argus.auth.models import User
@@ -90,15 +90,15 @@ def has_duplicate(cls, queryset: QuerySet, settings: dict) -> bool:
         return queryset.filter(settings__phone_number=settings["phone_number"]).exists()
 
     @classmethod
-    def get_relevant_addresses(cls, destinations: Iterable[DestinationConfig]) -> List[DestinationConfig]:
+    def get_relevant_addresses(cls, destinations: Iterable[DestinationConfig]) -> Set[DestinationConfig]:
         """Returns a list of phone numbers the message should be sent to"""
         phone_numbers = [
             destination.settings["phone_number"]
             for destination in destinations
             if destination.media_id == cls.MEDIA_SLUG
         ]
 
-        return phone_numbers
+        return set(phone_numbers)
 
     @classmethod
     def send(cls, event: Event, destinations: Iterable[DestinationConfig], **_) -> bool:
@@ -113,17 +113,22 @@ def send(cls, event: Event, destinations: Iterable[DestinationConfig], **_) -> b
             return
 
         phone_numbers = cls.get_relevant_addresses(destinations=destinations)
-
         if not phone_numbers:
             return False
 
+        # there is only one recipient, so failing to send a single message
+        # means something is wrong on the email server
+        sent = True
         for phone_number in phone_numbers:
-            send_email_safely(
+            sent = send_email_safely(
                 send_mail,
                 subject=f"sms {phone_number}",
                 message=f"{event.description}",
                 from_email=None,
                 recipient_list=[recipient],
             )
+            if not sent:
+                LOG.error("SMS: Failed to send")
+                break
 
-        return True
+        return sent

src/argus/notificationprofile/models.py

@@ -102,7 +102,7 @@ class FilterWrapper:
 
     def __init__(self, filterblob):
         self.fallback_filter = getattr(settings, "ARGUS_FALLBACK_FILTER", {})
-        self.filter = filterblob
+        self.filter = filterblob.copy()
 
     def _get_tristate(self, tristate):
         fallback_filter = self.fallback_filter.get(tristate, None)
@@ -209,15 +209,15 @@ def filtered_incidents(self):
 
     def incidents_with_source_systems(self, data=None):
         if not data:
-            data = self.filter
+            data = self.filter.copy()
         source_list = data.pop("sourceSystemIds", [])
         if source_list:
             return self.all_incidents.filter(source__in=source_list).distinct()
         return self.all_incidents.distinct()
 
     def source_system_fits(self, incident: Incident, data=None):
         if not data:
-            data = self.filter
+            data = self.filter.copy()
         source_list = data.pop("sourceSystemIds", [])
         if not source_list:
             # We're not limiting on sources!
@@ -226,15 +226,15 @@ def source_system_fits(self, incident: Incident, data=None):
 
     def incidents_with_tags(self, data=None):
         if not data:
-            data = self.filter
+            data = self.filter.copy()
         tags_list = data.pop("tags", [])
         if tags_list:
             return self.all_incidents.from_tags(*tags_list)
         return self.all_incidents.distinct()
 
     def tags_fit(self, incident: Incident, data=None):
         if not data:
-            data = self.filter
+            data = self.filter.copy()
         tags_list = data.pop("tags", [])
         if not tags_list:
             # We're not limiting on tags!
@@ -247,7 +247,7 @@ def incidents_fitting_tristates(
         data=None,
     ):
         if not data:
-            data = self.filter
+            data = self.filter.copy()
         fitting_incidents = self.all_incidents
         filter_open = data.pop("open", None)
         filter_acked = data.pop("acked", None)
@@ -269,7 +269,7 @@ def incidents_fitting_tristates(
 
     def incidents_fitting_maxlevel(self, data=None):
         if not data:
-            data = self.filter
+            data = self.filter.copy()
         maxlevel = data.pop("maxlevel", None)
         if not maxlevel:
             return self.all_incidents.distinct()
@@ -278,7 +278,7 @@ def incidents_fitting_maxlevel(self, data=None):
     def incident_fits(self, incident: Incident):
         if self.is_empty:
             return False  # Filter is empty!
-        data = self.filter
+        data = self.filter.copy()
         checks = {}
         checks["source"] = self.source_system_fits(incident, data)
         checks["tags"] = self.tags_fit(incident, data)
@@ -288,7 +288,9 @@ def incident_fits(self, incident: Incident):
         checks["max_level"] = self.filter_wrapper.incident_fits_maxlevel(incident)
         any_failed = False in checks.values()
         if any_failed:
-            LOG.debug("Filter: at least one incident check failed: %r", checks)
+            LOG.debug("Filter: %s: MISS! checks: %r", self, checks)
+        else:
+            LOG.debug("Filter: %s: HIT!", self)
         return not any_failed
 
     def event_fits(self, event: Event):