Launchpad itself

Merge lp://staging/~mbp/launchpad/show-timeline into lp://staging/launchpad

  1. show-timeline
  2. Merge into devel
Proposed by Martin Pool
Status: Merged
Approved by: Martin Pool
Approved revision: no longer in the source branch.
Merged at revision: 14511
Proposed branch: lp://staging/~mbp/launchpad/show-timeline
Merge into: lp://staging/launchpad
Diff against target: 159 lines (+62/-4)
7 files modified
lib/canonical/launchpad/icing/css/layout.css (+0/-1)
lib/canonical/launchpad/webapp/adapter.py (+13/-1)
lib/lp/app/browser/tales.py (+8/-0)
lib/lp/app/templates/base-layout-macros.pt (+27/-0)
lib/lp/app/templates/base-layout.pt (+6/-1)
lib/lp/app/templates/launchpad-loginstatus.pt (+6/-1)
lib/lp/translations/browser/tests/test_sharing_details.py (+2/-0)
To merge this branch: bzr merge lp://staging/~mbp/launchpad/show-timeline
Reviewer Review Type Date Requested Status
Robert Collins (community) Needs Fixing
Launchpad code reviewers Pending
Review via email: mp+80166@code.staging.launchpad.net

Commit message

[r=mbp][bug=886996] Show timeline in footer of page when visible_render_time is true

Description of the change

With this change, you can click the "%d queries/external actions" show to developers to actually see what the queries were. An HTML form of the request timeline is revealed at the bottom of the page. I think I would find this interesting or useful on pages that are slow but not failing, or that are intermittently oops.

I realize you can force it to oops, wait for the oops to sync, then look at it elsewhere but having the data right there seems easier and perhaps more likely to actually be inspected. Also, because performance depends on cache hits, retrying the request to get an oops may give different behaviour.

This just renders data already collected in the timeline so it should not affect performance very much.

It's not automatically tested yet. Pointers to where the tests ought to be added would be welcome.

I thought about highlighting the slow actions but I haven't done that either.

Clicking the link reveals the list but doesn't scroll to it; I have mixed feelings about whether it should, and in any case making the anchor the target and the javascript show it from onclick didn't seem to work on the first pageload. Advice welcome.

screenshots:
  http://people.canonical.com/~mbp/launchpad-screenshots/20111024_006.png
  http://people.canonical.com/~mbp/launchpad-screenshots/20111024_007.png

To post a comment you must log in.
Revision history for this message
Martin Pool (mbp) wrote :

I also looked into increasing the psql verbosity so that it tells you how the query was executed. It is possible to get this out of the client via storm, and to turn this on conditional on a feature flag. However, the debug log is extremely verbose and not very readable, so not obviously so helpful. I was hoping we would get something similar to explain analyze, but for free, while running the actual queries. Apparently not. Perhaps it's improved in a later postgresql release.

Revision history for this message
Robert Collins (lifeless) wrote :

Some pages will suffer from this, others will be fine (we have pages with 1000's of actions).

Also this is duplicated with the ++profile++show feature which also shows the timeline, I think its worth consolidating things to avoid duplication.

Broadly +1.

Revision history for this message
Martin Pool (mbp) wrote :

Also, this does put onto the screen and in to the html some content that may be sensitive, such as your session cookie. If someone carelessly puts up a screenshot or attaches the html to a bug it would be a security problem.

Options:
 - rely on developers being savvy about it
 - blacklist some particular queries or fields of queries
 - don't land this
 - make it more opt-in than just the flag - perhaps a session cookie that enables it

Revision history for this message
Robert Collins (lifeless) wrote :

I think that it needs two bits of work before a full review:
 - remove the duplication (which will also get you tests)
 - add in a cap so it doesn't go beyond 200 queries (thats -way- more than any page should need and more than enough to detect patterns of badness.

review: Needs Fixing
Revision history for this message
Martin Pool (mbp) wrote :

The formatting of ++profile++ is all done in profile.pt, which is very hardcoded to drawing profiles in particular: one 300 line function does all the preparation, and one template with all the results in line. It doesn't look very welcoming of reuse and this patch is small...

Revision history for this message
Martin Pool (mbp) wrote :

This update chops it off at 200 actions.

Tearing apart profile.pt so it can be reused looks like it will make this patch much larger. I'd like to ask if we could land this, see if it is actually useful for understanding production performance, and then if it is, merge them together, and otherwise pull it back out.

Revision history for this message
Martin Pool (mbp) wrote :

<lifeless> mmm
 If you want to land it, and then either refactor or remove it in january sometime, thats fine
 I don't want deliberate duplication in this area; it is as you note already clumsy, and duplication around it makes that worse.
 so I'm *not* ok with landing it and then forgetting about it.
<poolie> ok, deal
 i promise to at minimum delete it
<lifeless> ok
 thank you

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.