Merge pull request #11 from codecov/matt/add-webhook-redelivery-counters

add statsd counters: how many webhooks failed, and how many we requested redelivery for

tasks/github_app_webhooks_check.py

@@ -5,6 +5,7 @@
 
 from shared.celery_config import gh_app_webhook_check_task_name
 from shared.config import get_config
+from shared.metrics import metrics
 from shared.torngit import Github
 from shared.torngit.exceptions import (
     TorngitRateLimitError,
@@ -65,14 +66,24 @@ def event_filter(item: object) -> bool:
         return filter(event_filter, deliveries)
 
     def apply_filters_to_deliveries(self, deliveries: List[object]) -> List[object]:
-        filters_to_apply = [
-            self._apply_time_filter,
-            self._apply_status_filter,
-            self._apply_event_filter,
-        ]
-        for current_filter in filters_to_apply:
-            deliveries = current_filter(deliveries)
-        return list(deliveries)
+        deliveries = self._apply_time_filter(deliveries)
+        deliveries = self._apply_status_filter(deliveries)
+
+        # Filter objects are one-and-done iterables. We have to materialize it into a list
+        # if we want to take the length *and* continue using it.
+        # Since Python lists contain references to their contents, this shouldn't be too much
+        # more expensive than just iterating.
+        deliveries = list(deliveries)
+        metrics.incr("webhooks.github.deliveries.failed", count=len(deliveries))
+
+        # Same as above, we need to materialize our filter into a list so we can both take
+        # the length of it and return it.
+        deliveries = list(self._apply_event_filter(deliveries))
+        metrics.incr(
+            "webhooks.github.deliveries.retry_requested", count=len(deliveries)
+        )
+
+        return deliveries
 
     async def request_redeliveries(
         self, gh_handler: Github, deliveries_to_request: List[object]

tasks/tests/unit/test_github_app_weebhooks_check.py → tasks/tests/unit/test_github_app_webhooks_check.py

@@ -6,9 +6,33 @@
 from tasks.github_app_webhooks_check import Github, GitHubAppWebhooksCheckTask
 
 
+@pytest.fixture
+def mock_metrics(mocker):
+    from collections import defaultdict
+
+    metrics = defaultdict(int)
+
+    def incr(stat, count=1, rate=1):
+        metrics[stat] += count
+
+    def decr(stat, count=1, rate=1):
+        metrics[stat] -= count
+
+    mock_incr = mocker.patch("shared.metrics.metrics.incr")
+    mock_incr.side_effect = incr
+
+    mock_decr = mocker.patch("shared.metrics.metrics.decr")
+    mock_decr.side_effect = decr
+
+    yield metrics
+
+
 @pytest.fixture
 def sample_deliveries():
     sample_deliveries = [
+        # time filter: passes, because the `delivered_at` is updated below to be recent
+        # status filter: fails, because this was a successful delivery
+        # event filter: passes, because it's an installation event
         {
             "id": 17324040107,
             "guid": "53c93580-7a6e-11ed-96c9-5e1ce3e5574e",
@@ -23,6 +47,9 @@ def sample_deliveries():
             "repository_id": None,
             "url": "",
         },
+        # time filter: fails, because the `delivered_at` is old and not updated below
+        # status filter: fails, because this was a successful delivery
+        # event filter: passes, because it's an installation event
         {
             "id": 17324018336,
             "guid": "40d7f830-7a6e-11ed-8b90-0777e88b1858",
@@ -37,6 +64,9 @@ def sample_deliveries():
             "repository_id": None,
             "url": "",
         },
+        # time filter: passes, because the `delivered_at` is updated below to be recent
+        # status filter: passes, because this was a failed delivery
+        # event filter: passes, because it's an installation even
         {
             "id": 17323292984,
             "guid": "0498e8e0-7a6c-11ed-8834-c5eb5a4b102a",
@@ -51,6 +81,9 @@ def sample_deliveries():
             "repository_id": None,
             "url": "",
         },
+        # time filter: fails, because the `delivered_at` is old and not updated below
+        # status filter: passes, because this was a failed delivery
+        # event filter: passes, because it's an installation even
         {
             "id": 17323228732,
             "guid": "d41fa780-7a6b-11ed-8890-0619085a3f97",
@@ -65,6 +98,9 @@ def sample_deliveries():
             "repository_id": None,
             "url": "",
         },
+        # time filter: passes, because the `delivered_at` is updated below to be recent
+        # status filter: passes, because this was a failed delivery
+        # event filter: fails, because it isn't an installation event
         {
             "id": 17323228732,
             "guid": "d41fa780-7a6b-11ed-8890-0619085a3f97",
@@ -79,6 +115,9 @@ def sample_deliveries():
             "repository_id": None,
             "url": "",
         },
+        # time filter: fails, because the `delivered_at` is old and not updated below
+        # status filter: fails, because this was a successful delivery
+        # event filter: fails, because it isn't an installation event
         {
             "id": 17323228732,
             "guid": "d41fa780-7a6b-11ed-8890-0619085a3f97",
@@ -151,7 +190,6 @@ def test_apply_status_filter(self, sample_deliveries):
         assert filtered_deliveries == sample_deliveries[2:5]
 
     def test_apply_filters_to_deliveries(self, sample_deliveries):
-
         task = GitHubAppWebhooksCheckTask()
         filtered_deliveries = list(task.apply_filters_to_deliveries(sample_deliveries))
         assert len(filtered_deliveries) == 1
@@ -250,3 +288,34 @@ async def side_effect(*args, **kwargs):
         fake_list_deliveries.assert_called()
         fake_get_token.assert_called()
         fake_redelivery.assert_called()
+
+    @pytest.mark.asyncio
+    async def test_redelivery_counters(
+        self, dbsession, mocker, sample_deliveries, mock_metrics
+    ):
+        async def side_effect(*args, **kwargs):
+            yield sample_deliveries
+
+        fake_list_deliveries = mocker.patch.object(
+            Github,
+            "list_webhook_deliveries",
+            side_effect=side_effect,
+        )
+        fake_get_token = mocker.patch(
+            "tasks.github_app_webhooks_check.get_github_integration_token",
+            return_value="integration_jwt_token",
+        )
+        fake_redelivery = mocker.patch.object(
+            Github,
+            "request_webhook_redelivery",
+            return_value=True,
+        )
+        task = GitHubAppWebhooksCheckTask()
+        _ = await task.run_cron_task(dbsession)
+
+        # There are 3 failed deliveries above, but 1 of them is old and excluded from the query
+        assert mock_metrics["webhooks.github.deliveries.failed"] == 2
+
+        # There are 2 failed deliveries that pass the check above, but one isn't for an installation
+        # event so we don't ask for a redelivery
+        assert mock_metrics["webhooks.github.deliveries.retry_requested"] == 1