Adds UMA histogram metrics for SafeBrowsing PVer4 methods.

chrome/browser/safe_browsing/protocol_manager.cc

Index: chrome/browser/safe_browsing/protocol_manager.cc
diff --git a/chrome/browser/safe_browsing/protocol_manager.cc b/chrome/browser/safe_browsing/protocol_manager.cc
index cd69a9713a04dc360ad8a79ce96e993f21feeb52..f56103ff9ae8c328727aac6d31181c8d97fcadaf 100644
--- a/chrome/browser/safe_browsing/protocol_manager.cc
+++ b/chrome/browser/safe_browsing/protocol_manager.cc
@@ -93,6 +93,8 @@ static const size_t kSbMaxBackOff = 8;
 
 const char kGetHashUmaResponseMetricName[] = "SB2.GetHashResponseOrErrorCode";
 const char kGetChunkUmaResponseMetricName[] = "SB2.GetChunkResponseOrErrorCode";
+const char kUmaV4ResponseMetricName[] =
+    "SafeBrowsing.GetV4HashHttpResponseOrErrorCode";
 
 // The V4 URL prefix where browser fetches hashes from the V4 server.
 const char kSbV4UrlPrefix[] = "https://safebrowsing.googleapis.com/v4";
@@ -216,6 +218,13 @@ void SafeBrowsingProtocolManager::RecordGetHashResult(bool is_download,
   }
 }
 
+// static
+void SafeBrowsingProtocolManager::RecordGetV4HashResult(
+    ResultType result_type) {
+  UMA_HISTOGRAM_ENUMERATION("SafeBrowsing.GetV4HashResult", result_type,
+                            GET_HASH_RESULT_MAX);
+}
+
 void SafeBrowsingProtocolManager::RecordHttpResponseOrErrorCode(
     const char* metric_name,
     const net::URLRequestStatus& status,
@@ -373,7 +382,11 @@ void SafeBrowsingProtocolManager::GetV4FullHashes(
   // proceed with the request. If not, we are required to return empty results
   // (i.e. treat the page as safe).
   if (Time::Now() <= next_gethash_v4_time_) {
-    // TODO(kcarattini): Add UMA recording.
+    if (gethash_v4_error_count_) {
+      RecordGetV4HashResult(GET_HASH_BACKOFF_ERROR);
+    } else {
+      RecordGetV4HashResult(GET_HASH_MIN_WAIT_DURATION_ERROR);
+    }
     std::vector<SBFullHashResult> full_hashes;
     callback.Run(full_hashes, base::TimeDelta());
     return;
@@ -477,24 +490,34 @@ void SafeBrowsingProtocolManager::OnURLFetchComplete(
     hash_requests_.erase(it);
   } else if (v4_it != v4_hash_requests_.end()) {
     // V4 FindFullHashes response.
+    // TODO(kcarattini): Consider pulling all the V4 handling out into a
+    // separate V4ProtocolManager class.
+    RecordHttpResponseOrErrorCode(kUmaV4ResponseMetricName, status,
+                                  response_code);
     const FullHashDetails& details = v4_it->second;
     std::vector<SBFullHashResult> full_hashes;
     base::TimeDelta negative_cache_duration;
     if (status.is_success() && response_code == net::HTTP_OK) {
-      // TODO(kcarattini): Add UMA reporting.
+      RecordGetV4HashResult(GET_HASH_STATUS_200);
       ResetGetHashV4Errors();
       std::string data;
       source->GetResponseAsString(&data);
       if (!ParseV4HashResponse(data, &full_hashes, &negative_cache_duration)) {
         full_hashes.clear();
-        // TODO(kcarattini): Add UMA reporting.
+        RecordGetV4HashResult(GET_HASH_PARSE_ERROR);

[Steven Holte, 2016/01/13 21:25:43]

It's usually preferable to have non-overlapping enum values, so that you can say
if 30% of the counts in a histogram fall in bucket X then 30% of the operations
that recorded a result had that result.

In this path it looks like you are recording counts in both PARSE_ERROR and
STATUS_200, which will make interpreting those results more complicated.  It
would be cleaner to make these two cases (STATUS_200 w/o error and PARSE_ERROR)

Not sure if BACKOFF_ERROR/MIN_WAIT_DURATION_ERROR can also be recorded.

[kcarattini, 2016/01/13 22:53:20]

Nathan, please correct me if I'm wrong, but I assume we are interested in the
raw counts of these histogram buckets rather than percentage, and we only look
for large changes in these metrics to indicate something is amiss. Otherwise, as
Steve mentioned, interpreting the results is more complicated. I could remove
the GET_HASH_PARSE_ERROR bucket entirely because I have added a new histogram
for that in a follow up cl anyway, but the other buckets (backoff,
min_wait_duration etc.) would still overlap. I've added the new histograms
buckets in the same way that the existing v3 GetHashResult metrics are recorded
so I am assuming this is the way we want it. 

[Nathan Parker, 2016/01/14 02:04:25]

I agree with Steven but if it's already overlapping, there's not real harm in
adding more overlapping.  As long as this is described in the .xml text, then
this is fine.

       }
     } else {
       HandleGetHashV4Error(Time::Now());
-      // TODO(kcarattini): Add UMA reporting.
+
       DVLOG(1) << "SafeBrowsing GetEncodedFullHashes request for: " <<
           source->GetURL() << " failed with error: " << status.error() <<
           " and response code: " << response_code;
+
+      if (status.status() == net::URLRequestStatus::FAILED) {
+        RecordGetV4HashResult(GET_HASH_NETWORK_ERROR);
+      } else {
+        RecordGetV4HashResult(GET_HASH_HTTP_ERROR);
+      }
     }
 
     // Invoke the callback with full_hashes, even if there was a parse error or