Merge lp://staging/~gary/launchpad/sqlprofiler into lp://staging/launchpad

Proposed by Gary Poster
Status: Merged
Approved by: Gary Poster
Approved revision: no longer in the source branch.
Merged at revision: 13808
Proposed branch: lp://staging/~gary/launchpad/sqlprofiler
Merge into: lp://staging/launchpad
Diff against target: 432 lines (+151/-73)
4 files modified
lib/canonical/launchpad/webapp/adapter.py (+30/-9)
lib/lp/services/profile/profile.py (+13/-4)
lib/lp/services/stacktrace.py (+56/-42)
lib/lp/services/tests/test_stacktrace.py (+52/-18)
To merge this branch: bzr merge lp://staging/~gary/launchpad/sqlprofiler
Reviewer Review Type Date Requested Status
Benji York (community) code Approve
Review via email: mp+72765@code.staging.launchpad.net

Commit message

[bug=833972,834466] [r=benji][bug=833972] Fix ++profile++sqltrace for homepage, others

Description of the change

This branch fixes a problem observed in the QA for bug 832215: ++profile++sqltrace would cause Apache to fall over if used on some pages, such as the Launchpad home page. The problem was that strings need to be rendered immediately. This branch does that, as well as adding a quick hack to see a traceback if something like this happens again when trying to see a ++profile++ page.

lint is happy.

Tests are there for all of the core stacktrace changes. I did not add tests for the emergency hatch in ++profile++ traceback rendering, which I added for diagnosis of this problem and left in because it was convenient.

QA: go to https://qastaging.launchpad.net/++profile++sqltrace and actually see that it works. If it doesn't, I suggest that we still mark the branch qa-ok even as we mark the failure in the bug: this should not affect production.

Thank you

To post a comment you must log in.
Revision history for this message
Benji York (benji) wrote :

This branch looks good. Here are a couple of little things:

I would add MemoryError to the (SystemExit, KeyboardInterrupt)
exceptions. Since that tuple is repeated several times you might want
to define a "constant" for it as well.

I don't understand the comment near the end of the diff: "This should
never happen."

review: Approve (code)
Revision history for this message
Gary Poster (gary) wrote :

Change made, thanks.

comment: because non-iterating warnings are caught earlier in the pipeline of functions now, the rendering code should not actually encounter this.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.