launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/tidy-bzr-oopses into lp:launchpad.

Commit message:
Don't log HTTP 401 errors from XML-RPC as Bazaar OOPSes.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/tidy-bzr-oopses/+merge/263797

We get about 700 OOPSes a day from Nagios checking Launchpad branch privacy.  It doesn't really seem necessary to log 401s as OOPSes here, so let's not.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/tidy-bzr-oopses into lp:launchpad.

=== modified file 'lib/lp/codehosting/bzrutils.py'
--- lib/lp/codehosting/bzrutils.py	2012-07-02 14:13:33 +0000
+++ lib/lp/codehosting/bzrutils.py	2015-07-03 16:48:25 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Utilities for dealing with Bazaar.
@@ -27,6 +27,7 @@
 from contextlib import contextmanager
 import os
 import sys
+import xmlrpclib
 
 from bzrlib import (
     config,
@@ -60,10 +61,16 @@
 # Exception classes which are not converted into OOPSes
 NOT_OOPS_EXCEPTIONS = (AppendRevisionsOnlyViolation,)
 
-def should_log_oops(exc):
+
+def should_log_oops(exctype, excvalue):
     """Return true if exc should trigger an OOPS.
     """
-    return not issubclass(exc, NOT_OOPS_EXCEPTIONS)
+    if issubclass(exctype, NOT_OOPS_EXCEPTIONS):
+        return False
+    if (issubclass(exctype, xmlrpclib.ProtocolError) and
+            excvalue.errcode == 401):
+        return False
+    return True
 
 
 def is_branch_stackable(bzr_branch):
@@ -172,8 +179,9 @@
     """Make a hook for logging OOPSes."""
 
     def log_oops():
-        if should_log_oops(sys.exc_info()[0]):
-            error_utility.raising(sys.exc_info(), request)
+        exc = sys.exc_info()
+        if should_log_oops(exc[0], exc[1]):
+            error_utility.raising(exc, request)
     return log_oops
 
 

=== modified file 'lib/lp/codehosting/tests/test_bzrutils.py'
--- lib/lp/codehosting/tests/test_bzrutils.py	2014-06-10 16:13:03 +0000
+++ lib/lp/codehosting/tests/test_bzrutils.py	2015-07-03 16:48:25 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2014 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for bzrutils."""
@@ -7,6 +7,7 @@
 
 import gc
 import sys
+import xmlrpclib
 
 from bzrlib import (
     errors,
@@ -182,6 +183,26 @@
         self.logException(exception)
         self.assertEqual(0, len(self.oopses))
 
+    def test_doesnt_call_hook_for_unauthorized(self):
+        # 401 Unauthorized can happen in normal operation when somebody
+        # tries to access a branch they don't have access to, and isn't
+        # worth an OOPS.
+        self.assertEqual(0, len(self.oopses))
+        hook = install_oops_handler(1000)
+        self.addCleanup(remove_exception_logging_hook, hook)
+        exception = xmlrpclib.ProtocolError("", 401, "", "")
+        self.logException(exception)
+        self.assertEqual(0, len(self.oopses))
+
+    def test_calls_hook_for_other_protocol_error(self):
+        # Other XML-RPC protocol errors still log an OOPS.
+        self.assertEqual(0, len(self.oopses))
+        hook = install_oops_handler(1000)
+        self.addCleanup(remove_exception_logging_hook, hook)
+        exception = xmlrpclib.ProtocolError("", 500, "", "")
+        self.logException(exception)
+        self.assertEqual(1, len(self.oopses))
+
     def test_doesnt_call_hook_when_removed(self):
         # remove_exception_logging_hook removes the hook function, ensuring
         # it's not called when Bazaar logs an exception.