Add UMA metric for GetHash parsing errors.

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 f56103ff9ae8c328727aac6d31181c8d97fcadaf..863a9902b7b35d2171b9f87c55460fabc0748cc9 100644
--- a/chrome/browser/safe_browsing/protocol_manager.cc
+++ b/chrome/browser/safe_browsing/protocol_manager.cc
@@ -217,6 +217,11 @@ void SafeBrowsingProtocolManager::RecordGetHashResult(bool is_download,
                               GET_HASH_RESULT_MAX);
   }
 }
+void SafeBrowsingProtocolManager::RecordParseGetHashResult(
+    ParseResultType result_type) {
+  UMA_HISTOGRAM_ENUMERATION("SafeBrowsing.ParseV4HashResult", result_type,
+                            PARSE_GET_HASH_RESULT_MAX);
+}
 
 // static
 void SafeBrowsingProtocolManager::RecordGetV4HashResult(
@@ -314,7 +319,7 @@ bool SafeBrowsingProtocolManager::ParseV4HashResponse(
   FindFullHashesResponse response;
 
   if (!response.ParseFromString(data)) {
-    // TODO(kcarattini): Add UMA.
+    RecordParseGetHashResult(PARSE_FROM_STRING_ERROR);
     return false;
   }
 
@@ -336,6 +341,7 @@ bool SafeBrowsingProtocolManager::ParseV4HashResponse(
     if (!(match.has_threat_entry_type() &&
           match.threat_entry_type() == URL_EXPRESSION &&
           match.has_threat())) {
+      RecordParseGetHashResult(UNEXPECTED_THREAT_ENTRY_TYPE_ERROR);
       continue;
     }
 
@@ -350,19 +356,25 @@ bool SafeBrowsingProtocolManager::ParseV4HashResponse(
     }
 
     // Different threat types will handle the metadata differently.
-    if (match.has_threat_type() && match.threat_type() == API_ABUSE &&
-        match.has_platform_type() &&
-        match.platform_type() == CHROME_PLATFORM &&
-        match.has_threat_entry_metadata()) {
-      // For API Abuse, store a csv of the returned permissions.
-      for (const ThreatEntryMetadata::MetadataEntry& m :
-               match.threat_entry_metadata().entries()) {
-        if (m.key() == "permission") {
-          result.metadata += m.value() + ",";
+    if (match.has_threat_type() && match.threat_type() == API_ABUSE) {
+      if (match.has_platform_type() &&
+          match.platform_type() == CHROME_PLATFORM) {
+        if (match.has_threat_entry_metadata()) {
+          // For API Abuse, store a csv of the returned permissions.
+          for (const ThreatEntryMetadata::MetadataEntry& m :
+                   match.threat_entry_metadata().entries()) {
+            if (m.key() == "permission") {
+              result.metadata += m.value() + ",";
+            }
+          }
+        } else {
+          RecordParseGetHashResult(NO_METADATA_ERROR);
         }
+      } else {
+        RecordParseGetHashResult(UNEXPECTED_PLATFORM_TYPE_ERROR);
       }
     } else {
-      // TODO(kcarattini): Add UMA for unexpected threat type match.
+      RecordParseGetHashResult(UNEXPECTED_THREAT_TYPE_ERROR);
       return false;

[Nathan Parker, 2016/01/15 19:34:42]

Why does this this one return and the rest dont?  I'm noticing that some of
these can be reported multiple times per V4 HashReponse (if there are multiple
matches), and some at most once.  This makes it harder to calculate percentages
of partially-failed requests (full-failed requests will return false and the
caller reports a different metric).

We could report a PARTIAL_FAILURE error (once) when ANY of these are reported,
so we can expect that that will add up to the rest of them unless we're getting
multiple errors per response.  

[kcarattini, 2016/01/20 04:02:05]

I made them all return false, that way there is only one report per Response and
the Response is meaningless if any of these errors occur. What do you think? It
gets more complicated if we want to have partial errors. I think in a follow up
cl I'll add enforcement of the one-threat-type-per-request decision we made
previously so that it is explicit.

     }