Added UkmPageLoadMetricsObserver which sends top-level metrics to UKM

Added UkmPageLoadMetricsObserver which sends top-level metrics to UKM

Initial minimal version which just logs FirstContentfulPaint.

Requires https://codereview.chromium.org/2649303004/ and https://codereview.chromium.org/2649983005/#

R=bmcquade@chromium.org
BUG=684673
Review-Url: https://codereview.chromium.org/2654843003
Cr-Commit-Position: refs/heads/master@{#447571}
Committed: https://chromium.googlesource.com/chromium/src/+/b766497105ee42a5460c69d75c80f9a4858285d3

[oystein (OOO til 10th of July), 2017-01-25 18:54:12]

Description was changed from

==========
Added UkmPageLoadMetricsObserver which sends top-level metrics to UKM

Requires https://codereview.chromium.org/2649303004/ and
https://codereview.chromium.org/2649983005/#

R=bmcquade@chromium.org
BUG=684673
==========

to

==========
Added UkmPageLoadMetricsObserver which sends top-level metrics to UKM

Requires https://codereview.chromium.org/2649303004/ and
https://codereview.chromium.org/2649983005/#

R=bmcquade@chromium.org
BUG=684673
==========

[oystein (OOO til 10th of July), 2017-01-25 18:54:27]

ptal!

[oystein (OOO til 10th of July), 2017-01-25 18:55:05]

Description was changed from

==========
Added UkmPageLoadMetricsObserver which sends top-level metrics to UKM

Requires https://codereview.chromium.org/2649303004/ and
https://codereview.chromium.org/2649983005/#

R=bmcquade@chromium.org
BUG=684673
==========

to

==========
Added UkmPageLoadMetricsObserver which sends top-level metrics to UKM

Initial minimal version which just logs FirstContentfulPaint.

Requires https://codereview.chromium.org/2649303004/ and
https://codereview.chromium.org/2649983005/#

R=bmcquade@chromium.org
BUG=684673
==========

[oystein (OOO til 10th of July), 2017-01-28 01:19:44]

oysteine@chromium.org changed reviewers:
+ holte@chromium.org

[oystein (OOO til 10th of July), 2017-01-28 01:19:45]

(review comments from the combined CL)

+holte for ukm changes

https://codereview.chromium.org/2617883004/diff/120001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/2617883004/diff/120001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer_unittest.cc:5:
#include
"chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.h"
On 2017/01/26 00:50:59, Bryan McQuade wrote:
> if it's reasonably straightforward, it'd be good to add a browser test in
> page_load_metrics_browsertest.cc to make sure UKM logs are sent. This gets you
> end to end testing coverage. If there isn't yet sufficient UKM test infra for
> this, then it's fine to defer this.

Not trivially right now, I think it'd be better to defer this until we have the
generic proto format ready as that may change a fair amount of the interface
here.

https://codereview.chromium.org/2617883004/diff/120001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer_unittest.cc:56:
base::Time navigation_start_;
On 2017/01/26 00:50:59, Bryan McQuade wrote:
> looks like this isn't used - remove?

Done.

https://codereview.chromium.org/2617883004/diff/120001/chrome/test/base/testi...
File chrome/test/base/testing_browser_process.cc (right):

https://codereview.chromium.org/2617883004/diff/120001/chrome/test/base/testi...
chrome/test/base/testing_browser_process.cc:79: rappor_service_(nullptr),
On 2017/01/26 00:50:59, Bryan McQuade wrote:
> should set ukm_service_ to nullptr here

Done.

https://codereview.chromium.org/2617883004/diff/120001/components/ukm/ukm_sou...
File components/ukm/ukm_source.h (right):

https://codereview.chromium.org/2617883004/diff/120001/components/ukm/ukm_sou...
components/ukm/ukm_source.h:29: base::Time navigation_start() const { return
navigation_start_; }
On 2017/01/26 00:50:59, Bryan McQuade wrote:
> should we remove this and the setter and member variable for now as well?

Done.

[oystein (OOO til 10th of July), 2017-01-28 01:19:46]

(review comments from the combined CL)

+holte for ukm changes

https://codereview.chromium.org/2617883004/diff/120001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/2617883004/diff/120001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer_unittest.cc:5:
#include
"chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.h"
On 2017/01/26 00:50:59, Bryan McQuade wrote:
> if it's reasonably straightforward, it'd be good to add a browser test in
> page_load_metrics_browsertest.cc to make sure UKM logs are sent. This gets you
> end to end testing coverage. If there isn't yet sufficient UKM test infra for
> this, then it's fine to defer this.

Not trivially right now, I think it'd be better to defer this until we have the
generic proto format ready as that may change a fair amount of the interface
here.

https://codereview.chromium.org/2617883004/diff/120001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer_unittest.cc:56:
base::Time navigation_start_;
On 2017/01/26 00:50:59, Bryan McQuade wrote:
> looks like this isn't used - remove?

Done.

https://codereview.chromium.org/2617883004/diff/120001/chrome/test/base/testi...
File chrome/test/base/testing_browser_process.cc (right):

https://codereview.chromium.org/2617883004/diff/120001/chrome/test/base/testi...
chrome/test/base/testing_browser_process.cc:79: rappor_service_(nullptr),
On 2017/01/26 00:50:59, Bryan McQuade wrote:
> should set ukm_service_ to nullptr here

Done.

https://codereview.chromium.org/2617883004/diff/120001/components/ukm/ukm_sou...
File components/ukm/ukm_source.h (right):

https://codereview.chromium.org/2617883004/diff/120001/components/ukm/ukm_sou...
components/ukm/ukm_source.h:29: base::Time navigation_start() const { return
navigation_start_; }
On 2017/01/26 00:50:59, Bryan McQuade wrote:
> should we remove this and the setter and member variable for now as well?

Done.

[oystein (OOO til 10th of July), 2017-01-28 01:20:56]

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

[commit-bot: I haz the power, 2017-01-28 01:21:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-28 01:54:28]

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

[commit-bot: I haz the power, 2017-01-28 01:54:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Steven Holte, 2017-01-30 18:58:21]

lgtm

[oystein (OOO til 10th of July), 2017-01-30 19:13:41]

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

[commit-bot: I haz the power, 2017-01-30 19:14:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-30 20:42:29]

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

[commit-bot: I haz the power, 2017-01-30 20:42:29]

Dry run: This issue passed the CQ dry run.

[oystein (OOO til 10th of July), 2017-01-31 18:56:43]

ping bmcquade :)

[Bryan McQuade, 2017-01-31 19:02:49]

LGTM, thanks!

[oystein (OOO til 10th of July), 2017-01-31 19:09:00]

The CQ bit was checked by oysteine@chromium.org

[oystein (OOO til 10th of July), 2017-01-31 19:09:00]

The patchset sent to the CQ was uploaded after l-g-t-m from holte@chromium.org
Link to the patchset: https://codereview.chromium.org/2654843003/#ps60001
(title: "Test fix")

[commit-bot: I haz the power, 2017-01-31 19:09:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-31 19:18:10]

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

[commit-bot: I haz the power, 2017-01-31 19:18:14]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[oystein (OOO til 10th of July), 2017-01-31 19:20:36]

oysteine@chromium.org changed reviewers:
+ sky@chromium.org

[oystein (OOO til 10th of July), 2017-01-31 19:20:46]

+sky for chrome/test/base/

[rkaplow, 2017-01-31 19:23:41]

rkaplow@chromium.org changed reviewers:
+ rkaplow@chromium.org

[rkaplow, 2017-01-31 19:23:41]

last minute nit - I'm not sure I like the name
ukm_page_load_metrics_observer.cc

To me the primary part of this is the observing the URL, and the PLT metric is
coming along for the ride. In the end the most important part of this is the
recording of the URL since most metrics will be done outside this. So I kind of
feel a better name is something like ukm_url_observer.. WFYT?

[oystein (OOO til 10th of July), 2017-01-31 19:42:34]

On 2017/01/31 19:23:41, rkaplow wrote:
> last minute nit - I'm not sure I like the name
> ukm_page_load_metrics_observer.cc
> 
> To me the primary part of this is the observing the URL, and the PLT metric is
> coming along for the ride. In the end the most important part of this is the
> recording of the URL since most metrics will be done outside this. So I kind
of
> feel a better name is something like ukm_url_observer.. WFYT?

I think that depends on how we evolve the interface, which we're not super clear
on yet. Since our metrics are essentially scoped on the committed URL rather
than a navigation, once we've got metrics coming in from multiple sources this
class may not be the source of truth for the URL but rather just be contributing
PWMs.

Basically the next thing we have to figure out I think, as it'll be pretty
closely tied to the data format.

[rkaplow, 2017-01-31 20:09:12]

On 2017/01/31 19:42:34, oystein wrote:
> On 2017/01/31 19:23:41, rkaplow wrote:
> > last minute nit - I'm not sure I like the name
> > ukm_page_load_metrics_observer.cc
> > 
> > To me the primary part of this is the observing the URL, and the PLT metric
is
> > coming along for the ride. In the end the most important part of this is the
> > recording of the URL since most metrics will be done outside this. So I kind
> of
> > feel a better name is something like ukm_url_observer.. WFYT?
> 
> I think that depends on how we evolve the interface, which we're not super
clear
> on yet. Since our metrics are essentially scoped on the committed URL rather
> than a navigation, once we've got metrics coming in from multiple sources this
> class may not be the source of truth for the URL but rather just be
contributing
> PWMs.
> 
> Basically the next thing we have to figure out I think, as it'll be pretty
> closely tied to the data format.

fair enough, I'm fine leaving for now and we can see where we get to.

[rkaplow, 2017-01-31 20:09:15]

On 2017/01/31 19:42:34, oystein wrote:
> On 2017/01/31 19:23:41, rkaplow wrote:
> > last minute nit - I'm not sure I like the name
> > ukm_page_load_metrics_observer.cc
> > 
> > To me the primary part of this is the observing the URL, and the PLT metric
is
> > coming along for the ride. In the end the most important part of this is the
> > recording of the URL since most metrics will be done outside this. So I kind
> of
> > feel a better name is something like ukm_url_observer.. WFYT?
> 
> I think that depends on how we evolve the interface, which we're not super
clear
> on yet. Since our metrics are essentially scoped on the committed URL rather
> than a navigation, once we've got metrics coming in from multiple sources this
> class may not be the source of truth for the URL but rather just be
contributing
> PWMs.
> 
> Basically the next thing we have to figure out I think, as it'll be pretty
> closely tied to the data format.

fair enough, I'm fine leaving for now and we can see where we get to.

[Bryan McQuade, 2017-01-31 22:31:55]

https://codereview.chromium.org/2654843003/diff/60001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2654843003/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.cc:61:
source->set_first_contentful_paint(timing.first_contentful_paint.value());
just realized that timing.first_contentful_paint may not always be set when this
is called, and thus timing.first_contentful_paint.value() will assert. I'll send
a patch to address this shortly.

[Bryan McQuade, 2017-01-31 22:33:21]

On 2017/01/31 at 22:31:55, Bryan McQuade wrote:
>
https://codereview.chromium.org/2654843003/diff/60001/chrome/browser/page_loa...
> File
chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.cc
(right):
> 
>
https://codereview.chromium.org/2654843003/diff/60001/chrome/browser/page_loa...
>
chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.cc:61:
source->set_first_contentful_paint(timing.first_contentful_paint.value());
> just realized that timing.first_contentful_paint may not always be set when
this is called, and thus timing.first_contentful_paint.value() will assert. I'll
send a patch to address this shortly.

ah, given that this didn't land yet, let's get this fixed before we land. the
fix is:

if (timing.first_contentful_paint)
  source->set_first_contentful_paint(timing.first_contentful_paint.value());

We expect to receive reports where the value isn't set, for page loads where we
don't get to FCP by the time the page load is hidden or closed.

[oystein (OOO til 10th of July), 2017-01-31 22:37:20]

On 2017/01/31 22:33:21, Bryan McQuade wrote:
> On 2017/01/31 at 22:31:55, Bryan McQuade wrote:
> >
>
https://codereview.chromium.org/2654843003/diff/60001/chrome/browser/page_loa...
> > File
> chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.cc
> (right):
> > 
> >
>
https://codereview.chromium.org/2654843003/diff/60001/chrome/browser/page_loa...
> >
>
chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.cc:61:
> source->set_first_contentful_paint(timing.first_contentful_paint.value());
> > just realized that timing.first_contentful_paint may not always be set when
> this is called, and thus timing.first_contentful_paint.value() will assert.
I'll
> send a patch to address this shortly.
> 
> ah, given that this didn't land yet, let's get this fixed before we land. the
> fix is:
> 
> if (timing.first_contentful_paint)
>   source->set_first_contentful_paint(timing.first_contentful_paint.value());
> 
> We expect to receive reports where the value isn't set, for page loads where
we
> don't get to FCP by the time the page load is hidden or closed.

In the current CL we just don't send a report at all when fcp isn't set (we
check and do an early return), do we want that kind of empty reports instead?

[Bryan McQuade, 2017-01-31 22:41:35]

On 2017/01/31 at 22:37:20, oysteine wrote:
> On 2017/01/31 22:33:21, Bryan McQuade wrote:
> > On 2017/01/31 at 22:31:55, Bryan McQuade wrote:
> > >
> >
https://codereview.chromium.org/2654843003/diff/60001/chrome/browser/page_loa...
> > > File
> > chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.cc
> > (right):
> > > 
> > >
> >
https://codereview.chromium.org/2654843003/diff/60001/chrome/browser/page_loa...
> > >
> >
chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.cc:61:
> > source->set_first_contentful_paint(timing.first_contentful_paint.value());
> > > just realized that timing.first_contentful_paint may not always be set
when
> > this is called, and thus timing.first_contentful_paint.value() will assert.
I'll
> > send a patch to address this shortly.
> > 
> > ah, given that this didn't land yet, let's get this fixed before we land.
the
> > fix is:
> > 
> > if (timing.first_contentful_paint)
> >   source->set_first_contentful_paint(timing.first_contentful_paint.value());
> > 
> > We expect to receive reports where the value isn't set, for page loads where
we
> > don't get to FCP by the time the page load is hidden or closed.
> 
> In the current CL we just don't send a report at all when fcp isn't set (we
check and do an early return), do we want that kind of empty reports instead?

Oh, I'm sorry, I missed that, thanks!

For purposes of tracking aborted page loads, I think it's useful to track all
page loads, and leave the field blank in cases where FCP wasn't reported. What
do you think?

[Bryan McQuade, 2017-01-31 22:43:14]

(please feel free to proceed w/ checking this in if you like - we can
revisit the abort case in a follow on change, thanks!)

On Tue, Jan 31, 2017 at 5:41 PM <bmcquade@chromium.org> wrote:

> On 2017/01/31 at 22:37:20, oysteine wrote:
> > On 2017/01/31 22:33:21, Bryan McQuade wrote:
> > > On 2017/01/31 at 22:31:55, Bryan McQuade wrote:
> > > >
> > >
>
>
https://codereview.chromium.org/2654843003/diff/60001/chrome/browser/page_loa...
> > > > File
> > >
> chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.cc
> > > (right):
> > > >
> > > >
> > >
>
>
https://codereview.chromium.org/2654843003/diff/60001/chrome/browser/page_loa...
> > > >
> > >
>
>
chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.cc:61:
> > >
> source->set_first_contentful_paint(timing.first_contentful_paint.value());
> > > > just realized that timing.first_contentful_paint may not always be
> set
> when
> > > this is called, and thus timing.first_contentful_paint.value() will
> assert.
> I'll
> > > send a patch to address this shortly.
> > >
> > > ah, given that this didn't land yet, let's get this fixed before we
> land.
> the
> > > fix is:
> > >
> > > if (timing.first_contentful_paint)
> > >
> source->set_first_contentful_paint(timing.first_contentful_paint.value());
> > >
> > > We expect to receive reports where the value isn't set, for page loads
> where
> we
> > > don't get to FCP by the time the page load is hidden or closed.
> >
> > In the current CL we just don't send a report at all when fcp isn't set
> (we
> check and do an early return), do we want that kind of empty reports
> instead?
>
> Oh, I'm sorry, I missed that, thanks!
>
> For purposes of tracking aborted page loads, I think it's useful to track
> all
> page loads, and leave the field blank in cases where FCP wasn't reported.
> What
> do you think?
>
> https://codereview.chromium.org/2654843003/
>
> --
> You received this message because you are subscribed to the Google Groups
> "loading-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to loading-reviews+unsubscribe@chromium.org.
> To post to this group, send email to loading-reviews@chromium.org.
> To view this discussion on the web visit
>
https://groups.google.com/a/chromium.org/d/msgid/loading-reviews/94eb2c05053e...
>
<https://groups.google.com/a/chromium.org/d/msgid/loading-reviews/94eb2c05053e...>
> .
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2017-02-01 00:42:18]

LGTM

[oystein (OOO til 10th of July), 2017-02-01 18:17:12]

The CQ bit was checked by oysteine@chromium.org

[commit-bot: I haz the power, 2017-02-01 18:17:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-01 19:18:52]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485973032315260,
"parent_rev": "c19010b9bd9c531283b1157ed51a8af22ae665f4", "commit_rev":
"b766497105ee42a5460c69d75c80f9a4858285d3"}

[commit-bot: I haz the power, 2017-02-01 19:19:24]

Description was changed from

==========
Added UkmPageLoadMetricsObserver which sends top-level metrics to UKM

Initial minimal version which just logs FirstContentfulPaint.

Requires https://codereview.chromium.org/2649303004/ and
https://codereview.chromium.org/2649983005/#

R=bmcquade@chromium.org
BUG=684673
==========

to

==========
Added UkmPageLoadMetricsObserver which sends top-level metrics to UKM

Initial minimal version which just logs FirstContentfulPaint.

Requires https://codereview.chromium.org/2649303004/ and
https://codereview.chromium.org/2649983005/#

R=bmcquade@chromium.org
BUG=684673

Review-Url: https://codereview.chromium.org/2654843003
Cr-Commit-Position: refs/heads/master@{#447571}
Committed:
https://chromium.googlesource.com/chromium/src/+/b766497105ee42a5460c69d75c80...
==========

[commit-bot: I haz the power, 2017-02-01 19:19:26]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/b766497105ee42a5460c69d75c80...