Add detailed histograms for DownloadProtectionService URLFetcher errors.

chrome/browser/safe_browsing/download_protection_service.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/safe_browsing/download_protection_service.h"
 
 #include "base/bind.h"
 #include "base/compiler_specific.h"
 #include "base/format_macros.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/metrics/histogram.h"
+#include "base/metrics/sparse_histogram.h"
 #include "base/sequenced_task_runner_helpers.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "base/time/time.h"
 #include "chrome/browser/safe_browsing/download_feedback_service.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "chrome/browser/safe_browsing/sandboxed_zip_analyzer.h"
@@ skipping 358 matching lines @@
   }
 
   // From the net::URLFetcherDelegate interface.
   virtual void OnURLFetchComplete(const net::URLFetcher* source) OVERRIDE {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
     DCHECK_EQ(source, fetcher_.get());
     VLOG(2) << "Received a response for URL: "
             << item_->GetUrlChain().back() << ": success="
             << source->GetStatus().is_success() << " response_code="
             << source->GetResponseCode();
+    UMA_HISTOGRAM_ENUMERATION("SBClientDownload.CheckDownloadRequestStatus",
+                              source->GetStatus().status(),
+                              net::URLRequestStatus::STATUS_MAX);

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

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.

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

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

+    if (source->GetStatus().is_success()) {

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

is "is_success()" right here, or should I check "status() ==
net::URLRequestStatus::SUCCESS" ?

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

I think is_success() is the way to go.

+      UMA_HISTOGRAM_SPARSE_SLOWLY(
+          "SBClientDownload.CheckDownloadRequestResponseCode",
+          source->GetResponseCode());
+    } else if (source->GetStatus().status() == net::URLRequestStatus::FAILED) {
+      UMA_HISTOGRAM_SPARSE_SLOWLY(
+          "SBClientDownload.CheckDownloadRequestNetError",
+          source->GetStatus().error());

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

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.

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

Done.

+    }
     DownloadCheckResultReason reason = REASON_SERVER_PING_FAILED;
     DownloadCheckResult result = SAFE;
     if (source->GetStatus().is_success() &&
         net::HTTP_OK == source->GetResponseCode()) {
       ClientDownloadResponse response;
       std::string data;
       bool got_data = source->GetResponseAsString(&data);
       DCHECK(got_data);
       if (!response.ParseFromString(data)) {
         reason = REASON_INVALID_RESPONSE_PROTO;
@@ skipping 551 matching lines @@
   std::string url = kDownloadRequestUrl;
   std::string api_key = google_apis::GetAPIKey();
   if (!api_key.empty()) {
     base::StringAppendF(&url, "?key=%s",
                         net::EscapeQueryParamValue(api_key, true).c_str());
   }
   return url;
 }
 
 }  // namespace safe_browsing