Record UMA when googlezip proxies are removed from the proxy config.

Record UMA when googlezip proxies are removed from the proxy config.

This change records UMA about how often *.googlezip.net proxies are
found and removed from the effective proxy configuration.

This will allow us to recognize when the temporary fix to remove those
proxies from the proxy config is no longer necessary, and can be safely
deleted.

BUG=488208
Committed: https://crrev.com/8959c37c86bc8081e1a7cc353f3b90ae5155acbf
Cr-Commit-Position: refs/heads/master@{#330131}

[sclittle, 2015-05-14 20:43:40]

sclittle@chromium.org changed reviewers:
+ asvitkine@chromium.org, eroman@chromium.org

[sclittle, 2015-05-14 20:43:40]

eroman: chrome/browser/net/*
asvitkine: histograms.xml

Thanks in advance!

[Alexei Svitkine (slow), 2015-05-14 20:46:35]

lgtm % nit

https://codereview.chromium.org/1145513004/diff/1/chrome/browser/net/pref_pro...
File chrome/browser/net/pref_proxy_config_tracker_impl.cc (right):

https://codereview.chromium.org/1145513004/diff/1/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_tracker_impl.cc:76:
&proxy_rules->fallback_proxies) |
Nit: Are you using bitwise OR's to avoid short-circuit evaluation of logical
ORs? If so, add a comment, so someone doesn't go and "clean it up" later.
Otherwise, use logical ORs.

[sclittle, 2015-05-14 20:56:29]

https://codereview.chromium.org/1145513004/diff/1/chrome/browser/net/pref_pro...
File chrome/browser/net/pref_proxy_config_tracker_impl.cc (right):

https://codereview.chromium.org/1145513004/diff/1/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_tracker_impl.cc:76:
&proxy_rules->fallback_proxies) |
On 2015/05/14 20:46:34, Alexei Svitkine wrote:
> Nit: Are you using bitwise OR's to avoid short-circuit evaluation of logical
> ORs? If so, add a comment, so someone doesn't go and "clean it up" later.
> Otherwise, use logical ORs.

Done.

[sclittle, 2015-05-14 21:24:29]

sclittle@chromium.org changed reviewers:
+ bengr@chromium.org, jeremyim@chromium.org, kundaji@chromium.org

[sclittle, 2015-05-14 21:24:29]

Adding more DRP folks as reviewers to make sure the right stats are being
recorded here.

[jeremyim, 2015-05-14 21:39:31]

lgtm

[eroman, 2015-05-14 22:20:15]

LGTM once comments addressed

https://codereview.chromium.org/1145513004/diff/20001/chrome/browser/net/pref...
File chrome/browser/net/pref_proxy_config_tracker_impl.cc (right):

https://codereview.chromium.org/1145513004/diff/20001/chrome/browser/net/pref...
chrome/browser/net/pref_proxy_config_tracker_impl.cc:72: // *.googlezip.net
proxies are no longer present in the |proxy_rules|.
Is there a separate strategy to actually remove them from the prefs? (Since this
code here just works around bad prefs, but does not actually modify the prefs)

https://codereview.chromium.org/1145513004/diff/20001/chrome/browser/net/pref...
chrome/browser/net/pref_proxy_config_tracker_impl.cc:78: if
(RemoveGooglezipDataReductionProxiesFromList(
I don't like this. How about

bool removed = false;
removed |= RemoveGoogleZip...
removed |= RemovedGoogleZip..

Or alternately having RemoveGoogleZip return an integer count of how many were
removed. Then you could do:

size_t num_removed = RemoveGoogleZip...(...) + RemoveGoogleZip(...)

if (num_removed > 0) {
   RecordGoogleZip...
}

https://codereview.chromium.org/1145513004/diff/20001/chrome/browser/net/pref...
chrome/browser/net/pref_proxy_config_tracker_impl.cc:92:
RecordGooglezipProxyRemovalResultHistogram(
style: I don't like having two calls to RecordGooglezip...()
Maybe just set the ProxyRemovalResult, and then call RecordHistogram once.  And
in fact at that point may as well just inline it.

https://codereview.chromium.org/1145513004/diff/20001/chrome/browser/net/pref...
File chrome/browser/net/pref_proxy_config_tracker_impl_unittest.cc (right):

https://codereview.chromium.org/1145513004/diff/20001/chrome/browser/net/pref...
chrome/browser/net/pref_proxy_config_tracker_impl_unittest.cc:377:
histogram_tester.ExpectUniqueSample(
neat, thanks for adding the test!

https://codereview.chromium.org/1145513004/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1145513004/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:54446: +  <int value="0" label="No
*.googlezip.net proxies found"/>
I think it is more typical to have the enum value, but looks fine to me.

[Not at Google. Contact bengr, 2015-05-14 23:07:16]

lgtm

[sclittle, 2015-05-15 02:29:42]

https://codereview.chromium.org/1145513004/diff/20001/chrome/browser/net/pref...
File chrome/browser/net/pref_proxy_config_tracker_impl.cc (right):

https://codereview.chromium.org/1145513004/diff/20001/chrome/browser/net/pref...
chrome/browser/net/pref_proxy_config_tracker_impl.cc:72: // *.googlezip.net
proxies are no longer present in the |proxy_rules|.
On 2015/05/14 22:20:15, eroman (sick slow) wrote:
> Is there a separate strategy to actually remove them from the prefs? (Since
this
> code here just works around bad prefs, but does not actually modify the prefs)

The strategy to remove the bad pref actually already exists:
https://codereview.chromium.org/1104413003

The issue was that the problem was still occuring for clients with that fix. At
the moment, we suspect that this is caused by more than one bug in the code -
the pref migration issue that was addressed in
https://codereview.chromium.org/1104413003 - and something else that we haven't
uncovered yet.

This hack is just to make absolutely sure that the pref isn't the problem, just
in case there's a race with prefs or something like that which is triggering the
DRP to be used when it shouldn't be. We suspect that
https://codereview.chromium.org/1135153002 will treat the symptoms of that
second bug.

Ideally, the UMA added in this CL will indicate that this hack is helping a
negligible number of clients, and then we can remove this hack.

https://codereview.chromium.org/1145513004/diff/20001/chrome/browser/net/pref...
chrome/browser/net/pref_proxy_config_tracker_impl.cc:78: if
(RemoveGooglezipDataReductionProxiesFromList(
On 2015/05/14 22:20:15, eroman (sick slow) wrote:
> I don't like this. How about
> 
> bool removed = false;
> removed |= RemoveGoogleZip...
> removed |= RemovedGoogleZip..
> 
> Or alternately having RemoveGoogleZip return an integer count of how many were
> removed. Then you could do:
> 
> size_t num_removed = RemoveGoogleZip...(...) + RemoveGoogleZip(...)
> 
> if (num_removed > 0) {
>    RecordGoogleZip...
> }

Done. Changed to report total number of proxies removed.

https://codereview.chromium.org/1145513004/diff/20001/chrome/browser/net/pref...
chrome/browser/net/pref_proxy_config_tracker_impl.cc:92:
RecordGooglezipProxyRemovalResultHistogram(
On 2015/05/14 22:20:15, eroman (sick slow) wrote:
> style: I don't like having two calls to RecordGooglezip...()
> Maybe just set the ProxyRemovalResult, and then call RecordHistogram once. 
And
> in fact at that point may as well just inline it.

Now that it's reporting total number of proxies removed, there's only one
RecordHistogram call.

https://codereview.chromium.org/1145513004/diff/20001/chrome/browser/net/pref...
File chrome/browser/net/pref_proxy_config_tracker_impl_unittest.cc (right):

https://codereview.chromium.org/1145513004/diff/20001/chrome/browser/net/pref...
chrome/browser/net/pref_proxy_config_tracker_impl_unittest.cc:377:
histogram_tester.ExpectUniqueSample(
On 2015/05/14 22:20:15, eroman (sick slow) wrote:
> neat, thanks for adding the test!

Acknowledged.

https://codereview.chromium.org/1145513004/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1145513004/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:54446: +  <int value="0" label="No
*.googlezip.net proxies found"/>
On 2015/05/14 22:20:15, eroman (sick slow) wrote:
> I think it is more typical to have the enum value, but looks fine to me.

I've removed the enum, and it's just reporting the count of removed proxies now.

[eroman, 2015-05-15 02:38:28]

LGTM.

You might consider updating the CL title too.

[Alexei Svitkine (slow), 2015-05-15 14:52:25]

re-lgtm

[bengr, 2015-05-15 16:14:18]

On 2015/05/15 14:52:25, Alexei Svitkine wrote:
> re-lgtm

In this or in another CL, it would be good to track the clearing of the proxy
pref, and correlate with this.

[sclittle, 2015-05-15 17:09:46]

The CQ bit was checked by sclittle@chromium.org

[sclittle, 2015-05-15 17:09:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
kundaji@chromium.org, jeremyim@chromium.org
Link to the patchset: https://codereview.chromium.org/1145513004/#ps40001
(title: "Changed to report number of proxies removed")

[sclittle, 2015-05-15 17:09:58]

On 2015/05/15 16:14:18, bengr wrote:
> On 2015/05/15 14:52:25, Alexei Svitkine wrote:
> > re-lgtm
> 
> In this or in another CL, it would be good to track the clearing of the proxy
> pref, and correlate with this.

Sure, I'll write up that CL now.

[commit-bot: I haz the power, 2015-05-15 17:10:55]

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

[commit-bot: I haz the power, 2015-05-15 18:06:49]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-05-15 18:07:54]

Patchset 3 (id:??) landed as
https://crrev.com/8959c37c86bc8081e1a7cc353f3b90ae5155acbf
Cr-Commit-Position: refs/heads/master@{#330131}