Added UkmPageLoadMetricsObserver which sends top-level metrics to UKM (issue 2654843003 by oysteine@chromium.org)

[oyst...@chromium.org]

Reviewers: Bryan McQuade
CL: https://codereview.chromium.org/2654843003/

Message:
ptal!

Description:
Added UkmPageLoadMetricsObserver which sends top-level metrics to UKM

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

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

Affected files (+194, -0 lines):
M chrome/browser/BUILD.gn
A chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.h
A chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.cc
A chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer_unittest.cc
M chrome/browser/page_load_metrics/page_load_metrics_initialize.cc
M chrome/test/BUILD.gn

[oyst...@chromium.org]

(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.

https://codereview.chromium.org/2654843003/

[oyst...@chromium.org]

[ho...@chromium.org]

lgtm



https://codereview.chromium.org/2654843003/

[oyst...@chromium.org]

ping bmcquade :)

https://codereview.chromium.org/2654843003/

[bmcq...@chromium.org]

LGTM, thanks!

https://codereview.chromium.org/2654843003/

[oyst...@chromium.org]

+sky for chrome/test/base/

https://codereview.chromium.org/2654843003/

[rka...@chromium.org]

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?

https://codereview.chromium.org/2654843003/

[oyst...@chromium.org]

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.

https://codereview.chromium.org/2654843003/

[rka...@chromium.org]

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

https://codereview.chromium.org/2654843003/

[rka...@chromium.org]

[bmcq...@chromium.org]

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

https://codereview.chromium.org/2654843003/diff/60001/chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.cc#newcode61
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.

https://codereview.chromium.org/2654843003/

[bmcq...@chromium.org]

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.

https://codereview.chromium.org/2654843003/

[oyst...@chromium.org]

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?

https://codereview.chromium.org/2654843003/

[bmcq...@chromium.org]

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/

[Bryan McQuade]

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

--
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-revie...@chromium.org.
To post to this group, send email to loading...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/loading-reviews/94eb2c05053e65756005476ba208%40google.com.

[s...@chromium.org]

LGTM

https://codereview.chromium.org/2654843003/