Merge lp:~james-w/python-oops-tools/recent-oopses into lp:python-oops-tools

Proposed by James Westby on 2014-01-14
Status: Needs review
Proposed branch: lp:~james-w/python-oops-tools/recent-oopses
Merge into: lp:python-oops-tools
Diff against target: 142 lines (+28/-58)
2 files modified
src/oopstools/oops/templates/report.html (+8/-23)
src/oopstools/oops/views.py (+20/-35)
To merge this branch: bzr merge lp:~james-w/python-oops-tools/recent-oopses
Reviewer Review Type Date Requested Status
Natalia Bidart (community) Approve on 2014-01-14
python-oops-tools reviewers 2014-01-14 Pending
Review via email: mp+201639@code.launchpad.net

Commit message

Modify the recent oopses query to perform better with lots of oopses.

When there is a report with many prefixes, and the most recent oopses are
not for any of those prefixes postgres will pick a query plan that performs
poorly (not using the most selective index, assuming that the prefix ids
are distributed fairly uniformly through the oopses.)

We instead trick postgres in to using the more selective index using a window
function. Thanks to stub for the query.

We do a second query to load all the infestations for these oopses, to prevent
the ORM from doing 50 queries as we iterate over the list of oopses.

We also drop the pagination because it is not as simple to implement with the
raw query. It could be re-instated if it was really needed, but usually
I find looking at the most recent 50 oopses is sufficient.

Lastly, I re-ordered the columns in the oops listing, to put the oops
first as that is usually what you want to click on. I also dropped the linking
of the URL, as the escaping meant that it never worked, and the URL is often
not something you can simply click on either. The fact that the URL was first
in the table and a link meant that I often clicked on that rather than the
oops, wasting time.

Description of the change

Hi,

This is a few changes to hopefully improve the performance of some recent
oops views. The commit message has all the details.

Thanks,

James

To post a comment you must log in.
Natalia Bidart (nataliabidart) wrote :

Looks good!

review: Approve
Launchpad PQM Bot (launchpad-pqm) wrote :
Download full text (84.9 KiB)

The attempt to merge lp:~james-w/python-oops-tools/recent-oopses into lp:python-oops-tools failed.Below is the output from the failed tests.

find . -type f -name '*.py[co]' -exec rm -f {} \;
rm -f bin/buildout
rm -f src/oopstools/settings.py
bzr clean-tree --unknown --force
bzr up download-cache
python bootstrap.py \
  --setup-source=download-cache/ez_setup.py \
  --download-base=download-cache/dist --eggs=eggs \
  --version=1.5.2
Generated script '/home/pqm/pqm-workdir/oops-tools/bin/buildout'.
bin/buildout configuration:db-port=5433
Develop: '/home/pqm/pqm-workdir/oops-tools/.'
Uninstalling filetemplates.
Updating scripts.
Installing filetemplates.
Updating django.
Updating docs.
Updating tags.
bin/test
Creating test database for alias 'default'...
Destroying test database for alias 'default'...

W: line 4 [buildbot-staging]: Deprecated key 'location' used
I: This option will be removed in the future; please update your configuration
W: line 2 [pqm]: Deprecated key 'location' used
I: This option will be removed in the future; please update your configuration
W: line 3 [pqm-oops-tools]: Deprecated key 'location' used
I: This option will be removed in the future; please update your configuration
W: line 1 [precise]: Deprecated key 'location' used
I: This option will be removed in the future; please update your configuration
I: [pqm-oops-tools chroot] Running command: "cd /home/pqm/pqm-workdir/oops-tools && make check"
Nothing to delete.
+N dist/bzr-2.6.0.tar.gz
+N dist/d2to1-0.2.10.tar.gz
+N dist/dkimpy-0.5.4.tar.gz
+N dist/keyring-1.2.2-notestrunner.zip
+N dist/mock-1.0.1.tar.gz
+N dist/oslo.config-1.1.1.tar.gz
+N dist/pbr-0.5.20.tar.gz
+N dist/pip-1.4.tar.gz
+N dist/prettytable-0.7.2.tar.bz2
+N dist/python-keystoneclient-0.3.1.tar.gz
+N dist/python-swiftclient-1.5.0.tar.gz
+N dist/python_keystoneclient-0.3.1-py2.6.egg
+N dist/python_keystoneclient-0.3.1-py2.7.egg
+N dist/python_swiftclient-1.5.0-py2.6.egg
+N dist/python_swiftclient-1.5.0-py2.7.egg
+N dist/requests-1.2.3.tar.gz
+N dist/s4-0.1.2.tar.gz
+N dist/setuptools-git-1.0.tar.gz
+N dist/six-1.3.0.tar.gz
+N dist/yui_3.10.3.zip
All changes applied successfully.
Updated to revision 568 of branch http://bazaar.launchpad.net/~launchpad/lp-source-dependencies/trunk
/home/pqm/pqm-workdir/oops-tools/eggs/Django-1.3.3-py2.6.egg/django/template/loaders/filesystem.py:58: DeprecationWarning: 'django.template.loaders.filesystem.load_template_source' is deprecated; use 'django.template.loaders.filesystem.Loader' instead.
  DeprecationWarning
/home/pqm/pqm-workdir/oops-tools/eggs/Django-1.3.3-py2.6.egg/django/template/loaders/app_directories.py:71: DeprecationWarning: 'django.template.loaders.app_directories.load_template_source' is deprecated; use 'django.template.loaders.app_directories.Loader' instead.
  DeprecationWarning
/home/pqm/pqm-workdir/oops-tools/eggs/Django-1.3.3-py2.6.egg/django/contrib/auth/__init__.py:26: DeprecationWarning: Authentication backends without a `supports_object_permissions` attribute are deprecated. Please define it in <class 'django_openid_auth.auth.OpenIDBackend'>.
  DeprecationWarning)
/home/pqm/pqm-workdir/oops-tools/eggs/Django-1.3.3-py2.6.egg/d...

Launchpad PQM Bot (launchpad-pqm) wrote :
Download full text (7.1 KiB)

The attempt to merge lp:~james-w/python-oops-tools/recent-oopses into lp:python-oops-tools failed.Below is the output from the failed tests.

find . -type f -name '*.py[co]' -exec rm -f {} \;
rm -f bin/buildout
rm -f src/oopstools/settings.py
bzr clean-tree --unknown --force
bzr up download-cache
python bootstrap.py \
  --setup-source=download-cache/ez_setup.py \
  --download-base=download-cache/dist --eggs=eggs \
  --version=1.5.2
Generated script '/home/pqm/pqm-workdir/oops-tools/bin/buildout'.
bin/buildout configuration:db-port=5433
Develop: '/home/pqm/pqm-workdir/oops-tools/.'
Uninstalling filetemplates.
Updating scripts.
Installing filetemplates.
Updating django.
Updating docs.
Updating tags.
bin/test
Creating test database for alias 'default'...
Destroying test database for alias 'default'...

W: line 4 [buildbot-staging]: Deprecated key 'location' used
I: This option will be removed in the future; please update your configuration
W: line 2 [pqm]: Deprecated key 'location' used
I: This option will be removed in the future; please update your configuration
W: line 3 [pqm-oops-tools]: Deprecated key 'location' used
I: This option will be removed in the future; please update your configuration
W: line 1 [precise]: Deprecated key 'location' used
I: This option will be removed in the future; please update your configuration
I: [pqm-oops-tools chroot] Running command: "cd /home/pqm/pqm-workdir/oops-tools && make check"
Nothing to delete.
+N dist/cssselect-0.9.1.tar.gz
+N dist/subvertpy-0.9.1.tar.gz
All changes applied successfully.
Updated to revision 569 of branch http://bazaar.launchpad.net/~launchpad/lp-source-dependencies/trunk
/home/pqm/pqm-workdir/oops-tools/eggs/Django-1.3.3-py2.6.egg/django/template/loaders/filesystem.py:58: DeprecationWarning: 'django.template.loaders.filesystem.load_template_source' is deprecated; use 'django.template.loaders.filesystem.Loader' instead.
  DeprecationWarning
/home/pqm/pqm-workdir/oops-tools/eggs/Django-1.3.3-py2.6.egg/django/template/loaders/app_directories.py:71: DeprecationWarning: 'django.template.loaders.app_directories.load_template_source' is deprecated; use 'django.template.loaders.app_directories.Loader' instead.
  DeprecationWarning
/home/pqm/pqm-workdir/oops-tools/eggs/Django-1.3.3-py2.6.egg/django/contrib/auth/__init__.py:26: DeprecationWarning: Authentication backends without a `supports_object_permissions` attribute are deprecated. Please define it in <class 'django_openid_auth.auth.OpenIDBackend'>.
  DeprecationWarning)
/home/pqm/pqm-workdir/oops-tools/eggs/Django-1.3.3-py2.6.egg/django/contrib/auth/__init__.py:31: DeprecationWarning: Authentication backends without a `supports_anonymous_user` attribute are deprecated. Please define it in <class 'django_openid_auth.auth.OpenIDBackend'>.
  DeprecationWarning)
/home/pqm/pqm-workdir/oops-tools/eggs/Django-1.3.3-py2.6.egg/django/contrib/auth/models.py:393: DeprecationWarning: The user messaging API is deprecated. Please update your code to use the new messages framework.
  category=DeprecationWarning)
........................................E.EE
======================================================================
ERROR: test_no_recent_o...

Read more...

Launchpad PQM Bot (launchpad-pqm) wrote :
Download full text (9.1 KiB)

The attempt to merge lp:~james-w/python-oops-tools/recent-oopses into lp:python-oops-tools failed.Below is the output from the failed tests.

find . -type f -name '*.py[co]' -exec rm -f {} \;
rm -f bin/buildout
rm -f src/oopstools/settings.py
bzr clean-tree --unknown --force
bzr up download-cache
python bootstrap.py \
  --setup-source=download-cache/ez_setup.py \
  --download-base=download-cache/dist --eggs=eggs \
  --version=1.5.2
Generated script '/home/pqm/pqm-workdir/oops-tools/bin/buildout'.
bin/buildout configuration:db-port=5433
Develop: '/home/pqm/pqm-workdir/oops-tools/.'
Uninstalling filetemplates.
Updating scripts.
Installing filetemplates.
Updating django.
Updating docs.
Updating tags.
bin/test
Creating test database for alias 'default'...
Destroying test database for alias 'default'...

W: line 4 [buildbot-staging]: Deprecated key 'location' used
I: This option will be removed in the future; please update your configuration
W: line 2 [pqm]: Deprecated key 'location' used
I: This option will be removed in the future; please update your configuration
W: line 3 [pqm-oops-tools]: Deprecated key 'location' used
I: This option will be removed in the future; please update your configuration
W: line 1 [precise]: Deprecated key 'location' used
I: This option will be removed in the future; please update your configuration
I: [pqm-oops-tools chroot] Running command: "cd /home/pqm/pqm-workdir/oops-tools && make check"
Nothing to delete.
Tree is up to date at revision 569 of branch http://bazaar.launchpad.net/~launchpad/lp-source-dependencies/trunk
/home/pqm/pqm-workdir/oops-tools/eggs/Django-1.3.3-py2.6.egg/django/template/loaders/filesystem.py:58: DeprecationWarning: 'django.template.loaders.filesystem.load_template_source' is deprecated; use 'django.template.loaders.filesystem.Loader' instead.
  DeprecationWarning
/home/pqm/pqm-workdir/oops-tools/eggs/Django-1.3.3-py2.6.egg/django/template/loaders/app_directories.py:71: DeprecationWarning: 'django.template.loaders.app_directories.load_template_source' is deprecated; use 'django.template.loaders.app_directories.Loader' instead.
  DeprecationWarning
/home/pqm/pqm-workdir/oops-tools/eggs/Django-1.3.3-py2.6.egg/django/contrib/auth/__init__.py:26: DeprecationWarning: Authentication backends without a `supports_object_permissions` attribute are deprecated. Please define it in <class 'django_openid_auth.auth.OpenIDBackend'>.
  DeprecationWarning)
/home/pqm/pqm-workdir/oops-tools/eggs/Django-1.3.3-py2.6.egg/django/contrib/auth/__init__.py:31: DeprecationWarning: Authentication backends without a `supports_anonymous_user` attribute are deprecated. Please define it in <class 'django_openid_auth.auth.OpenIDBackend'>.
  DeprecationWarning)
/home/pqm/pqm-workdir/oops-tools/eggs/Django-1.3.3-py2.6.egg/django/contrib/auth/models.py:393: DeprecationWarning: The user messaging API is deprecated. Please update your code to use the new messages framework.
  category=DeprecationWarning)
........................................E.EE
======================================================================
ERROR: test_no_recent_oopses (test_report.ReportTests)
-------------------------------------------------------...

Read more...

Unmerged revisions

59. By James Westby on 2014-01-31

Correct the variable name.

58. By James Westby on 2014-01-30

Revert to old python syntax until tests are run on a modern python.

57. By James Westby on 2014-01-14

Use more expressions. Thanks nessita.

56. By James Westby on 2014-01-14

Modify the recent oopses query to perform better with lots of oopses.

When there is a report with many prefixes, and the most recent oopses are
not for any of those prefixes postgres will pick a query plan that performs
poorly (not using the most selective index, assuming that the prefix ids
are distributed fairly uniformly through the oopses.)

We instead trick postgres in to using the more selective index using a window
function. Thanks to stub for the query.

We do a second query to load all the infestations for these oopses, to prevent
the ORM from doing 50 queries as we iterate over the list of oopses.

We also drop the pagination because it is not as simple to implement with the
raw query. It could be re-instated if it was really needed, but usually
I find looking at the most recent 50 oopses is sufficient.

Lastly, I re-ordered the columns in the oops listing, to put the oops
first as that is usually what you want to click on. I also dropped the linking
of the URL, as the escaping meant that it never worked, and the URL is often
not something you can simply click on either. The fact that the URL was first
in the table and a link meant that I often clicked on that rather than the
oops, wasting time.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/oopstools/oops/templates/report.html'
2--- src/oopstools/oops/templates/report.html 2012-11-29 21:33:52 +0000
3+++ src/oopstools/oops/templates/report.html 2014-01-31 16:38:48 +0000
4@@ -17,37 +17,22 @@
5 <table>
6 <th><tr>
7 <td>Date/Time</td>
8+ <td>Oops</td>
9+ <td>Exception Type</td>
10+ <td>Duration (ms)</td>
11 <td>HTTP Method</td>
12 <td>URL</td>
13- <td>Duration (ms)</td>
14- <td>Exception Type</td>
15- <td>Oops</td>
16 </tr></th>
17- {% for oops in recent.object_list %}
18+ {% for oops, exception_type in recent %}
19 <tr>
20 <td>{{oops.date|date:"Y-m-d H:i:s"}}</td>
21+ <td><a href="{% url oopstools.oops.views.index %}?oopsid={{oops.oopsid}}">{{oops.oopsid}}</a></td>
22+ <td>{{exception_type}}</td>
23+ <td>{{oops.total_time}}</td>
24 <td>{{oops.http_method}}</td>
25- <td>{% if oops.url %}<a href="{{oops.url}}">{{oops.url}}</a>{% endif %}</td>
26- <td>{{oops.total_time}}</td>
27- <td>{{oops.exception_type}}</td>
28- <td><a href="{% url oopstools.oops.views.index %}?oopsid={{oops.oopsid}}">{{oops.oopsid}}</a></td>
29+ <td>{% if oops.url %}{{oops.url}}{% endif %}</td>
30 </tr>
31 {% endfor %}
32 </table>
33- <div class="pagination">
34- <span class="step-links">
35- {% if recent.has_previous %}
36- <a href="?page={{ recent.previous_page_number }}">newer</a>
37- {% endif %}
38-
39- <span class="current">
40- Page {{ recent.number }} of {{ recent.paginator.num_pages }}.
41- </span>
42-
43- {% if recent.has_next %}
44- <a href="?page={{ recent.next_page_number }}">older</a>
45- {% endif %}
46- </span>
47- </div>
48 </body>
49 </html>
50
51=== modified file 'src/oopstools/oops/views.py'
52--- src/oopstools/oops/views.py 2012-12-06 01:07:17 +0000
53+++ src/oopstools/oops/views.py 2014-01-31 16:38:48 +0000
54@@ -18,13 +18,12 @@
55 from datetime import datetime, timedelta
56
57 from django.conf import settings
58-from django.core.paginator import Paginator, EmptyPage, PageNotAnInteger
59 from django.http import Http404, HttpResponseRedirect
60 from django.shortcuts import render_to_response
61
62 from oopstools.oops import dbsummaries
63 from oopstools.oops.helpers import parsedate
64-from oopstools.oops.models import Oops, Report
65+from oopstools.oops.models import Infestation, Oops, Report
66 from oopstools.oops.prefixloader import PrefixLoader
67 from oopstools.oops.oopsstore import OopsStore, OopsNotFound
68
69@@ -97,30 +96,23 @@
70 "prefixes": prefixes})
71
72
73-def get_page_from_query_args(paginator, query_args):
74- """Get the page from ``paginator`` specified in the query params.
75-
76- If the request specified ``page`` in the query params, then that
77- page of the paginator will be retrieved.
78-
79- This function aims to return a page regardless of the page requested,
80- e.g. requesting a page beyond the last page will just return the
81- last page.
82-
83- :param paginator: a ``Paginator`` object.
84- :param query_args: a ``dict`` of query params.
85- :return: a ``Page`` from the ``Paginator``.
86- """
87- page_num = query_args.get('page', 1)
88- try:
89- page = paginator.page(page_num)
90- except PageNotAnInteger:
91- # If page is not an integer, deliver first page.
92- page = paginator.page(1)
93- except EmptyPage:
94- # If page is out of range (e.g. 9999), deliver last page of results.
95- page = paginator.page(paginator.num_pages)
96- return page
97+def get_recent_oops_info(report):
98+ prefix_ids = list(report.prefixes.values_list('pk', flat=True))
99+ recent_oopses = list(Oops.objects.raw("""
100+ SELECT whatever.*, pos FROM (
101+ select *, rank() OVER w AS pos
102+ FROM oops_oops
103+ WHERE "oops_oops"."prefix_id" = ANY(%s)
104+ WINDOW w AS (PARTITION BY prefix_id ORDER BY date DESC)
105+ ) AS whatever
106+ WHERE pos <= 50
107+ ORDER BY date DESC
108+ LIMIT 50;
109+ """, [prefix_ids]))
110+ infestation_pks = set([oops.oopsinfestation_id for oops in recent_oopses])
111+ infestations = Infestation.objects.filter(id__in=infestation_pks).all()
112+ exception_types = dict([(i.id, i.exception_type) for i in infestations])
113+ return [(oops, exception_types[oops.oopsinfestation_id]) for oops in recent_oopses]
114
115
116 def report(request, report_name):
117@@ -135,17 +127,12 @@
118 dates = []
119 for day in range(1, 11):
120 dates.append(now - timedelta(day))
121- prefix_ids = list(r.prefixes.values_list('pk', flat=True))
122- oopses = Oops.objects.filter(
123- prefix__in=prefix_ids).select_related(
124- 'oopsinfestation').order_by('-date')
125- paginator = Paginator(oopses, 50)
126- recent_oopses = get_page_from_query_args(paginator, request.GET)
127+ recent = get_recent_oops_info(r)
128 data = {
129 'report': r,
130 'dates': dates,
131 'SUMMARY_URI': settings.SUMMARY_URI,
132- 'recent': recent_oopses,
133+ 'recent': recent,
134 }
135 return render_to_response("report.html", dictionary=data)
136
137@@ -184,5 +171,3 @@
138 }
139 return render_to_response("summary.html", dictionary=data)
140
141-
142-

Subscribers

People subscribed via source and target branches

to all changes: