Add histograms for requests that were fetched successfully

Add histograms for requests that were fetched successfully

Add histograms for requests that were fetched successfully
through data reduction proxy. Also, replace is_fallback by
proxy_index in DataReductionProxyTypeInfo.

BUG=615495
Committed: https://crrev.com/9a89e4db53afe3a849700af9ac981de529d4258b
Cr-Commit-Position: refs/heads/master@{#396598}

[tbansal1, 2016-05-27 19:45:11]

Patchset #1 (id:1) has been deleted

[tbansal1, 2016-05-27 19:46:21]

Description was changed from

==========
DRP Add histograms for requests that were successfully fetched

BUG=
==========

to

==========
Add histograms for requests that were fetched successfully

Add histograms for requests that were fetched successfully 
through data reduction proxy. Also, replace is_fallback by
proxy_index in DataReductionProxyTypeInfo.


BUG=
==========

[tbansal1, 2016-05-27 19:52:13]

Description was changed from

==========
Add histograms for requests that were fetched successfully

Add histograms for requests that were fetched successfully 
through data reduction proxy. Also, replace is_fallback by
proxy_index in DataReductionProxyTypeInfo.


BUG=
==========

to

==========
Add histograms for requests that were fetched successfully

Add histograms for requests that were fetched successfully 
through data reduction proxy. Also, replace is_fallback by
proxy_index in DataReductionProxyTypeInfo.

BUG=615495
==========

[tbansal1, 2016-05-27 19:52:22]

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

[tbansal1, 2016-05-27 19:52:36]

sclittle: PTAL. Thanks.

[sclittle, 2016-05-27 20:10:34]

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

https://codereview.chromium.org/2019783002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats_unittest.cc:475:
for (size_t i = 0; i < arraysize(test_cases); ++i) {
nit: it's slightly shorter to write "for (const auto& test_case : test_cases)
{", and that way you'd be able to just write test_case instead of test_cases[i]
below.

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

https://codereview.chromium.org/2019783002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:350:
for (const auto proxy_it : proxy_list) {
You don't need to do this for loop or anything - you could just keep the
original code that used find_if, and replace the proxy_info->is_fallback setting
with:

proxy_info->proxy_index = static_cast<size_t>(proxy_it - proxy_list.begin());

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

https://codereview.chromium.org/2019783002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:692:
net::ProxyServer expected_first;
optional nit: instead of having expected_first to expected_third, you could just
have a std::vector<net::ProxyServer> expected_proxy_list, and use different
slices of the proxies_for_http vector for the results.

e.g., the first two cases in |tests| could be changed to:
{ proxies_for_http[0].host_port_pair(), true, proxies_for_http, 0 },
{ proxies_for_http[1].host_port_pair(), true,
std::vector<net::ProxyServer>(proxies_for_http.begin() + 1,
proxies_for_http.end()), 1 },

[tbansal1, 2016-05-27 21:11:02]

Patchset #2 (id:40001) has been deleted

[tbansal1, 2016-05-27 21:13:50]

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

[tbansal1, 2016-05-27 21:13:51]

sclittle: PTAL.
asvitkine: PTAL at histograms.xml.

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

https://codereview.chromium.org/2019783002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats_unittest.cc:475:
for (size_t i = 0; i < arraysize(test_cases); ++i) {
On 2016/05/27 20:10:34, sclittle wrote:
> nit: it's slightly shorter to write "for (const auto& test_case : test_cases)
> {", and that way you'd be able to just write test_case instead of
test_cases[i]
> below.

Done.

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

https://codereview.chromium.org/2019783002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:350:
for (const auto proxy_it : proxy_list) {
On 2016/05/27 20:10:34, sclittle wrote:
> You don't need to do this for loop or anything - you could just keep the
> original code that used find_if, and replace the proxy_info->is_fallback
setting
> with:
> 
> proxy_info->proxy_index = static_cast<size_t>(proxy_it - proxy_list.begin());

Done.

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

https://codereview.chromium.org/2019783002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:692:
net::ProxyServer expected_first;
On 2016/05/27 20:10:34, sclittle wrote:
> optional nit: instead of having expected_first to expected_third, you could
just
> have a std::vector<net::ProxyServer> expected_proxy_list, and use different
> slices of the proxies_for_http vector for the results.
> 
> e.g., the first two cases in |tests| could be changed to:
> { proxies_for_http[0].host_port_pair(), true, proxies_for_http, 0 },
> { proxies_for_http[1].host_port_pair(), true,
> std::vector<net::ProxyServer>(proxies_for_http.begin() + 1,
> proxies_for_http.end()), 1 },

Done.

[Alexei Svitkine (slow), 2016-05-27 21:16:37]

lgtm

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

https://codereview.chromium.org/2019783002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:7629: +<histogram
name="DataReductionProxy.SuccessfulRequestCompletionCounts">
Nit: add units=

Same below.

[sclittle, 2016-05-27 21:17:28]

lgtm % nit

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

https://codereview.chromium.org/2019783002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:360:
proxy_info->proxy_index = proxy_it - proxy_list.begin();
nit: add a static_cast<size_t> here. (proxy_it - proxy_list.begin()) is a
ptrdiff_t, which is signed, not unsigned like a size_t. The only reason this
doesn't cause a compiler warning is because those warning were turned off as so
much code in chromium does these implicit signed/unsigned casts.

[tbansal1, 2016-05-27 22:31:07]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2016-05-27 22:31:07]

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/2019783002/#ps80001
(title: "Addressed sclittle, asvitkine comments")

[commit-bot: I haz the power, 2016-05-27 22:31:53]

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

[tbansal1, 2016-05-27 22:35:11]

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

https://codereview.chromium.org/2019783002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:360:
proxy_info->proxy_index = proxy_it - proxy_list.begin();
On 2016/05/27 21:17:28, sclittle wrote:
> nit: add a static_cast<size_t> here. (proxy_it - proxy_list.begin()) is a
> ptrdiff_t, which is signed, not unsigned like a size_t. The only reason this
> doesn't cause a compiler warning is because those warning were turned off as
so
> much code in chromium does these implicit signed/unsigned casts.

Done.

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

https://codereview.chromium.org/2019783002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:7629: +<histogram
name="DataReductionProxy.SuccessfulRequestCompletionCounts">
On 2016/05/27 21:16:37, Alexei Svitkine (slow) wrote:
> Nit: add units=
> 
> Same below.

Done.

[commit-bot: I haz the power, 2016-05-27 23:37:07]

Description was changed from

==========
Add histograms for requests that were fetched successfully

Add histograms for requests that were fetched successfully 
through data reduction proxy. Also, replace is_fallback by
proxy_index in DataReductionProxyTypeInfo.

BUG=615495
==========

to

==========
Add histograms for requests that were fetched successfully

Add histograms for requests that were fetched successfully 
through data reduction proxy. Also, replace is_fallback by
proxy_index in DataReductionProxyTypeInfo.

BUG=615495
==========

[commit-bot: I haz the power, 2016-05-27 23:37:08]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2016-05-27 23:39:21]

Description was changed from

==========
Add histograms for requests that were fetched successfully

Add histograms for requests that were fetched successfully 
through data reduction proxy. Also, replace is_fallback by
proxy_index in DataReductionProxyTypeInfo.

BUG=615495
==========

to

==========
Add histograms for requests that were fetched successfully

Add histograms for requests that were fetched successfully
through data reduction proxy. Also, replace is_fallback by
proxy_index in DataReductionProxyTypeInfo.

BUG=615495

Committed: https://crrev.com/9a89e4db53afe3a849700af9ac981de529d4258b
Cr-Commit-Position: refs/heads/master@{#396598}
==========

[commit-bot: I haz the power, 2016-05-27 23:39:22]

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