Add tracing and improve typing of NotificationService

Adds tracing to the various `notify` implementations, and improves their typing.
In particular, it fixes mismatches between the base and derived classes, and fixes up type annotations to match their usage.

helpers/notifier.py

@@ -72,9 +72,7 @@ def send_to_provider(self, pull, message):
     def build_message(self) -> str:
         raise NotImplementedError
 
-    def notify(
-        self,
-    ) -> NotifierResult:
+    def notify(self) -> NotifierResult:
         pull = self.get_pull()
         if pull is None:
             log.info(

services/comparison/changes.py

@@ -3,6 +3,7 @@
 from collections import defaultdict
 from typing import Any, Dict, Iterator, List, Tuple, Union
 
+import sentry_sdk
 from shared.helpers.numeric import ratio
 from shared.reports.resources import Report
 from shared.reports.types import Change, ReportTotals
@@ -80,6 +81,7 @@ def get_segment_offsets(segments) -> Tuple[Dict[int, Any], List[int]]:
     return dict([(k, v) for k, v in offsets.items() if v != 0]), additions, removals
 
 
+@sentry_sdk.trace
 def get_changes(
     base_report: Report, head_report: Report, diff_json: dict[str, Any] | None
 ) -> list[Change] | None:

services/notification/__init__.py

@@ -105,7 +105,7 @@ def _use_status_and_possibly_checks_notifiers(
         key: StatusType,
         title: str,
         status_config: dict,
-    ) -> AbstractBaseNotifier | StatusNotifier:
+    ) -> AbstractBaseNotifier:
         status_notifier_class = get_status_notifier_class(key, "status")
         if self._should_use_checks_notifier(status_type=key):
             checks_notifier = get_status_notifier_class(key, "checks")
@@ -249,14 +249,10 @@ def notify(self, comparison: ComparisonProxy) -> list[NotificationResult]:
                 repoid=comparison.head.commit.repoid,
             ),
         )
-        notification_instances = [
-            notifier
-            for notifier in self.get_notifiers_instances()
-            if notifier.is_enabled()
-        ]
         results = [
             self.notify_individual_notifier(notifier, comparison)
-            for notifier in notification_instances
+            for notifier in self.get_notifiers_instances()
+            if notifier.is_enabled()
         ]
         return results
 

services/notification/notifiers/base.py

@@ -4,7 +4,7 @@
 
 from database.models import Repository
 from database.models.core import GITHUB_APP_INSTALLATION_DEFAULT_NAME
-from services.comparison.types import Comparison
+from services.comparison import ComparisonProxy
 from services.decoration import Decoration
 
 log = logging.getLogger(__name__)
@@ -78,13 +78,13 @@ def __init__(
     def name(self) -> str:
         raise NotImplementedError()
 
-    def notify(self, comparison: Comparison, **extra_data) -> NotificationResult:
+    def notify(self, comparison: ComparisonProxy) -> NotificationResult:
         raise NotImplementedError()
 
     def is_enabled(self) -> bool:
         raise NotImplementedError()
 
-    def store_results(self, comparison: Comparison, result: NotificationResult):
+    def store_results(self, comparison: ComparisonProxy, result: NotificationResult):
         """
             This function stores the result in the notification wherever it needs to be saved
             This is the only function in this class allowed to have side-effects in the database

services/notification/notifiers/checks/base.py

@@ -2,9 +2,11 @@
 from contextlib import nullcontext
 from typing import Dict
 
+import sentry_sdk
 from asgiref.sync import async_to_sync
 from shared.torngit.exceptions import TorngitClientError, TorngitError
 
+from services.comparison import ComparisonProxy
 from services.notification.notifiers.base import Comparison, NotificationResult
 from services.notification.notifiers.status.base import StatusNotifier
 from services.urls import (
@@ -28,7 +30,7 @@ def __init__(self, *args, **kwargs) -> None:
     def is_enabled(self) -> bool:
         return True
 
-    def store_results(self, comparison: Comparison, result: NotificationResult) -> bool:
+    def store_results(self, comparison: ComparisonProxy, result: NotificationResult):
         pass
 
     @property
@@ -71,7 +73,8 @@ def get_status_external_name(self) -> str:
         status_piece = f"/{self.title}" if self.title != "default" else ""
         return f"codecov/{self.context}{status_piece}"
 
-    def notify(self, comparison: Comparison):
+    @sentry_sdk.trace
+    def notify(self, comparison: ComparisonProxy) -> NotificationResult:
         if comparison.pull is None or ():
             log.debug(
                 "Falling back to commit_status: Not a pull request",

services/notification/notifiers/checks/checks_with_fallback.py

@@ -2,7 +2,11 @@
 
 from shared.torngit.exceptions import TorngitClientError
 
-from services.notification.notifiers.base import AbstractBaseNotifier
+from services.comparison import ComparisonProxy
+from services.notification.notifiers.base import (
+    AbstractBaseNotifier,
+    NotificationResult,
+)
 
 log = logging.getLogger(__name__)
 
@@ -15,7 +19,11 @@ class ChecksWithFallback(AbstractBaseNotifier):
     Note: This class is not meant to store results.
     """
 
-    def __init__(self, checks_notifier, status_notifier):
+    def __init__(
+        self,
+        checks_notifier: AbstractBaseNotifier,
+        status_notifier: AbstractBaseNotifier,
+    ):
         self._checks_notifier = checks_notifier
         self._status_notifier = status_notifier
         self._decoration_type = checks_notifier.decoration_type
@@ -42,10 +50,10 @@ def notification_type(self):
     def decoration_type(self):
         return self._decoration_type
 
-    def store_results(self, comparison, res):
+    def store_results(self, comparison: ComparisonProxy, result: NotificationResult):
         pass
 
-    def notify(self, comparison):
+    def notify(self, comparison: ComparisonProxy) -> NotificationResult:
         try:
             res = self._checks_notifier.notify(comparison)
             if not res.notification_successful and (

services/notification/notifiers/codecov_slack_app.py

@@ -6,6 +6,7 @@
 
 from database.enums import Notification
 from database.models import Commit
+from services.comparison import ComparisonProxy
 from services.notification.notifiers.base import (
     AbstractBaseNotifier,
     NotificationResult,
@@ -37,7 +38,7 @@ def is_enabled(self) -> bool:
         elif isinstance(self.notifier_yaml_settings, bool):
             return self.notifier_yaml_settings
 
-    def store_results(self, comparison: Comparison, result: NotificationResult):
+    def store_results(self, comparison: ComparisonProxy, result: NotificationResult):
         pass
 
     def serialize_commit(self, commit: Commit):
@@ -101,7 +102,7 @@ def build_payload(self, comparison: Comparison):
             "head_totals_c": str(comparison.head.report.totals.coverage),
         }
 
-    def notify(self, comparison: Comparison, **extra_data) -> NotificationResult:
+    def notify(self, comparison: ComparisonProxy) -> NotificationResult:
         request_url = f"{CODECOV_SLACK_APP_URL}/notify"
 
         headers = {

services/notification/notifiers/comment/__init__.py

@@ -56,7 +56,7 @@ def repository_service(self):
             )
         return self._repository_service
 
-    def store_results(self, comparison: Comparison, result: NotificationResult):
+    def store_results(self, comparison: ComparisonProxy, result: NotificationResult):
         pull = comparison.pull
         if not result.notification_attempted or not result.notification_successful:
             return
@@ -78,7 +78,8 @@ def notification_type(self) -> Notification:
     def get_diff(self, comparison: Comparison):
         return comparison.get_diff()
 
-    def notify(self, comparison: ComparisonProxy, **extra_data) -> NotificationResult:
+    @sentry_sdk.trace
+    def notify(self, comparison: ComparisonProxy) -> NotificationResult:
         # TODO: remove this when we don't need it anymore
         # this line is measuring how often we try to comment on a PR that is closed
         if comparison.pull is not None and comparison.pull.state != "open":

services/notification/notifiers/generics.py

@@ -1,15 +1,16 @@
 import json
 import logging
-from contextlib import nullcontext
 from decimal import Decimal
 from typing import Any, Mapping
 from urllib.parse import urlparse
 
 import httpx
+import sentry_sdk
 from shared.config import get_config
 
 from helpers.match import match
 from helpers.metrics import metrics
+from services.comparison import ComparisonProxy
 from services.comparison.types import Comparison
 from services.notification.notifiers.base import (
     AbstractBaseNotifier,
@@ -45,7 +46,7 @@ def repository_service(self):
             )
         return self._repository_service
 
-    def store_results(self, comparison: Comparison, result: NotificationResult) -> bool:
+    def store_results(self, comparison: ComparisonProxy, result: NotificationResult):
         pass
 
     @property
@@ -90,21 +91,20 @@ def should_notify_comparison(self, comparison: Comparison) -> bool:
             return False
         return True
 
-    def notify(self, comparison: Comparison, **extra_data) -> NotificationResult:
+    @sentry_sdk.trace
+    def notify(self, comparison: ComparisonProxy) -> NotificationResult:
         filtered_comparison = comparison.get_filtered_comparison(
             **self.get_notifier_filters()
         )
-        with nullcontext():
-            with nullcontext():
-                if self.should_notify_comparison(filtered_comparison):
-                    result = self.do_notify(filtered_comparison, **extra_data)
-                else:
-                    result = NotificationResult(
-                        notification_attempted=False,
-                        notification_successful=None,
-                        explanation="Did not fit criteria",
-                        data_sent=None,
-                    )
+        if self.should_notify_comparison(filtered_comparison):
+            result = self.do_notify(filtered_comparison)
+        else:
+            result = NotificationResult(
+                notification_attempted=False,
+                notification_successful=None,
+                explanation="Did not fit criteria",
+                data_sent=None,
+            )
         return result
 
     def get_notifier_filters(self) -> dict:

services/notification/notifiers/status/base.py

@@ -1,6 +1,7 @@
 import logging
 from typing import Dict
 
+import sentry_sdk
 from asgiref.sync import async_to_sync
 from shared.config import get_config
 from shared.helpers.cache import NO_VALUE, make_hash_sha256
@@ -32,7 +33,7 @@ def __init__(self, *args, **kwargs) -> None:
     def is_enabled(self) -> bool:
         return True
 
-    def store_results(self, comparison: Comparison, result: NotificationResult) -> bool:
+    def store_results(self, comparison: ComparisonProxy, result: NotificationResult):
         pass
 
     @property
@@ -158,7 +159,8 @@ def get_github_app_used(self) -> int | None:
         )
         return selected_installation_id
 
-    def notify(self, comparison: ComparisonProxy):
+    @sentry_sdk.trace
+    def notify(self, comparison: ComparisonProxy) -> NotificationResult:
         payload = None
         if not self.can_we_set_this_status(comparison):
             return NotificationResult(