Initial UKMPageLoadMetricsObserver

chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.cc

Index: chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.cc
diff --git a/chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.cc b/chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.cc
new file mode 100644
index 0000000000000000000000000000000000000000..ee069659b2ac9d3ce97fb0bdceaa9bcdb90224f9
--- /dev/null
+++ b/chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.cc
@@ -0,0 +1,60 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.h"
+#include "chrome/browser/browser_process.h"
+#include "components/ukm/ukm_service.h"
+#include "components/ukm/ukm_source.h"
+
+// static
+std::unique_ptr<page_load_metrics::PageLoadMetricsObserver>
+UKMPageLoadMetricsObserver::CreateIfNeeded() {
+  if (!g_browser_process->ukm_service())
+    return nullptr;
+
+  return base::MakeUnique<UKMPageLoadMetricsObserver>();
+}
+
+UKMPageLoadMetricsObserver::UKMPageLoadMetricsObserver() {}
+
+page_load_metrics::PageLoadMetricsObserver::ObservePolicy
+UKMPageLoadMetricsObserver::OnStart(
+    content::NavigationHandle* navigation_handle,
+    const GURL& currently_committed_url,
+    bool started_in_foreground) {
+  navigation_start_ = navigation_handle->NavigationStart();
+  return started_in_foreground ? CONTINUE_OBSERVING : STOP_OBSERVING;
+}
+
+page_load_metrics::PageLoadMetricsObserver::ObservePolicy
+UKMPageLoadMetricsObserver::OnHidden(
+    const page_load_metrics::PageLoadTiming& timing,
+    const page_load_metrics::PageLoadExtraInfo& extra_info) {
+  return STOP_OBSERVING;
+}
+
+void UKMPageLoadMetricsObserver::OnComplete(

[Bryan McQuade, 2017/01/17 21:54:29]

When logging via OnComplete, we've observed that we lose about 13% of page load
records on Android due to Chrome being killed when it goes into the background.
The loss shifts the resulting page load metrics distribution somewhat
substantially: the mean first contentful paint is 7% higher when logging in
OnComplete, for example, as opposed to logging in OnFirstContentfulPaint. More
details here:
https://groups.google.com/a/google.com/d/msg/uma-users/BcJRPxNlJ4E/7O1pJ2ULAgAJ

To address this, you could do one of the following:

1. emit the metrics data out to UKM as each event is observed, as opposed to
once in OnComplete (e.g. emit first contentful paint in OnFirstContentfulPaint,
etc)

or

2. implement PageLoadMetricsObserver::FlushMetricsOnAppEnterBackground and
PLMO::OnHidden, returning STOP_OBSERVING from both, and emitting any metrics
observed up to that point in those callbacks, as well as from OnComplete as you
do currently. This is what the data reduction proxy observer does in order to
address the data loss issue:
https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/observe...

+    const page_load_metrics::PageLoadTiming& timing,
+    const page_load_metrics::PageLoadExtraInfo& info) {
+  if (info.committed_url.is_empty())
+    return;
+
+  LOG(ERROR) << "UKMPageLoadMetricsObserver::OnComplete for '"
+             << info.committed_url << "': FMP "
+             << timing.first_meaningful_paint.value();
+
+  ukm::UkmService* ukm_service = g_browser_process->ukm_service();
+  if (!ukm_service) {
+    LOG(ERROR) << "No UKM service :'(";
+    return;
+  }
+
+  std::unique_ptr<ukm::UkmSource> source =
+      base::WrapUnique(new ukm::UkmSource());
+  source->set_committed_url(info.committed_url);
+  source->set_navigation_start(navigation_start_);
+  source->set_first_meaningful_paint(timing.first_meaningful_paint.value());

[Bryan McQuade, 2017/01/17 21:54:29]

FMP is a great metric in concept, but it's still experimental and actively being
modified. Given this, I'd suggest that we:

1. Be consistent with other places where we log this metric, and include
'experimental' in the field name, until we think FMP impl is stable. See the UMA
name: PageLoad.Experimental.PaintTiming.NavigationToFirstMeaningfulPaint as well
as the field name sent in the data reduction proxy pingback:
experimental_first_meaningful_paint
(https://cs.chromium.org/chromium/src/components/data_reduction_proxy/core/com...)
for other places where we do this. So I'd propose we rename the field to
'experimental_first_meaningful_paint' to be consistent w/ the other cases. We
can rename it to 'first_meaningful_paint' once it has stabilized and we're ready
to support the metric long term. It may continue to change after being
stabilized, but at a much less frequent rate. While it's being actively
developed I think we should reflect that in the field name somehow (e.g.
'experimental_' prefix).

2. Add a first_contentful_paint field, given that FCP is very stable & gives
another useful signal that we can have better confidence will not change
significantly going forward. This one is most important IMO, as we need at least
one page load metric that's stable so we can do analysis of performance over
time w/o being affected by changes in metric implementation that will happen for
FMP.

+
+  ukm_service->RecordSource(std::move(source));
+}