Merge into devel : bug838878 : Code : Launchpad itself

Status:

Merged

Approved by:

Gary Poster on 2011-09-01

Approved revision:

no longer in the source branch.

Merged at revision:

13858

Proposed branch:

lp://staging/~gary/launchpad/bug838878

Merge into:

lp://staging/launchpad

Prerequisite:

lp://staging/~gary/launchpad/bug838869

Diff against target:

913 lines (+430/-117)

3 files modified

lib/lp/services/profile/profile.pt (+121/-36)
lib/lp/services/profile/profile.py (+153/-53)
lib/lp/services/profile/tests.py (+156/-28)

To merge this branch:

bzr merge lp://staging/~gary/launchpad/bug838878

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
j.c.sackett (community)		2011-09-01	Approve on 2011-09-01
Review via email: mp+73702@code.staging.launchpad.net

Commit message

[r=jcsackett][bug=838878] ++profile++ has a new conditional sqltrace feature and a new sql option.

Description of the change

This branch lets ++profile++ expose the new conditional sqltrace functionality enabled by changes in lp:~gary/launchpad/bug838869 .

This branch is just over the limit, for which I apologize.

The new features for ++profile++ are as follows:

- ++profile++sqltrace adds a mini-language for specifying what SQL requests you want to trace. ++profile++sqltrace continues to get tracebacks for all SQL. "++profile++sqltrace:includes STRING" only gets tracebacks for SQL statements that include STRING (whitespace- and case-normalized). "++profile++sqltrace:startswith STRING" gets tracebacks for SQL statements that start with STRING. "++profile++sqltrace:endswith STRING" does the same if the SQL ends with STRING. They can be combined with a pipe, "|", meaning logical or. Therefore, "++profile++sqltrace:includes FOOBAR BAZ| includes shazam" will match SQL statements that include "FOOBAR BAZ" or "SHAZAM" (again, case- and whitespace- normalized).

- ++profile++sql is now available, only giving SQL without any profiling or stacktrace information.

The changes for ++profile++ are as follows:

You combine requests in ++profile++ with "&" now, not ",", so "++profile++show,pstats" becomes "++profile++show&pstats". This is so that the strings used by the conditions described above can include commas, which SQL often does.

Lint is happy except for its usual confusion with the beginning of the template.

Trying this out on a dev machine should be as easy as going to https://launchpad.dev/++profile++ and messing around with some of the new options, like "https://launchpad.dev/++profile++sql" and "https://launchpad.dev/++profile++sqltrace:includes AccountPassword.id".

Revision history for this message

j.c.sackett (jcsackett) wrote on 2011-09-01:

#

Gary--

This looks good. I see one possible problem in the updated profile template, pointed out below.

> === modified file 'lib/lp/services/profile/profile.pt'
> --- lib/lp/services/profile/profile.pt 2011-08-22 17:08:02 +0000
> +++ lib/lp/services/profile/profile.pt 2011-09-01 17:17:32 +0000
> @@ -64,15 +64,21 @@
> get immediate profiling results in the browser, use
> <code>++profile++show</code>.</tal:block> <tal:block condition="not:
> always_log">In order to get profiling results, you need to ask for an HTML
> - view (<code>++profile++show</code>), a KCacheGrind-friendly log on the
> + view of the profile and OOPS data including SQL calls
> + (<code>++profile++show</code>), a KCacheGrind-friendly log on the
> filesystem (<code>++profile++callgrind</code>), a PStats-friendly log
> (Python standard library) on the filesystem
> - (<code>++profile++pstats</code>), or an HTML view of the SQL and the
> + (<code>++profile++pstats</code>), an HTML view of the SQL and the
> Python stacktrace when each SQL call was made
> - (<code>++profile++sqltrace</code>). You can also combine these
> - (<code>++profile++show,callgrind</code> or
> - <code>++profile++show,pstats</code> or others,
> - with each action separated with commas).</tal:block></p>
> + (<code>++profile++sqltrace</code>), or an HTML view of only the SQL
> + (<code>++profile++sql</code>). You can also combine these
> + (<code>++profile++show&callgrind</code> or
> + <code>++profile++show&pstats</code> or others, with each action separated
> + with commas).</tal:block></p>

I may be misunderstanding this, but shouldn't this be "separated with ambersands" as shown in the example?

> === modified file 'lib/lp/services/profile/profile.py'
> @@ -505,24 +521,108 @@
> # include traversal in the profile.
> actions, slash, tail = end.partition('/')
> result.update(
> - action for action in (
> - item.strip().lower() for item in actions.split(','))
> - if action)
> + (name.strip(), config.strip())
> + for name, partition, config in (
> + item.strip().lower().partition(':')
> + for item in actions.split('&'))
> + if name.strip())

There's no problem in the above bit, just thought that was one hell of a generator expression. :-P

Gary--

This looks good. I see one possible problem in the updated profile template, pointed out below.

> === modified file 'lib/lp/services/profile/profile.pt'
> --- lib/lp/services/profile/profile.pt	2011-08-22 17:08:02 +0000
> +++ lib/lp/services/profile/profile.pt	2011-09-01 17:17:32 +0000
> @@ -64,15 +64,21 @@
>      get immediate profiling results in the browser, use
>      <code>++profile++show</code>.</tal:block> <tal:block condition="not:
>      always_log">In order to get profiling results, you need to ask for an HTML
> -    view (<code>++profile++show</code>), a KCacheGrind-friendly log on the
> +    view of the profile and OOPS data including SQL calls
> +    (<code>++profile++show</code>), a KCacheGrind-friendly log on the
>      filesystem (<code>++profile++callgrind</code>), a PStats-friendly log
>      (Python standard library) on the filesystem
> -    (<code>++profile++pstats</code>), or an HTML view of the SQL and the
> +    (<code>++profile++pstats</code>), an HTML view of the SQL and the
>      Python stacktrace when each SQL call was made
> -    (<code>++profile++sqltrace</code>). You can also combine these
> -    (<code>++profile++show,callgrind</code> or
> -    <code>++profile++show,pstats</code> or others,
> -    with each action separated with commas).</tal:block></p>
> +    (<code>++profile++sqltrace</code>), or an HTML view of only the SQL
> +    (<code>++profile++sql</code>). You can also combine these
> +    (<code>++profile++show&callgrind</code> or
> +    <code>++profile++show&pstats</code> or others, with each action separated
> +    with commas).</tal:block></p>
 
I may be misunderstanding this, but shouldn't this be "separated with ambersands" as shown in the example?
 
> === modified file 'lib/lp/services/profile/profile.py'
> @@ -505,24 +521,108 @@
>              # include traversal in the profile.
>              actions, slash, tail = end.partition('/')
>              result.update(
> -                action for action in (
> -                    item.strip().lower() for item in actions.split(','))
> -                if action)
> +                (name.strip(), config.strip())
> +                for name, partition, config in (
> +                    item.strip().lower().partition(':')
> +                    for item in actions.split('&'))
> +                if name.strip())
 
There's no problem in the above bit, just thought that was one hell of a generator expression. :-P

review: Approve

Revision history for this message

Gary Poster (gary) wrote on 2011-09-01:

#

On Sep 1, 2011, at 3:45 PM, j.c.sackett wrote:

> Review: Approve
> Gary--
>
> This looks good. I see one possible problem in the updated profile template, pointed out below.

...

>
> I may be misunderstanding this, but shouldn't this be "separated with ambersands" as shown in the example?

Ah, yes, thanks, good catch!

...

> There's no problem in the above bit, just thought that was one hell of a generator expression. :-P

Yeah...not the prettiest thing in the world. Maybe I should have been more explicit/less concise...

Thanks again,

Gary

Launchpad itself

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

Commit message

Description of the change

Preview Diff

Subscribers