UMA for data savings displayed to users.

UMA for data savings displayed to users.

Daily reported UMA for number of bytes and data savings percent over the
past 30 days.

Committed: https://crrev.com/d530b6c23c9e32775674b343cc80c164b9861d40
Cr-Commit-Position: refs/heads/master@{#333136}

[Not at Google. Contact bengr, 2015-06-04 22:56:36]

kundaji@chromium.org changed reviewers:
+ asvitkine@chromium.org, bengr@chromium.org

[Not at Google. Contact bengr, 2015-06-04 22:59:22]

bengr:
 
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc
 
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h

asvitkine:
  tools/metrics/histograms/histograms.xml

[Alexei Svitkine (slow), 2015-06-05 15:46:53]

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

https://codereview.chromium.org/1167543010/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:734:
float user_visible_savings_percent =
Nit: No need for float, since the histogram truncated to an int anyway.

https://codereview.chromium.org/1167543010/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:735:
(float)user_visible_savings_bytes * 100 / original_content_length;
Is there any chance that original_content_length would be 0? If so, this would
crash.

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

https://codereview.chromium.org/1167543010/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:19411: +<histogram
name="Net.DailyUserVisibleSavingsBytes_DataRedictionProxyEnabled"
Nit: It's misleading to have "Bytes" in the name, if the unit is KB. Either
change to KB or omit that substring entirely.

[Not at Google. Contact bengr, 2015-06-05 17:59:43]

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

https://codereview.chromium.org/1167543010/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:734:
float user_visible_savings_percent =
On 2015/06/05 15:46:53, Alexei Svitkine wrote:
> Nit: No need for float, since the histogram truncated to an int anyway.

Done.

https://codereview.chromium.org/1167543010/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:735:
(float)user_visible_savings_bytes * 100 / original_content_length;
On 2015/06/05 15:46:53, Alexei Svitkine wrote:
> Is there any chance that original_content_length would be 0? If so, this would
> crash.

Good catch. Fixed.

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

https://codereview.chromium.org/1167543010/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:19411: +<histogram
name="Net.DailyUserVisibleSavingsBytes_DataRedictionProxyEnabled"
On 2015/06/05 15:46:53, Alexei Svitkine wrote:
> Nit: It's misleading to have "Bytes" in the name, if the unit is KB. Either
> change to KB or omit that substring entirely.

Changed to "Size". WDYT?

[Alexei Svitkine (slow), 2015-06-05 18:01:43]

lgtm

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

https://codereview.chromium.org/1167543010/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:732:
if (original_content_length == 0) {
Nit: Most of Chromium style prefers no {}'s for 1-liners.

[Not at Google. Contact bengr, 2015-06-05 18:36:10]

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

https://codereview.chromium.org/1167543010/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:732:
if (original_content_length == 0) {
On 2015/06/05 18:01:43, Alexei Svitkine wrote:
> Nit: Most of Chromium style prefers no {}'s for 1-liners.

Done.

[bengr, 2015-06-05 20:58:17]

Please write tests for this new histogram.

https://codereview.chromium.org/1167543010/diff/60001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc
(right):

https://codereview.chromium.org/1167543010/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:744:
user_visible_savings_bytes >> 10);
Why is this shifted 10?

[Not at Google. Contact bengr, 2015-06-05 21:00:33]

https://codereview.chromium.org/1167543010/diff/60001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc
(right):

https://codereview.chromium.org/1167543010/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:744:
user_visible_savings_bytes >> 10);
On 2015/06/05 20:58:17, bengr wrote:
> Why is this shifted 10?

To convert to KB.

[bengr, 2015-06-05 21:14:05]

https://codereview.chromium.org/1167543010/diff/60001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc
(right):

https://codereview.chromium.org/1167543010/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:744:
user_visible_savings_bytes >> 10);
On 2015/06/05 21:00:32, kundaji wrote:
> On 2015/06/05 20:58:17, bengr wrote:
> > Why is this shifted 10?
> 
> To convert to KB.

Ok. Dividing by 1024 is clearer imho.

[bengr, 2015-06-05 21:14:41]

lgtm

[Not at Google. Contact bengr, 2015-06-05 21:17:09]

The CQ bit was checked by kundaji@chromium.org

[Not at Google. Contact bengr, 2015-06-05 21:17:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1167543010/#ps60001
(title: "Remove {} for 1 line if.")

[commit-bot: I haz the power, 2015-06-05 21:17:32]

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

[Not at Google. Contact bengr, 2015-06-05 21:19:42]

https://codereview.chromium.org/1167543010/diff/60001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc
(right):

https://codereview.chromium.org/1167543010/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:744:
user_visible_savings_bytes >> 10);
On 2015/06/05 21:14:05, bengr wrote:
> On 2015/06/05 21:00:32, kundaji wrote:
> > On 2015/06/05 20:58:17, bengr wrote:
> > > Why is this shifted 10?
> > 
> > To convert to KB.
> 
> Ok. Dividing by 1024 is clearer imho.

Discussed in person. This file is using ">> 10" throughout, so keeping as is for
consistency.

[commit-bot: I haz the power, 2015-06-05 21:22:22]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-06-05 21:23:20]

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