From 48d94d96b9dac92cecadc75e5f2defd2e1d056a2 Mon Sep 17 00:00:00 2001 From: Samuel Merritt Date: Fri, 6 Jun 2014 14:29:44 -0700 Subject: [PATCH] Don't count 412 or 416 as errors in stats The backend HTTP servers emit StatsD metrics of the form ..timing and ..errors.timing (e.g. object-server.GET.timing). Whether something counts as an error or not is based on the response's HTTP status code. Prior to this commit, "success" was 2xx, 3xx, or 404, while everything else was considered "error". This adds 412 and 416 to the "success" set. Like 404, these status codes indicate that we got the request and processed it without error, but the response was "no". They shouldn't be in the same category as statuses like 507 that indicate something stopped the request from being processed. Change-Id: I5582a51cf6f64aa22c890da01aaaa077f3a54202 --- swift/common/utils.py | 26 ++++++++++++++++++++++---- test/unit/common/test_utils.py | 14 ++++++++++++++ 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/swift/common/utils.py b/swift/common/utils.py index a5badaab9b..2e6de38cb4 100644 --- a/swift/common/utils.py +++ b/swift/common/utils.py @@ -64,7 +64,9 @@ utf8_encoder = codecs.getencoder('utf-8') from swift import gettext_ as _ import swift.common.exceptions -from swift.common.http import is_success, is_redirection, HTTP_NOT_FOUND +from swift.common.http import is_success, is_redirection, HTTP_NOT_FOUND, \ + HTTP_PRECONDITION_FAILED, HTTP_REQUESTED_RANGE_NOT_SATISFIABLE + # logging doesn't import patched as cleanly as one would like from logging.handlers import SysLogHandler @@ -989,6 +991,24 @@ class StatsdClient(object): sample_rate) +def server_handled_successfully(status_int): + """ + True for successful responses *or* error codes that are not Swift's fault, + False otherwise. For example, 500 is definitely the server's fault, but + 412 is an error code (4xx are all errors) that is due to a header the + client sent. + + If one is tracking error rates to monitor server health, one would be + advised to use a function like this one, lest a client cause a flurry of + 404s or 416s and make a spurious spike in your errors graph. + """ + return (is_success(status_int) or + is_redirection(status_int) or + status_int == HTTP_NOT_FOUND or + status_int == HTTP_PRECONDITION_FAILED or + status_int == HTTP_REQUESTED_RANGE_NOT_SATISFIABLE) + + def timing_stats(**dec_kwargs): """ Returns a decorator that logs timing events or errors for public methods in @@ -1001,9 +1021,7 @@ def timing_stats(**dec_kwargs): def _timing_stats(ctrl, *args, **kwargs): start_time = time.time() resp = func(ctrl, *args, **kwargs) - if is_success(resp.status_int) or \ - is_redirection(resp.status_int) or \ - resp.status_int == HTTP_NOT_FOUND: + if server_handled_successfully(resp.status_int): ctrl.logger.timing_since(method + '.timing', start_time, **dec_kwargs) else: diff --git a/test/unit/common/test_utils.py b/test/unit/common/test_utils.py index 4d874c4da8..059674f01f 100644 --- a/test/unit/common/test_utils.py +++ b/test/unit/common/test_utils.py @@ -3050,6 +3050,20 @@ class TestStatsdLogging(unittest.TestCase): self.assertEquals(mock_controller.args[0], 'METHOD.timing') self.assert_(mock_controller.args[1] > 0) + mock_controller = MockController(412) + METHOD(mock_controller) + self.assertEquals(len(mock_controller.args), 2) + self.assertEquals(mock_controller.called, 'timing') + self.assertEquals(mock_controller.args[0], 'METHOD.timing') + self.assert_(mock_controller.args[1] > 0) + + mock_controller = MockController(416) + METHOD(mock_controller) + self.assertEquals(len(mock_controller.args), 2) + self.assertEquals(mock_controller.called, 'timing') + self.assertEquals(mock_controller.args[0], 'METHOD.timing') + self.assert_(mock_controller.args[1] > 0) + mock_controller = MockController(401) METHOD(mock_controller) self.assertEquals(len(mock_controller.args), 2)