Merge lp:~toykeeper/wsgi-oops/stop-eating-errors into lp:wsgi-oops

[Selene ToyKeeper]

Fixed an error in the error logger for the error logger. Oh, metabugs, how do I love thee?

ISD keeps getting blank OOPS reports. It turns out there is an error in the error logger. Wsgi-oops has a failsafe to log that sort of thing, but unfortunately it has a bug too, which results in more blank errors. So, there is an error in the error logger for the error logger. It's kind of a metabug.

This fixes the outer most layer, so the outer errors will show up in the failsafe log. Once this is deployed, I can find out what the original error logger issue is, and fix that. Then after deploying that, I can hopefully find out what the original app errors are.

I basically reverted parts of revno: 43 by natalia.bidart. It seems not to have the desired effect (the code worked only 2 times out of 70615 errors in my log), and even the test for the feature didn't work. It still passed after removing the feature it was supposed to test.

ISD keeps getting blank OOPS reports. It turns out there is an error in the error logger. Wsgi-oops has a failsafe to log that sort of thing, but unfortunately it has a bug too, which results in more blank errors. So, there is an error in the error logger for the error logger. It's kind of a metabug.

This fixes the outer most layer, so the outer errors will show up in the failsafe log. Once this is deployed, I can find out what the original error logger issue is, and fix that. Then after deploying that, I can hopefully find out what the original app errors are.

I basically reverted parts of revno: 43 by natalia.bidart. It seems not to have the desired effect (the code worked only 2 times out of 70615 errors in my log), and even the test for the feature didn't work. It still passed after removing the feature it was supposed to test.

[Ricardo Kirkner (ricardokirkner)]

I'm a bit hesitant about this change. The original code doesn't *look* wrong at first sight. Since this is a shared codebase, I wouldn't want us to "fix" something and break other peoples expectations.

Can we have natalia involved in the review/discussion?

review: Needs Information

[Selene ToyKeeper (toykeeper)]

To clarify, wsgi-oops has *two* failsafes which each attempt to catch and log errors. One is in the file serializer, and one is in the reporter (which calls the serializer). In my logs, I see that the inner failsafe worked twice out of ~70,000 errors, and the outer one worked (sort of) the rest of the time. Unfortunately, the inner failsafe ate the exception most of the time, so the outer failsafe had no traceback to log.

This change removes the inner failsafe, so that the outer one can function properly. From what I can tell from my logs, the inner failsafe isn't working anyway, and I discovered during testing that the test for it doesn't work either. Since they're redundant, I removed them.

[Tim Cole (tcole)]

Hmm, looks reasonable, particularly since the test was spuriously passing.

review: Approve

[Selene ToyKeeper (toykeeper)]

Forwarded from Natalia...

* Natalia Bidart <email address hidden> wrote:
> Hello there!
>
> So, It's been ages (literally) since I don't touch that code, so as
> far as I see there should be not issues with that.
> If Tim Cole says +1, then I say +1 as well.
>
> Cheers, Natalia.

review: Approve

=== modified file 'canonical/oops/serializer.py'
--- canonical/oops/serializer.py	2010-12-21 00:51:42 +0000
+++ canonical/oops/serializer.py	2011-08-10 10:11:13 +0000
@@ -46,7 +46,6 @@
 
 from datetime import datetime, timedelta
 
-import logging
 import os
 import pytz
 import re
@@ -245,15 +244,12 @@
 
     def dump(self, oops):
         """Dump OOPS to disk."""
-        dump_filename = None
-        try:
-            dump_filename = self._start_dump(oops)
-            # We need to pass the filename as parameter to avoid
-            # race conditions between threads. Same for an oops.
-            self._write_file(dump_filename, oops)
-            self._finish_dump(dump_filename)
-        except:
-            logging.exception("%s.dump failed.", self.__class__.__name__)
+        dump_filename = self._start_dump(oops)
+
+        # We need to pass the filename as parameter to avoid
+        # race conditions between threads. Same for an oops.
+        self._write_file(dump_filename, oops)
+        self._finish_dump(dump_filename)
 
         return dump_filename
 

=== modified file 'canonical/oops/tests/test_serializer.py'
--- canonical/oops/tests/test_serializer.py	2009-11-03 13:55:56 +0000
+++ canonical/oops/tests/test_serializer.py	2011-08-10 10:11:13 +0000
@@ -49,11 +49,6 @@
     'session' : 549855,
 }
 
-class CrashyOOPSFileSerializer(OOPSFileSerializer):
-
-    def start_dump(self, oops):
-        raise Exception
-
 class OopsFileSerializerTests(unittest.TestCase):
     """Test oops file serializer."""
     serializer_class = OOPSFileSerializer
@@ -107,19 +102,6 @@
             self.result = None
 
 
-class OopsFileSerializerExceptionsTests(OopsFileSerializerTests):
-    """Test oops file serializer exceptions."""
-    serializer_class = CrashyOOPSFileSerializer
-
-    def setUp(self):
-        """Init this test."""
-        pass
-
-    def test_dump_catches_exceptions(self):
-        oops = self._build_oops()
-        self.result = self._get_result_from_oops_dump(oops)
-
-
 class OopsRFC822SerializerTests(OopsFileSerializerTests):
     """Test oops RFC822 serializer."""
     serializer_class = OOPSRFC822Serializer