Log more data reduction proxy config service UMA

Log more data reduction proxy config service UMA

(1) Latency penalty when 407 is received from the data saver
proxy, and the request had to be restarted.
(2) Count of requests that did not go through the data saver
proxy because the proxy config was unavailable.

BUG=587149
Committed: https://crrev.com/5800e9cf759b79eb537dcf5af8e4278d99a55a40
Cr-Commit-Position: refs/heads/master@{#375936}

[tbansal1, 2016-02-17 01:05:05]

Description was changed from

==========
Log more data reduction proxy config service UMA

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

to

==========
Log more data reduction proxy config service UMA

(1) Latency penalty when 407 is received from the data saver 
proxy, and the request had to be restarted
(2) Count of requests that did not go through the data saver
proxy because proxy config was unavailable.

BUG=587149
==========

[tbansal1, 2016-02-17 01:05:13]

Description was changed from

==========
Log more data reduction proxy config service UMA

(1) Latency penalty when 407 is received from the data saver 
proxy, and the request had to be restarted
(2) Count of requests that did not go through the data saver
proxy because proxy config was unavailable.

BUG=587149
==========

to

==========
Log more data reduction proxy config service UMA

(1) Latency penalty when 407 is received from the data saver 
proxy, and the request had to be restarted.
(2) Count of requests that did not go through the data saver
proxy because the proxy config was unavailable.

BUG=587149
==========

[tbansal1, 2016-02-17 01:08:39]

Patchset #1 (id:1) has been deleted

[tbansal1, 2016-02-17 01:09:00]

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

[tbansal1, 2016-02-17 01:09:00]

sclittle: PTAL at *.
Thanks!!

[sclittle, 2016-02-17 02:17:50]

lgtm % some minor style nits

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

https://codereview.chromium.org/1702253002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:364:
if (!data_reduction_proxy_info.proxy_server().is_direct()) {
nit: unnecessary braces

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

https://codereview.chromium.org/1702253002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:702:
// Empty retry info map
nit: comments should typically be complete sentences

https://codereview.chromium.org/1702253002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:705:
// Direct
nit: complete sentences?

https://codereview.chromium.org/1702253002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:713:
// result.Use(other_proxy_info);
nit: remove commented out code?

[tbansal1, 2016-02-17 02:23:31]

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

[tbansal1, 2016-02-17 02:23:32]

asvitkine: PTAL at histograms.xml. Thanks!

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

https://codereview.chromium.org/1702253002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:364:
if (!data_reduction_proxy_info.proxy_server().is_direct()) {
On 2016/02/17 02:17:50, sclittle wrote:
> nit: unnecessary braces

Done.

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

https://codereview.chromium.org/1702253002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:702:
// Empty retry info map
On 2016/02/17 02:17:50, sclittle wrote:
> nit: comments should typically be complete sentences

Copied from elsewhere in this file :P. Removed them as they do not provide much
information. Done.

https://codereview.chromium.org/1702253002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:705:
// Direct
On 2016/02/17 02:17:50, sclittle wrote:
> nit: complete sentences?

Done.

https://codereview.chromium.org/1702253002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:713:
// result.Use(other_proxy_info);
On 2016/02/17 02:17:50, sclittle wrote:
> nit: remove commented out code?

Done.

[Alexei Svitkine (slow), 2016-02-17 16:32:24]

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

https://codereview.chromium.org/1702253002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:370:
"DataReductionProxy.ConfigService.MissingConfigHTTPRequests", 1);
If you're always logging 1, it's more efficient to use an
UMA_HISTOGRAM_BOOLEAN() - as it allocates less memory (and results in the same
metric).

However, logging just a value without any baseline isn't meaningful. What would
a number like 23221 tell you - without knowing some other piece of information
(number of users, requests, etc). So it's often good to log both true and false,
so you can see your metric as a ratio of the overall. (Whatever you define
overall as).

[tbansal1, 2016-02-17 17:42:59]

asvitkine: ptal.

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

https://codereview.chromium.org/1702253002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:370:
"DataReductionProxy.ConfigService.MissingConfigHTTPRequests", 1);
On 2016/02/17 16:32:24, Alexei Svitkine wrote:
> If you're always logging 1, it's more efficient to use an
> UMA_HISTOGRAM_BOOLEAN() - as it allocates less memory (and results in the same
> metric).
> 
> However, logging just a value without any baseline isn't meaningful. What
would
> a number like 23221 tell you - without knowing some other piece of information
> (number of users, requests, etc). So it's often good to log both true and
false,
> so you can see your metric as a ratio of the overall. (Whatever you define
> overall as).

Done, I was planning to compare with another UMA metric that is logged elsewhere
(DataReductionProxy.RequestCompletionCodes.*) but this seems easier for
analysis.

[Alexei Svitkine (slow), 2016-02-17 17:47:53]

lgtm

[tbansal1, 2016-02-17 18:16:37]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2016-02-17 18:16:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
sclittle@chromium.org
Link to the patchset: https://codereview.chromium.org/1702253002/#ps60001
(title: "Rebased, use boolean histogram")

[commit-bot: I haz the power, 2016-02-17 18:18:29]

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

[commit-bot: I haz the power, 2016-02-17 18:54:56]

Description was changed from

==========
Log more data reduction proxy config service UMA

(1) Latency penalty when 407 is received from the data saver 
proxy, and the request had to be restarted.
(2) Count of requests that did not go through the data saver
proxy because the proxy config was unavailable.

BUG=587149
==========

to

==========
Log more data reduction proxy config service UMA

(1) Latency penalty when 407 is received from the data saver 
proxy, and the request had to be restarted.
(2) Count of requests that did not go through the data saver
proxy because the proxy config was unavailable.

BUG=587149
==========

[commit-bot: I haz the power, 2016-02-17 18:54:57]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-02-17 18:55:53]

Description was changed from

==========
Log more data reduction proxy config service UMA

(1) Latency penalty when 407 is received from the data saver 
proxy, and the request had to be restarted.
(2) Count of requests that did not go through the data saver
proxy because the proxy config was unavailable.

BUG=587149
==========

to

==========
Log more data reduction proxy config service UMA

(1) Latency penalty when 407 is received from the data saver
proxy, and the request had to be restarted.
(2) Count of requests that did not go through the data saver
proxy because the proxy config was unavailable.

BUG=587149

Committed: https://crrev.com/5800e9cf759b79eb537dcf5af8e4278d99a55a40
Cr-Commit-Position: refs/heads/master@{#375936}
==========

[commit-bot: I haz the power, 2016-02-17 18:55:54]

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