support a profile decorator for use in staging and development environments

Bug #598289 reported by Robert Collins
10
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Launchpad itself
Fix Released
High
Gary Poster

Bug Description

08:11 < lifeless> you know the ++oops++ magic decorator thingy
08:11 < lifeless> ^ technical term
08:11 < sinzui> I do
08:11 < lifeless> what might be really nice
08:11 < lifeless> is a magical ++profile++ decorator enabled only on staging (and for non-devs it turns itself off once the user is determined)
08:12 < mars> ooo
08:12 < lifeless> it could write the profile to a directory that gets rsynced to devpad
08:12 < mars> +1
08:12 < lifeless> and put the filename in the timing block at the end of the page
08:12 < lifeless> so you could just do this
08:12 < lifeless> wait a couple of mintes
08:12 < lifeless> and have a kcachegrind ready analysis
08:13 -!- brianchidester [~brianchid@nat/canonical/x-iitfxemoircxxppr] has quit [Quit: Ex-Chat]
08:13 < sinzui> thanks
08:13 < lifeless> (I can show you the exact functions in bzrlib to call to use its lsprof glue; though we may want a tweaked version or a lock around the profiling to kick other threads off, or something)
08:14 < lifeless> mars: launchpad-foundations would be a good place to record that idea, yes ?
08:14 < mars> lifeless, yep!
08:14 < lifeless> I shall file a bug

The way I think this would work is as follows:
I would type in https://staging.launchpad.net/++profile++/bzr
the decorator would kick in and:
take a 'I'm profiling, yo' lock;
take a 'no other requests permitted lock' (maybe the same one)
wait for all other requests to finish (to avoid reporting their work)
assign an id for the profile in a similar way to oops assignment
continue with the request
the request would include the profile id in its comment block at the bottom
request completes and unwinds
profile is collated into a kcachegrind file - see the bzrlib.lsprof module for all of the profiling stuff here - do not roll your own; its annoying to get right
locks are released

cron job/inotify whatever copies the kcachegrind files on staging to devpad
developers can look locally if they are doing this with dev.launchpad.net

profit!

Related branches

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

+1, I want to schedule this.

Changed in launchpad-foundations:
status: New → Triaged
tags: added: performance
Changed in launchpad-foundations:
importance: Undecided → High
Revision history for this message
Robert Collins (lifeless) wrote :

Things that can help move it along:
 - help me land https://bugs.edge.launchpad.net/zope.app.publication/+bug/598816 and
  https://code.edge.launchpad.net/~lifeless/zope.publisher/start-request-event/+merge/28558 - they seem stalled even though Tres was happy with them.

 - We either need to move to a newer zope, or apply a small patch I have to our zope.app.publication (a backport of the two above patches) : I think its a matter of 'make a new egg in the sourcedeps, edit the versions list to use it', but I'm not sure of the QA process used on changes at that level.

 - I have a patch to profiling which then uses this event and makes it easier to reason about.

 - my uniquefileallocator patch passes all tests and will land once PQM opens again; that provides the basis for writing the file to disk Just-In-Time

 - bzrlib's lsprof wrapper is ready to provide mutex support for this - but we can just ignore that in the short term, things will be tolerable ;)

 - I, or someone else, needs to move profiling from per-thread to per-request using uniquefileallocator

 - And finally convert to using callgrind files.

All in all only a couple more hours work, but longer than that due to various latencies. I'll keep this bug apprised of my progress.

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

https://code.edge.launchpad.net/~lifeless/launchpad/lsprof/+merge/28583 is the profiling branch that probably needs a rereview as we just added cachegrind/lsprof support.

Changed in launchpad-foundations:
assignee: nobody → Māris Fogels (mars)
Revision history for this message
Gary Poster (gary) wrote :

zope.publisher 3.12.4 has Robert's patch for that package. I merged his patch to zope.app.publication but did not make a release because it didn't seem like we were in a rush. In any case, upstream now has both passes.

Revision history for this message
Māris Fogels (mars) wrote :

Ok, the lsprof branch has landed. That branch enables us to turn profiling on and off for all requests at once.

I have started a new branch to add on-demand profiling support for the ++profile++ URL:

  lp:~mars/launchpad/lsprof-on-demand

My branch adds the config switch to enable on-demand profiling, and some code that runs the on-demand profile test. However, getting Zope to skip the ++profile++ part of the URL is a problem. We do this for ++oops++, but Gary says that it is a hack, and not something we should copy. So the next step is to ask Gary about a better way to do traversal skips like this.

We might also want ++profile++ to become a real view - one that not only generates the profile, but shows you a helpful page telling you where to get it (or that lets you download it directly).

Revision history for this message
Māris Fogels (mars) wrote :

I have to put this work on hold for now. If someone else wants to tackle the on-demand bit, please feel free to do so.

Gary Poster (gary)
Changed in launchpad-foundations:
assignee: Māris Fogels (mars) → Gary Poster (gary)
Revision history for this message
Gary Poster (gary) wrote :

It is now available for development environments. I do not believe that it is safe for staging environments yet. It needs to wait on feature flag work to be completed, or farther along, at which point we will limit the functionality on staging to launchpad developers.

Revision history for this message
Robert Collins (lifeless) wrote : Re: [Bug 598289] Re: support a profile decorator for use in staging and development environments

What is missing from feature flags?

Revision history for this message
Launchpad QA Bot (lpqabot) wrote : Bug fixed by a commit
Changed in launchpad-foundations:
milestone: none → 10.09
tags: added: qa-needstesting
Changed in launchpad-foundations:
status: Triaged → Fix Committed
Revision history for this message
Gary Poster (gary) wrote :

> What is missing from feature flags?

Perhaps nothing. The docs I found seemed incomplete when I looked at this, but I may have been reading the wrong docs, or too fast.

AIUI, this effort will have two parts: defining a proper scope for Canonical Launchpad developers (or something similar), and then hooking up the feature flag to the ++profile++ code.

It may be a bit tricky because the profile may be started before the request's principal is authenticated. If that's the case--and I think it is--then the only downside is that arbitrary users will be able to start a profile, even though they will not actually receive the data. For staging, hopefully that's not too bad. For production, that seems unacceptable to me.

Revision history for this message
Robert Collins (lifeless) wrote : Re: [Bug 598289] Re: support a profile decorator for use in staging and development environments

I'm fairly sure authentication happens before traversal starts in our
system, so its unprofiled. I agree it would be unacceptable - because
profiled locks other stuff out!

Gary Poster (gary)
tags: added: qa-ok
removed: qa-needstesting
Curtis Hovey (sinzui)
Changed in launchpad-foundations:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.