Data reduction proxy enabled/disabled UMA for all platforms

Data reduction proxy enabled/disabled UMA for all platforms

New histogram DataReductionProxy.EnabledState is added to count whenever
user toggles ON/OFF switch in settings menu for reducing data usage.

For desktop, Off-to-On histogram count will increment when the data
saver extension is installed as well.

BUG=489731
Committed: https://crrev.com/8a0046e4a9f9770f916aa4db26c915fbd4c9a816
Cr-Commit-Position: refs/heads/master@{#354199}

[Raj, 2015-10-09 20:28:31]

rajendrant@chromium.org changed reviewers:
+ bengr@chromium.org, jwd@chromium.org, megjablon@chromium.org,
sclittle@chromium.org

[bengr, 2015-10-09 20:45:16]

bengr@chromium.org changed reviewers:
- megjablon@chromium.org

[sclittle, 2015-10-09 20:52:48]

https://codereview.chromium.org/1400903002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc
(right):

https://codereview.chromium.org/1400903002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:298:
void DataReductionProxySettings::RecordSettingsEnabledState(
It doesn't look like this function needs to be a method of
DataReductionProxySettings - could you move this function into the anonymous
namespace at the top of this file? You can remove the method definition in the
.h file as well.

https://codereview.chromium.org/1400903002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h
(right):

https://codereview.chromium.org/1400903002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h:254:
FRIEND_TEST_ALL_PREFIXES(DataReductionProxySettingsTest,
I don't think you need this FRIEND_TEST_ALL_PREFIXES, since your test isn't
trying to call any protected or private methods of DataReductionProxySettings.

https://codereview.chromium.org/1400903002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc
(right):

https://codereview.chromium.org/1400903002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc:643:
EXPECT_EQ(0, samples->TotalCount());
Could this be replaced with:

histogram_tester.ExpectTotalCount(kUMAEnabledState, 0);

You could then remove the samples variable.

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

https://codereview.chromium.org/1400903002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:5799: +    platforms(android, IOS,
desktop extension). Data is collected whenever user
nit: add space between "platforms" and "("

https://codereview.chromium.org/1400903002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:54914: +  <int value="0" label="Turned
On"/>
nit: Just for consistency, could you label these values "Off-to-On" and
"On-to-Off"?

[jwd, 2015-10-09 21:33:07]

I think you typed the bug id wrong, I'm getting 404.

Do you have metrics tracking the total number of clients with it on or off? I
assume this metric is just to when users are playing with the setting.

Are you planning on doing any analysis that will look at if users change this
repeatedly over time, or are do you just want to see counts of times it's
changed?

[Raj, 2015-10-09 22:18:55]

On 2015/10/09 21:33:07, Jesse Doherty wrote:
> I think you typed the bug id wrong, I'm getting 404.
> 
> Do you have metrics tracking the total number of clients with it on or off? I
> assume this metric is just to when users are playing with the setting.
> 
> Are you planning on doing any analysis that will look at if users change this
> repeatedly over time, or are do you just want to see counts of times it's
> changed?

Sorry. updated bugid - crbug.com/489731

Yes. this metric is just to when users toggle the setting.

We have metric "DataReductionProxy.StartupState" to track if it on/off when
chrome starts.

[bengr, 2015-10-09 23:23:58]

On 2015/10/09 22:18:55, Raj wrote:
> On 2015/10/09 21:33:07, Jesse Doherty wrote:
> > I think you typed the bug id wrong, I'm getting 404.
> > 
> > Do you have metrics tracking the total number of clients with it on or off?
I
> > assume this metric is just to when users are playing with the setting.
> > 
> > Are you planning on doing any analysis that will look at if users change
this
> > repeatedly over time, or are do you just want to see counts of times it's
> > changed?
> 
> Sorry. updated bugid - crbug.com/489731
> 
> Yes. this metric is just to when users toggle the setting.
> 
> We have metric "DataReductionProxy.StartupState" to track if it on/off when
> chrome starts.

Both. We are interested in counts and to see how common it is for users to
toggle the setting multiple times during a session.

[bengr, 2015-10-09 23:24:18]

bengr@chromium.org changed reviewers:
- bengr@chromium.org

[Raj, 2015-10-10 01:51:47]

https://codereview.chromium.org/1400903002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc
(right):

https://codereview.chromium.org/1400903002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:298:
void DataReductionProxySettings::RecordSettingsEnabledState(
On 2015/10/09 20:52:48, sclittle wrote:
> It doesn't look like this function needs to be a method of
> DataReductionProxySettings - could you move this function into the anonymous
> namespace at the top of this file? You can remove the method definition in the
> .h file as well.

Done.

https://codereview.chromium.org/1400903002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h
(right):

https://codereview.chromium.org/1400903002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h:254:
FRIEND_TEST_ALL_PREFIXES(DataReductionProxySettingsTest,
On 2015/10/09 20:52:48, sclittle wrote:
> I don't think you need this FRIEND_TEST_ALL_PREFIXES, since your test isn't
> trying to call any protected or private methods of DataReductionProxySettings.

TestSettingsEnabledStateHistograms() calls settings_->InitPrefMembers() which
requires it to be friend

https://codereview.chromium.org/1400903002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc
(right):

https://codereview.chromium.org/1400903002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc:643:
EXPECT_EQ(0, samples->TotalCount());
On 2015/10/09 20:52:48, sclittle wrote:
> Could this be replaced with:
> 
> histogram_tester.ExpectTotalCount(kUMAEnabledState, 0);
> 
> You could then remove the samples variable.

Done.

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

https://codereview.chromium.org/1400903002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:5799: +    platforms(android, IOS,
desktop extension). Data is collected whenever user
On 2015/10/09 20:52:48, sclittle wrote:
> nit: add space between "platforms" and "("

Done.

https://codereview.chromium.org/1400903002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:54914: +  <int value="0" label="Turned
On"/>
On 2015/10/09 20:52:48, sclittle wrote:
> nit: Just for consistency, could you label these values "Off-to-On" and
> "On-to-Off"?

Done.

[sclittle, 2015-10-12 20:16:04]

lgtm

[tbansal1, 2015-10-13 01:44:57]

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

[tbansal1, 2015-10-13 01:44:58]

drive-by-review.

lgtm % comments.

https://codereview.chromium.org/1400903002/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h
(right):

https://codereview.chromium.org/1400903002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h:60:
// DataReductionProxyEnabledState in
nit: Seems like this can go back on previous line and still stay within 80 char
limit?

https://codereview.chromium.org/1400903002/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc
(right):

https://codereview.chromium.org/1400903002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc:639:
// No settings state histograms should be recorded during startup
Missing period at end.

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

https://codereview.chromium.org/1400903002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:5802: +    For desktop, Off-to-On
histogram count will increment when the data saver
Now, I am wondering what happens when the extension is uninstalled :)

[jwd, 2015-10-14 19:30:49]

lgtm

[Raj, 2015-10-14 23:14:48]

https://codereview.chromium.org/1400903002/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h
(right):

https://codereview.chromium.org/1400903002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h:60:
// DataReductionProxyEnabledState in
On 2015/10/13 01:44:57, tbansal1 wrote:
> nit: Seems like this can go back on previous line and still stay within 80
char
> limit?

Done.

https://codereview.chromium.org/1400903002/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc
(right):

https://codereview.chromium.org/1400903002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc:639:
// No settings state histograms should be recorded during startup
On 2015/10/13 01:44:58, tbansal1 wrote:
> Missing period at end.

Done.

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

https://codereview.chromium.org/1400903002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:5802: +    For desktop, Off-to-On
histogram count will increment when the data saver
On 2015/10/13 01:44:58, tbansal1 wrote:
> Now, I am wondering what happens when the extension is uninstalled :)

good q, uninstall will count as On-to-Off - added this info as well.

[sclittle, 2015-10-14 23:16:31]

Still LGTM

[Raj, 2015-10-15 00:57:48]

The CQ bit was checked by rajendrant@chromium.org

[Raj, 2015-10-15 00:57:49]

The patchset sent to the CQ was uploaded after l-g-t-m from jwd@chromium.org,
tbansal@chromium.org
Link to the patchset: https://codereview.chromium.org/1400903002/#ps60001
(title: "Addressed tbansal1 comments")

[commit-bot: I haz the power, 2015-10-15 00:58:17]

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

[commit-bot: I haz the power, 2015-10-15 02:05:07]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-10-15 02:06:43]

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