Improved and generalized page load metric immediate event logging.

Improved and generalized page load metric immediate event logging.

Our current page load metric logging approach has been to log at
the very end of the page lifecycle, in the PageLoadMetricsObserver
OnComplete callback. This keeps metric logging logic simple, but
has some costs. In particular, it means that metrics may be logged
long after events were actually observed on a page load, which leads
to metrics landing in the 'wrong' UMA record, and on Android we see
>10% loss in logging and 7% skew in the mean, likely due to Android
killing the browser process after backgrounding, leading to our code
failing to log metrics for some pages.

This patch introduces a class that makes it easy for us to log metrics
immediately. In a follow up change, this class will be used to log
additional metrics.

BUG=611740
Committed: https://crrev.com/9a521d48e39c57ee5cb42326e9e50bd76dbb4bb2
Cr-Commit-Position: refs/heads/master@{#393599}

[Bryan McQuade, 2016-05-13 15:14:59]

bmcquade@chromium.org changed reviewers:
+ csharrison@chromium.org

[Bryan McQuade, 2016-05-13 15:15:04]

PTAL

[Bryan McQuade, 2016-05-13 15:18:37]

Description was changed from

==========
Improved and generalized page load metric immediate event logging.

Our current page load metric logging approach has been to log at
the very end of the page lifecycle, in the PageLoadMetricsObserver
OnComplete callback. This keeps metric logging logic simple, but
has some costs. In particular, it means that metrics may be logged
long after events were actually observed on a page load, which leads
to metrics landing in the 'wrong' UMA record, and on Android we see
>10% loss in logging and 7% skew in the mean, likely due to Android
killing the browser process after backgrounding, leading to our code
failing to log metrics for some pages.

This patch introduces a class that makes it easy for us to log metrics
immediately. In a follow up change, this class will be used to log
additional metrics.

BUG=611704
==========

to

==========
Improved and generalized page load metric immediate event logging.

Our current page load metric logging approach has been to log at
the very end of the page lifecycle, in the PageLoadMetricsObserver
OnComplete callback. This keeps metric logging logic simple, but
has some costs. In particular, it means that metrics may be logged
long after events were actually observed on a page load, which leads
to metrics landing in the 'wrong' UMA record, and on Android we see
>10% loss in logging and 7% skew in the mean, likely due to Android
killing the browser process after backgrounding, leading to our code
failing to log metrics for some pages.

This patch introduces a class that makes it easy for us to log metrics
immediately. In a follow up change, this class will be used to log
additional metrics.

BUG=611740
==========

[Charlie Harrison, 2016-05-13 16:46:21]

Generally looks good. What's your reasoning for making this a separate object?
Why wouldn't observers just want to be PLMObservers and EventObservers?

[Bryan McQuade, 2016-05-13 16:50:40]

On 2016/05/13 at 16:46:21, csharrison wrote:
> Generally looks good. What's your reasoning for making this a separate object?
Why wouldn't observers just want to be PLMObservers and EventObservers?

I think we may want to migrate to that eventually, but I feel like this new API
is somewhat experimental, so I'm keeping it separate for now.

If we decide that this ends up being the more recommended approach in the future
then I think it may make sense to do a big refactor and update all existing
observers to use the new callback where possible, but I think we should save
that for the future.

[Charlie Harrison, 2016-05-13 17:03:41]

A big refactor isn't necessary, just allow them to optionally override
OnFirstContentfulPaint, etc.

I think you could show that it's experimental via a comment or something. It
just reduces the boilerplate of implementing these callbacks from:
1. Adding another class implementing the EventObserver
2. Adding another member to the original observer
3. Implementing OnUpdateTiming and piping it to the new member

to:
1. Add another method on the original observer.

[Bryan McQuade, 2016-05-13 17:58:08]

On 2016/05/13 at 17:03:41, csharrison wrote:
> A big refactor isn't necessary, just allow them to optionally override
OnFirstContentfulPaint, etc.
> 
> I think you could show that it's experimental via a comment or something. It
just reduces the boilerplate of implementing these callbacks from:
> 1. Adding another class implementing the EventObserver
> 2. Adding another member to the original observer
> 3. Implementing OnUpdateTiming and piping it to the new member
> 
> to:
> 1. Add another method on the original observer.

Ok, patch updated to add these methods to PLMO. PTAL.

[Charlie Harrison, 2016-05-13 18:07:39]

lgtm thanks.

[Charlie Harrison, 2016-05-13 18:07:39]

lgtm thanks.

[Bryan McQuade, 2016-05-13 18:09:26]

The CQ bit was checked by bmcquade@chromium.org

[commit-bot: I haz the power, 2016-05-13 18:10:18]

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

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

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-13 19:17:30]

Description was changed from

==========
Improved and generalized page load metric immediate event logging.

Our current page load metric logging approach has been to log at
the very end of the page lifecycle, in the PageLoadMetricsObserver
OnComplete callback. This keeps metric logging logic simple, but
has some costs. In particular, it means that metrics may be logged
long after events were actually observed on a page load, which leads
to metrics landing in the 'wrong' UMA record, and on Android we see
>10% loss in logging and 7% skew in the mean, likely due to Android
killing the browser process after backgrounding, leading to our code
failing to log metrics for some pages.

This patch introduces a class that makes it easy for us to log metrics
immediately. In a follow up change, this class will be used to log
additional metrics.

BUG=611740
==========

to

==========
Improved and generalized page load metric immediate event logging.

Our current page load metric logging approach has been to log at
the very end of the page lifecycle, in the PageLoadMetricsObserver
OnComplete callback. This keeps metric logging logic simple, but
has some costs. In particular, it means that metrics may be logged
long after events were actually observed on a page load, which leads
to metrics landing in the 'wrong' UMA record, and on Android we see
>10% loss in logging and 7% skew in the mean, likely due to Android
killing the browser process after backgrounding, leading to our code
failing to log metrics for some pages.

This patch introduces a class that makes it easy for us to log metrics
immediately. In a follow up change, this class will be used to log
additional metrics.

BUG=611740

Committed: https://crrev.com/9a521d48e39c57ee5cb42326e9e50bd76dbb4bb2
Cr-Commit-Position: refs/heads/master@{#393599}
==========

[commit-bot: I haz the power, 2016-05-13 19:17:31]

Patchset 4 (id:??) landed as
https://crrev.com/9a521d48e39c57ee5cb42326e9e50bd76dbb4bb2
Cr-Commit-Position: refs/heads/master@{#393599}