Fix UMA to track video data usage.

Fix UMA to track video data usage.

Add prefs to store data usage for mime-types used by video content. Write the data from prefs to UMA once a day.
The initial implementation of video data usage UMAs was incorrect since it was not maintaining historic data, and was incorrectly assigning data usage to the mime type of the request which triggered reporting.

BUG=516822, 481025
Committed: https://crrev.com/dde0c436fe9c4f95b826bbeb779e77fcff657305
Cr-Commit-Position: refs/heads/master@{#344515}

Committed: https://crrev.com/3538199987e27b6bf98e8e15421d5f8f8722ab3d
Cr-Commit-Position: refs/heads/master@{#344634}

[Not at Google. Contact bengr, 2015-08-11 00:04:16]

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

[Not at Google. Contact bengr, 2015-08-11 00:04:41]

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
  components/data_reduction_proxy/core/common/data_reduction_proxy_pref_names.cc
  components/data_reduction_proxy/core/common/data_reduction_proxy_pref_names.h

[bengr, 2015-08-11 16:57:22]

Please add the specifics of what you fixed to the CL description.

[bengr, 2015-08-11 17:10:25]

In general, our list of prefs containing statistics has grown to an absurd
level. I'd like to prioritize moving all of this to a db. I filed
https://code.google.com/p/chromium/issues/detail?id=519572

https://codereview.chromium.org/1286643002/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc
(right):

https://codereview.chromium.org/1286643002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:674:
DailyDataSavingUpdate proxy_enabled_application(
Could you write another helper akin to RecordDailySavingsUMA for these cases?

https://codereview.chromium.org/1286643002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:871:
void DataReductionProxyCompressionStats::RecordDailySavingsUma(
Test?

https://codereview.chromium.org/1286643002/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h
(right):

https://codereview.chromium.org/1286643002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h:145:
void RecordDailySavingsUma(const char* original_prefname,
I think we've been using all caps for UMA.

[Not at Google. Contact bengr, 2015-08-12 19:49:59]

Added more information to description.

Yes, moving these prefs to storage is a good idea. The changes will be
non-trivial but seem important.

https://codereview.chromium.org/1286643002/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc
(right):

https://codereview.chromium.org/1286643002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:674:
DailyDataSavingUpdate proxy_enabled_application(
On 2015/08/11 17:10:24, bengr wrote:
> Could you write another helper akin to RecordDailySavingsUMA for these cases?

Done. Added AdjustDailyDataSavingUpdate().

https://codereview.chromium.org/1286643002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:871:
void DataReductionProxyCompressionStats::RecordDailySavingsUma(
On 2015/08/11 17:10:24, bengr wrote:
> Test?

Added new tests as well as changed existing tests to also apply to mime type
unknown.

Note that this method had to be removed because UMA_HISTOGRAM_COUNTS cannot be
used with a variable for its first argument.

https://codereview.chromium.org/1286643002/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h
(right):

https://codereview.chromium.org/1286643002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h:145:
void RecordDailySavingsUma(const char* original_prefname,
On 2015/08/11 17:10:25, bengr wrote:
> I think we've been using all caps for UMA.

Done.

[bengr, 2015-08-12 22:16:27]

lgtm with nit.

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

https://codereview.chromium.org/1286643002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h:146:
// is for current day. Maintains invariant that lists have length
for -> for the

[Not at Google. Contact bengr, 2015-08-17 22:34:11]

Prefs used to log daily savings UMA are maintaining stats for 60 days even
though only the previous day is reported by the UMA.
I changed the new prefs to int64. PTAL.

When the existing prefs are migrated to a data store, they should be changed
from a list pref to a int64 pref.

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

https://codereview.chromium.org/1286643002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h:146:
// is for current day. Maintains invariant that lists have length
On 2015/08/12 22:16:27, bengr wrote:
> for -> for the

Removed method.

[bengr, 2015-08-18 19:44:57]

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

https://codereview.chromium.org/1286643002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:711:
RECORD_INT64PREF_TO_HISTOGRAM(
Instead of all of these macro calls, could you construct an array of const chase
and loop through it?

https://codereview.chromium.org/1286643002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:955:
IncrementInt64Pref(
This code seems repetitive with is_video. Can you combine? At the very least, it
might help to do something like:

else if (is_video) {
  RecordDataUseForVideo(
      with_data_reduction_proxy_enabled, request_type,
      original_content_length, received_content_length);
} else if (is_mime_type_empty) {
  RecordDataUseForUnknownType(
    with_data_reduction_proxy_enabled, request_type,
    original_content_length, received_content_length)
}

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

https://codereview.chromium.org/1286643002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h:116:
// Increments pref value in the |DataReductionProxyPrefMap| map.
pref -> the pref

https://codereview.chromium.org/1286643002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h:118:
void IncrementInt64Pref(const char* pref_path, int64 pref_increment);
Would it be possible to move to the new hotness and use int64_t from stdint.h
instead of int64?

https://codereview.chromium.org/1286643002/diff/60001/components/data_reducti...
File
components/data_reduction_proxy/core/common/data_reduction_proxy_pref_names.h
(right):

https://codereview.chromium.org/1286643002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_pref_names.h:19:
extern const char kDailyContentLengthViaDataReductionProxyApplication[];
Egad.

[Not at Google. Contact bengr, 2015-08-18 22:39:55]

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

https://codereview.chromium.org/1286643002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:711:
RECORD_INT64PREF_TO_HISTOGRAM(
On 2015/08/18 19:44:57, bengr wrote:
> Instead of all of these macro calls, could you construct an array of const
chase
> and loop through it?

Cannot do this unfortunately. First argument to RECORD_INT64PREF_TO_HISTOGRAM
needs to be a inline string and not a variable.

https://codereview.chromium.org/1286643002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:955:
IncrementInt64Pref(
On 2015/08/18 19:44:57, bengr wrote:
> This code seems repetitive with is_video. Can you combine? At the very least,
it
> might help to do something like:
> 
> else if (is_video) {
>   RecordDataUseForVideo(
>       with_data_reduction_proxy_enabled, request_type,
>       original_content_length, received_content_length);
> } else if (is_mime_type_empty) {
>   RecordDataUseForUnknownType(
>     with_data_reduction_proxy_enabled, request_type,
>     original_content_length, received_content_length)
> }

Re-factored a common method which can then be called for each mime-type. PTAL.

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

https://codereview.chromium.org/1286643002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h:116:
// Increments pref value in the |DataReductionProxyPrefMap| map.
On 2015/08/18 19:44:57, bengr wrote:
> pref -> the pref

Done.

https://codereview.chromium.org/1286643002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.h:118:
void IncrementInt64Pref(const char* pref_path, int64 pref_increment);
On 2015/08/18 19:44:57, bengr wrote:
> Would it be possible to move to the new hotness and use int64_t from stdint.h
> instead of int64?

Done.

https://codereview.chromium.org/1286643002/diff/60001/components/data_reducti...
File
components/data_reduction_proxy/core/common/data_reduction_proxy_pref_names.h
(right):

https://codereview.chromium.org/1286643002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_pref_names.h:19:
extern const char kDailyContentLengthViaDataReductionProxyApplication[];
On 2015/08/18 19:44:57, bengr wrote:
> Egad.

Acknowledged.

[Not at Google. Contact bengr, 2015-08-19 19:11:25]

The CQ bit was checked by kundaji@chromium.org

[Not at Google. Contact bengr, 2015-08-19 19:11:25]

The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org
Link to the patchset: https://codereview.chromium.org/1286643002/#ps120001
(title: "sync to head")

[commit-bot: I haz the power, 2015-08-19 19:11:48]

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

[commit-bot: I haz the power, 2015-08-19 19:18:03]

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

[commit-bot: I haz the power, 2015-08-19 19:18:04]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Not at Google. Contact bengr, 2015-08-20 00:31:42]

The CQ bit was checked by kundaji@chromium.org

[Not at Google. Contact bengr, 2015-08-20 00:31:42]

The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org
Link to the patchset: https://codereview.chromium.org/1286643002/#ps160001
(title: "Comments")

[commit-bot: I haz the power, 2015-08-20 00:32:28]

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

[commit-bot: I haz the power, 2015-08-20 02:33:43]

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

[commit-bot: I haz the power, 2015-08-20 02:33:44]

Internal error: failed to checkout. Please try again.

[commit-bot: I haz the power, 2015-08-20 02:34:02]

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

[Not at Google. Contact bengr, 2015-08-20 16:15:31]

The CQ bit was checked by kundaji@chromium.org

[commit-bot: I haz the power, 2015-08-20 16:16:13]

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

[commit-bot: I haz the power, 2015-08-20 16:21:14]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-08-20 16:21:59]

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

[lfg, 2015-08-20 20:48:04]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/1303813005/ by lfg@chromium.org.

The reason for reverting is: After this patch, we try to look for an entry
"data_reduction.daily_original_length_application" in
data_reduction_proxy::DataReductionProxyCompressionStats::pref_map_ that doesn't
exist and crash on
data_reduction_proxy::DataReductionProxyCompressionStats::GetInt64. Crash only
shows up in debug.

Here's a backtrace:

#0  0x00007ffff0010cc9 in __GI_raise (sig=sig@entry=6) at
../nptl/sysdeps/unix/sysv/linux/raise.c:56
#1  0x00007ffff00140d8 in __GI_abort () at abort.c:89
#2  0x00007ffff0664fc5 in __gnu_debug::_Error_formatter::_M_error
(this=0x7fffffffb1d8) at ../../../../../src/libstdc++-v3/src/c++11/debug.cc:781
#3  0x000055555e4a5b3a in
__gnu_debug::_Safe_iterator<std::_Rb_tree_iterator<std::pair<char const* const,
long> >, std::__debug::map<char const*, long, std::less<char const*>,
std::allocator<std::pair<char const* const, long> > > >::operat
or-> (this=0x7fffffffb448) at
/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/debug/safe_iterator.h:277
#4  0x000055555e4a0003 in
data_reduction_proxy::DataReductionProxyCompressionStats::GetInt64
(this=0x2f5b919fc560, 
    pref_path=0x555562293980
<data_reduction_proxy::prefs::kDailyHttpOriginalContentLengthApplication>
"data_reduction.daily_original_length_application")
    at
../../components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:475
#5  0x000055555e4a233b in
data_reduction_proxy::DataReductionProxyCompressionStats::IncrementInt64Pref
(this=0x2f5b919fc560, 
    pref_path=0x555562293980
<data_reduction_proxy::prefs::kDailyHttpOriginalContentLengthApplication>
"data_reduction.daily_original_length_application", pref_increment=54546)
    at
../../components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:492
#6  0x000055555e4a481a in
data_reduction_proxy::DataReductionProxyCompressionStats::IncrementDailyUmaPrefs
(this=0x2f5b919fc560, original_size=54546, received_size=54546, 
    original_size_pref=0x555562293980
<data_reduction_proxy::prefs::kDailyHttpOriginalContentLengthApplication>
"data_reduction.daily_original_length_application", 
    received_size_pref=0x555562293a50
<data_reduction_proxy::prefs::kDailyHttpReceivedContentLengthApplication>
"data_reduction.daily_received_length_application",
data_reduction_proxy_enabled=false, 
    original_size_with_proxy_enabled_pref=0x555562293c70
<data_reduction_proxy::prefs::kDailyOriginalContentLengthWithDataReductionProxyEnabledApplication>
"data_reduction.daily_original_length_with_data_reduction_proxy_enabled_application
", recevied_size_with_proxy_enabled_pref=0x555562293830
<data_reduction_proxy::prefs::kDailyContentLengthWithDataReductionProxyEnabledApplication>
"data_reduction.daily_received_length_with_data_reduction_proxy_enabled_application",

    via_data_reduction_proxy=false, 
    original_size_via_proxy_pref=0x555562293b30
<data_reduction_proxy::prefs::kDailyOriginalContentLengthViaDataReductionProxyApplication>
"data_reduction.daily_original_length_via_data_reduction_proxy_application", 
    received_size_via_proxy_pref=0x5555622936f0
<data_reduction_proxy::prefs::kDailyContentLengthViaDataReductionProxyApplication>
"data_reduction.daily_received_length_via_data_reduction_proxy_application")
    at
../../components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:1029
#7  0x000055555e4a2041 in
data_reduction_proxy::DataReductionProxyCompressionStats::RecordRequestSizePrefs
(this=0x2f5b919fc560, data_used=54546, original_size=54546,
with_data_reduction_proxy_enabled=false, 
Python Exception <class 'gdb.error'> No type "Time" within class or namespace
"base".: 
    request_type=data_reduction_proxy::HTTPS, mime_type="application/json",
now=...) at
../../components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:971
#8  0x000055555e49fe70 in
data_reduction_proxy::DataReductionProxyCompressionStats::UpdateContentLengths
(this=0x2f5b919fc560, data_used=54546, original_size=54546,
data_reduction_proxy_enabled=false, 
    request_type=data_reduction_proxy::HTTPS, data_usage_host="www.google.ca",
mime_type="application/json") at
../../components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:455
#9  0x000055555e4c56a0 in
data_reduction_proxy::DataReductionProxyService::UpdateContentLengths
(this=0x2f5b9147d160, data_used=54546, original_size=54546,
data_reduction_proxy_enabled=false, request_type=data_reduction_proxy::HTTPS, 
    data_usage_host="www.google.ca", mime_type="application/json") at
../../components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:109
#10 0x000055555e4ba565 in base::internal::RunnableAdapter<void
(data_reduction_proxy::DataReductionProxyService::*)(long, long, bool,
data_reduction_proxy::DataReductionProxyRequestType, std::string const&,
std::string const&)>::Run (
    this=0x7fffffffc240, object=0x2f5b9147d160, args=..., args=..., args=...,
args=..., args=..., args=...) at ../../base/bind_internal.h:176
.

[Not at Google. Contact bengr, 2015-08-20 23:23:33]

The CQ bit was checked by kundaji@chromium.org

[Not at Google. Contact bengr, 2015-08-20 23:23:33]

The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org
Link to the patchset: https://codereview.chromium.org/1286643002/#ps180001
(title: "Init prefs in compression stats map")

[commit-bot: I haz the power, 2015-08-20 23:24:18]

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

[commit-bot: I haz the power, 2015-08-21 01:05:42]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-08-21 01:06:27]

Patchset 10 (id:??) landed as
https://crrev.com/3538199987e27b6bf98e8e15421d5f8f8722ab3d
Cr-Commit-Position: refs/heads/master@{#344634}