From 59427aa6cac4e6e98202f172111992b4cc33305f Mon Sep 17 00:00:00 2001 From: Hanne Moa Date: Fri, 23 Feb 2024 13:05:01 +0100 Subject: [PATCH 1/6] Improve logging when checking notification filters --- src/argus/notificationprofile/models.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/argus/notificationprofile/models.py b/src/argus/notificationprofile/models.py index 08f07aac9..0a163b57e 100644 --- a/src/argus/notificationprofile/models.py +++ b/src/argus/notificationprofile/models.py @@ -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): From 14447b72f9fca2671afbae2087ed6074d241b2d4 Mon Sep 17 00:00:00 2001 From: Hanne Moa Date: Fri, 23 Feb 2024 14:58:19 +0100 Subject: [PATCH 2/6] Log when notification could not be sent --- src/argus/notificationprofile/media/__init__.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/argus/notificationprofile/media/__init__.py b/src/argus/notificationprofile/media/__init__.py index b3e99602f..7ad4ba6ce 100644 --- a/src/argus/notificationprofile/media/__init__.py +++ b/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): From 1f22a169b378c302c4ccb891ea40155c36ae5624 Mon Sep 17 00:00:00 2001 From: Hanne Moa Date: Fri, 23 Feb 2024 15:10:14 +0100 Subject: [PATCH 3/6] Avoid mutating filter when checking it --- src/argus/notificationprofile/models.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/argus/notificationprofile/models.py b/src/argus/notificationprofile/models.py index 0a163b57e..6994a720c 100644 --- a/src/argus/notificationprofile/models.py +++ b/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,7 +209,7 @@ 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() @@ -217,7 +217,7 @@ def incidents_with_source_systems(self, data=None): 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,7 +226,7 @@ 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) @@ -234,7 +234,7 @@ def incidents_with_tags(self, data=None): 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) From a5c8c709cc8151964a87839aa27f98e588a5bf09 Mon Sep 17 00:00:00 2001 From: Hanne Moa Date: Fri, 23 Feb 2024 15:14:01 +0100 Subject: [PATCH 4/6] Log when not all emails could be sent --- src/argus/notificationprofile/media/email.py | 26 ++++++++++++++----- .../notificationprofile/media/sms_as_email.py | 11 +++++--- 2 files changed, 28 insertions(+), 9 deletions(-) diff --git a/src/argus/notificationprofile/media/email.py b/src/argus/notificationprofile/media/email.py index f6d6f7ad9..30a6ef902 100644 --- a/src/argus/notificationprofile/media/email.py +++ b/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,7 +143,7 @@ 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"] @@ -151,7 +151,7 @@ def get_relevant_addresses(cls, destinations: Iterable[DestinationConfig]) -> Li if destination.media_id == cls.MEDIA_SLUG ] - return email_addresses + return set(email_addresses) @staticmethod def create_message_context(event: Event): @@ -183,11 +183,13 @@ 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, @@ -195,5 +197,17 @@ def send(cls, event: Event, destinations: Iterable[DestinationConfig], **_) -> b 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 diff --git a/src/argus/notificationprofile/media/sms_as_email.py b/src/argus/notificationprofile/media/sms_as_email.py index c880f3c42..6dc27ea76 100644 --- a/src/argus/notificationprofile/media/sms_as_email.py +++ b/src/argus/notificationprofile/media/sms_as_email.py @@ -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 From e754b06c774ec154bb99058e715e983ee456b4eb Mon Sep 17 00:00:00 2001 From: Hanne Moa Date: Thu, 18 Apr 2024 12:51:23 +0200 Subject: [PATCH 5/6] Always return a set from Medium.get_relevant_addresses --- src/argus/notificationprofile/media/base.py | 6 +++--- src/argus/notificationprofile/media/sms_as_email.py | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/argus/notificationprofile/media/base.py b/src/argus/notificationprofile/media/base.py index 235e4f0b5..d5c330e7a 100644 --- a/src/argus/notificationprofile/media/base.py +++ b/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 diff --git a/src/argus/notificationprofile/media/sms_as_email.py b/src/argus/notificationprofile/media/sms_as_email.py index 6dc27ea76..6f945229c 100644 --- a/src/argus/notificationprofile/media/sms_as_email.py +++ b/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,7 +90,7 @@ 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"] @@ -98,7 +98,7 @@ def get_relevant_addresses(cls, destinations: Iterable[DestinationConfig]) -> Li if destination.media_id == cls.MEDIA_SLUG ] - return phone_numbers + return set(phone_numbers) @classmethod def send(cls, event: Event, destinations: Iterable[DestinationConfig], **_) -> bool: From 582e82c71051f4788f73e4c5fa7d525bba3d46e0 Mon Sep 17 00:00:00 2001 From: Hanne Moa Date: Thu, 18 Apr 2024 12:52:25 +0200 Subject: [PATCH 6/6] Add changelogs --- changelog.d/+debug-notification-sending-1.changed.md | 2 ++ changelog.d/+debug-notification-sending-2.changed.md | 1 + 2 files changed, 3 insertions(+) create mode 100644 changelog.d/+debug-notification-sending-1.changed.md create mode 100644 changelog.d/+debug-notification-sending-2.changed.md diff --git a/changelog.d/+debug-notification-sending-1.changed.md b/changelog.d/+debug-notification-sending-1.changed.md new file mode 100644 index 000000000..17a954c4c --- /dev/null +++ b/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. diff --git a/changelog.d/+debug-notification-sending-2.changed.md b/changelog.d/+debug-notification-sending-2.changed.md new file mode 100644 index 000000000..e0bc8c26c --- /dev/null +++ b/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.