Merge lp:~elachuni/rnr-server/amqp-oopses into lp:rnr-server

[Anthony Lenton]

This branch switches to configglue 0.0.5 for setting amqp oops publishing.

There is also a custom wsgi handler now that will generate an oops id for every exception, and display the oops id on the error page correctly.

Finally, a custom runserver command is provided to run the custom wsgi stack in dev, to generate oopses in dev.

[lp:~elachuni/rnr-server/amqp-oopses]

234. By Anthony Lenton on 2013-03-01

Removed storm from requirements

[Anthony Lenton (elachuni)]

Also, storm is no longer required since we moved from wsgi_oops to oops_wsgi...

[Michael Nelson (michael.nelson)]

If we update to Django 1.4, the custom runserver command isn't needed:

https://docs.djangoproject.com/en/1.4/ref/settings/#std:setting-WSGI_APPLICATION

review: Approve

[Natalia Bidart (nataliabidart)]

Setting MP as Rejected to reduce numbers of landing candidates in @reviewlist. Change status again if this MP is still current.

Unmerged revisions

234. By Anthony Lenton on 2013-03-01

Removed storm from requirements

233. By Anthony Lenton on 2013-03-01

Plugged in oops_dictconfig 0.0.5.
Display an oops id on every error page.
Enable oops tracking in dev.
Don't track oopses for 404 responses.

=== modified file '.bzrignore'
--- .bzrignore	2012-07-13 19:45:12 +0000
+++ .bzrignore	2013-03-01 18:28:22 +0000
@@ -21,3 +21,4 @@
 sourcecode/*
 django_project/scaclient.py
 django_project/txstatsd
+oopses/

=== modified file 'django_project/config_dev/config/main.cfg'
--- django_project/config_dev/config/main.cfg	2012-08-07 13:01:38 +0000
+++ django_project/config_dev/config/main.cfg	2013-03-01 18:28:22 +0000
@@ -1,3 +1,6 @@
+[__main__]
+serve_static_media = True
+
 # Django settings for reviews project.
 [django]
 debug = True
@@ -113,6 +116,7 @@
 openid_update_details_from_sreg = True
 openid_sso_server_url = https://login.staging.ubuntu.com/
 openid_launchpad_teams_mapping = lp_team_mapping
+openid_launchpad_staff_teams = rnr-moderators
 
 [rnr]
 moderators_group = rnr-moderators
@@ -158,8 +162,8 @@
 	mem
 
 [logging]
-logging_config_file = logging.conf
+logging_config_file = django_project/logging.conf
 
 [graphite]
 txstatsd_host = localhost
-txstatsd_port = 8125
\ No newline at end of file
+txstatsd_port = 8125

=== modified file 'django_project/config_dev/settings.py'
--- django_project/config_dev/settings.py	2012-03-26 21:44:24 +0000
+++ django_project/config_dev/settings.py	2013-03-01 18:28:22 +0000
@@ -14,6 +14,3 @@
                 if os.path.exists(path)]
 
 configglue(RnRSchema, config_files, __name__)
-
-if 'LOGGING_CONFIG_FILE' in locals():
-    logging.config.fileConfig(os.path.join(current_dir, LOGGING_CONFIG_FILE))

=== modified file 'requirements.txt'
--- requirements.txt	2013-02-25 19:42:16 +0000
+++ requirements.txt	2013-03-01 18:28:22 +0000
@@ -4,7 +4,7 @@
 configglue==1.0.1
 coverage
 django-configglue==0.6.1
-django-openid-auth==0.2
+django-openid-auth==0.4
 django-piston
 django-preflight
 django==1.3
@@ -15,12 +15,14 @@
 mechanize
 mock
 oauthlib
+oops-wsgi==0.0.10
+oops-dictconfig==0.0.5
+oops-datedir_repo==0.0.17
 pep8
 psycopg2==2.4.1
 python-openid
 pytz
 setuptools
 south==0.7
-storm
 testtools
 wsgi_intercept==0.4

=== added file 'src/reviewsapp/management/commands/runserver.py'
--- src/reviewsapp/management/commands/runserver.py	1970-01-01 00:00:00 +0000
+++ src/reviewsapp/management/commands/runserver.py	2013-03-01 18:28:22 +0000
@@ -0,0 +1,36 @@
+#   -*- coding: utf-8 -*-
+#   This file is part of Software Center Ratings and Reviews
+#   Copyright (C) 2010-2013 Canonical Ltd.
+#
+#   This program is free software: you can redistribute it and/or modify
+#   it under the terms of the GNU Affero General Public License as
+#   published by the Free Software Foundation, either version 3 of the
+#   License, or (at your option) any later version.
+#
+#   This program is distributed in the hope that it will be useful,
+#   but WITHOUT ANY WARRANTY; without even the implied warranty of
+#   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+#   GNU Affero General Public License for more details.
+#
+#   You should have received a copy of the GNU Affero General Public License
+#   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+"""Customized runserver command that runs rnr-server'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 reviewsapp.wsgi import make_app
+        handler = make_app()
+        return AdminMediaHandler(handler, options.get('admin_media_path', ''))

=== modified file 'src/reviewsapp/schema.py'
--- src/reviewsapp/schema.py	2012-08-29 20:01:15 +0000
+++ src/reviewsapp/schema.py	2013-03-01 18:28:22 +0000
@@ -26,6 +26,7 @@
 import django
 from configglue import schema
 from django_configglue.schema import schemas
+from oops_dictconfig.configglue_options import OopsOption
 
 DjangoSchema = schemas.get(django.get_version())
 
@@ -115,3 +116,19 @@
         txstatsd_host = schema.StringOption()
         txstatsd_port = schema.IntOption()
         txstatsd_instance_type = schema.StringOption(default='staging')
+
+    # oops
+    class oops_wsgi(schema.Section):
+        oopses = OopsOption(
+            default={
+                'publishers': [
+                    {
+                        'type': 'datedir',
+                        'error_dir': 'oopses',
+                        'instance_id': 'dev',
+                        'new_only': False,
+                        'inherit_id': True,
+                    }
+                ],
+            }
+        )

=== modified file 'src/reviewsapp/templates/500.html'
--- src/reviewsapp/templates/500.html	2011-04-01 15:08:05 +0000
+++ src/reviewsapp/templates/500.html	2013-03-01 18:28:22 +0000
@@ -9,9 +9,7 @@
 <h1>{% trans 'Server Error <em>(500)</em>' %}</h1>
 {% if errormsg %}<p>{{ errormsg }}</p>{% endif %}
 <p>{% blocktrans %}An error occurred which prevented the page you requested from loading. If this problem continues, please consider reporting this problem by sending an e-mail to <a href="mailto:[e-mail]">[e-mail]</a>{% endblocktrans %}</p>
-<p>
-	{% blocktrans %}(Error <abbr>ID</abbr>: ){% endblocktrans %} {{ oopsid }}
-</p>
+<p id="oops-id">{{ oops_id }}</p>
 <pre style="overflow:auto">
 	{{ traceback }}
 </pre>

=== modified file 'src/reviewsapp/tests/test_views.py'
--- src/reviewsapp/tests/test_views.py	2012-11-21 11:05:24 +0000
+++ src/reviewsapp/tests/test_views.py	2013-03-01 18:28:22 +0000
@@ -533,7 +533,7 @@
             # We need to reload urls.py to recalculate all urls
             reload(reviewsapp.urls)
 
-            response = self.client.get('/reviews/die/')
+            response = self.client.get('/reviews/delay/1.0/')
 
         self.assertEquals(404, response.status_code)
         reload(reviewsapp.urls)

=== modified file 'src/reviewsapp/tests/test_wsgi.py'
--- src/reviewsapp/tests/test_wsgi.py	2012-06-12 17:44:57 +0000
+++ src/reviewsapp/tests/test_wsgi.py	2013-03-01 18:28:22 +0000
@@ -1,3 +1,4 @@
+import bson
 import os
 
 from tempfile import mkdtemp
@@ -49,16 +50,6 @@
         rmtree(self.oops_dir)
         settings.MIDDLEWARE_CLASSES = self.orig_middleware
 
-    def request_404_exception(self, data={}):
-        """Helper for creating a 404 exception with a referrer."""
-        self.environ['PATH_INFO'] = '/reviews/made_up_url'
-        self.environ['HTTP_REFERER'] = '/'
-        if data != {}:
-            self.environ['QUERY_STRING'] = urlencode(data, doseq=True)
-
-        response = self.request_wsgi_response(self.environ)
-        return response
-
     def request_500_exception(self, data={}):
         """Helper for creating a 500 exception."""
         self.environ['PATH_INFO'] = reverse('rnr-overview')
@@ -73,7 +64,15 @@
 
     def request_wsgi_response(self, environ):
         start_response = Mock()
-        with patch_settings(OOPS_DIR=self.oops_dir):
+        oops_config = {
+            'publishers': [{
+                'type': 'datedir',
+                'error_dir': self.oops_dir,
+                'inherit_id': True,
+                'new_only': False,
+            }],
+        }
+        with patch_settings(OOPSES=oops_config):
             app = make_app()
             response = app(environ, start_response).next()
         return response
@@ -92,31 +91,19 @@
         oops_path = self.oops_dir + '/' + todays_oops_path + '/' + oops_list[0]
         with open(oops_path, "r") as oops_file:
             oops_file_contents = oops_file.read()
-        return oops_file_contents
-
-    def test_oopsid_in_404_response(self):
-        # The rendered response for a view exception includes the oops id.
-        response = self.request_404_exception()
-
-        self.assertTrue(self.environ['OOPSID'] in response)
+        return bson.loads(oops_file_contents)
 
     def test_oopsid_in_500_response(self):
         # The rendered response for a 500 exception includes the oops id.
         response = self.request_500_exception()
 
-        self.assertTrue(self.environ['OOPSID'] in response)
-
-    def test_exception_in_404_oops_on_disk(self):
-        # A request resulting in an 404 with a referrer writes an oops to disk
-        self.request_404_exception()
-
-        self.assertTrue("Exception-Type:" in self.read_oops_from_disk())
-
-    def test_traceback_in_500_oops_on_disk(self):
+        oops_id = self.environ['oops.report']['id']
+        self.assertTrue(oops_id in response)
+
+    def test_oops_id_in_500_oops_on_disk(self):
         # A request resulting in a 500 writes the oops to disk.
-        self.request_500_exception()
+        response = self.request_500_exception()
 
-        # The oops contains the stacktrace as part of a log message.
-        self.assertTrue(
-            "'message': 'Traceback (most recent call last):" in
-            self.read_oops_from_disk())
+        oops_id = self.environ['oops.report']['id']
+        oops_report = self.read_oops_from_disk()
+        self.assertEqual(oops_id, oops_report['id'])

=== modified file 'src/reviewsapp/urls.py'
--- src/reviewsapp/urls.py	2012-06-20 12:19:49 +0000
+++ src/reviewsapp/urls.py	2013-03-01 18:28:22 +0000
@@ -58,6 +58,7 @@
     (r'^forbidden', never_cache(moderation.forbidden)),
 
     url(r'^logout/$', name='logout', view=logout, kwargs={'next_page': '/'}),
+    url(r'^die/$', 'die', name='debug-die'),
 
     # Include the API urls
     (r'^api/', include('reviewsapp.api.urls')),
@@ -66,6 +67,5 @@
 if getattr(settings, 'ENABLE_ARTIFICIAL_OOPS', False):
     urlpatterns += patterns(
         'reviewsapp.views',
-        (r'^die/$', 'die'),
         (r'^delay/(?P<amount>[\d.]+)/$', 'delay'),
     )

=== modified file 'src/reviewsapp/views/moderation.py'
--- src/reviewsapp/views/moderation.py	2013-02-20 21:39:32 +0000
+++ src/reviewsapp/views/moderation.py	2013-03-01 18:28:22 +0000
@@ -27,14 +27,15 @@
 ]
 
 
+import json
 from urllib import urlencode
 
-from django import http
-from django.db.models import Count
 from django.contrib.auth import logout
 from django.contrib.auth.decorators import login_required, permission_required
 from django.core.urlresolvers import reverse
+from django.db.models import Count
 from django.http import (
+    HttpResponse,
     HttpResponseForbidden,
     HttpResponseRedirect,
 )
@@ -42,7 +43,8 @@
     get_object_or_404,
     render_to_response,
 )
-from django.template import loader, RequestContext
+from django.template import RequestContext
+from django.template.loader import get_template
 from django.utils.safestring import mark_safe
 from django.utils.translation import ugettext as _
 
@@ -205,18 +207,39 @@
         self.errormsg = errormsg
 
     def __call__(self, request):
-        return self.render_error(request)
-
-    def render_error(self, request):
-        template = loader.get_template(self.template_name)
-        oopsid = request.META.get('OOPSID')
+        accept = request.META.get('ACCEPT', '')
+        if 'application/json' in accept and not 'text/html' in accept:
+            return self.render_json_error(request)
+        else:
+            return self.render_html_error(request)
+
+    def render_json_error(self, request):
+        oops_id = self.get_oops_id(request)
+        oops_context = request.META.get('oops.context', {})
+        exc_info = oops_context.get('exc_info', (None, None, None))
+        exception_type = unicode(exc_info[0])
+        response = json.dumps({
+            'oops_id': oops_id,
+            'error_type': exception_type,
+            'error_msg': "An error occurred and has been logged.",
+        })
+        return HttpResponse(response, status=self.status_code,
+                            content_type='application/json')
+
+    def render_html_error(self, request):
+        oops_id = self.get_oops_id(request)
+        template = get_template(self.template_name)
         atts = {
-            'oopsid': oopsid,
+            'oops_id': oops_id,
             'errormsg': self.errormsg,
         }
         context = RequestContext(request, atts)
-        return http.HttpResponse(template.render(context),
-                                 status=self.status_code)
+        return HttpResponse(template.render(context), status=self.status_code)
+
+    def get_oops_id(self, request):
+        oops_report = request.environ.get('oops.report', {})
+        return oops_report.get('id', 'OOPS ID not available.')
+
 
 server_error = ErrorPage(500)
 page_not_found = ErrorPage(404)

=== modified file 'src/reviewsapp/wsgi.py'
--- src/reviewsapp/wsgi.py	2012-06-20 12:19:49 +0000
+++ src/reviewsapp/wsgi.py	2013-03-01 18:28:22 +0000
@@ -1,25 +1,78 @@
+#   This file is part of Software Center Ratings and Reviews
+#   Copyright (C) 2010-2013 Canonical Ltd.
+#
+#   This program is free software: you can redistribute it and/or modify
+#   it under the terms of the GNU Affero General Public License as
+#   published by the Free Software Foundation, either version 3 of the
+#   License, or (at your option) any later version.
+#
+#   This program is distributed in the hope that it will be useful,
+#   but WITHOUT ANY WARRANTY; without even the implied warranty of
+#   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+#   GNU Affero General Public License for more details.
+#
+#   You should have received a copy of the GNU Affero General Public License
+#   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+"""wsgi stack for the Ratings and Reviews app."""
+
+import logging.config
+import oops_dictconfig
+import oops_wsgi
+import oops_wsgi.django
+import os
 import platform
+import uuid
 
 from django.conf import settings
-from django.core.handlers.wsgi import WSGIHandler
-
-from canonical.oops.wsgi import OopsWare
-
-
-class RatingsWSGIHandler(WSGIHandler):
-    def __init__(self):
-        """Setup logging for the SCA web app."""
-        super(RatingsWSGIHandler, self).__init__()
-
-app = RatingsWSGIHandler()
+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 webcatalog handler in an OOPS app."""
+    if settings.FORCE_HTTPS_IN_ENVIRONMENT:
+        os.environ['HTTPS'] = 'on'
+
     default_id = ''.join(x for x in platform.node() if x.isalpha())
     appserver_id = getattr(settings, 'APPSERVER_ID', default_id)
-    oops_dir = getattr(settings, 'OOPS_DIR', '/tmp')
-    oops_app = OopsWare(
-        app, oops_dir=oops_dir, key=appserver_id, hide_meta=False,
-        serializer_factory_name=(
-            'canonical.oops.serializer.OOPSRFC822Serializer'))
+    logging_config = settings.LOGGING_CONFIG_FILE
+    if logging_config:
+        logging.config.fileConfig(logging_config)
+
+    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