Add error logging in hc.lib.s3.get_object

healthchecks · Apr 14, 2023 · 9ead449 · 9ead449
1 parent b1e9bdd
commit 9ead449
Show file tree

Hide file tree

Showing 2 changed files with 39 additions and 3 deletions.
diff --git a/hc/lib/s3.py b/hc/lib/s3.py
@@ -1,5 +1,6 @@
 from __future__ import annotations
 
+import logging
 import time
 from io import BytesIO
 from threading import Thread
@@ -17,6 +18,7 @@
     settings.S3_BUCKET = None
 
 _client = None
+logger = logging.getLogger(__name__)
 
 
 def client():
@@ -74,10 +76,17 @@ def get_object(code, n):
     try:
         response = client().get_object(settings.S3_BUCKET, key)
         return response.read()
-    except S3Error:
+    except S3Error as e:
+        if e.code == "NoSuchKey":
+            # It's not an error condition if an object does not exist.
+            # Return None, don't log exception, don't increase error counter.
+            return None
+
+        logger.exception("S3Error in hc.lib.s3.get_object")
         statsd.incr("hc.lib.s3.getObjectErrors")
         return None
     except HTTPError:
+        logger.exception("HTTPError in hc.lib.s3.get_object")
         statsd.incr("hc.lib.s3.getObjectErrors")
         return None
     finally:

diff --git a/hc/lib/tests/test_s3.py b/hc/lib/tests/test_s3.py
@@ -1,5 +1,7 @@
 from __future__ import annotations
 
+import logging
+from functools import wraps
 from unittest import skipIf
 from unittest.mock import Mock, patch
 
@@ -17,16 +19,41 @@
     have_minio = False
 
 
+def nolog(func):
+    @wraps(func)
+    def wrapper_func(*args, **kwargs):
+        logging.disable(logging.CRITICAL)
+        result = func(*args, **kwargs)
+        logging.disable(logging.NOTSET)
+        return result
+
+    return wrapper_func
+
+
 @skipIf(not have_minio, "minio not installed")
 @override_settings(S3_BUCKET="dummy-bucket")
 class S3TestCase(TestCase):
+    @patch("hc.lib.s3.statsd")
+    @patch("hc.lib.s3._client")
+    def test_get_object_handles_nosuchkey(self, mock_client, mock_statsd):
+        e = S3Error("NoSuchKey", "b", "c", "d", "e", "f")
+        mock_client.get_object.return_value.read = Mock(side_effect=e)
+        self.assertIsNone(get_object("dummy-code", 1))
+        self.assertTrue(mock_client.get_object.called)
+        # Should not increase the error counter for NoSuchKey responses
+        self.assertFalse(mock_statsd.incr.called)
+
+    @nolog
+    @patch("hc.lib.s3.statsd")
     @patch("hc.lib.s3._client")
-    def test_get_object_handles_s3error(self, mock_client):
-        e = S3Error("a", "b", "c", "d", "e", "f")
+    def test_get_object_handles_s3error(self, mock_client, mock_statsd):
+        e = S3Error("DummyError", "b", "c", "d", "e", "f")
         mock_client.get_object.return_value.read = Mock(side_effect=e)
         self.assertIsNone(get_object("dummy-code", 1))
         self.assertTrue(mock_client.get_object.called)
+        self.assertTrue(mock_statsd.incr.called)
 
+    @nolog
     @patch("hc.lib.s3._client")
     def test_get_object_handles_urllib_exceptions(self, mock_client):
         for e in [ProtocolError, InvalidHeader]: