Rewrite firstPaintMetric to support extracting multiple navigations from a single trace

Rewrite firstPaintMetric to support extracting multiple navigations from a single trace

Depends on FrameLoader objects snapshot to be added in CL:
https://codereview.chromium.org/1964803002/

BUG=catapult:#2081

Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/d1739687cdc0babb9cd59c00b7e0965bac1f5cde

[kouhei (in TOK), 2016-05-10 07:27:47]

Description was changed from

==========
Rewrite firstPaintMetric to support extracting multiple navigations from a
single trace

BUG=catapult:#2081
==========

to

==========
Rewrite firstPaintMetric to support extracting multiple navigations from a
single trace

Depends on FrameLoader objects snapshot to be added in CL:
https://codereview.chromium.org/1964803002/

BUG=catapult:#2081
==========

[kouhei (in TOK), 2016-05-10 07:27:47]

kouhei@chromium.org changed reviewers:
+ eakuefner@chromium.org, nednguyen@google.com, tzik@chromium.org

[kouhei (in TOK), 2016-05-10 07:29:48]

kouhei@chromium.org changed reviewers:
+ benjhayden@chromium.org

[kouhei (in TOK), 2016-05-10 07:29:49]

eakuefner, nednguyen: Would you give a high-level look?
Ben: How should I merge this with your LoadExpectations? re: 
https://github.com/catapult-project/catapult/issues/2302

[kouhei (in TOK), 2016-05-10 07:30:35]

+cc: ksakamoto@

[nduca, 2016-05-10 16:05:53]

nduca@chromium.org changed reviewers:
+ caseq@chromium.org, nduca@chromium.org

[nduca, 2016-05-10 16:05:54]

caseq, looks like there's a c++ side probe here that we're depending on that is
using 'frame' references. is that all normative?

[caseq, 2016-05-10 17:32:02]

On 2016/05/10 16:05:54, nduca wrote:
> caseq, looks like there's a c++ side probe here that we're depending on that
is
> using 'frame' references. is that all normative?

Yes, we do a lot of it for Timeline and I think it's likely to remain the same
even once we get to use the new object ids (i.e. we'll keep frames identified by
LocalFrame* in blink, we'll just be able to link them to other ids as well).

[benjhayden, 2016-05-10 19:54:46]

On 2016/05/10 at 07:29:49, kouhei wrote:
> Ben: How should I merge this with your LoadExpectations? re: 
> https://github.com/catapult-project/catapult/issues/2302

Don't worry about it for now. I'll update this with you when I rewrite
LoadExpectation.
Ideally, this file should reduce to something like
model.userModel.expectations.forEach(function(ue) {
  if (!(ue instanceof LoadExpectation)) return;
  var timeToFirstContentfulPaint = ue.firstContentfulPaintEvent.start -
ue.navigationEvent.start;
  ...
});

Whatever you want to do until then lgtm, with one question about how you want to
structure the metric values.

https://codereview.chromium.org/1963583005/diff/1/tracing/tracing/metrics/sys...
File tracing/tracing/metrics/system_health/first_paint_metric.html (right):

https://codereview.chromium.org/1963583005/diff/1/tracing/tracing/metrics/sys...
tracing/tracing/metrics/system_health/first_paint_metric.html:147:
valueList.addValue(new tr.v.NumericValue(
Instead of several scalars, do you want to use a Numeric (histogram) instead?

[benjhayden, 2016-05-10 20:25:44]

https://codereview.chromium.org/1963583005/diff/1/tracing/tracing/metrics/sys...
File tracing/tracing/metrics/system_health/first_paint_metric.html (right):

https://codereview.chromium.org/1963583005/diff/1/tracing/tracing/metrics/sys...
tracing/tracing/metrics/system_health/first_paint_metric.html:148:
model.canonicalUrlThatCreatedThisTrace, 'firstContentfulPaint',
This is spelled "model.canonicalUrl" now.

[nednguyen, 2016-05-10 20:47:53]

On 2016/05/10 20:25:44, benjhayden_chromium wrote:
>
https://codereview.chromium.org/1963583005/diff/1/tracing/tracing/metrics/sys...
> File tracing/tracing/metrics/system_health/first_paint_metric.html (right):
> 
>
https://codereview.chromium.org/1963583005/diff/1/tracing/tracing/metrics/sys...
> tracing/tracing/metrics/system_health/first_paint_metric.html:148:
> model.canonicalUrlThatCreatedThisTrace, 'firstContentfulPaint',
> This is spelled "model.canonicalUrl" now.

Where is the logic of ignoring loading in 'telemetry.internal.warmCache' region?
(https://codereview.chromium.org/1964843002/)

[kouhei (in TOK), 2016-05-11 06:39:33]

Updated unit tests. PTAL. 

> Where is the logic of ignoring loading in 'telemetry.internal.warmCache'
region?
> (https://codereview.chromium.org/1964843002/)

This is in isTelemetryInternalEvent().

https://codereview.chromium.org/1963583005/diff/1/tracing/tracing/metrics/sys...
File tracing/tracing/metrics/system_health/first_paint_metric.html (right):

https://codereview.chromium.org/1963583005/diff/1/tracing/tracing/metrics/sys...
tracing/tracing/metrics/system_health/first_paint_metric.html:147:
valueList.addValue(new tr.v.NumericValue(
On 2016/05/10 19:54:46, benjhayden_chromium wrote:
> Instead of several scalars, do you want to use a Numeric (histogram) instead?

I think we want separate individual values, as histogram of ttfcp of different
URLs wouldn't make much sense.

https://codereview.chromium.org/1963583005/diff/1/tracing/tracing/metrics/sys...
tracing/tracing/metrics/system_health/first_paint_metric.html:148:
model.canonicalUrlThatCreatedThisTrace, 'firstContentfulPaint',
On 2016/05/10 20:25:43, benjhayden_chromium wrote:
> This is spelled "model.canonicalUrl" now.

Done.

[nednguyen, 2016-05-11 21:36:39]

https://codereview.chromium.org/1963583005/diff/1/tracing/tracing/metrics/sys...
File tracing/tracing/metrics/system_health/first_paint_metric.html (right):

https://codereview.chromium.org/1963583005/diff/1/tracing/tracing/metrics/sys...
tracing/tracing/metrics/system_health/first_paint_metric.html:147:
valueList.addValue(new tr.v.NumericValue(
On 2016/05/11 06:39:33, kouhei wrote:
> On 2016/05/10 19:54:46, benjhayden_chromium wrote:
> > Instead of several scalars, do you want to use a Numeric (histogram)
instead?
> 
> I think we want separate individual values, as histogram of ttfcp of different
> URLs wouldn't make much sense.

Why do you think the histogram doesn't make sense? In the first phase, we will
only one single number, but for later phase, when we handle trace from the real
world, I think it makes sense to use the histogram for loading metrics.

[nednguyen, 2016-05-11 21:38:22]

https://codereview.chromium.org/1963583005/diff/20001/tracing/tracing/metrics...
File tracing/tracing/metrics/system_health/first_paint_metric.html (right):

https://codereview.chromium.org/1963583005/diff/20001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/first_paint_metric.html:149: (ev) =>
!isTelemetryInternalEvent(ev));
Should we also filter out the "about:blank"?

[kouhei (in TOK), 2016-05-12 05:41:25]

https://codereview.chromium.org/1963583005/diff/1/tracing/tracing/metrics/sys...
File tracing/tracing/metrics/system_health/first_paint_metric.html (right):

https://codereview.chromium.org/1963583005/diff/1/tracing/tracing/metrics/sys...
tracing/tracing/metrics/system_health/first_paint_metric.html:147:
valueList.addValue(new tr.v.NumericValue(
On 2016/05/11 21:36:38, nednguyen wrote:
> On 2016/05/11 06:39:33, kouhei wrote:
> > On 2016/05/10 19:54:46, benjhayden_chromium wrote:
> > > Instead of several scalars, do you want to use a Numeric (histogram)
> instead?
> > 
> > I think we want separate individual values, as histogram of ttfcp of
different
> > URLs wouldn't make much sense.
> 
> Why do you think the histogram doesn't make sense? In the first phase, we will
> only one single number, but for later phase, when we handle trace from the
real
> world, I think it makes sense to use the histogram for loading metrics.

Added Numeric histogram, but not sure how we can utilize this. Would you give me
specific use case for it?

https://codereview.chromium.org/1963583005/diff/20001/tracing/tracing/metrics...
File tracing/tracing/metrics/system_health/first_paint_metric.html (right):

https://codereview.chromium.org/1963583005/diff/20001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/first_paint_metric.html:149: (ev) =>
!isTelemetryInternalEvent(ev));
On 2016/05/11 21:38:22, nednguyen wrote:
> Should we also filter out the "about:blank"?

Done.

[nednguyen, 2016-05-12 15:35:30]

https://codereview.chromium.org/1963583005/diff/1/tracing/tracing/metrics/sys...
File tracing/tracing/metrics/system_health/first_paint_metric.html (right):

https://codereview.chromium.org/1963583005/diff/1/tracing/tracing/metrics/sys...
tracing/tracing/metrics/system_health/first_paint_metric.html:147:
valueList.addValue(new tr.v.NumericValue(
On 2016/05/12 05:41:25, kouhei wrote:
> On 2016/05/11 21:36:38, nednguyen wrote:
> > On 2016/05/11 06:39:33, kouhei wrote:
> > > On 2016/05/10 19:54:46, benjhayden_chromium wrote:
> > > > Instead of several scalars, do you want to use a Numeric (histogram)
> > instead?
> > > 
> > > I think we want separate individual values, as histogram of ttfcp of
> different
> > > URLs wouldn't make much sense.
> > 
> > Why do you think the histogram doesn't make sense? In the first phase, we
will
> > only one single number, but for later phase, when we handle trace from the
> real
> > world, I think it makes sense to use the histogram for loading metrics.
> 
> Added Numeric histogram, but not sure how we can utilize this. Would you give
me
> specific use case for it?

For future use cases of Pcv2, I can imagine we have a page that does not do a
single navigation, but multiple navs. e.g: google.com, search for X, search for
Y. 
It could be important for us to capture the "overall" loading performance of the
site, rather than individual loads.
(See
https://docs.google.com/document/d/1InrKT88tUGGQWf3EopXYakK4_Khv8AJoHWTR_2ksu...)

[kouhei (in TOK), 2016-05-13 01:59:16]

On 2016/05/12 15:35:30, nednguyen wrote:
>
https://codereview.chromium.org/1963583005/diff/1/tracing/tracing/metrics/sys...
> File tracing/tracing/metrics/system_health/first_paint_metric.html (right):
> 
>
https://codereview.chromium.org/1963583005/diff/1/tracing/tracing/metrics/sys...
> tracing/tracing/metrics/system_health/first_paint_metric.html:147:
> valueList.addValue(new tr.v.NumericValue(
> On 2016/05/12 05:41:25, kouhei wrote:
> > On 2016/05/11 21:36:38, nednguyen wrote:
> > > On 2016/05/11 06:39:33, kouhei wrote:
> > > > On 2016/05/10 19:54:46, benjhayden_chromium wrote:
> > > > > Instead of several scalars, do you want to use a Numeric (histogram)
> > > instead?
> > > > 
> > > > I think we want separate individual values, as histogram of ttfcp of
> > different
> > > > URLs wouldn't make much sense.
> > > 
> > > Why do you think the histogram doesn't make sense? In the first phase, we
> will
> > > only one single number, but for later phase, when we handle trace from the
> > real
> > > world, I think it makes sense to use the histogram for loading metrics.
> > 
> > Added Numeric histogram, but not sure how we can utilize this. Would you
give
> me
> > specific use case for it?
> 
> For future use cases of Pcv2, I can imagine we have a page that does not do a
> single navigation, but multiple navs. e.g: http://google.com, search for X,
search for
> Y. 
> It could be important for us to capture the "overall" loading performance of
the
> site, rather than individual loads.
> (See
>
https://docs.google.com/document/d/1InrKT88tUGGQWf3EopXYakK4_Khv8AJoHWTR_2ksu...)

Got it.
Ned: Would you see if the code changes are OK?

[nednguyen, 2016-05-13 03:18:22]

lgtm with some comments

https://codereview.chromium.org/1963583005/diff/40001/tracing/tracing/metrics...
File tracing/tracing/metrics/system_health/first_paint_metric.html (right):

https://codereview.chromium.org/1963583005/diff/40001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/first_paint_metric.html:156:
.addExponentialBins(20000, 100);
why exponential bins?

https://codereview.chromium.org/1963583005/diff/40001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/first_paint_metric.html:186:
valueList.addValue(new tr.v.NumericValue(
You shouldn't add this because this creates duplicate values with same name. The
histogram should be enough.

https://codereview.chromium.org/1963583005/diff/40001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/first_paint_metric.html:194:
model.canonicalUrl, 'firstContentfulPaint-histogram',
Ethan: is that our convention to include the suffix "-histogram" in the metric
name or we should just leave it out?

[kouhei (in TOK), 2016-05-13 03:43:20]

https://codereview.chromium.org/1963583005/diff/40001/tracing/tracing/metrics...
File tracing/tracing/metrics/system_health/first_paint_metric.html (right):

https://codereview.chromium.org/1963583005/diff/40001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/first_paint_metric.html:156:
.addExponentialBins(20000, 100);
On 2016/05/13 03:18:21, nednguyen wrote:
> why exponential bins?

I just noticed that we can mix linear&exponential bins.
I added linear bins for fine-tuning 0-3s ttfcp

https://codereview.chromium.org/1963583005/diff/40001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/first_paint_metric.html:186:
valueList.addValue(new tr.v.NumericValue(
On 2016/05/13 03:18:21, nednguyen wrote:
> You shouldn't add this because this creates duplicate values with same name.
The
> histogram should be enough.

Done.

https://codereview.chromium.org/1963583005/diff/40001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/first_paint_metric.html:194:
model.canonicalUrl, 'firstContentfulPaint-histogram',
On 2016/05/13 03:18:21, nednguyen wrote:
> Ethan: is that our convention to include the suffix "-histogram" in the metric
> name or we should just leave it out?

Removed the suffix

[kouhei (in TOK), 2016-05-13 04:07:10]

The CQ bit was checked by kouhei@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-13 04:07:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1963583005/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1963583005/100001

[commit-bot: I haz the power, 2016-05-13 04:09:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-13 04:09:14]

Dry run: Try jobs failed on following builders:
  Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)

[kouhei (in TOK), 2016-05-13 04:35:58]

The CQ bit was checked by kouhei@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-13 04:36:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1963583005/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1963583005/120001

[commit-bot: I haz the power, 2016-05-13 05:46:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-13 05:46:23]

Dry run: This issue passed the CQ dry run.

[kouhei (in TOK), 2016-05-13 07:03:02]

The CQ bit was checked by kouhei@chromium.org

[kouhei (in TOK), 2016-05-13 07:03:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
benjhayden@chromium.org, nednguyen@google.com
Link to the patchset: https://codereview.chromium.org/1963583005/#ps120001
(title: "rebased")

[commit-bot: I haz the power, 2016-05-13 07:03:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1963583005/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1963583005/120001

[commit-bot: I haz the power, 2016-05-13 07:04:04]

Description was changed from

==========
Rewrite firstPaintMetric to support extracting multiple navigations from a
single trace

Depends on FrameLoader objects snapshot to be added in CL:
https://codereview.chromium.org/1964803002/

BUG=catapult:#2081
==========

to

==========
Rewrite firstPaintMetric to support extracting multiple navigations from a
single trace

Depends on FrameLoader objects snapshot to be added in CL:
https://codereview.chromium.org/1964803002/

BUG=catapult:#2081

Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2016-05-13 07:04:05]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...