Merge lp:~james-w/pkgme-service/oops-integration into lp:pkgme-service
Status: | Rejected |
---|---|
Rejected by: | James Westby |
Proposed branch: | lp:~james-w/pkgme-service/oops-integration |
Merge into: | lp:pkgme-service |
Prerequisite: | lp:~james-w/pkgme-service/log-oopses |
Diff against target: |
311 lines (+123/-42) 11 files modified
dev_config/manifests/pkgme_service.pp (+8/-14) dev_config/templates/django.wsgi.erb (+42/-10) dev_config/templates/production_credentials.cfg.erb (+15/-0) dev_config/templates/production_paths.py.erb (+0/-13) django_project/dev.cfg (+10/-0) django_project/main.cfg (+1/-0) django_project/manage.py (+22/-4) django_project/production.cfg (+5/-0) django_project/production_credentials.cfg.example (+15/-0) fabtasks/deploy.py (+3/-1) src/djpkgme/views.py (+2/-0) |
To merge this branch: | bzr merge lp:~james-w/pkgme-service/oops-integration |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Canonical Consumer Applications Hackers | Pending | ||
Review via email: mp+90033@code.launchpad.net |
Description of the change
Hi,
Here's a sketch of how oops integration works for a django app.
As it stands this hooks in at the wsgi layer, so it doesn't have
any effect on a local environment (with runserver.)
As it is written it just writes the oops to the filesystem. It's
easy to have it write the oops to rabbit instead I think.
You can spin up this branch in ec2, hit /pkgme/+oops and see
an oops appear in ~/pkgme-
As it stands the caller doesn't get told the oops id, that's a bug in django.
I'm putting an irc log from the end where lifeless and jamesh discuss
ways of doing it.
I'm going to set this to "Work in progress" as I don't think we should
land it as-is. I mainly did this to have a better understanding for discussing
it on the IS call tomorrow.
Thanks,
James
<james_w> lifeless, have you looked in to how to encourage django to use a response with the oops id on an uncaught exception?
<james_w> if you don't provide a 500 template then you get the oops one, but the oops it points to is an exception complaining that you don't have a 500 template
<lifeless> jamesh: has
<lifeless> oh, for oops-tools itself I know we need to provide a 500 template
<lifeless> there is a patch for django, last I checked it was a bit stalled
<lifeless> https:/
<james_w> oh, I thought that was what the django.py fixed
<lifeless> it works around it
<james_w> http://
<lifeless> https:/
<lifeless> thats the actual fix
<lifeless> the django module in oops-wsgi sniffs the exception to ensure you get an oops
<lifeless> but it can't influence django to make it expose the error to the wsgi stack
<jamesh> james_w: you mean to display the OOPS ID on the error page?
<lifeless> so the main oops module just sees a successful reponse
<james_w> jamesh, yeah
<james_w> lifeless, ah, I see
<jamesh> james_w: I haven't been able to work out a good way to solve that with the new infrastructure
<james_w> so if it were properly fixed then oops' response would take over
<james_w> ok, it's not so important to us at this point
<lifeless> james_w: do you get an OOPS emitted to (rabbit|disk)
<james_w> lifeless, yep
<jamesh> the old oops code we have in U1 can preallocate the oops IDs on demand, so we can use that in the error page
<lifeless> jamesh: we could allow that too, if you wanted to use a timeuuid or something, and put the id in the context
<jamesh> one idea I had for the new code might be to set a short lived cookie in the WSGI middleware, and look that up with JS in the error page
<lifeless> jamesh: ooh, nice.
<jamesh> that has the downside that cookies are shared between all tabs in a browser
<james_w> lifeless, for this patch, how much do you care about a test?
<james_w> it's not the easiest thing to test after all
<lifeless> 'meh'
<jamesh> so if you open 20 pages at once and five fail, you might see the same OOPS ID on each
<lifeless> yeah
<lifeless> uhm
* poolie has quit (Quit: Ex-Chat)
<lifeless> wonder if you can inspect the response headers from the page itself
<lifeless> jamesh: shoving id in the context would be fine.
<lifeless> james_w: should be easy to test if you want to; just have an outer start_response(
<james_w> lifeless, ah, good point
<jamesh> I don't think there is a way to inspect response headers unfortunately
<lifeless> so, a django error template that puts content[
<lifeless> jamesh: ^
<wgrant> Hmm
<wgrant> Regression
<wgrant> 5633 AssertionError: Ambiguous view name.
<wgrant> GET: 5629 Other: 4 Robots: 98 Local: 239
<wgrant> 168 http://
<wgrant> OOPS-00033c67366d1e7513397953f71c9b29, OOPS-001af265e7d5322fdd12c9b12b4fed8c, OOPS-00ae6fedb93aa14a7fc508d51b9fa6a7, OOPS-010f606986cdc5008d999fabe085e6aa, OOPS-01432c961a9928b26bb0d210d723a53b
<wgrant> Ah
<wgrant> I bet it's the bug listings release
<wgrant> It doesn't work with feeds.
<wgrant> Which are always anonymous, so were never tested.
<wgrant> lifeless: ^^
<lifeless> grah
<lifeless> I guess we need to rollback
<jamesh> lifeless: I guess that would work. Do we lose anything by having the view assign an ID instead of the publisher doing so?
<wgrant> lifeless: It's been broken for a day, and it's a major thing.
<wgrant> "it" == bug listings
<wgrant> So I think we should keep it on.
<wgrant> It's not as if anybody uses feeds anyway.
<lifeless> well, 5633 polls for them
<lifeless> wgrant: lets start with a bug
<lifeless> jamesh: I think its fine to allow the earliest point that knows an error is on the way to assign an id - the hash based aproach is just one way to get a unique id
<lifeless> jamesh: oops can render errors itself
<lifeless> jamesh: or you could pass a django template into oops
<lifeless> jamesh: or you can assign the id earlier, if the framework really wants to render its own errors
<lifeless> jamesh: the existing facilities are the trivial string template that oops has itself or a callback you can supply to the middleware constructor
<jamesh> lifeless: I think giving frameworks the ability to render their own error pages makes python-oops-wsgi more appealing, since it can be dropped into an existing app
<lifeless> jamesh: I think the first thing I would attempt would be to discard the django error page but wire up the callback to let me use a django template to render the error page
<james_w> so, I can't do this patch as I am yet to get close to having a working buildout
<wgrant> lifeless: Hm
<lifeless> james_w: oh? what happens?
<wgrant> 'bugs.dynamic_
<lifeless> wgrant: might work, we should try
<wgrant> That seems to be the only popular affected page.
<jamesh> lifeless: well, once we've got that django bug report I filed fixed, that should be possible: if start_response() is called with an exc_info, just re-raise it
<lifeless> jamesh: that seems to be slightly different
<jamesh> then present an error page after the exception bubbles back up to the middleware
<lifeless> oh right
<lifeless> so yes, get the middleware's start_response to be called with exc_info
<jamesh> with that in mind, the debug error page Django presents is pretty nice
<lifeless> ah, make_app(
<jamesh> so when developing an app, I'd prefer to see that than an oops-wsgi error page
<lifeless> jamesh: sure, but not for deployed apps ?
<lifeless> jamesh: so have dev mode eat the exceptions and render itself
<lifeless> jamesh: anyhow, I'm happy to let the app allocate ids
<lifeless> pragmatic
<james_w> lifeless, confusing error messages
<jamesh> lifeless: I'm of two minds there. The error pages we've got wired up in U1 don't do much , so would be pretty easy to reimplement in the middleware
<lifeless> james_w: pastebin ?
<james_w> apparently when it said I might have misspelled a dependency, it really meant that it couldn't install from the cache because the cache is currently empty
<lifeless> james_w: you're using the LP cache, right ?
<jamesh> on the other hand, if I could just get/create an OOPS ID in the existing error page code, that would be a much smaller change
<james_w> lifeless, nope
<lifeless> james_w: use the LP cache
<jamesh> and I suspect other people trying to integrate python-oops would feel the same
<lifeless> jamesh: these are not exclusive options
<jamesh> lifeless: I understand that.
<lifeless> :)
<lifeless> I think we should make it easy to get going
<jamesh> I'm just thinking out loud about why I might pick one option over the other
<lifeless> with better/more organised stuff being something you can grow into
<lifeless> jamesh: I'm glad you are doing so :) I was just adding commentary
Unmerged revisions
- 41. By James Westby
-
Try out the new oops stuff.
- 40. By James Westby
-
Install the extra packages (until pkgme-service-
dependencies depends on them) - 39. By James Westby
-
Merge the log oops branch.
- 38. By James Westby
-
Add a basic oops integration to the wsgi for devstaging deployments.