Merge lp:~elachuni/ubuntu-recommender/oops-ids into lp:ubuntu-recommender

[Anthony Lenton]

Added a custom wsgi handler to get oops ids on all error responses.

This branch adds a custom wsgi handler to get oops ids on all error pages, and in API errors too.

It also customizes Django's runserver command to use our custom wsgi stack, so that now oops reports are also generated in dev as they are in prod. Dev only has a datedir publisher configured by default.

Also, it updates to oops-dictconfig 0.0.4 so that we can set inherit_id=True on our publishers.

While I was there, I removed all static external resources from the error pages, that aren't set up on staging/production and make the error pages look ugly. If we can provide better error pages eventually, great.

Finally, I also made 'fab bootstrap' not start from scratch every time. If you want the current behaviour you will now need to do 'fab clean bootstrap'.

[James Westby (james-w)]

> Also, it updates to oops-dictconfig 0.0.4 so that we can set inherit_id=True on our publishers.

Actually 0.0.4 is needed so you can set 'new_only = False' as well, otherwise you won't get any
oopses saved to disk.

110 'instance_id': 'dev',

I've since learned that setting this is deprecated. It won't matter with the
pre-generation of ids, but please remove it anyway.

339 + self.assertTrue(expected in last_output)

Isn't there assertIn for a better error message?

503 + """Pre-generates an oops id for all requests."""

It only pre-generates an oops id on exception.

Aside from this the oops setup looks good. I haven't reviewed the css/template/test/fabric changes.

Thanks,

James

[Anthony Lenton (elachuni)]

review: Approve

[Ubuntu One Auto Pilot (otto-pilot)]

The attempt to merge lp:~elachuni/ubuntu-recommender/oops-ids into lp:ubuntu-recommender failed. Below is the output from the failed tests.

[localhost] local: /usr/bin/python /usr/bin/virtualenv --no-site-packages virtualenv
The --no-site-packages flag is deprecated; it is now the default behavior.
New python executable in virtualenv/bin/python
Installing distribute.............................................................................................................................................................................................done.
Installing pip...............done.
[localhost] local: virtualenv/bin/pip install -r requirements.txt
Downloading/unpacking bzr (from -r requirements.txt (line 1))
  Running setup.py egg_info for package bzr
    No Cython, trying Pyrex...

    The python package 'Pyrex' is not available. If the .c files are available,
    they will be built, but modifying the .pyx files will not rebuild them.

Downloading/unpacking configglue==1.0.1 (from -r requirements.txt (line 2))
  Downloading configglue-1.0.1.tar.gz
  Running setup.py egg_info for package configglue

Downloading/unpacking coverage (from -r requirements.txt (line 3))
  Running setup.py egg_info for package coverage

    warning: no previously-included files matching '*.pyc' found anywhere in distribution
Downloading/unpacking django==1.3 (from -r requirements.txt (line 4))
  Running setup.py egg_info for package django

Downloading/unpacking django-configglue==0.6.1 (from -r requirements.txt (line 5))
  Downloading django-configglue-0.6.1.tar.gz
  Running setup.py egg_info for package django-configglue

    warning: no previously-included files matching '*.pyc' found anywhere in distribution
Obtaining django-pgtools from bzr+http://bazaar.launchpad.net/~canonical-isd-hackers/django-pgtools/trunk (from -r requirements.txt (line 6))
  Checking out http://bazaar.launchpad.net/~canonical-isd-hackers/django-pgtools/trunk to ./virtualenv/src/django-pgtools
  Running setup.py egg_info for package django-pgtools

Obtaining django-openid-auth from bzr+http://bazaar.launchpad.net/~django-openid-auth/django-openid-auth/trunk (from -r requirements.txt (line 7))
  Checking out http://bazaar.launchpad.net/~django-openid-auth/django-openid-auth/trunk to ./virtualenv/src/django-openid-auth
  Running setup.py egg_info for package django-openid-auth

Downloading/unpacking django-piston==0.2.2.1 (from -r requirements.txt (line 8))
  Downloading django-piston-0.2.2.1.tar.gz
  Running setup.py egg_info for package django-piston

Downloading/unpacking fabric (from -r requirements.txt (line 9))
  Running setup.py egg_info for package fabric

    warning: no previously-included files matching '*' found under directory 'docs/_build'
    warning: no previously-included files matching '*.pyc' found under directory 'tests'
    warning: no previously-included files matching '*.pyo' found under directory 'tests'
Downloading/unpacking mock (from -r requirements.txt (line 10))
  Running setup.py egg_info for package mock

    warning: no files found matching '*.png' under directory 'docs'
    warning: no files found ma...

=== modified file '.bzrignore'
--- .bzrignore	2013-02-20 20:34:25 +0000
+++ .bzrignore	2013-02-25 12:37:19 +0000
@@ -11,3 +11,4 @@
 rec-webapp.log
 rec-auth.log
 data
+oopses/

=== modified file 'django_project/config/main.cfg'
--- django_project/config/main.cfg	2012-09-28 21:24:33 +0000
+++ django_project/config/main.cfg	2013-02-25 12:37:19 +0000
@@ -4,7 +4,7 @@
 database_engine = sqlite3
 database_host =
 database_name = test.db
-debug = true
+debug = false
 email_host = localhost
 email_port = 1025
 email_backend = django.core.mail.backends.locmem.EmailBackend
@@ -125,7 +125,7 @@
 rnr_api = https://reviews.ubuntu.com/reviews/api/1.0
 
 [logging]
-webapp_logging_config = staging_logging.conf
+webapp_logging_config = django_project/config/staging_logging.conf
 
 [preflight]
 preflight_groups = hackers

=== modified file 'fabtasks/bootstrap.py'
--- fabtasks/bootstrap.py	2013-02-20 20:34:25 +0000
+++ fabtasks/bootstrap.py	2013-02-25 12:37:19 +0000
@@ -1,13 +1,12 @@
 import os
 import sys
 
-from fabric.api import local
+from fabric.api import local, lcd
 
 from .database import create_database
 
 
 def virtualenv_create():
-    local("rm -rf virtualenv")
     local("%s /usr/bin/virtualenv --no-site-packages virtualenv"
         % sys.executable, capture=False)
 
@@ -19,9 +18,12 @@
         revision = "--revision={0}".format(revision)
     branch_path = os.path.join("branches", name)
     if os.path.exists(branch_path):
-        local("rm -r {0}".format(branch_path))
-    local("bzr branch {0} {1} {2}".format(
-        revision, repo, branch_path), capture=False)
+        with lcd(branch_path):
+            local("bzr pull {0} {1}".format(
+                revision, repo), capture=False)
+    else:
+        local("bzr branch {0} {1} {2}".format(
+            revision, repo, branch_path), capture=False)
 
 
 def _symlink(source, link_name):

=== modified file 'requirements.txt'
--- requirements.txt	2013-02-20 20:34:25 +0000
+++ requirements.txt	2013-02-25 12:37:19 +0000
@@ -15,6 +15,6 @@
 django-factory
 psycopg2==2.4.1
 oops-wsgi==0.0.10
-oops-dictconfig==0.0.1
+oops-dictconfig==0.0.4
 # XXX 2012-03-07 bug=948857 Currently this pulls in lazr.restfulclient and co.
 oops_datedir_repo==0.0.17

=== added file 'src/recommender/management/commands/runserver.py'
--- src/recommender/management/commands/runserver.py	1970-01-01 00:00:00 +0000
+++ src/recommender/management/commands/runserver.py	2013-02-25 12:37:19 +0000
@@ -0,0 +1,20 @@
+# Slightly customized version of Django's runserver command
+# that runs ubuntu-recommender's wsgi stack.
+
+from optparse import make_option
+from django.core.management.commands.runserver import BaseRunserverCommand
+from django.core.servers.basehttp import AdminMediaHandler
+
+
+class Command(BaseRunserverCommand):
+    option_list = BaseRunserverCommand.option_list + (
+        make_option(
+            '--adminmedia', dest='admin_media_path', default='',
+            help='Specifies the directory from which to serve admin media.'),
+    )
+
+    def get_handler(self, *args, **options):
+        """Serves admin media like old-school (deprecation pending)."""
+        from recommender.wsgi import make_app
+        handler = make_app()
+        return AdminMediaHandler(handler, options.get('admin_media_path', ''))

=== modified file 'src/recommender/schema.py'
--- src/recommender/schema.py	2013-02-20 20:34:25 +0000
+++ src/recommender/schema.py	2013-02-25 12:37:19 +0000
@@ -114,7 +114,8 @@
                     {
                         'type': 'datedir',
                         'error_dir': 'oopses',
-                        'instance_id': 'dev',
+                        'inherit_id': True,
+                        'new_only': False,
                     }
                 ],
             }

=== modified file 'src/recommender/templates/500.html'
--- src/recommender/templates/500.html	2012-08-17 22:20:06 +0000
+++ src/recommender/templates/500.html	2013-02-25 12:37:19 +0000
@@ -34,7 +34,7 @@
 body {
     font-family: 'Ubuntu Beta', UbuntuBeta, Ubuntu, 'Bitstream Vera Sans', 'DejaVu Sans', Tahoma, sans-serif;
     color: #333;
-    background: white url(../images/body_bg.png);
+    background: white;
     font-size: 12px;
     line-height: 16px;
     margin: 0px;
@@ -57,62 +57,13 @@
     -webkit-box-shadow: #bbb 0px 0px 5px;
 }
 #header {
-    background: #dd4814 url(../images/header_bg.png) top left repeat-x;
+    background: #dd4814 top left repeat-x;
     height: 64px;
     margin: 0px;
     padding: 0px;
     position: relative;
 }
 
-#menu-search {
-    height: 40px;
-    margin: 0 16px;
-}
-#search-box {
-    margin-left: 720px;
-    -moz-border-radius: 0px 0px 4px 4px;
-    -webkit-border-bottom-left-radius: 4px;
-    -webkit-border-bottom-right-radius: 4px;
-    -moz-box-shadow: #bbb 0px 0px 2px;
-    -webkit-box-shadow: #bbb 0px 0px 2px;
-    background-color: #f2f2f2;
-    width: 224px;
-    height: 40px;
-}
-#search-box-input {
-    float: left;
-    width: 172px;
-    height: 20px;
-    border-color: #d1d1d1;
-    border-style: solid none none solid;
-    border-width: 1px medium medium 1px;
-    margin: 8px 0 0 8px;
-    padding: 1px;
-    font-size: 12px;
-}
-#search-box-button {
-    float: left;
-    background: url(../images/search_submit_bg_2.png) transparent;
-    width: 24px;
-    height: 24px;
-    margin: 8px 0 0 8px;
-    padding: 0;
-    border: none;
-    cursor: pointer;
-}
-#search-box-button:hover {
-    background-position: 0 -23px;
-}
-#search-box-button:active {
-    background-position: 0 -46px;
-}
-
-#search-box button span {
-    position: absolute;
-    overflow: hidden;
-    text-indent: -9999px;
-}
-
 #title {
     padding: 28px 24px;
 }
@@ -134,15 +85,12 @@
     line-height: 14px;
 }
 #copyright a {
-    background-image: url(../images/footer_logo.png);
     width: 118px;
     height: 27px;
     display: block;
     float: right;
 }
 #ubuntu-header {
-    background-image: url(../images/header_logo.png);
-    background-repeat: no-repeat;
     height: 32px;
     left: 841px;
     margin: 0;
@@ -159,13 +107,12 @@
     width: 100%;
 }
 
-#main-menu, {
+#main-menu {
     list-style: none;
     margin: 0;
     margin-left: 16px;
     padding: 0;
     height: 100%;
-    background: url(../images/topnav_divider.png) no-repeat scroll left top transparent !important;
 }
 #main-menu li {
     float: left;
@@ -174,7 +121,6 @@
     height: 64px;
     font-size: 14px;
     line-height: 16px;
-    background: url(../images/topnav_divider.png) no-repeat scroll right top transparent !important;
 }
 #main-menu a {
     display: block;
@@ -185,40 +131,6 @@
     color: #fff;
     text-shadow: #000 0px 1px;
 }
-#main-menu a:hover, #main-menu li.active a,
-#main-menu li ul {
-    position: absolute;
-    top: 64px;
-    left: 16px;
-    height: 40px;
-    width: 716px;
-    background-color: #F2F2F2;
-    -moz-border-radius: 0px 0px 4px 4px;
-    -webkit-border-radius: 0px 0px 4px 4px;
-    border-radius: 0px 0px 4px 4px;
-    -moz-box-shadow: #bbb 0px 0px 2px;
-    -webkit-box-shadow: #bbb 0px 0px 2px;
-    box-shadow: #bbb 0px 0px 2px;
-}
-#main-menu li li {
-    height: 40px;
-    padding: 0;
-    background-color: #F2F2F2;
-    background-image: none;
-    list-style-image: none;
-}
-#main-menu li.active li a {
-    padding: 13px 8px 0px 10px;
-    height: 27px;
-    background-color: #F2F2F2;
-    color: #333;
-    text-shadow: none;
-    font-size: 12px;
-    background: transparent none !important;
-}
-#main-menu li.active li a:hover {
-    color: #772953;
-}
 h1, h2, h3, h4, h5 {
     padding: 0;
     margin: 0;
@@ -237,12 +149,11 @@
     color: #dd4814;
     text-decoration: none;
 }
-a:hover {
+#main-menu a:hover {
     text-decoration: underline;
 }
 ul {
     margin-bottom: 16px;
-    list-style-image: url(../images/bullet.png);
 }
 ul li {
     margin-bottom: 8px;
@@ -285,29 +196,15 @@
                         <li><a href="http://shop.canonical.com/">Shop</a></li>
                     </ul>
                 </div>
-                <div id="menu-search">
-                    {% block search %}
-                    <div id="search-box">
-                        <form id="sitesearch" action="http://www.ubuntu.com/search/node" method="post">
-                          <input type="text" maxlength="128" name="search_theme_form" id="search-box-input"
-                                 value="Type to search" title="Enter the terms you wish to search for."
-                                 onblur="if(this.value==&#039;&#039;){this.value=&#039;Type to search&#039;;}"
-                                 onfocus="if(this.value==&#039;Type to search&#039;){this.value=&#039;&#039;}"
-                          />
-                          <button type="submit" name="op" id="search-box-button"><span>go</span></button>
-                          <input type="hidden" name="form_build_id" id="form-967ff0ccc1a2f6f6d92f3e8c0822866d" value="form-967ff0ccc1a2f6f6d92f3e8c0822866d" />
-                          <input type="hidden" name="form_token" id="a-unique-id" value="656a58b3b2c4f37a2af1d6b188a4a595" />
-                          <input type="hidden" name="form_id" id="edit-search-theme-form" value="search_theme_form" />
-                        </form>
-                    </div>
-                    {% endblock %}
-                </div>
                 <div id="title">
-                    <h1>{% block header %}Internal Server Error{% endblock %}</h1>
+                    <h1>{% block header %}Oops!{% endblock %}</h1>
                 </div>
 {% block content_container %}
                 <div id="content" class="clearfix content-area">
-{% block content %}There's been an error. It's been reported to the site administrators and should be fixed shortly. Thanks for your patience.{% endblock %}
+{% block content %}
+    <p>There's been an error. It's been reported to the site administrators and should be fixed shortly. Thanks for your patience.</p>
+    <p>{{ oops_id }}</p>
+{% endblock %}
                 </div>
 {% endblock %}
             </div>

=== modified file 'src/recommender/tests/__init__.py'
--- src/recommender/tests/__init__.py	2013-02-20 17:19:26 +0000
+++ src/recommender/tests/__init__.py	2013-02-25 12:37:19 +0000
@@ -12,3 +12,4 @@
 from .test_slopeone import *
 from .test_utilities import *
 from .test_views_oops import *
+from .test_wsgi import *

=== modified file 'src/recommender/tests/test_commands.py'
--- src/recommender/tests/test_commands.py	2012-09-28 21:24:33 +0000
+++ src/recommender/tests/test_commands.py	2013-02-25 12:37:19 +0000
@@ -168,9 +168,8 @@
 
         ImportRnRData().handle(verbosity='1', bootstrap=True)
 
-        self.assertTrue(
-            "Processing reviews for super-package" in
-            sys.stdout.getvalue())
+        expected = "Processing reviews for super-package"
+        self.assertIn(expected, sys.stdout.getvalue())
 
         sys.stdout = _stdout
 
@@ -263,7 +262,7 @@
 
         last_output = self.mock_stdout.write.call_args_list[-1][0][0]
         expected = 'Error fetching data: No JSON object could be decoded\n'
-        self.assertEqual(expected, last_output)
+        self.assertIn(expected, last_output)
 
     @patch('httplib2.Http.request')
     def test_department_is_updated(self, mock_request):

=== added file 'src/recommender/tests/test_wsgi.py'
--- src/recommender/tests/test_wsgi.py	1970-01-01 00:00:00 +0000
+++ src/recommender/tests/test_wsgi.py	2013-02-25 12:37:19 +0000
@@ -0,0 +1,122 @@
+import bson
+import json
+import os
+import re
+
+from tempfile import mkdtemp
+from shutil import rmtree
+
+from django.core.urlresolvers import reverse
+from django.test import TestCase
+from mock import Mock, patch
+
+from recommender.tests.helpers import patch_settings
+from recommender.wsgi import make_app
+
+oopsid_re = re.compile('OOPS-[0-9a-f]{32}')
+
+
+class WSGIDispatchTestCase(TestCase):
+
+    def setUp(self):
+        """Create a temporary directory for oopses."""
+        self.oops_dir = mkdtemp()
+        self.addCleanup(rmtree, self.oops_dir)
+
+        self.environ = {
+            'PATH_INFO': '/',
+            'QUERY_STRING': '',
+            'REMOTE_ADDR': '127.0.0.1',
+            'REQUEST_METHOD': 'GET',
+            'SCRIPT_NAME': '',
+            'SERVER_NAME': 'testserver',
+            'SERVER_PORT': '80',
+            'SERVER_PROTOCOL': 'HTTP/1.1',
+            'wsgi.version': (1, 0),
+            'wsgi.url_scheme': 'http',
+            'wsgi.multiprocess': True,
+            'wsgi.multithread': False,
+            'wsgi.run_once': False,
+            'wsgi.input': '',
+        }
+
+    def request_wsgi_response(self):
+        start_response = Mock()
+        oopses = {
+            'publishers': [{
+                'type': 'datedir',
+                'error_dir': self.oops_dir,
+                'instance_id': 'dev',
+            }],
+        }
+        with patch_settings(OOPSES=oopses):
+            app = make_app()
+            response = app(self.environ, start_response).next()
+        return response
+
+    def request_api_exception(self, data={}):
+        """Helper for creating exceptions when calling api."""
+        self.environ['PATH_INFO'] = '/api/1.0/recommend_app/firefox/'
+        self.environ['ACCEPT'] = 'application/json'
+        packages_filter_fn = (
+            'recommender.models.Package.objects.filter')
+        with patch(packages_filter_fn) as mock_packages_filter:
+            mock_packages_filter.side_effect = ZeroDivisionError
+            response = self.request_wsgi_response()
+        return response
+
+    def request_view_exception(self):
+        """Helper for creating exceptions when calling a view."""
+        self.environ['PATH_INFO'] = reverse('debug-die')
+        return self.request_wsgi_response()
+
+    def read_oops_from_disk(self):
+        """Helper to read in the oops written to disk."""
+        # There is only one date directory in the oops directory.
+        dated_dirs = os.listdir(self.oops_dir)
+        self.assertEqual(1, len(dated_dirs))
+
+        # There is only one oops.
+        todays_oops_path = os.path.join(self.oops_dir, dated_dirs[0])
+        oops_list = os.listdir(todays_oops_path)
+        self.assertEqual(1, len(oops_list))
+
+        oops_path = os.path.join(todays_oops_path, oops_list[0])
+        with open(oops_path) as oops_file:
+            oops_file_contents = oops_file.read()
+        return bson.loads(oops_file_contents)
+
+    def test_traceback_in_api_response(self):
+        response = self.request_api_exception()
+
+        data = json.loads(response)
+        self.assertTrue(oopsid_re.match(data['oops_id']))
+        self.assertTrue('traceback' in data)
+
+    def test_traceback_in_api_oops_on_disk(self):
+        """An api oops writes the traceback to the file."""
+        self.request_api_exception()
+
+        oops_content = self.read_oops_from_disk()
+        self.assertIn('id', oops_content)
+        self.assertIn('tb_text', oops_content)
+        self.assertTrue('raise effect' in oops_content['tb_text'])
+
+    def test_traceback_in_view_oops_on_disk(self):
+        """A request resulting in an exception writes the traceback to disk."""
+        self.request_view_exception()
+
+        # The oops contains the stacktrace as part of a log message.
+        oops_content = self.read_oops_from_disk()
+        self.assertIn('id', oops_content)
+        self.assertIn('tb_text', oops_content)
+        self.assertTrue('ArtificialOopsException' in oops_content['tb_text'])
+        self.assertEqual('ArtificialOopsException', oops_content['type'])
+
+    def test_traceback_not_in_view_oops_response(self):
+        """An exception doesn't display the traceback."""
+        response = self.request_view_exception()
+
+        self.assertTrue(oopsid_re.search(response))
+        self.assertNotIn(response, 'ArtificialOopsException')
+        self.assertNotIn(response, 'Traceback')

=== modified file 'src/recommender/views.py'
--- src/recommender/views.py	2013-02-20 17:30:06 +0000
+++ src/recommender/views.py	2013-02-25 12:37:19 +0000
@@ -61,8 +61,10 @@
                 if isinstance(tb_object, traceback.types.TracebackType):
                     traceback_formatted = mark_safe("\n".join(
                         traceback.format_tb(exc_info[2])))
+        oops_report = request.environ.get('oops.report', {})
+        oops_id = oops_report.get('id', 'OOPS ID not available.')
         atts = {
-            'oops_id': request.META.get('OOPSID', 'OOPS ID not available.'),
+            'oops_id': oops_id,
             'traceback': traceback_formatted,
         }
 

=== added file 'src/recommender/wsgi.py'
--- src/recommender/wsgi.py	1970-01-01 00:00:00 +0000
+++ src/recommender/wsgi.py	2013-02-25 12:37:19 +0000
@@ -0,0 +1,51 @@
+import uuid
+import logging.config
+import oops_dictconfig
+import oops_wsgi
+import platform
+from django.conf import settings
+from django.core.handlers import wsgi
+
+
+class EagerOOPSWSGIHandler(wsgi.WSGIHandler):
+    """Pre-generates an oops id on exception."""
+    def handle_uncaught_exception(self, request, resolver, exc_info):
+        # This will pass the exception information back to oops.  It
+        # should not be necessary once
+        # https://code.djangoproject.com/ticket/16674 has been merged.
+        if 'oops.context' in request.environ:
+            request.environ['oops.context']['exc_info'] = exc_info
+        # Generate OOPS id early so that we can render the error page right
+        # away and not depend on passing thread locals around.
+        if 'oops.report' in request.environ:
+            unique_id = uuid.uuid4().hex
+            request.environ['oops.report']['id'] = "OOPS-%s" % unique_id
+
+        return super(EagerOOPSWSGIHandler, self).handle_uncaught_exception(
+            request, resolver, exc_info)
+
+    def __call__(self, environ, start_response):
+        def start_response_with_exc_info(status, headers, exc_info=None):
+            """Custom start_response callback for wsgi."""
+            # This will pass the exception information back to oops_wgi.  It
+            # should not be necessary once
+            # https://code.djangoproject.com/ticket/16674 has been merged.
+            if exc_info is None:
+                exc_info = environ['oops.context'].get('exc_info', None)
+            return start_response(status, headers, exc_info)
+        return super(EagerOOPSWSGIHandler, self).__call__(
+            environ, start_response_with_exc_info)
+
+
+def make_app():
+    """Encapsulate our REC handler in an OOPS app for error reporting."""
+    logging.config.fileConfig(settings.WEBAPP_LOGGING_CONFIG,
+                              disable_existing_loggers=False)
+
+    non_oops_app = EagerOOPSWSGIHandler()
+
+    config = oops_dictconfig.config_from_dict(settings.OOPSES)
+    oops_wsgi.install_hooks(config)
+    oops_app = oops_wsgi.make_app(non_oops_app, config,
+                                  oops_on_status=['500'])
+    return oops_app