Add metrics for DelayNavigationThrottle

Add metrics for DelayNavigationThrottle

We add 3 'internal' metrics to make sure DelayNavigationThrottle is
adding the expected delays, and to understand the difference between
the specified delay and the actual delay. We expect there to be
differences in cases where the main UI thread is congested, for example.

BUG=704117
Review-Url: https://codereview.chromium.org/2803673002
Cr-Commit-Position: refs/heads/master@{#462626}
Committed: https://chromium.googlesource.com/chromium/src/+/5339a7833dc51eae11a0e5b2f29c719122fa79a7

[Bryan McQuade, 2017-04-05 14:35:30]

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

[commit-bot: I haz the power, 2017-04-05 14:35:54]

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

[Bryan McQuade, 2017-04-05 14:37:54]

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

[commit-bot: I haz the power, 2017-04-05 14:38:27]

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-04-05 16:08:31]

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

[commit-bot: I haz the power, 2017-04-05 16:08:32]

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

[Bryan McQuade, 2017-04-05 16:10:10]

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

[commit-bot: I haz the power, 2017-04-05 16:10:34]

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

[Bryan McQuade, 2017-04-05 16:12:33]

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

[Bryan McQuade, 2017-04-05 16:12:57]

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

[Bryan McQuade, 2017-04-05 16:13:53]

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

[commit-bot: I haz the power, 2017-04-05 16:13:55]

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

[Bryan McQuade, 2017-04-05 16:48:36]

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

[Bryan McQuade, 2017-04-05 16:49:06]

holte, PTAL for histograms.xml change, thanks!

[Charlie Harrison, 2017-04-05 16:54:36]

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

[Charlie Harrison, 2017-04-05 16:54:37]

drive-by

https://codereview.chromium.org/2803673002/diff/60001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/experiments/delay_navigation_throttle.cc
(right):

https://codereview.chromium.org/2803673002/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/experiments/delay_navigation_throttle.cc:125:
UMA_HISTOGRAM_TIMES(kHistogramNavigationDelayDelta, delay_delta.magnitude());
if actual_delay < navigation_delay_ this could be problematic. If we expect this
to not happen very often, could we just let negative values clamp to 0?

[Bryan McQuade, 2017-04-05 16:58:04]

On 2017/04/05 at 16:54:37, csharrison wrote:
> drive-by
> 
>
https://codereview.chromium.org/2803673002/diff/60001/chrome/browser/page_loa...
> File chrome/browser/page_load_metrics/experiments/delay_navigation_throttle.cc
(right):
> 
>
https://codereview.chromium.org/2803673002/diff/60001/chrome/browser/page_loa...
> chrome/browser/page_load_metrics/experiments/delay_navigation_throttle.cc:125:
UMA_HISTOGRAM_TIMES(kHistogramNavigationDelayDelta, delay_delta.magnitude());
> if actual_delay < navigation_delay_ this could be problematic. If we expect
this to not happen very often, could we just let negative values clamp to 0?

Yeah, I thought about this a bit. The best thing would be to add 2 histograms,
one for positive and one for negative, but I think that's probably overkill.

I'm hesitant to clamp to zero as this prevents us from seeing the delta in cases
where it's negative. The goal of the delta histo is just to give us insight into
the accuracy of the delays overall.

We'll be able to infer the net effect of pos vs neg deltas by comparing the
'specified' and 'actual' histograms, and seeing how e.g. the difference in means
differs from the mean of the delta histo.

How does all that sound?

[Charlie Harrison, 2017-04-05 17:01:39]

On 2017/04/05 16:58:04, Bryan McQuade wrote:
> On 2017/04/05 at 16:54:37, csharrison wrote:
> > drive-by
> > 
> >
>
https://codereview.chromium.org/2803673002/diff/60001/chrome/browser/page_loa...
> > File
chrome/browser/page_load_metrics/experiments/delay_navigation_throttle.cc
> (right):
> > 
> >
>
https://codereview.chromium.org/2803673002/diff/60001/chrome/browser/page_loa...
> >
chrome/browser/page_load_metrics/experiments/delay_navigation_throttle.cc:125:
> UMA_HISTOGRAM_TIMES(kHistogramNavigationDelayDelta, delay_delta.magnitude());
> > if actual_delay < navigation_delay_ this could be problematic. If we expect
> this to not happen very often, could we just let negative values clamp to 0?
> 
> Yeah, I thought about this a bit. The best thing would be to add 2 histograms,
> one for positive and one for negative, but I think that's probably overkill.
> 
> I'm hesitant to clamp to zero as this prevents us from seeing the delta in
cases
> where it's negative. The goal of the delta histo is just to give us insight
into
> the accuracy of the delays overall.
> 
> We'll be able to infer the net effect of pos vs neg deltas by comparing the
> 'specified' and 'actual' histograms, and seeing how e.g. the difference in
means
> differs from the mean of the delta histo.
> 
> How does all that sound?

OK LGTM.

If we notice that we're getting negatives would you replace this with two
histograms though?

[Bryan McQuade, 2017-04-05 17:03:57]

On 2017/04/05 at 17:01:39, csharrison wrote:
> On 2017/04/05 16:58:04, Bryan McQuade wrote:
> > On 2017/04/05 at 16:54:37, csharrison wrote:
> > > drive-by
> > > 
> > >
> >
https://codereview.chromium.org/2803673002/diff/60001/chrome/browser/page_loa...
> > > File
chrome/browser/page_load_metrics/experiments/delay_navigation_throttle.cc
> > (right):
> > > 
> > >
> >
https://codereview.chromium.org/2803673002/diff/60001/chrome/browser/page_loa...
> > >
chrome/browser/page_load_metrics/experiments/delay_navigation_throttle.cc:125:
> > UMA_HISTOGRAM_TIMES(kHistogramNavigationDelayDelta,
delay_delta.magnitude());
> > > if actual_delay < navigation_delay_ this could be problematic. If we
expect
> > this to not happen very often, could we just let negative values clamp to 0?
> > 
> > Yeah, I thought about this a bit. The best thing would be to add 2
histograms,
> > one for positive and one for negative, but I think that's probably overkill.
> > 
> > I'm hesitant to clamp to zero as this prevents us from seeing the delta in
cases
> > where it's negative. The goal of the delta histo is just to give us insight
into
> > the accuracy of the delays overall.
> > 
> > We'll be able to infer the net effect of pos vs neg deltas by comparing the
> > 'specified' and 'actual' histograms, and seeing how e.g. the difference in
means
> > differs from the mean of the delta histo.
> > 
> > How does all that sound?
> 
> OK LGTM.
> 
> If we notice that we're getting negatives would you replace this with two
histograms though?

As long as specified and actual are reasonably close I'm not too worried about
it. If the deltas are large then we either need to understand why and fix, or
factor the actual deltas into our experiment analysis. So yeah if we see
evidence of large negative deltas we might add a histogram for that. But I don't
expect that case to be common (would be a bug in the event loop impl - seems
unlikely).

[Charlie Harrison, 2017-04-05 17:12:40]

On 2017/04/05 17:03:57, Bryan McQuade wrote:
> On 2017/04/05 at 17:01:39, csharrison wrote:
> > On 2017/04/05 16:58:04, Bryan McQuade wrote:
> > > On 2017/04/05 at 16:54:37, csharrison wrote:
> > > > drive-by
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/2803673002/diff/60001/chrome/browser/page_loa...
> > > > File
> chrome/browser/page_load_metrics/experiments/delay_navigation_throttle.cc
> > > (right):
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/2803673002/diff/60001/chrome/browser/page_loa...
> > > >
> chrome/browser/page_load_metrics/experiments/delay_navigation_throttle.cc:125:
> > > UMA_HISTOGRAM_TIMES(kHistogramNavigationDelayDelta,
> delay_delta.magnitude());
> > > > if actual_delay < navigation_delay_ this could be problematic. If we
> expect
> > > this to not happen very often, could we just let negative values clamp to
0?
> > > 
> > > Yeah, I thought about this a bit. The best thing would be to add 2
> histograms,
> > > one for positive and one for negative, but I think that's probably
overkill.
> > > 
> > > I'm hesitant to clamp to zero as this prevents us from seeing the delta in
> cases
> > > where it's negative. The goal of the delta histo is just to give us
insight
> into
> > > the accuracy of the delays overall.
> > > 
> > > We'll be able to infer the net effect of pos vs neg deltas by comparing
the
> > > 'specified' and 'actual' histograms, and seeing how e.g. the difference in
> means
> > > differs from the mean of the delta histo.
> > > 
> > > How does all that sound?
> > 
> > OK LGTM.
> > 
> > If we notice that we're getting negatives would you replace this with two
> histograms though?
> 
> As long as specified and actual are reasonably close I'm not too worried about
> it. If the deltas are large then we either need to understand why and fix, or
> factor the actual deltas into our experiment analysis. So yeah if we see
> evidence of large negative deltas we might add a histogram for that. But I
don't
> expect that case to be common (would be a bug in the event loop impl - seems
> unlikely).

^ Agreed on all points.

[commit-bot: I haz the power, 2017-04-05 17:14:11]

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

[commit-bot: I haz the power, 2017-04-05 17:14:12]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2017-04-05 19:47:11]

Description was changed from

==========
Add metrics for DelayNavigationThrottle

We add 3 'internal' metrics to make sure DelayNavigationThrottle is
adding the expected delays, and to understand the difference between
the specified delay and the actual delay. We expect there to be
differene in cases where the main UI thread is congested, for example.

BUG=704117
==========

to

==========
Add metrics for DelayNavigationThrottle

We add 3 'internal' metrics to make sure DelayNavigationThrottle is
adding the expected delays, and to understand the difference between
the specified delay and the actual delay. We expect there to be
differences in cases where the main UI thread is congested, for example.

BUG=704117
==========

[Bryan McQuade, 2017-04-05 20:11:05]

https://codereview.chromium.org/2803673002/diff/60001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/experiments/delay_navigation_throttle.cc
(right):

https://codereview.chromium.org/2803673002/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/experiments/delay_navigation_throttle.cc:125:
UMA_HISTOGRAM_TIMES(kHistogramNavigationDelayDelta, delay_delta.magnitude());
On 2017/04/05 at 16:54:37, Charlie Harrison wrote:
> if actual_delay < navigation_delay_ this could be problematic. If we expect
this to not happen very often, could we just let negative values clamp to 0?

copying the relevant part of my general reply here:

I'm hesitant to clamp to zero as this prevents us from seeing the delta in cases
where it's negative. The goal of the delta histo is just to give us insight into
the accuracy of the delays overall.

[Steven Holte, 2017-04-06 17:51:57]

lgtm

[Bryan McQuade, 2017-04-06 17:52:10]

The CQ bit was checked by bmcquade@chromium.org

[commit-bot: I haz the power, 2017-04-06 17:53:07]

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

[Bryan McQuade, 2017-04-06 20:02:29]

The CQ bit was unchecked by bmcquade@chromium.org

[Bryan McQuade, 2017-04-06 20:02:32]

The CQ bit was checked by bmcquade@chromium.org

[commit-bot: I haz the power, 2017-04-06 20:03:38]

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-04-06 21:24:27]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491508952704650,
"parent_rev": "300ed63a3d3f33a6ddcf3d1d9fa49b2582e36cbd", "commit_rev":
"5339a7833dc51eae11a0e5b2f29c719122fa79a7"}

[commit-bot: I haz the power, 2017-04-06 21:25:23]

Description was changed from

==========
Add metrics for DelayNavigationThrottle

We add 3 'internal' metrics to make sure DelayNavigationThrottle is
adding the expected delays, and to understand the difference between
the specified delay and the actual delay. We expect there to be
differences in cases where the main UI thread is congested, for example.

BUG=704117
==========

to

==========
Add metrics for DelayNavigationThrottle

We add 3 'internal' metrics to make sure DelayNavigationThrottle is
adding the expected delays, and to understand the difference between
the specified delay and the actual delay. We expect there to be
differences in cases where the main UI thread is congested, for example.

BUG=704117

Review-Url: https://codereview.chromium.org/2803673002
Cr-Commit-Position: refs/heads/master@{#462626}
Committed:
https://chromium.googlesource.com/chromium/src/+/5339a7833dc51eae11a0e5b2f29c...
==========

[commit-bot: I haz the power, 2017-04-06 21:25:24]

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