Add UMA histograms for Google cached copy experiment on network error interstitial

Add UMA histograms for Google cached copy experiment on network error interstitial

Tracks usage:
+ of promoted 'Google cached copy' link doctor button.
+ on each of the two button label variants.

BUG=474848
Committed: https://crrev.com/6bda7c88cd5dcf2423254ca08c047cbaf7cddfcd
Cr-Commit-Position: refs/heads/master@{#339714}

[edwardjung, 2015-07-13 18:34:01]

edwardjung@chromium.org changed reviewers:
+ asvitkine@google.com, jochen@chromium.org, mmenke@chromium.org

[edwardjung, 2015-07-13 18:34:01]

Hi, please could you take a look at this CL as follows:

jochen:
chrome/renderer/resources/neterror.js
chrome/common/localized_error.cc

mmenke: 	
chrome/renderer/net/*
components/error_page/renderer/*
components/error_page/common/net_error_info.h

asvitkine: 
tools/metrics/histograms/histograms.xml

Thanks.

[Alexei Svitkine (slow), 2015-07-13 18:40:42]

asvitkine@chromium.org changed reviewers:
+ asvitkine@chromium.org
- asvitkine@google.com

[Alexei Svitkine (slow), 2015-07-13 18:40:49]

lgtm

[mmenke, 2015-07-13 18:56:51]

I'm reluctant to land any changes here until we have http://crbug.com/500556
figured out - unclear what's going on, but I'm concerned about it skewing stats,
or making them not mean what we think they mean.

[edwardjung, 2015-07-14 10:52:43]

On 2015/07/13 18:56:51, mmenke wrote:
> I'm reluctant to land any changes here until we have http://crbug.com/500556
> figured out - unclear what's going on, but I'm concerned about it skewing
stats,
> or making them not mean what we think they mean.

I don't think the change here would affect the current stats as it's just adding
extra histogram items. But understand this CL does change the executeButtonPress
function. I'll hold off for now. 

What's your estimated time scale for collecting enough metrics? Are you waiting
for this to land in stable.

[mmenke, 2015-07-14 13:37:03]

On 2015/07/14 10:52:43, edwardjung wrote:
> On 2015/07/13 18:56:51, mmenke wrote:
> > I'm reluctant to land any changes here until we have http://crbug.com/500556
> > figured out - unclear what's going on, but I'm concerned about it skewing
> stats,
> > or making them not mean what we think they mean.
> 
> I don't think the change here would affect the current stats as it's just
adding
> extra histogram items. But understand this CL does change the
executeButtonPress
> function. I'll hold off for now. 
> 
> What's your estimated time scale for collecting enough metrics? Are you
waiting
> for this to land in stable.

I'm not concerned about this skewing other stats...I'm concerned about the bug
skewing the stats this adds, and any decisions made based on them.  There's
clearly something funky going on with the error pages.

[edwardjung, 2015-07-14 16:13:16]

> I'm not concerned about this skewing other stats...I'm concerned about the bug
> skewing the stats this adds, and any decisions made based on them.  There's
> clearly something funky going on with the error pages.

Understood. Saw your update on the bug so will wait for an update on any
findings you get.

[mmenke, 2015-07-14 16:33:48]

On 2015/07/14 16:13:16, edwardjung wrote:
> > I'm not concerned about this skewing other stats...I'm concerned about the
bug
> > skewing the stats this adds, and any decisions made based on them.  There's
> > clearly something funky going on with the error pages.
> 
> Understood. Saw your update on the bug so will wait for an update on any
> findings you get.

I'm less concerned about the other bug, now that we have histograms that seem to
indicate the problem is not in NetErrorHelperCore's logic, but want to spend a
bit more time investigating before I sign off.

[mmenke, 2015-07-20 18:14:28]

Done a little digging on the other issue.  Still not solved, but I'm less
concerned than I was, so not going to block this CL on it.

https://codereview.chromium.org/1214303003/diff/80001/components/error_page/c...
File components/error_page/common/net_error_info.h (right):

https://codereview.chromium.org/1214303003/diff/80001/components/error_page/c...
components/error_page/common/net_error_info.h:40:
NETWORK_ERROR_PAGE_CACHED_PAGE_BUTTON_CLICKED,
Is the only difference here the label?  I don't think that's at all clear from
the code here, in net_error_helper_core.h, or in localized_error.cc.

Suggest some comment making it clearer.  Maybe 'For "Google cached copy"
button.' -> 'For "Google cached copy" button label experiment.'  Suggest doing
something similar at the other locations.

https://codereview.chromium.org/1214303003/diff/80001/components/error_page/c...
components/error_page/common/net_error_info.h:40:
NETWORK_ERROR_PAGE_CACHED_PAGE_BUTTON_CLICKED,
These are presumably going to be removed at some point.  To avoid making the
histogram a weird mix of old and new values, that gives us two options:

1)  Add numbers to the events, and when events are removed, comment out the old
values, so new histograms won't re-use the values.
2)  Rename the histogram whenever we change it.  This has the upside of not
mixing data from the two different histograms.  Not as important here, as this
is more of an event count than an actual histogram, but does make sure we're
comparing comparable values.

[edwardjung, 2015-07-21 11:43:37]

Thanks, added numbers to the events as suggested.

https://codereview.chromium.org/1214303003/diff/80001/components/error_page/c...
File components/error_page/common/net_error_info.h (right):

https://codereview.chromium.org/1214303003/diff/80001/components/error_page/c...
components/error_page/common/net_error_info.h:40:
NETWORK_ERROR_PAGE_CACHED_PAGE_BUTTON_CLICKED,
On 2015/07/20 18:14:27, mmenke wrote:
> These are presumably going to be removed at some point.  To avoid making the
> histogram a weird mix of old and new values, that gives us two options:
> 
> 1)  Add numbers to the events, and when events are removed, comment out the
old
> values, so new histograms won't re-use the values.
> 2)  Rename the histogram whenever we change it.  This has the upside of not
> mixing data from the two different histograms.  Not as important here, as this
> is more of an event count than an actual histogram, but does make sure we're
> comparing comparable values.

Yes these would be removed once the experiment would be over. I'll implement
option 1 for now.

https://codereview.chromium.org/1214303003/diff/80001/components/error_page/c...
components/error_page/common/net_error_info.h:40:
NETWORK_ERROR_PAGE_CACHED_PAGE_BUTTON_CLICKED,
On 2015/07/20 18:14:27, mmenke wrote:
> Is the only difference here the label?  I don't think that's at all clear from
> the code here, in net_error_helper_core.h, or in localized_error.cc.
> 
> Suggest some comment making it clearer.  Maybe 'For "Google cached copy"
> button.' -> 'For "Google cached copy" button label experiment.'  Suggest doing
> something similar at the other locations.

Makes sense. Added comments.

[mmenke, 2015-07-21 14:46:53]

LGTM

https://codereview.chromium.org/1214303003/diff/100001/components/error_page/...
File components/error_page/renderer/net_error_helper_core.h (right):

https://codereview.chromium.org/1214303003/diff/100001/components/error_page/...
components/error_page/renderer/net_error_helper_core.h:52:
SHOW_CACHED_PAGE_BUTTON, // "Google cached page" button label experiment.
nit:  2 spaces before comment.

[edwardjung, 2015-07-21 15:05:48]

jochen@ could you take a look at:
chrome/common/localized_error.*
chrome/renderer/resources/neterror.js

Thanks.

https://codereview.chromium.org/1214303003/diff/100001/components/error_page/...
File components/error_page/renderer/net_error_helper_core.h (right):

https://codereview.chromium.org/1214303003/diff/100001/components/error_page/...
components/error_page/renderer/net_error_helper_core.h:52:
SHOW_CACHED_PAGE_BUTTON, // "Google cached page" button label experiment.
On 2015/07/21 14:46:53, mmenke wrote:
> nit:  2 spaces before comment.

Done.

[jochen (gone - plz use gerrit), 2015-07-21 15:08:09]

lgtm

[edwardjung, 2015-07-21 15:28:08]

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

[commit-bot: I haz the power, 2015-07-21 15:28:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1214303003/120001

[commit-bot: I haz the power, 2015-07-21 15:44:41]

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

[commit-bot: I haz the power, 2015-07-21 15:44:42]

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

[edwardjung, 2015-07-21 18:45:49]

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

[commit-bot: I haz the power, 2015-07-21 18:47:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1214303003/140001

[commit-bot: I haz the power, 2015-07-21 19:14:03]

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

[commit-bot: I haz the power, 2015-07-21 19:14:05]

Dry run: This issue passed the CQ dry run.

[edwardjung, 2015-07-21 19:15:14]

The CQ bit was checked by edwardjung@chromium.org

[edwardjung, 2015-07-21 19:15:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, mmenke@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1214303003/#ps140001
(title: "Fix NetErrorHelperCore unittest")

[commit-bot: I haz the power, 2015-07-21 19:15:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1214303003/140001

[commit-bot: I haz the power, 2015-07-21 19:23:28]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-07-21 19:49:06]

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