Add detailed histograms for DownloadProtectionService URLFetcher errors.

Add detailed histograms for DownloadProtectionService URLFetcher errors.

BUG=263590
R=isherman@chromium.org, mmenke@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213747

[mattm, 2013-07-24 02:19:00]

https://codereview.chromium.org/19599012/diff/7001/chrome/browser/safe_browsi...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

https://codereview.chromium.org/19599012/diff/7001/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/download_protection_service.cc:395: if
(source->GetStatus().is_success()) {
is "is_success()" right here, or should I check "status() ==
net::URLRequestStatus::SUCCESS" ?

https://codereview.chromium.org/19599012/diff/7001/net/url_request/url_reques...
File net/url_request/url_request_status.h (right):

https://codereview.chromium.org/19599012/diff/7001/net/url_request/url_reques...
net/url_request/url_request_status.h:33: STATUS_MAX
any objections to this?

[mmenke, 2013-07-24 14:26:50]

https://codereview.chromium.org/19599012/diff/7001/chrome/browser/safe_browsi...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

https://codereview.chromium.org/19599012/diff/7001/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/download_protection_service.cc:394:
net::URLRequestStatus::STATUS_MAX);
I'm not sure this is needed - this seems pretty redundant with the NetError
histogram.  ERR_ABORTED is the same as CANCELED, OK is the same as SUCCESS.  All
other net errors map to failed.

So if we just have a single histogram with source->GetStatus().error(), that
should cover everything.  Separate histograms may be a bit easier to read, but I
think we suffer from histogram bloat enough as it is.

https://codereview.chromium.org/19599012/diff/7001/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/download_protection_service.cc:395: if
(source->GetStatus().is_success()) {
On 2013/07/24 02:19:00, mattm wrote:
> is "is_success()" right here, or should I check "status() ==
> net::URLRequestStatus::SUCCESS" ?

I think is_success() is the way to go.

https://codereview.chromium.org/19599012/diff/7001/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/download_protection_service.cc:402:
source->GetStatus().error());
This should be "-source->GetStatus().error()".  Net error codes are all positive
(Except OK, which is 0), and the histogram code only logs positive values, so
all the histograms with net error codes have to flip the sign.

https://codereview.chromium.org/19599012/diff/7001/net/url_request/url_reques...
File net/url_request/url_request_status.h (right):

https://codereview.chromium.org/19599012/diff/7001/net/url_request/url_reques...
net/url_request/url_request_status.h:33: STATUS_MAX
On 2013/07/24 02:19:00, mattm wrote:
> any objections to this?

Nope, I'm fine with it...  But I'm not sure we need it, per other comment.

https://codereview.chromium.org/19599012/diff/7001/tools/metrics/histograms/h...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/19599012/diff/7001/tools/metrics/histograms/h...
tools/metrics/histograms/histograms.xml:12396: <histogram
name="SBClientDownload.CheckDownloadRequestNetError"
nit:  Think the "Check" isn't needed.  This is a histogram of just the
DownloadRequestNetError codes.  Check doesn't seem to add anything.  Or it is
just for sorting purposes?

[mattm, 2013-07-24 22:39:27]

https://codereview.chromium.org/19599012/diff/7001/chrome/browser/safe_browsi...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

https://codereview.chromium.org/19599012/diff/7001/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/download_protection_service.cc:394:
net::URLRequestStatus::STATUS_MAX);
On 2013/07/24 14:26:50, mmenke wrote:
> I'm not sure this is needed - this seems pretty redundant with the NetError
> histogram.  ERR_ABORTED is the same as CANCELED, OK is the same as SUCCESS. 
All
> other net errors map to failed.
> 
> So if we just have a single histogram with source->GetStatus().error(), that
> should cover everything.  Separate histograms may be a bit easier to read, but
I
> think we suffer from histogram bloat enough as it is.

Sounds good, I didn't realize CANCELED would have an error code too.

https://codereview.chromium.org/19599012/diff/7001/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/download_protection_service.cc:402:
source->GetStatus().error());
On 2013/07/24 14:26:50, mmenke wrote:
> This should be "-source->GetStatus().error()".  Net error codes are all
positive
> (Except OK, which is 0), and the histogram code only logs positive values, so
> all the histograms with net error codes have to flip the sign.

Done.

https://codereview.chromium.org/19599012/diff/7001/tools/metrics/histograms/h...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/19599012/diff/7001/tools/metrics/histograms/h...
tools/metrics/histograms/histograms.xml:12396: <histogram
name="SBClientDownload.CheckDownloadRequestNetError"
On 2013/07/24 14:26:50, mmenke wrote:
> nit:  Think the "Check" isn't needed.  This is a histogram of just the
> DownloadRequestNetError codes.  Check doesn't seem to add anything.  Or it is
> just for sorting purposes?

Mostly it was to sort it near the CheckDownloadStats histogram, but I see there
is also a DownloadRequestDuration histogram, so I guess the shorter form would
be fine too.

[mmenke, 2013-07-24 22:40:45]

LGTM!

[mattm, 2013-07-24 22:46:32]

+isherman for tools/metrics/histograms/

[Ilya Sherman, 2013-07-25 03:30:52]

LGTM

https://codereview.chromium.org/19599012/diff/24001/tools/metrics/histograms/...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/19599012/diff/24001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:12422: The net error code for all
CheckClientDownloadRequest URLFetchers.
nit: I'd recommend replacing "net" with "network", as "net" is a somewhat
overloaded term.

[mmenke, 2013-07-25 03:39:18]

https://codereview.chromium.org/19599012/diff/24001/tools/metrics/histograms/...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/19599012/diff/24001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:12422: The net error code for all
CheckClientDownloadRequest URLFetchers.
On 2013/07/25 03:30:52, Ilya Sherman wrote:
> nit: I'd recommend replacing "net" with "network", as "net" is a somewhat
> overloaded term.

"net" specifically refers to src/net, and the "net" namespace.  It's seems more
accurate than network, despite the overloading.

[commit-bot: I haz the power, 2013-07-25 09:10:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/19599012/24001

[commit-bot: I haz the power, 2013-07-25 12:52:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/19599012/24001

[commit-bot: I haz the power, 2013-07-26 00:19:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/19599012/24001

[mattm, 2013-07-26 03:08:50]

Committed patchset #2 manually as r213747 (presubmit successful).