Add DataReductionProxyExperimentsStats and UMA for measuring potentially non-compressed bytes.

Add DataReductionProxyExperimentsStats and UMA for measuring potentially non-compressed bytes.

If no valid Data Reduction Proxy configuration is available to the client,
then the client does not get the benefit of the Data Reduction Proxy during
the period when a configuration is being retrieved. This CL sets up a
configurable simulation where all requests will continue to go through the
Data Reduction Proxy, but if the request is initiated during a period when
the client would request a configuration from the service, we will record
the reduction statistics to determine the overall effect on data compression.

BUG=484864
Committed: https://crrev.com/0323138fb70b031af8b60d7bc07152c3e2ddb7b8
Cr-Commit-Position: refs/heads/master@{#330133}

[jeremyim, 2015-05-05 22:00:59]

jeremyim@chromium.org changed reviewers:
+ sclittle@chromium.org

[sclittle, 2015-05-06 01:59:22]

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

https://codereview.chromium.org/1127893002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_experiments_stats.cc:60:
std::string config_always_stale_value = variations::GetVariationParamValue(
Instead of having multiple (e.g. 10) experimental groups targeting a small
percentage (e.g. 10%) each of clients with 1 version of each parameter, you
could instead have 1 umbrella experimental group targeting 100% of clients with
10 versions of each parameter. You'd collect 10 times as much data.

e.g., you could have only 1 parameter, but have it be a simple serialized
protobuf (maybe base64 encoded) of the form:

message ConfigRetrievalExperimentParams {
  message Param {
    required bool config_always_stale = 1;
    required int64 config_roundtrip_milliseconds = 2;
    required int64 config_expiration_seconds = 3;
  }
  repeated Param param = 1;
}

Then you could report multiple histograms suffixed with 1, 2, 3, etc. according
to which index of |param|, up to 10 or whatever.

Maybe you could have ConfigRetrievalParams hold a |Param| tuple and a histogram
name and maybe a timer, then have DRPExperimentsStats hold on to a vector of
ConfigRetrievalParams or something.

https://codereview.chromium.org/1127893002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_experiments_stats.cc:195:
"DataReductionProxy.ConfigFetchLostBytesCL_WithValidOCL",
Is it important whether or not there was a valid OCL? Do we need to report these
WithValidOCL histograms?

https://codereview.chromium.org/1127893002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_experiments_stats.cc:210:
UMA_HISTOGRAM_COUNTS("DataReductionProxy.ConfigFetchLostBytesCL",
As we discussed offline, it might be useful to (probably in a separate CL)
measure how many bytes are saved while waiting for the secure proxy check to
finish, and also measure how long the secure proxy check takes.

The real delay to fetch a config would probably be somewhere between (secure
proxy check delay) and 2 * (secure proxy check delay), so measuring bytes saved
during both periods might be a more accurate representation of the typical
losses while waiting for a config.

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

https://codereview.chromium.org/1127893002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_experiments_stats.h:35:
extern const char kConfigRoundtripMilliseconds[];
nit: Rename to make it clearer that it's a param name, not a number of
milliseconds. Same below.

https://codereview.chromium.org/1127893002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_experiments_stats.h:56:
// executed.
Could you add to this comment (or a different comment somewhere) describing what
these different experiments are and what they do?

https://codereview.chromium.org/1127893002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_experiments_stats.h:136:
// Used to persist experiment data.
Which experiment data? Could you be more specific?

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

https://codereview.chromium.org/1127893002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_experiments_stats_unittest.cc:25:
const int kConfigExpirationSeconds = 60 * 60 * 24;
nit: change to int64

https://codereview.chromium.org/1127893002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_experiments_stats_unittest.cc:261:
histogram_tester.ExpectTotalCount(
This is a lot of lines of histogram_tester. Maybe this could be more concise by
storing the expected values in variables and computing them in these if
statements, then having all the histogram_tester lines once at the end?

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

https://codereview.chromium.org/1127893002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:150:
&DataReductionProxyIOData::SetInt64Pref, base::Unretained(this))));
If you wanted, I think you might be able to get rid of the IOData::SetInt64Pref
function altogether, and just register the callback as something like:

base::Bind(&base::TaskRunner::PostTask, ui_task_runner_, FROM_HERE,
           base::Bind(&DataReductionProxyService::SetInt64Pref, service_));

You also don't need base::Unretained this way.

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

https://codereview.chromium.org/1127893002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:94:
void SetInt64Pref(const std::string& pref_path, int64 value);
Does this method need to be public?

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

https://codereview.chromium.org/1127893002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h:94:
// Stores an int64 value in |prefs_service_|.
you mean |prefs_|

[jeremyim, 2015-05-08 22:25:31]

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

https://codereview.chromium.org/1127893002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_experiments_stats.cc:60:
std::string config_always_stale_value = variations::GetVariationParamValue(
On 2015/05/06 01:59:21, sclittle wrote:
> Instead of having multiple (e.g. 10) experimental groups targeting a small
> percentage (e.g. 10%) each of clients with 1 version of each parameter, you
> could instead have 1 umbrella experimental group targeting 100% of clients
with
> 10 versions of each parameter. You'd collect 10 times as much data.
> 
> e.g., you could have only 1 parameter, but have it be a simple serialized
> protobuf (maybe base64 encoded) of the form:
> 
> message ConfigRetrievalExperimentParams {
>   message Param {
>     required bool config_always_stale = 1;
>     required int64 config_roundtrip_milliseconds = 2;
>     required int64 config_expiration_seconds = 3;
>   }
>   repeated Param param = 1;
> }
> 
> Then you could report multiple histograms suffixed with 1, 2, 3, etc.
according
> to which index of |param|, up to 10 or whatever.
> 
> Maybe you could have ConfigRetrievalParams hold a |Param| tuple and a
histogram
> name and maybe a timer, then have DRPExperimentsStats hold on to a vector of
> ConfigRetrievalParams or something.

Done.

https://codereview.chromium.org/1127893002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_experiments_stats.cc:195:
"DataReductionProxy.ConfigFetchLostBytesCL_WithValidOCL",
On 2015/05/06 01:59:21, sclittle wrote:
> Is it important whether or not there was a valid OCL? Do we need to report
these
> WithValidOCL histograms?

If we don't get a valid OCL, we can't get a valid diff (since without a valid
OCL we get a diff of 0). OTOH, we also want to know what the frequency of
missing OCL's is, to understand the total picture.

https://codereview.chromium.org/1127893002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_experiments_stats.cc:210:
UMA_HISTOGRAM_COUNTS("DataReductionProxy.ConfigFetchLostBytesCL",
On 2015/05/06 01:59:21, sclittle wrote:
> As we discussed offline, it might be useful to (probably in a separate CL)
> measure how many bytes are saved while waiting for the secure proxy check to
> finish, and also measure how long the secure proxy check takes.
> 
> The real delay to fetch a config would probably be somewhere between (secure
> proxy check delay) and 2 * (secure proxy check delay), so measuring bytes
saved
> during both periods might be a more accurate representation of the typical
> losses while waiting for a config.

Acknowledged.

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

https://codereview.chromium.org/1127893002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_experiments_stats.h:35:
extern const char kConfigRoundtripMilliseconds[];
On 2015/05/06 01:59:21, sclittle wrote:
> nit: Rename to make it clearer that it's a param name, not a number of
> milliseconds. Same below.

Done.

https://codereview.chromium.org/1127893002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_experiments_stats.h:56:
// executed.
On 2015/05/06 01:59:21, sclittle wrote:
> Could you add to this comment (or a different comment somewhere) describing
what
> these different experiments are and what they do?

Done.

https://codereview.chromium.org/1127893002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_experiments_stats.h:136:
// Used to persist experiment data.
On 2015/05/06 01:59:21, sclittle wrote:
> Which experiment data? Could you be more specific?

Done.

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

https://codereview.chromium.org/1127893002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_experiments_stats_unittest.cc:25:
const int kConfigExpirationSeconds = 60 * 60 * 24;
On 2015/05/06 01:59:22, sclittle wrote:
> nit: change to int64

Done.

https://codereview.chromium.org/1127893002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_experiments_stats_unittest.cc:261:
histogram_tester.ExpectTotalCount(
On 2015/05/06 01:59:21, sclittle wrote:
> This is a lot of lines of histogram_tester. Maybe this could be more concise
by
> storing the expected values in variables and computing them in these if
> statements, then having all the histogram_tester lines once at the end?

Done.

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

https://codereview.chromium.org/1127893002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:150:
&DataReductionProxyIOData::SetInt64Pref, base::Unretained(this))));
On 2015/05/06 01:59:22, sclittle wrote:
> If you wanted, I think you might be able to get rid of the
IOData::SetInt64Pref
> function altogether, and just register the callback as something like:
> 
> base::Bind(&base::TaskRunner::PostTask, ui_task_runner_, FROM_HERE,
>            base::Bind(&DataReductionProxyService::SetInt64Pref, service_));
> 
> You also don't need base::Unretained this way.

I don't think the signature coercion can occur since PostTask takes no
parameters, so there's no way to pass in additional parameters to the internal
callback.

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

https://codereview.chromium.org/1127893002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:94:
void SetInt64Pref(const std::string& pref_path, int64 value);
On 2015/05/06 01:59:22, sclittle wrote:
> Does this method need to be public?

Done.

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

https://codereview.chromium.org/1127893002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h:94:
// Stores an int64 value in |prefs_service_|.
On 2015/05/06 01:59:22, sclittle wrote:
> you mean |prefs_|

Done.

[sclittle, 2015-05-12 19:32:25]

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

https://codereview.chromium.org/1127893002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_experiments_stats.cc:112:
if (now > config_expiration) {
Could some of this be pulled out into helper functions? This method seems a bit
long and dense.

https://codereview.chromium.org/1127893002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_experiments_stats.cc:127:
if (roundtrip_multiplier_value.empty() ||
|roundtrip_multiplier_value| doesn't appear to be set to anything, so won't it
always be empty?

https://codereview.chromium.org/1127893002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_experiments_stats.cc:133:
std::string roundtrip_milliseconds_increment_value;
Same here, this variable doesn't get assigned anywhere that I can see.

https://codereview.chromium.org/1127893002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_experiments_stats.cc:150:
} while (++params_index < kConfigFetchGroups);
Could this be done as a for loop instead for clarity?

for (int params_index = 0; params_index < kConfigFetchGroups; ++params_index)

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

https://codereview.chromium.org/1127893002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_experiments_stats.h:59:
// considered
nit: line wrapping

https://codereview.chromium.org/1127893002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_experiments_stats.h:89:
class Variation {
Could you add a comment describing what this class is for?

https://codereview.chromium.org/1127893002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_experiments_stats.h:91:
Variation(int index, const base::Time& simulated_config_retrieved);
optional nit: This is three levels deep of classes - have you considered moving
ConfigRetrievalParams to its own file or something like that?

I'm not sure what it would be called -
DataReductionProxySimulatedConfigRetrievalParams or something doesn't exactly
roll off the tongue - but it might help clear up this file and the .cc, as well
as the tests (instead of needing to friend
DataReductionProxyExperimentsStatsTest from ConfigRetrievalParams).

https://codereview.chromium.org/1127893002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_experiments_stats.h:154:
DataReductionProxyExperimentsStats(Int64ValueSetter value_setter);
nit: const Int64ValueSetter&

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

https://codereview.chromium.org/1127893002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:66250: +  <suffix name="0" label="Bucket
1 of 10 to gather multiple data points."/>
It's confusing that "Bucket 4 of 10 ..." has the suffix 3.

Be consistent with your numbering scheme - either stick to starting at 0 or 1.

[jeremyim, 2015-05-12 20:38:29]

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

https://codereview.chromium.org/1127893002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_experiments_stats.cc:112:
if (now > config_expiration) {
On 2015/05/12 19:32:25, sclittle wrote:
> Could some of this be pulled out into helper functions? This method seems a
bit
> long and dense.

Done.

https://codereview.chromium.org/1127893002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_experiments_stats.cc:127:
if (roundtrip_multiplier_value.empty() ||
On 2015/05/12 19:32:25, sclittle wrote:
> |roundtrip_multiplier_value| doesn't appear to be set to anything, so won't it
> always be empty?

Done.

https://codereview.chromium.org/1127893002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_experiments_stats.cc:133:
std::string roundtrip_milliseconds_increment_value;
On 2015/05/12 19:32:25, sclittle wrote:
> Same here, this variable doesn't get assigned anywhere that I can see.

Done.

https://codereview.chromium.org/1127893002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_experiments_stats.cc:150:
} while (++params_index < kConfigFetchGroups);
On 2015/05/12 19:32:25, sclittle wrote:
> Could this be done as a for loop instead for clarity?
> 
> for (int params_index = 0; params_index < kConfigFetchGroups; ++params_index)

Done.

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

https://codereview.chromium.org/1127893002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_experiments_stats.h:59:
// considered
On 2015/05/12 19:32:25, sclittle wrote:
> nit: line wrapping

Done.

https://codereview.chromium.org/1127893002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_experiments_stats.h:89:
class Variation {
On 2015/05/12 19:32:25, sclittle wrote:
> Could you add a comment describing what this class is for?

Done.

https://codereview.chromium.org/1127893002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_experiments_stats.h:91:
Variation(int index, const base::Time& simulated_config_retrieved);
On 2015/05/12 19:32:25, sclittle wrote:
> optional nit: This is three levels deep of classes - have you considered
moving
> ConfigRetrievalParams to its own file or something like that?
> 
> I'm not sure what it would be called -
> DataReductionProxySimulatedConfigRetrievalParams or something doesn't exactly
> roll off the tongue - but it might help clear up this file and the .cc, as
well
> as the tests (instead of needing to friend
> DataReductionProxyExperimentsStatsTest from ConfigRetrievalParams).

Done.

https://codereview.chromium.org/1127893002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_experiments_stats.h:154:
DataReductionProxyExperimentsStats(Int64ValueSetter value_setter);
On 2015/05/12 19:32:25, sclittle wrote:
> nit: const Int64ValueSetter&

Done.

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

https://codereview.chromium.org/1127893002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:66250: +  <suffix name="0" label="Bucket
1 of 10 to gather multiple data points."/>
On 2015/05/12 19:32:25, sclittle wrote:
> It's confusing that "Bucket 4 of 10 ..." has the suffix 3.
> 
> Be consistent with your numbering scheme - either stick to starting at 0 or 1.

Done.

[sclittle, 2015-05-14 00:23:39]

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

https://codereview.chromium.org/1127893002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_retrieval_params.cc:52:
!base::StringToInt64(variation_value, &variation_numeric))
nit: add curlies for these if statements that span multiple lines.

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

https://codereview.chromium.org/1127893002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_retrieval_params_unittest.cc:34:
namespace data_reduction_proxy {
nit: add blank line between namespaces

https://codereview.chromium.org/1127893002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_retrieval_params_unittest.cc:208:
} else
nit: add curlies since to visually balance out the "} else" line.

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

https://codereview.chromium.org/1127893002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_experiments_stats.cc:34:
if (config_retrieval_params_) {
It looks like InitializeOnUIThread must be called before InitailizeOnIOThread,
otherwise the timer will never be started.

Is this ordering always guaranteed? Could you add DCHECKs or something (maybe an
|is_initialized_on_ui_thread_| bool) to verify that a future ordering change
doesn't silently break this?

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

https://codereview.chromium.org/1127893002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h:46:
// The caller must ensure that |settings|, |prefs|, |request_context|, and
Can |prefs| be null?

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

https://codereview.chromium.org/1127893002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_pref_names.cc:95:
const char kSimulatedConfigExpireTime[] =
Could you save the most recent expiration time of the config, instead of the
future expiration time?

If you save the future expiration time like you are right now, then that means
if I accidentally set the experiment param in microseconds instead of seconds or
something, clients could be in a bad state until that future expiration time
arrives.

[jeremyim, 2015-05-14 03:32:20]

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

https://codereview.chromium.org/1127893002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_retrieval_params.cc:52:
!base::StringToInt64(variation_value, &variation_numeric))
On 2015/05/14 00:23:39, sclittle wrote:
> nit: add curlies for these if statements that span multiple lines.

Done.

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

https://codereview.chromium.org/1127893002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_retrieval_params_unittest.cc:34:
namespace data_reduction_proxy {
On 2015/05/14 00:23:39, sclittle wrote:
> nit: add blank line between namespaces

Done.

https://codereview.chromium.org/1127893002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_retrieval_params_unittest.cc:208:
} else
On 2015/05/14 00:23:39, sclittle wrote:
> nit: add curlies since to visually balance out the "} else" line.

Done.

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

https://codereview.chromium.org/1127893002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_experiments_stats.cc:34:
if (config_retrieval_params_) {
On 2015/05/14 00:23:39, sclittle wrote:
> It looks like InitializeOnUIThread must be called before InitailizeOnIOThread,
> otherwise the timer will never be started.
> 
> Is this ordering always guaranteed? Could you add DCHECKs or something (maybe
an
> |is_initialized_on_ui_thread_| bool) to verify that a future ordering change
> doesn't silently break this?

Done.

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

https://codereview.chromium.org/1127893002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h:46:
// The caller must ensure that |settings|, |prefs|, |request_context|, and
On 2015/05/14 00:23:39, sclittle wrote:
> Can |prefs| be null?

Done.

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

https://codereview.chromium.org/1127893002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_pref_names.cc:95:
const char kSimulatedConfigExpireTime[] =
On 2015/05/14 00:23:39, sclittle wrote:
> Could you save the most recent expiration time of the config, instead of the
> future expiration time?
> 
> If you save the future expiration time like you are right now, then that means
> if I accidentally set the experiment param in microseconds instead of seconds
or
> something, clients could be in a bad state until that future expiration time
> arrives.

Done.

[jeremyim, 2015-05-14 15:31:23]

jeremyim@chromium.org changed reviewers:
+ asvitkine@chromium.org, sgurun@chromium.org

[jeremyim, 2015-05-14 15:31:25]

PTAL. Thanks!

asvitkine => histograms.xml
sgurun => aw_browser_context.cc

[Alexei Svitkine (slow), 2015-05-14 17:00:18]

https://codereview.chromium.org/1127893002/diff/100001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_retrieval_params.cc
(right):

https://codereview.chromium.org/1127893002/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_retrieval_params.cc:38:
base::HistogramBase* GetHistogramWithSuffix(const char* histogram, int suffix) {
Nit: Add some comments for these free-standing functions.

https://codereview.chromium.org/1127893002/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_retrieval_params.cc:69:
variation_numeric < min_value)
Nit: {}'s

https://codereview.chromium.org/1127893002/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_retrieval_params.cc:85:
variation_numeric < min_value)
Nit: {}'s

https://codereview.chromium.org/1127893002/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_retrieval_params.cc:159:
experiments::kConfigFetchTrialGroup,
Why not have this code be in experiments namespace so you don't need to prefix
it?

[jeremyim, 2015-05-14 17:28:02]

https://codereview.chromium.org/1127893002/diff/100001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_retrieval_params.cc
(right):

https://codereview.chromium.org/1127893002/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_retrieval_params.cc:38:
base::HistogramBase* GetHistogramWithSuffix(const char* histogram, int suffix) {
On 2015/05/14 17:00:17, Alexei Svitkine wrote:
> Nit: Add some comments for these free-standing functions.

Done.

https://codereview.chromium.org/1127893002/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_retrieval_params.cc:69:
variation_numeric < min_value)
On 2015/05/14 17:00:17, Alexei Svitkine wrote:
> Nit: {}'s

Done.

https://codereview.chromium.org/1127893002/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_retrieval_params.cc:85:
variation_numeric < min_value)
On 2015/05/14 17:00:17, Alexei Svitkine wrote:
> Nit: {}'s

Done.

https://codereview.chromium.org/1127893002/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_retrieval_params.cc:159:
experiments::kConfigFetchTrialGroup,
On 2015/05/14 17:00:17, Alexei Svitkine wrote:
> Why not have this code be in experiments namespace so you don't need to prefix
> it?

Removed the namespace; having the namespace means that there is going to be some
location at which the namespace must be explicitly prefixed, and there aren't
any obvious places for it.

[Alexei Svitkine (slow), 2015-05-14 18:15:13]

https://codereview.chromium.org/1127893002/diff/100002/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1127893002/diff/100002/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:4255: +    configuration fetch is taking
place.
Histograms can't log negative values (they'll be logged as 0).

If you're ok only tracking positive differences, mention it in the comment.
Otherwise, you can have a different histogram for negative deltas, if it is
interesting.

[jeremyim, 2015-05-14 18:28:43]

Patchset #8 (id:130001) has been deleted

[jeremyim, 2015-05-14 18:32:27]

https://codereview.chromium.org/1127893002/diff/100002/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1127893002/diff/100002/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:4255: +    configuration fetch is taking
place.
On 2015/05/14 18:15:13, Alexei Svitkine wrote:
> Histograms can't log negative values (they'll be logged as 0).
> 
> If you're ok only tracking positive differences, mention it in the comment.
> Otherwise, you can have a different histogram for negative deltas, if it is
> interesting.

Done.

[sclittle, 2015-05-14 18:33:47]

LGTM

https://codereview.chromium.org/1127893002/diff/150001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1127893002/diff/150001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:4254: +    place. Only positive values
are logged, so if  X-Original-Content-Length is
nit: remove extra space between "if" and "X-Original..."

[Alexei Svitkine (slow), 2015-05-14 18:34:50]

lgtm

https://codereview.chromium.org/1127893002/diff/150001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1127893002/diff/150001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:4255: +    not specified or exceeds the
content length, it is not logged.
With your current code, it also won't be logged if it's equal. Document that (or
make 0 logged - up to you).

[jeremyim, 2015-05-14 20:14:01]

jeremyim@chromium.org changed reviewers:
+ mmenke@chromium.org

[jeremyim, 2015-05-14 20:14:02]

mmenke: Please review cronet/*

Thanks!

[sgurun-gerrit only, 2015-05-14 23:18:21]

On 2015/05/14 20:14:02, jeremyim wrote:
> mmenke: Please review cronet/*
> 
> Thanks!

are the bot failures related? can you make sure things are green :)

[jeremyim, 2015-05-15 02:31:47]

On 2015/05/14 23:18:21, sgurun wrote:
> On 2015/05/14 20:14:02, jeremyim wrote:
> > mmenke: Please review cronet/*
> > 
> > Thanks!
> 
> are the bot failures related? can you make sure things are green :)

Bot failures fixed.

[mmenke, 2015-05-15 02:33:34]

cronet/ LGTM

[sgurun-gerrit only, 2015-05-15 03:28:05]

On 2015/05/15 02:33:34, mmenke wrote:
> cronet/ LGTM

win failing...

[jeremyim, 2015-05-15 04:25:16]

On 2015/05/15 03:28:05, sgurun wrote:
> On 2015/05/15 02:33:34, mmenke wrote:
> > cronet/ LGTM
> 
> win failing...

Looks like a flaky test; rerunning it succeeded.

[sgurun-gerrit only, 2015-05-15 16:52:04]

On 2015/05/15 04:25:16, jeremyim wrote:
> On 2015/05/15 03:28:05, sgurun wrote:
> > On 2015/05/15 02:33:34, mmenke wrote:
> > > cronet/ LGTM
> > 
> > win failing...
> 
> Looks like a flaky test; rerunning it succeeded.

lgtm

[jeremyim, 2015-05-15 18:05:00]

The CQ bit was checked by jeremyim@chromium.org

[jeremyim, 2015-05-15 18:05:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, sclittle@chromium.org
Link to the patchset: https://codereview.chromium.org/1127893002/#ps200001
(title: "Fix test failures")

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

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

[commit-bot: I haz the power, 2015-05-15 18:13:38]

Committed patchset #11 (id:200001)

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

Patchset 11 (id:??) landed as
https://crrev.com/0323138fb70b031af8b60d7bc07152c3e2ddb7b8
Cr-Commit-Position: refs/heads/master@{#330133}