Add CLIENT_REDIRECT variants for page load metrics abort types

Add CLIENT_REDIRECT variants for page load metrics abort types

This patch adds support for client redirects aborting navigations. Right
now it only adds variants to the global abort metrics, because these
metrics are not user initiated and probably we should eventually end up
filtering them out.

Hopefully, this variant eats up a lot of the mass in non user-initiated
timescales in the current abort histograms.

BUG=557430, 633329
Committed: https://crrev.com/8a0cfcc13e7be3684a6f6143160a3d655f2e1a51
Cr-Commit-Position: refs/heads/master@{#409831}

[Charlie Harrison, 2016-08-01 20:29:07]

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

[commit-bot: I haz the power, 2016-08-01 20:29:27]

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

[Charlie Harrison, 2016-08-01 20:32:00]

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

[Charlie Harrison, 2016-08-01 20:32:01]

Bryan, PTAL. Hopefully this goes a long way into solving our aborts problem.

[Charlie Harrison, 2016-08-01 20:36:54]

Oh I forgot to add histograms.xml. I'll add it in the next PS.

Also, WDYT we should do about versioning the histograms?

[commit-bot: I haz the power, 2016-08-01 20:44:00]

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

[commit-bot: I haz the power, 2016-08-01 20:44:02]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Bryan McQuade, 2016-08-02 13:49:10]

On 2016/08/01 at 20:36:54, csharrison wrote:
> Oh I forgot to add histograms.xml. I'll add it in the next PS.
> 
> Also, WDYT we should do about versioning the histograms?

This seems like a good change, thank you! Looks like some of the try job bots
are red though. Can you take a look?

[Charlie Harrison, 2016-08-02 13:57:47]

Yeah it's just rebase errors. I'll rebase off the most recent aborts PS.

[Charlie Harrison, 2016-08-02 17:26:01]

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

[commit-bot: I haz the power, 2016-08-02 17:26:44]

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, 2016-08-02 18:23:34]

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

[commit-bot: I haz the power, 2016-08-02 18:23:35]

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_...)

[Charlie Harrison, 2016-08-02 19:45:40]

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

[commit-bot: I haz the power, 2016-08-02 19:46:40]

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

[Charlie Harrison, 2016-08-02 20:41:15]

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

[Charlie Harrison, 2016-08-02 20:41:15]

bmcquade@ PTAL.

mushan@, this patch adds another bucket to the aborts, but does not expose them
to FromGoogleSearch metrics because they are not user initiated. Is this okay
with you? It will reduce mass in your histograms but I am hoping it is the noisy
mass we don't like :) WDYT? I added you as a reviewer for your sign off.

[Bryan McQuade, 2016-08-04 11:35:29]

lgtm thanks! just wrapping up code reviews - this looks good. looking forward to
have it!

https://codereview.chromium.org/2199953002/diff/60001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2199953002/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc:29:
"PageLoad.AbortTiming.ClientRedirect.BeforePaint";
just for consistency for now, let's use the 'AfterCommit.BeforePaint' suffix
like the others.

[mushan, 2016-08-04 11:47:02]

Why is this depending on 2132603002?

I will assume most aborts in FromGoogleSearch should not trigger client
redirects. But there will still be some cases page from google search fire
client redirects right? I would prefer also add this to FromGoogleSearch. That
would eliminate one type of concern we would have towards aborts.

lgtm. Thanks a lot!

[mushan, 2016-08-04 12:00:11]

On 2016/08/04 at 11:47:02, mushan wrote:
> Why is this depending on 2132603002?
> 
> I will assume most aborts in FromGoogleSearch should not trigger client
redirects. But there will still be some cases page from google search fire
client redirects right? I would prefer also add this to FromGoogleSearch. That
would eliminate one type of concern we would have towards aborts.
> 
> lgtm. Thanks a lot!

By 'adding this to FromGoogleSearch' I'm actually suggesting removing this kind
of aborts from other user initiated aborts in FromGoogleSearch.

Please let me know if I understand mistakenly.

[Charlie Harrison, 2016-08-04 12:02:24]

On 2016/08/04 12:00:11, mushan wrote:
> On 2016/08/04 at 11:47:02, mushan wrote:
> > Why is this depending on 2132603002?
> > 
> > I will assume most aborts in FromGoogleSearch should not trigger client
> redirects. But there will still be some cases page from google search fire
> client redirects right? I would prefer also add this to FromGoogleSearch. That
> would eliminate one type of concern we would have towards aborts.
> > 
> > lgtm. Thanks a lot!
> 
> By 'adding this to FromGoogleSearch' I'm actually suggesting removing this
kind
> of aborts from other user initiated aborts in FromGoogleSearch.
The current CL has this behavior. By adding another bucket we ensure client
redirects that previously took up space in other histograms (i.e. most went to
NewNavigation) end up being filtered. This is the case for FromGoogleSearch
navigations.

We would only add a histogram if you actually care about the client redirect
timings.

> 
> Please let me know if I understand mistakenly.

[mushan, 2016-08-04 12:15:59]

On 2016/08/04 at 12:02:24, csharrison wrote:
> The current CL has this behavior. By adding another bucket we ensure client
redirects that previously took up space in other histograms (i.e. most went to
NewNavigation) end up being filtered. This is the case for FromGoogleSearch
navigations.
> 
> We would only add a histogram if you actually care about the client redirect
timings.
> 

That's perfect. We don't case about the client redirect timings. Please keep it
like this way.

Thanks a lot for implementing this!

[Charlie Harrison, 2016-08-04 13:19:15]

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

[Charlie Harrison, 2016-08-04 13:19:33]

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

[Charlie Harrison, 2016-08-04 13:19:34]

+holte for histograms.xml, thanks!

https://codereview.chromium.org/2199953002/diff/60001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2199953002/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc:29:
"PageLoad.AbortTiming.ClientRedirect.BeforePaint";
On 2016/08/04 11:35:29, Bryan McQuade wrote:
> just for consistency for now, let's use the 'AfterCommit.BeforePaint' suffix
> like the others.

Oops good catch. Done.

[commit-bot: I haz the power, 2016-08-04 13:19:46]

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, 2016-08-04 14:29:23]

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

[commit-bot: I haz the power, 2016-08-04 14:29:24]

Dry run: This issue passed the CQ dry run.

[Steven Holte, 2016-08-04 17:55:21]

lgtm

[Charlie Harrison, 2016-08-04 17:58:43]

Thanks, everyone

[Charlie Harrison, 2016-08-04 17:58:49]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2016-08-04 17:58:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmcquade@chromium.org, mushan@chromium.org
Link to the patchset: https://codereview.chromium.org/2199953002/#ps80001
(title: "fix histogram name")

[commit-bot: I haz the power, 2016-08-04 17:59:25]

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, 2016-08-04 18:03:47]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-04 18:06:06]

Description was changed from

==========
Add CLIENT_REDIRECT variants for page load metrics abort types

This patch adds support for client redirects aborting navigations. Right
now it only adds variants to the global abort metrics, because these
metrics are not user initiated and probably we should eventually end up
filtering them out.

Hopefully, this variant eats up a lot of the mass in non user-initiated
timescales in the current abort histograms.

BUG=557430,633329
==========

to

==========
Add CLIENT_REDIRECT variants for page load metrics abort types

This patch adds support for client redirects aborting navigations. Right
now it only adds variants to the global abort metrics, because these
metrics are not user initiated and probably we should eventually end up
filtering them out.

Hopefully, this variant eats up a lot of the mass in non user-initiated
timescales in the current abort histograms.

BUG=557430,633329

Committed: https://crrev.com/8a0cfcc13e7be3684a6f6143160a3d655f2e1a51
Cr-Commit-Position: refs/heads/master@{#409831}
==========

[commit-bot: I haz the power, 2016-08-04 18:06:08]

Patchset 5 (id:??) landed as
https://crrev.com/8a0cfcc13e7be3684a6f6143160a3d655f2e1a51
Cr-Commit-Position: refs/heads/master@{#409831}