Issue 618071  has been merged into this issue.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6a49bedce62c4dff0031bd3cfc135440dc219903

commit 6a49bedce62c4dff0031bd3cfc135440dc219903
Author: bmcquade <bmcquade@chromium.org>
Date: Wed Jul 27 19:25:56 2016

Add MetricsProvider::OnAppEnterBackground callback.

This allows MetricsProviders to log any histograms for data that's
buffered. For example, The page load metrics subsystem buffers some
metrics data until the end of the page load. These metrics should
also be logged when the app enters the background, in order to avoid
data loss.

https://codereview.chromium.org/2189543002 is a follow up change that
makes use of this new callback when tracking page load metrics.

See https://groups.google.com/a/google.com/d/topic/uma-users/BcJRPxNlJ4E/discussion
for additional context on this change.

BUG= 608360 

Review-Url: https://codereview.chromium.org/2175743002
Cr-Commit-Position: refs/heads/master@{#408212}

[modify] https://crrev.com/6a49bedce62c4dff0031bd3cfc135440dc219903/components/metrics/metrics_provider.cc
[modify] https://crrev.com/6a49bedce62c4dff0031bd3cfc135440dc219903/components/metrics/metrics_provider.h
[modify] https://crrev.com/6a49bedce62c4dff0031bd3cfc135440dc219903/components/metrics/metrics_service.cc

Adding ryansturm as the data reduction proxy observer would like to have such a callback.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9eca220417aa1050fbe8b431fee890f0b5b42240

commit 9eca220417aa1050fbe8b431fee890f0b5b42240
Author: bmcquade <bmcquade@chromium.org>
Date: Fri Jul 29 21:07:37 2016

Notify page load metrics when the app enters the background.

This change adds a PageLoadMetricsProvider, which implements the new
OnAppEnterBackground hook, and informs all MetricsWebContentsObservers
and in turn all PageLoadTrackers when the app enters the background.

On Android (and iOS), when an app enters the background, it may be
killed without further notifications. Thus, having this callback
will allow us to persist metrics before the app may be killed.
Our page load metrics subsystem isn't used on iOS, so we only target
Android in this change.

Unfortunately, we can't use the existing WebContents hidden callback,
as this gets invoked after the UMA subsystem has already flushed
metrics to disk. Thus, we need to add this explicit callback which
allows us to persist metrics as part of the UMA subsystem handling
the backgrounding callback.

For now, we log a debugging histogram in PageLoadTracker to help
understand how often trackers enter the background, and how often
those trackers are destroyed without running their destructors. In a
subsequent change, we'll add support for informing PLMObservers that
the app entered the background. This will allow any PLMObsevers that
implement OnComplete or OnFailedProvisionalLoad to also get called back
in cases where the tracker may be destroyed before its destructor runs,
due to the application being killed.

BUG= 608360 

Review-Url: https://codereview.chromium.org/2189543002
Cr-Commit-Position: refs/heads/master@{#408744}

[modify] https://crrev.com/9eca220417aa1050fbe8b431fee890f0b5b42240/chrome/browser/metrics/chrome_metrics_service_client.cc
[add] https://crrev.com/9eca220417aa1050fbe8b431fee890f0b5b42240/chrome/browser/metrics/page_load_metrics_provider.cc
[add] https://crrev.com/9eca220417aa1050fbe8b431fee890f0b5b42240/chrome/browser/metrics/page_load_metrics_provider.h
[modify] https://crrev.com/9eca220417aa1050fbe8b431fee890f0b5b42240/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
[modify] https://crrev.com/9eca220417aa1050fbe8b431fee890f0b5b42240/chrome/browser/page_load_metrics/metrics_web_contents_observer.h
[modify] https://crrev.com/9eca220417aa1050fbe8b431fee890f0b5b42240/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
[modify] https://crrev.com/9eca220417aa1050fbe8b431fee890f0b5b42240/chrome/chrome_browser.gypi
[modify] https://crrev.com/9eca220417aa1050fbe8b431fee890f0b5b42240/tools/metrics/histograms/histograms.xml

Data coming in suggests that roughly 1/3 of page loads that are active while Chrome is backgrounded on Android do not get a chance to run final callbacks in the current implementation, due to Chrome being killed.

We're now looking at how to expose a hook to observers to persist histograms on being backgrounded.

There are a few implementation options:

1. just invoke PLMObserver::OnComplete as part of being backgrounded, and do not invoke any subsequent observer callbacks at this point

pro: simplest solution; observers don't need to change implementation; will just do the right thing for most observers most of the time
con: if a page is foregrounded again before Chrome is killed, observers may potentially miss out on additional metrics or timing data for that page load

2. add a new callback specific to this case, e.g. PLMObserver::FlushMetricsOnAppEnterBackground

pro: easy to reason about what this new hook may be called for
con: adds complexity to PLMObserver implementations that each implementer must manage. implmenters must think about when this new callback can be invoked vs when OnComplete may be invoked. added complexity means added risk for bugs

3. add a new callback specific to this case, e.g. PLMObserver::FlushMetricsOnAppEnterBackground, have its default implementation invoke OnComplete and prevent subsequent callbacks from being invoked

pro: maximum flexibility; implementers can override to provide a custom implementation while getting a reasonable default implementation that will work correctly for most observers
con: slightly more metrics infrastructure complexity, but i think i prefer concentrating complexity in one place rather than pushing it to every observer implementation

I considered doing (1) initially but concluded that this is not sufficient for all use cases. More on that in a subsequent comment.

Charles pointed out that (1) is not sufficient because httpsengagementobserver, which measures total time in the foreground, wants to be notified in the event that the app happens to be foregrounded again before being killed. so (1) is not sufficient.

Ryan pointed out that background histograms like PageLoad.Timing2.NavigationToCommit.Background wouldn't get logged in cases where Chrome is backgrounded. We do want to allow for logging of this and similar histograms if the page doesn't end up getting killed while in the background. this one is a little tricky - if for instance we encounter the following sequence of events:
* navigation starts
* chrome backgrounded
* navigation commits
* OnComplete invoked as a result of a new page navigation being initiated
* chrome killed

Then we still have a chance to invoke the callback here if we continue to call OnComplete after being backgrounded, which seems beneficial. However if the timing shifts:

* navigation starts
* chrome backgrounded
* navigation commits
* chrome killed

Here, OnComplete was never invoked, since no new page navigation was initiated before being killed, and thus the commit histogram was not logged.

So I think my recommendation for this specific case is to log the commit histogram in the OnCommit callback rather than the OnComplete callback, and the more general recommendation would be to log as little as possible in OnComplete. We log commit in OnComplete for historical reasons, and my hope has been that we can actually kill the commit histogram since the time we record isn't quite right, and since I think we want to steer people towards higher level metrics like PageLoad.ParseTiming.NavigationToParseStart which is essentially the same as commit time but at a more well defined point in the lifecycle of the page load.

With regard to OnComplete and a callback when the app enters the background, I think my recommendation would be that by default, any logging logic that normally runs in OnComplete should probably run in the chrome app background callback instead, since there's no guarantee that OnComplete will ever get called. There will be exceptions to this, but I think this is a good general default behavior.

Ryan, for the DRP observer, would running the logic in OnComplete at the time the app is backgrounded result in the correct/intended behavior for you?

This sounds reasonable, though I don't want to kill the commit histogram without consult with navigation folks. PlzNavigate might get in a situation where we've committed the navigation but the render process isn't fully ready (so obviously we haven't started parsing).

As far as the DRP observer goes, running OnComplete at the time the app was backgrounded will have no functional impact on the result. Every timing event used in OnComplete is checked against background events before they are sent in the pingback.

Great, thanks! Looking at this more, I concluded that option (2) is best - the default implementation of the new callback will do nothing, and the observer implementer will be responsible for doing the right thing for their implementation. The reason for this is that OnComplete is only currently invoked for committed loads, so we'd need to call OnComplete conditionally from the default implementation only on commit, and this felt like a little too much policy to make it the default.

Charles I also took an idea from your chrome:// change and added an enum to allow the observer to indicate whether or not they want to continue observing metrics. We can use this here and in your change as well.

Here's a proposed implementation: https://codereview.chromium.org/2371883002

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6e7584a091cbb8078ed4289ffa739281bf170cbb

commit 6e7584a091cbb8078ed4289ffa739281bf170cbb
Author: bmcquade <bmcquade@chromium.org>
Date: Tue Sep 27 14:42:58 2016

Add PLMObserver::FlushMetricsOnAppEnterBackground.

PLMObserver::FlushMetricsOnAppEnterBackground gets invoked when the metrics
subsystem is flushing metrics as part of the app entering the background.

Observers that buffer metrics information and flush it in the OnComplete
callback may want to implement this method, in order to avoid losing data
due to the application being killed after being backgrounded.

BUG= 608360 

Review-Url: https://codereview.chromium.org/2371883002
Cr-Commit-Position: refs/heads/master@{#421206}

[modify] https://crrev.com/6e7584a091cbb8078ed4289ffa739281bf170cbb/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
[modify] https://crrev.com/6e7584a091cbb8078ed4289ffa739281bf170cbb/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
[modify] https://crrev.com/6e7584a091cbb8078ed4289ffa739281bf170cbb/chrome/browser/page_load_metrics/page_load_metrics_observer.cc
[modify] https://crrev.com/6e7584a091cbb8078ed4289ffa739281bf170cbb/chrome/browser/page_load_metrics/page_load_metrics_observer.h