Skip to content
This repository
Browse code

Added middleware to catch ES exceptions (bug 767578)

  • Loading branch information...
commit 016d37c0a076ecb2d1fcc0e5880676b6360cb6c0 1 parent e970d07
Rob Hudson robhudson authored
2  apps/amo/decorators.py
@@ -81,6 +81,7 @@ def wrapper(request, *args, **kw):
81 81 return wrapper
82 82 return decorator
83 83
  84 +
84 85 def restricted_content(f):
85 86 """
86 87 Prevent access to a view function for accounts restricted from
@@ -97,7 +98,6 @@ def wrapper(request, *args, **kw):
97 98 return wrapper
98 99
99 100
100   -
101 101 def modal_view(f):
102 102 @functools.wraps(f)
103 103 def wrapper(*args, **kw):
3  apps/amo/search.py
... ... @@ -1,11 +1,10 @@
1 1 import logging
2 2 from operator import itemgetter
3 3
4   -from django.conf import settings
5   -
6 4 import elasticutils
7 5 from django_statsd.clients import statsd
8 6
  7 +
9 8 log = logging.getLogger('z.es')
10 9
11 10
17 apps/search/middleware.py
... ... @@ -0,0 +1,17 @@
  1 +import logging
  2 +
  3 +import jingo
  4 +from pyes.exceptions import ElasticSearchException
  5 +from pyes.urllib3.connectionpool import HTTPError
  6 +
  7 +
  8 +log = logging.getLogger('z.es')
  9 +
  10 +
  11 +class ElasticsearchExceptionMiddleware(object):
  12 +
  13 + def process_exception(self, request, exception):
  14 + if (issubclass(exception.__class__, (ElasticSearchException,
  15 + HTTPError))):
  16 + log.error(u'Elasticsearch error: %s' % exception)
  17 + return jingo.render(request, 'search/down.html', status=503)
8 apps/search/templates/search/down.html
... ... @@ -0,0 +1,8 @@
  1 +{% extends "impala/base_shared.html" %}
  2 +
  3 +{% block content %}
  4 +<section id="search-down" class="main">
  5 + <h1>{{ _('Search Unavailable') }}</h1>
  6 + <p>{{ _('Search is temporarily unavailable. Please try again in a few minutes.') }}</p>
  7 +</section>
  8 +{% endblock %}
36 apps/search/tests/test_middleware.py
... ... @@ -0,0 +1,36 @@
  1 +import jingo
  2 +import mock
  3 +from nose.tools import eq_
  4 +from pyes.exceptions import ElasticSearchException, IndexMissingException
  5 +from pyes.urllib3.connectionpool import HTTPError, MaxRetryError, TimeoutError
  6 +from test_utils import RequestFactory
  7 +
  8 +import amo.tests
  9 +from search.middleware import ElasticsearchExceptionMiddleware as ESM
  10 +
  11 +
  12 +class TestElasticsearchExceptionMiddleware(amo.tests.TestCase):
  13 +
  14 + def setUp(self):
  15 + self.request = RequestFactory()
  16 +
  17 + @mock.patch.object(jingo, 'render')
  18 + def test_exceptions_we_catch(self, jingo_mock):
  19 + # These are instantiated with an error string.
  20 + for e in [ElasticSearchException, IndexMissingException]:
  21 + ESM().process_exception(self.request, e('ES ERROR'))
  22 + jingo_mock.assert_called_with(self.request, 'search/down.html',
  23 + status=503)
  24 + jingo_mock.reset_mock()
  25 +
  26 + # These are just Exception classes.
  27 + for e in [HTTPError, MaxRetryError, TimeoutError]:
  28 + ESM().process_exception(self.request, e('ES ERROR'))
  29 + jingo_mock.assert_called_with(self.request, 'search/down.html',
  30 + status=503)
  31 + jingo_mock.reset_mock()
  32 +
  33 + @mock.patch.object(jingo, 'render')
  34 + def test_exceptions_we_do_not_catch(self, jingo_mock):
  35 + ESM().process_exception(self.request, Exception)
  36 + eq_(jingo_mock.called, False)
1  lib/settings_base.py
@@ -322,6 +322,7 @@ def JINJA_CONFIG():
322 322 'django.contrib.messages.middleware.MessageMiddleware',
323 323 'django.contrib.auth.middleware.AuthenticationMiddleware',
324 324 'commonware.log.ThreadRequestMiddleware',
  325 + 'apps.search.middleware.ElasticsearchExceptionMiddleware',
325 326 'session_csrf.CsrfMiddleware',
326 327
327 328 # This should come after authentication middleware
8 mkt/search/templates/search/down.html
... ... @@ -0,0 +1,8 @@
  1 +{% extends "mkt/base.html" %}
  2 +
  3 +{% block content %}
  4 +<section id="search-down" class="c main">
  5 + <h1>{{ _('Search Unavailable') }}</h1>
  6 + <p>{{ _('Search is temporarily unavailable. Please try again in a few minutes.') }}</p>
  7 +</section>
  8 +{% endblock %}
2  mkt/search/views.py
@@ -4,9 +4,9 @@
4 4 from tower import ugettext as _
5 5
6 6 import amo
7   -from amo.urlresolvers import reverse
8 7 import amo.utils
9 8 from amo.decorators import json_view
  9 +from amo.urlresolvers import reverse
10 10 from apps.addons.models import Category
11 11 from apps.search.views import name_query, WebappSuggestionsAjax
12 12 from mkt.webapps.models import Webapp

5 comments on commit 016d37c

Christopher Van
Collaborator
cvan commented on 016d37c

:heart: it!

Andy McKay
Owner

Will we get a log or a statsd ping of the number of times this occurs?

Rob Hudson
Collaborator

There's a log in the middleware. We could add statsd?

Andy McKay
Owner

It looks like there will be a 503 statsd ping, so all good.

Christopher Van
Collaborator
cvan commented on 016d37c

can we add like one traceback to sentry? I don't know. or is a statsd ping enough?

Andy McKay

you win. longest test case name ever?

Kumar McMillan

The name TestESExceptionMiddleware :cry: from rejection

Please sign in to comment.
Something went wrong with that request. Please try again.