From 0b685e8b5ab625a9fc77d60f83cc9b7566b886fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C4=93teris=20Caune?= Date: Fri, 30 Oct 2020 12:36:17 +0200 Subject: [PATCH] Disable retries when testing webhook integration Normally, when a webhook call fails (timeout, connection error, non-2xx response), the HTTP request is retried up to two times (so up to 3 times total). This is useful when sending actual notifications, in case the webhook target has a temporary glitch. When interactively testing a webhook integration ("Send Test Notification" in the "Integrations" page), we would prefer to see any errors ASAP on the screen instead of retrying and so possibly swallowing them. One specific use case is webhook targets that take long time to generate a response. "Send Test Notification" is synchronous, meaning that the user could be stuck for 5 x 3 = 15 seconds waiting for the test HTTP request to time out three times. --- CHANGELOG.md | 1 + hc/api/models.py | 3 ++- hc/api/tests/test_notify.py | 27 +++++++++++++++++++++++++++ hc/api/transports.py | 28 +++++++++++++++------------- 4 files changed, 45 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index feac5dfe..125a54d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ All notable changes to this project will be documented in this file. - Reduce the number of SQL queries used in the "Get Checks" API call - Add support for script's exit status in ping URLs (#429) - Improve phone number sanitization: remove spaces and hyphens +- Change the "Test Integration" behavior for webhooks: don't retry failed requests ## v1.17.0 - 2020-10-14 diff --git a/hc/api/models.py b/hc/api/models.py index 52d4a562..bc2cda39 100644 --- a/hc/api/models.py +++ b/hc/api/models.py @@ -489,7 +489,8 @@ class Channel(models.Model): n.save() # These are not database fields. It is just a convenient way to pass - # status_url to transport classes. + # status_url and the is_test flag to transport classes. + check.is_test = is_test check.status_url = n.status_url() error = self.transport.notify(check) or "" diff --git a/hc/api/tests/test_notify.py b/hc/api/tests/test_notify.py index deae430c..637966da 100644 --- a/hc/api/tests/test_notify.py +++ b/hc/api/tests/test_notify.py @@ -58,6 +58,9 @@ class NotifyTestCase(BaseTestCase): self._setup_data("webhook", json.dumps(definition)) self.channel.notify(self.check) + # The transport should have retried 3 times + self.assertEqual(mock_get.call_count, 3) + n = Notification.objects.get() self.assertEqual(n.error, "Connection timed out") @@ -75,6 +78,9 @@ class NotifyTestCase(BaseTestCase): self._setup_data("webhook", json.dumps(definition)) self.channel.notify(self.check) + # The transport should have retried 3 times + self.assertEqual(mock_get.call_count, 3) + n = Notification.objects.get() self.assertEqual(n.error, "Connection failed") @@ -92,9 +98,30 @@ class NotifyTestCase(BaseTestCase): self.channel.notify(self.check) + # The transport should have retried 3 times + self.assertEqual(mock_get.call_count, 3) + n = Notification.objects.get() self.assertEqual(n.error, "Received status code 500") + @patch("hc.api.transports.requests.request", side_effect=Timeout) + def test_webhooks_dont_retry_when_sending_test_notifications(self, mock_get): + definition = { + "method_down": "GET", + "url_down": "http://example", + "body_down": "", + "headers_down": {}, + } + + self._setup_data("webhook", json.dumps(definition)) + self.channel.notify(self.check, is_test=True) + + # is_test flag is set, the transport should not retry: + self.assertEqual(mock_get.call_count, 1) + + n = Notification.objects.get() + self.assertEqual(n.error, "Connection timed out") + @patch("hc.api.transports.requests.request") def test_webhooks_support_variables(self, mock_get): definition = { diff --git a/hc/api/transports.py b/hc/api/transports.py index ba0f1006..4da12981 100644 --- a/hc/api/transports.py +++ b/hc/api/transports.py @@ -169,9 +169,8 @@ class HttpTransport(Transport): return "Connection failed" @classmethod - def get(cls, url, **kwargs): - # Make 3 attempts-- - for x in range(0, 3): + def get(cls, url, num_tries=3, **kwargs): + for x in range(0, num_tries): error = cls._request("get", url, **kwargs) if error is None: break @@ -179,9 +178,8 @@ class HttpTransport(Transport): return error @classmethod - def post(cls, url, **kwargs): - # Make 3 attempts-- - for x in range(0, 3): + def post(cls, url, num_tries=3, **kwargs): + for x in range(0, num_tries): error = cls._request("post", url, **kwargs) if error is None: break @@ -189,9 +187,8 @@ class HttpTransport(Transport): return error @classmethod - def put(cls, url, **kwargs): - # Make 3 attempts-- - for x in range(0, 3): + def put(cls, url, num_tries=3, **kwargs): + for x in range(0, num_tries): error = cls._request("put", url, **kwargs) if error is None: break @@ -240,14 +237,19 @@ class Webhook(HttpTransport): body = spec["body"] if body: - body = self.prepare(body, check) + body = self.prepare(body, check).encode() + + num_tries = 3 + if getattr(check, "is_test"): + # When sending a test notification, don't retry on failures. + num_tries = 1 if spec["method"] == "GET": - return self.get(url, headers=headers) + return self.get(url, num_tries=num_tries, headers=headers) elif spec["method"] == "POST": - return self.post(url, data=body.encode(), headers=headers) + return self.post(url, num_tries=num_tries, data=body, headers=headers) elif spec["method"] == "PUT": - return self.put(url, data=body.encode(), headers=headers) + return self.put(url, num_tries=num_tries, data=body, headers=headers) class Slack(HttpTransport):