[Telemetry] Add 'name' attribute for pages, which allows for more human-readable printing

[Telemetry] Add 'name' attribute for pages, which allows for more human-readable printing

BUG=284547
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221312

[dtu, 2013-09-04 00:50:56]

Very thorough. lgtm with nits

https://chromiumcodereview.appspot.com/23545023/diff/1/tools/telemetry/teleme...
File tools/telemetry/telemetry/page/buildbot_page_measurement_results.py
(right):

https://chromiumcodereview.appspot.com/23545023/diff/1/tools/telemetry/teleme...
tools/telemetry/telemetry/page/buildbot_page_measurement_results.py:37: # unique
URLs.
Missed one :)

https://chromiumcodereview.appspot.com/23545023/diff/1/tools/telemetry/teleme...
tools/telemetry/telemetry/page/buildbot_page_measurement_results.py:59:
value_name = (value.value, page_values.page.display_name)
This variable name is weird now.

https://chromiumcodereview.appspot.com/23545023/diff/1/tools/telemetry/teleme...
File tools/telemetry/telemetry/page/page.py (right):

https://chromiumcodereview.appspot.com/23545023/diff/1/tools/telemetry/teleme...
tools/telemetry/telemetry/page/page.py:92: # A version of this page's URL that's
safe to use as a filename.
This comment

[edmundyan, 2013-09-04 03:03:36]

https://codereview.chromium.org/23545023/diff/1/tools/telemetry/telemetry/pag...
File tools/telemetry/telemetry/page/buildbot_page_measurement_results.py
(right):

https://codereview.chromium.org/23545023/diff/1/tools/telemetry/telemetry/pag...
tools/telemetry/telemetry/page/buildbot_page_measurement_results.py:37: # unique
URLs.
On 2013/09/04 00:50:56, Dave Tu wrote:
> Missed one :)

Done.

https://codereview.chromium.org/23545023/diff/1/tools/telemetry/telemetry/pag...
tools/telemetry/telemetry/page/buildbot_page_measurement_results.py:59:
value_name = (value.value, page_values.page.display_name)
On 2013/09/04 00:50:56, Dave Tu wrote:
> This variable name is weird now.
value_and_display_name? This is getting verbose...

https://codereview.chromium.org/23545023/diff/1/tools/telemetry/telemetry/pag...
File tools/telemetry/telemetry/page/page.py (right):

https://codereview.chromium.org/23545023/diff/1/tools/telemetry/telemetry/pag...
tools/telemetry/telemetry/page/page.py:92: # A version of this page's URL that's
safe to use as a filename.
On 2013/09/04 00:50:56, Dave Tu wrote:
> This comment

Done.

[tonyg, 2013-09-04 16:32:18]

lgtm

[commit-bot: I haz the power, 2013-09-04 16:42:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/edmundyan@chromium.org/23545023/6001

[commit-bot: I haz the power, 2013-09-04 18:04:14]

Retried try job too often on mac_rel for step(s) telemetry_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-09-04 18:35:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/edmundyan@chromium.org/23545023/26001

[commit-bot: I haz the power, 2013-09-04 18:42:01]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-09-04 23:48:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/edmundyan@chromium.org/23545023/26001

[commit-bot: I haz the power, 2013-09-05 00:09:19]

Change committed as 221312

[tonyg, 2013-09-06 04:32:24]

https://codereview.chromium.org/23545023/diff/26001/tools/telemetry/telemetry...
File
tools/telemetry/telemetry/page/buildbot_page_measurement_results_unittest.py
(right):

https://codereview.chromium.org/23545023/diff/26001/tools/telemetry/telemetry...
tools/telemetry/telemetry/page/buildbot_page_measurement_results_unittest.py:46:
expected = ['RESULT a_by_name: http___www.foo.com_= 3 seconds',
Sorry I missed this before, but the dashboard had special handling for the
_by_url results.

We should either revert these back to _by_url or update the dashboard to treat
_by_name like it used to treat _by_url.

I'll leave it up to you and Annie to decide which is better.

[edmundyan, 2013-09-06 04:58:17]

On 2013/09/06 04:32:24, tonyg wrote:
>
https://codereview.chromium.org/23545023/diff/26001/tools/telemetry/telemetry...
> File
> tools/telemetry/telemetry/page/buildbot_page_measurement_results_unittest.py
> (right):
> 
>
https://codereview.chromium.org/23545023/diff/26001/tools/telemetry/telemetry...
>
tools/telemetry/telemetry/page/buildbot_page_measurement_results_unittest.py:46:
> expected = ['RESULT a_by_name: http___www.foo.com_= 3 seconds',
> Sorry I missed this before, but the dashboard had special handling for the
> _by_url results.
> 
> We should either revert these back to _by_url or update the dashboard to treat
> _by_name like it used to treat _by_url.
> 
> I'll leave it up to you and Annie to decide which is better.

I'm assuming this is breaking graphs now?  Let's revert. (someone else w/
committer acces please?).  Endure isn't even using this yet.

[edmundyan, 2013-09-09 16:58:11]

On 2013/09/06 04:58:17, edmundyan wrote:
> On 2013/09/06 04:32:24, tonyg wrote:
> >
>
https://codereview.chromium.org/23545023/diff/26001/tools/telemetry/telemetry...
> > File
> > tools/telemetry/telemetry/page/buildbot_page_measurement_results_unittest.py
> > (right):
> > 
> >
>
https://codereview.chromium.org/23545023/diff/26001/tools/telemetry/telemetry...
> >
>
tools/telemetry/telemetry/page/buildbot_page_measurement_results_unittest.py:46:
> > expected = ['RESULT a_by_name: http___www.foo.com_= 3 seconds',
> > Sorry I missed this before, but the dashboard had special handling for the
> > _by_url results.
> > 
> > We should either revert these back to _by_url or update the dashboard to
treat
> > _by_name like it used to treat _by_url.
> > 
> > I'll leave it up to you and Annie to decide which is better.
> 
> I'm assuming this is breaking graphs now?  Let's revert. (someone else w/
> committer acces please?).  Endure isn't even using this yet.

Tony/Annie what exactly is this breaking?  There's some endure code I would like
to land that uses the "name" attribute, but would like to wait until this is
finished.

[sullivan1, 2013-09-09 17:13:22]

Sorry I missed this before.

The old perf dashboard couldn't selectively toggle traces on/off, so we had
to separate out the times and by_url times into separate graphs. We did
this using the "_by_url" suffix on the graph. But the new dashboard can
show the data on the same chart, with the individual urls turned off.

Since we supported the old and new dashboard at the same time, we just had
special treatment for "_by_url"-- it merges "times" and "times_by_url", etc.

How we resolve it depends on what you're trying to do here. We could change
the dashboard to also do the same thing for "_by_name" that it does for
"_by_url". But why do we need these separated from the main chart anyway?


On Mon, Sep 9, 2013 at 12:58 PM, <edmundyan@chromium.org> wrote:

> On 2013/09/06 04:58:17, edmundyan wrote:
>
>> On 2013/09/06 04:32:24, tonyg wrote:
>> >
>>
>
> https://codereview.chromium.**org/23545023/diff/26001/tools/**
>
telemetry/telemetry/page/**buildbot_page_measurement_**results_unittest.py<https://codereview.chromium.org/23545023/diff/26001/tools/telemetry/telemetry/page/buildbot_page_measurement_results_unittest.py>
>
>> > File
>> > tools/telemetry/telemetry/**page/buildbot_page_**
>> measurement_results_unittest.**py
>> > (right):
>> >
>> >
>>
>
> https://codereview.chromium.**org/23545023/diff/26001/tools/**
> telemetry/telemetry/page/**buildbot_page_measurement_**
>
results_unittest.py#newcode46<https://codereview.chromium.org/23545023/diff/26001/tools/telemetry/telemetry/page/buildbot_page_measurement_results_unittest.py#newcode46>
>
>> >
>>
>
> tools/telemetry/telemetry/**page/buildbot_page_**
> measurement_results_unittest.**py:46:
>
>> > expected = ['RESULT a_by_name: http___www.foo.com_= 3 seconds',
>> > Sorry I missed this before, but the dashboard had special handling for
>> the
>> > _by_url results.
>> >
>> > We should either revert these back to _by_url or update the dashboard to
>>
> treat
>
>> > _by_name like it used to treat _by_url.
>> >
>> > I'll leave it up to you and Annie to decide which is better.
>>
>
>  I'm assuming this is breaking graphs now?  Let's revert. (someone else w/
>> committer acces please?).  Endure isn't even using this yet.
>>
>
> Tony/Annie what exactly is this breaking?  There's some endure code I
> would like
> to land that uses the "name" attribute, but would like to wait until this
> is
> finished.
>
>
https://codereview.chromium.**org/23545023/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[edmundyan, 2013-09-09 18:27:51]

On 2013/09/09 17:13:22, sullivan_google.com wrote:
> Sorry I missed this before.
> 
> The old perf dashboard couldn't selectively toggle traces on/off, so we had
> to separate out the times and by_url times into separate graphs. We did
> this using the "_by_url" suffix on the graph. But the new dashboard can
> show the data on the same chart, with the individual urls turned off.
> 
> Since we supported the old and new dashboard at the same time, we just had
> special treatment for "_by_url"-- it merges "times" and "times_by_url", etc.
> 
> How we resolve it depends on what you're trying to do here. We could change
> the dashboard to also do the same thing for "_by_name" that it does for
> "_by_url". But why do we need these separated from the main chart anyway?

So current Endure bots don't even parse buildbot output, we're actually parsing
the csv output.  So I have no preference for by_name or by_url.  I only changed
buildbot output so that it would be more uniform with the other outputs (namely
csv).

Annie, are you saying that there is no longer a need (on your end) to do
additional parsing for "_by_url" because the new dashboard can toggle on/off
urls?  If so, I would just strip all that stuff out since we aren't using it
either.  I can do this if you'd like.

[tonyg, 2013-09-10 02:45:37]

I think the easiest thing to do here is just to change the names back to
_by_url. Edmund, would you mind putting together a patch for that?


On Mon, Sep 9, 2013 at 11:27 AM, <edmundyan@chromium.org> wrote:

> On 2013/09/09 17:13:22, sullivan_google.com wrote:
>
>> Sorry I missed this before.
>>
>
>  The old perf dashboard couldn't selectively toggle traces on/off, so we
>> had
>> to separate out the times and by_url times into separate graphs. We did
>> this using the "_by_url" suffix on the graph. But the new dashboard can
>> show the data on the same chart, with the individual urls turned off.
>>
>
>  Since we supported the old and new dashboard at the same time, we just had
>> special treatment for "_by_url"-- it merges "times" and "times_by_url",
>> etc.
>>
>
>  How we resolve it depends on what you're trying to do here. We could
>> change
>> the dashboard to also do the same thing for "_by_name" that it does for
>> "_by_url". But why do we need these separated from the main chart anyway?
>>
>
> So current Endure bots don't even parse buildbot output, we're actually
> parsing
> the csv output.  So I have no preference for by_name or by_url.  I only
> changed
> buildbot output so that it would be more uniform with the other outputs
> (namely
> csv).
>
> Annie, are you saying that there is no longer a need (on your end) to do
> additional parsing for "_by_url" because the new dashboard can toggle
> on/off
> urls?  If so, I would just strip all that stuff out since we aren't using
> it
> either.  I can do this if you'd like.
>
>
https://codereview.chromium.**org/23545023/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.