Add UMA metric for GetHash parsing errors.

chrome/browser/safe_browsing/protocol_manager.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/protocol_manager.h"
 
 #include <utility>
 
 #include "base/base64.h"
 #include "base/environment.h"
@@ skipping 199 matching lines @@
 void SafeBrowsingProtocolManager::RecordGetHashResult(bool is_download,
                                                       ResultType result_type) {
   if (is_download) {
     UMA_HISTOGRAM_ENUMERATION("SB2.GetHashResultDownload", result_type,
                               GET_HASH_RESULT_MAX);
   } else {
     UMA_HISTOGRAM_ENUMERATION("SB2.GetHashResult", result_type,
                               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(
     ResultType result_type) {
   UMA_HISTOGRAM_ENUMERATION("SafeBrowsing.GetV4HashResult", result_type,
                             GET_HASH_RESULT_MAX);
 }
 
 void SafeBrowsingProtocolManager::RecordHttpResponseOrErrorCode(
     const char* metric_name,
@@ skipping 77 matching lines @@
   return req_base64;
 }
 
 bool SafeBrowsingProtocolManager::ParseV4HashResponse(
     const std::string& data,
     std::vector<SBFullHashResult>* full_hashes,
     base::TimeDelta* negative_cache_duration) {
   FindFullHashesResponse response;
 
   if (!response.ParseFromString(data)) {
-    // TODO(kcarattini): Add UMA.
+    RecordParseGetHashResult(PARSE_FROM_STRING_ERROR);
     return false;
   }
 
   if (response.has_negative_cache_duration()) {
     // Seconds resolution is good enough so we ignore the nanos field.
     *negative_cache_duration = base::TimeDelta::FromSeconds(
         response.negative_cache_duration().seconds());
   }
 
   if (response.has_minimum_wait_duration()) {
     // Seconds resolution is good enough so we ignore the nanos field.
     next_gethash_v4_time_ = Time::Now() + base::TimeDelta::FromSeconds(
         response.minimum_wait_duration().seconds());
   }
 
   // Loop over the threat matches and fill in full_hashes.
   for (const ThreatMatch& match : response.matches()) {
     // Make sure the platform and threat entry type match.
     if (!(match.has_threat_entry_type() &&
           match.threat_entry_type() == URL_EXPRESSION &&
           match.has_threat())) {
+      RecordParseGetHashResult(UNEXPECTED_THREAT_ENTRY_TYPE_ERROR);
       continue;
     }
 
     // Fill in the full hash.
     SBFullHashResult result;
     result.hash = StringToSBFullHash(match.threat().hash());
 
     if (match.has_cache_duration()) {
       // Seconds resolution is good enough so we ignore the nanos field.
       result.cache_duration = base::TimeDelta::FromSeconds(
           match.cache_duration().seconds());
     }
 
     // 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.

     }
 
     full_hashes->push_back(result);
   }
   return true;
 }
 
 void SafeBrowsingProtocolManager::GetV4FullHashes(
     const std::vector<SBPrefix>& prefixes,
     const std::vector<PlatformType>& platforms,
@@ skipping 657 matching lines @@
 SafeBrowsingProtocolManager::FullHashDetails::FullHashDetails(
     FullHashCallback callback,
     bool is_download)
     : callback(callback), is_download(is_download) {}
 
 SafeBrowsingProtocolManager::FullHashDetails::~FullHashDetails() {}
 
 SafeBrowsingProtocolManagerDelegate::~SafeBrowsingProtocolManagerDelegate() {}
 
 }  // namespace safe_browsing