Improve logging of DRP.ConfigService.HTTPRequests

Improve logging of DRP.ConfigService.HTTPRequests

Record DRP.ConfigService.HTTPRequests only if either Data
Reduction Proxy (DRP) is going to be used for fetching a
request, or if DRP can't be used because no valid DRP
proxy is available.

Also, renamed a histogram to match
DataReductionProxy.ConfigService.* prefix. The new name is
also the one present in histograms.xml.

BUG=649794
Committed: https://crrev.com/7016bb232e86e08cbc9d999f8ab3dcce40b3cfbb
Cr-Commit-Position: refs/heads/master@{#421662}

[tbansal1, 2016-09-23 00:50:40]

Description was changed from

==========
fix config service

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

to

==========
fix config service nits

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

[tbansal1, 2016-09-23 00:50:49]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 00:51:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 01:32:29]

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

[commit-bot: I haz the power, 2016-09-23 01:32:29]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2016-09-23 18:44:22]

Patchset #2 (id:20001) has been deleted

[tbansal1, 2016-09-23 18:44:27]

Patchset #1 (id:1) has been deleted

[tbansal1, 2016-09-23 19:53:34]

Description was changed from

==========
fix config service nits

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

to

==========
Improve logging of DRP.ConfigService.HTTPRequests

Record DRP.ConfigService.HTTPRequests only if either Data 
Reduction Proxy (DRP) is going to be used for fetching a 
request, or if DRP can't be used because no valid DRP
proxy is available.

BUG=649794
==========

[tbansal1, 2016-09-23 19:54:44]

Patchset #2 (id:60001) has been deleted

[tbansal1, 2016-09-23 19:55:29]

tbansal@chromium.org changed reviewers:
+ megjablon@chromium.org, ryansturm@chromium.org

[tbansal1, 2016-09-23 19:55:30]

ryansturm, megjablon: ptal. thanks.

[RyanSturm, 2016-09-23 20:54:22]

https://codereview.chromium.org/2365823002/diff/80001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc
(right):

https://codereview.chromium.org/2365823002/diff/80001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc:116:
"DataReductionProxy.ConfigService.AuthExpiredSessionKey", state,
nit: Should this be a separate CL?

https://codereview.chromium.org/2365823002/diff/80001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc
(right):

https://codereview.chromium.org/2365823002/diff/80001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc:186:
DCHECK(data_reduction_proxy_config.is_valid() || !data_saver_proxy_used);
nit: Why are you adding this DCHECK? It's the first thing checked in
ApplyProxyConfigToProxyInfo. I suspect that if that implementation changes this
DCHECK will just have to be changed?

I don't have objections to it being here; I'm just curious why you are adding
it.

[tbansal1, 2016-09-23 21:58:43]

Description was changed from

==========
Improve logging of DRP.ConfigService.HTTPRequests

Record DRP.ConfigService.HTTPRequests only if either Data 
Reduction Proxy (DRP) is going to be used for fetching a 
request, or if DRP can't be used because no valid DRP
proxy is available.

BUG=649794
==========

to

==========
Improve logging of DRP.ConfigService.HTTPRequests

Record DRP.ConfigService.HTTPRequests only if either Data 
Reduction Proxy (DRP) is going to be used for fetching a 
request, or if DRP can't be used because no valid DRP
proxy is available.

Also, renamed a histogram to match 
DataReductionProxy.ConfigService.* prefix. The new name is
also the one present in histograms.xml.

BUG=649794
==========

[tbansal1, 2016-09-23 22:00:05]

https://codereview.chromium.org/2365823002/diff/80001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc
(right):

https://codereview.chromium.org/2365823002/diff/80001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc:116:
"DataReductionProxy.ConfigService.AuthExpiredSessionKey", state,
On 2016/09/23 20:54:22, Ryan Sturm wrote:
> nit: Should this be a separate CL?

I updated the CL description to include this.

It could be a different CL, but it is a pretty simple change.

https://codereview.chromium.org/2365823002/diff/80001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc
(right):

https://codereview.chromium.org/2365823002/diff/80001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc:186:
DCHECK(data_reduction_proxy_config.is_valid() || !data_saver_proxy_used);
On 2016/09/23 20:54:22, Ryan Sturm wrote:
> nit: Why are you adding this DCHECK? It's the first thing checked in
> ApplyProxyConfigToProxyInfo. I suspect that if that implementation changes
this
> DCHECK will just have to be changed?
> 
> I don't have objections to it being here; I'm just curious why you are adding
> it.

Because the implementation below depends on the statement inside the DCHECK
being correct.
If somehow it was possible for DRP config to be invalid, and
|data_saver_proxy_used| to be still true,
then we would be recording incorrect data.

[RyanSturm, 2016-09-23 22:06:45]

lgtm

[tbansal1, 2016-09-27 00:48:49]

megjablon: ping.

[megjablon, 2016-09-27 01:12:23]

https://chromiumcodereview.appspot.com/2365823002/diff/80001/components/data_...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc
(right):

https://chromiumcodereview.appspot.com/2365823002/diff/80001/components/data_...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc:185:
// must be false.
Can you change this comment so it reflects what the DCHECK actually does? The
negatives make this a bit confusing as is.

i.e. // The |data_reduction_proxy_config| must be valid otherwise the proxy
cannot be used.

[tbansal1, 2016-09-28 00:48:56]

megjablon: ptal. thanks.

https://chromiumcodereview.appspot.com/2365823002/diff/80001/components/data_...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc
(right):

https://chromiumcodereview.appspot.com/2365823002/diff/80001/components/data_...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc:185:
// must be false.
On 2016/09/27 01:12:23, megjablon wrote:
> Can you change this comment so it reflects what the DCHECK actually does? The
> negatives make this a bit confusing as is.
> 
> i.e. // The |data_reduction_proxy_config| must be valid otherwise the proxy
> cannot be used. 

Done.

[tbansal1, 2016-09-28 00:49:20]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 00:49:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 02:18:11]

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

[commit-bot: I haz the power, 2016-09-28 02:18:12]

Dry run: This issue passed the CQ dry run.

[megjablon, 2016-09-28 16:56:28]

lgtm

[tbansal1, 2016-09-28 22:24:29]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2016-09-28 22:24:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
ryansturm@chromium.org
Link to the patchset: https://codereview.chromium.org/2365823002/#ps100001
(title: "Rebased, Addressed megjablon comments")

[commit-bot: I haz the power, 2016-09-28 22:25:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 22:31:22]

Description was changed from

==========
Improve logging of DRP.ConfigService.HTTPRequests

Record DRP.ConfigService.HTTPRequests only if either Data 
Reduction Proxy (DRP) is going to be used for fetching a 
request, or if DRP can't be used because no valid DRP
proxy is available.

Also, renamed a histogram to match 
DataReductionProxy.ConfigService.* prefix. The new name is
also the one present in histograms.xml.

BUG=649794
==========

to

==========
Improve logging of DRP.ConfigService.HTTPRequests

Record DRP.ConfigService.HTTPRequests only if either Data 
Reduction Proxy (DRP) is going to be used for fetching a 
request, or if DRP can't be used because no valid DRP
proxy is available.

Also, renamed a histogram to match 
DataReductionProxy.ConfigService.* prefix. The new name is
also the one present in histograms.xml.

BUG=649794
==========

[commit-bot: I haz the power, 2016-09-28 22:31:23]

Committed patchset #3 (id:100001)

[commit-bot: I haz the power, 2016-09-28 22:33:16]

Description was changed from

==========
Improve logging of DRP.ConfigService.HTTPRequests

Record DRP.ConfigService.HTTPRequests only if either Data 
Reduction Proxy (DRP) is going to be used for fetching a 
request, or if DRP can't be used because no valid DRP
proxy is available.

Also, renamed a histogram to match 
DataReductionProxy.ConfigService.* prefix. The new name is
also the one present in histograms.xml.

BUG=649794
==========

to

==========
Improve logging of DRP.ConfigService.HTTPRequests

Record DRP.ConfigService.HTTPRequests only if either Data
Reduction Proxy (DRP) is going to be used for fetching a
request, or if DRP can't be used because no valid DRP
proxy is available.

Also, renamed a histogram to match
DataReductionProxy.ConfigService.* prefix. The new name is
also the one present in histograms.xml.

BUG=649794

Committed: https://crrev.com/7016bb232e86e08cbc9d999f8ab3dcce40b3cfbb
Cr-Commit-Position: refs/heads/master@{#421662}
==========

[commit-bot: I haz the power, 2016-09-28 22:33:17]

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