Adds UMA histogram metrics for SafeBrowsing PVer4 methods.

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 74 matching lines @@
 // Maximum time, in seconds, from start up before we must issue an update query.
 static const int kSbTimerStartIntervalSecMax = 300;
 
 // The maximum time, in seconds, to wait for a response to an update request.
 static const int kSbMaxUpdateWaitSec = 30;
 
 // Maximum back off multiplier.
 static const size_t kSbMaxBackOff = 8;
 
 const char kUmaHashResponseMetricName[] = "SB2.GetHashResponseOrErrorCode";
+const char kUmaV4ResponseMetricName[] =
+    "SafeBrowsing.V4HttpResponseOrErrorCode";

[Nathan Parker, 2016/01/07 05:28:29]

Should this have "Hash" in it somewhere? (since there will he other V4Http
Responses)

[kcarattini, 2016/01/07 06:06:13]

Done.

 
 // The V4 URL prefix where browser fetches hashes from the V4 server.
 const char kSbV4UrlPrefix[] = "https://safebrowsing.googleapis.com/v4";
 
 // The default SBProtocolManagerFactory.
 class SBProtocolManagerFactoryImpl : public SBProtocolManagerFactory {
  public:
   SBProtocolManagerFactoryImpl() {}
   ~SBProtocolManagerFactoryImpl() override {}
   SafeBrowsingProtocolManager* CreateProtocolManager(
@@ skipping 74 matching lines @@
                                                       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);
   }
 }
 
+// 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,
     int response_code) {
   UMA_HISTOGRAM_SPARSE_SLOWLY(
       metric_name, status.is_success() ? response_code : status.error());
 }
 
 bool SafeBrowsingProtocolManager::IsUpdateScheduled() const {
   return update_timer_.IsRunning();
@@ skipping 132 matching lines @@
     const std::vector<SBPrefix>& prefixes,
     const std::vector<PlatformType>& platforms,
     ThreatType threat_type,
     FullHashCallback callback) {
   DCHECK(CalledOnValidThread());
   // We need to wait the minimum waiting duration, and if we are in backoff,
   // we need to check if we're past the next allowed time. If we are, we can
   // 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;
   }
 
   std::string req_base64 = GetV4HashRequest(prefixes, platforms, threat_type);
   GURL gethash_url = GetV4HashUrl(req_base64);
 
   net::URLFetcher* fetcher =
       net::URLFetcher::Create(url_fetcher_id_++, gethash_url,
@@ skipping 35 matching lines @@
 //              required, the SafeBrowsing servers will tell us to get it again.
 void SafeBrowsingProtocolManager::OnURLFetchComplete(
     const net::URLFetcher* source) {
   DCHECK(CalledOnValidThread());
   scoped_ptr<const net::URLFetcher> fetcher;
 
   HashRequests::iterator it = hash_requests_.find(source);
   HashRequests::iterator v4_it = v4_hash_requests_.find(source);
   int response_code = source->GetResponseCode();
   net::URLRequestStatus status = source->GetStatus();
-  RecordHttpResponseOrErrorCode(kUmaHashResponseMetricName, status,
-                                response_code);
+
   if (it != hash_requests_.end()) {
     // GetHash response.
+    RecordHttpResponseOrErrorCode(kUmaHashResponseMetricName, status,
+                                  response_code);
     fetcher.reset(it->first);
     const FullHashDetails& details = it->second;
     std::vector<SBFullHashResult> full_hashes;
     base::TimeDelta cache_lifetime;
     if (status.is_success() && (response_code == net::HTTP_OK ||
                                 response_code == net::HTTP_NO_CONTENT)) {
       // For tracking our GetHash false positive (net::HTTP_NO_CONTENT) rate,
       // compared to real (net::HTTP_OK) responses.
       if (response_code == net::HTTP_OK)
         RecordGetHashResult(details.is_download, GET_HASH_STATUS_200);
@@ skipping 25 matching lines @@
     }
 
     // Invoke the callback with full_hashes, even if there was a parse error or
     // an error response code (in which case full_hashes will be empty). The
     // caller can't be blocked indefinitely.
     details.callback.Run(full_hashes, cache_lifetime);
 
     hash_requests_.erase(it);
   } else if (v4_it != v4_hash_requests_.end()) {
     // V4 FindFullHashes response.
+    RecordHttpResponseOrErrorCode(kUmaV4ResponseMetricName, status,
+                                  response_code);
     fetcher.reset(v4_it->first);
     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);
       gethash_v4_error_count_ = 0;
       gethash_v4_back_off_mult_ = 1;
       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);
       }
     } else {
       HandleGetHashV4Error(Time::Now());
-      // TODO(kcarattini): Add UMA reporting.
       if (status.status() == net::URLRequestStatus::FAILED) {
+        RecordGetV4HashResult(GET_HASH_NETWORK_ERROR);
         DVLOG(1) << "SafeBrowsing GetEncodedFullHashes request for: " <<
             source->GetURL() << " failed with error: " << status.error();
       } else {
+        RecordGetV4HashResult(GET_HASH_HTTP_ERROR);
         DVLOG(1) << "SafeBrowsing GetEncodedFullHashes request for: " <<
             source->GetURL() << " failed with error: " << response_code;
       }
     }
 
     // Invoke the callback with full_hashes, even if there was a parse error or
     // an error response code (in which case full_hashes will be empty). The
     // caller can't be blocked indefinitely.
     details.callback.Run(full_hashes, negative_cache_duration);
 
     v4_hash_requests_.erase(it);
   } else {
     // Update or chunk response.

[Nathan Parker, 2016/01/07 05:28:29]

I always thought it was odd that this OnURLFetchComplete() handles both chunk
responses and hash responses, since they are quite different... Do you think
there is an advantage to having the v4 hash handling in here as well, rather
than in a separate class?  If there's no overlap, it could make both simpler by
creating a V4ProtocolManager or some such.  Just a thought.  You could land this
stuff and consider pull it out later.  Or we could leave it here till all the V3
code goes away (quarters).

[kcarattini, 2016/01/07 06:06:13]

Added a TODO to consider pulling this out in the future. There is a small amount
of method and type definition reuse that is useful in keeping these together
e.g. the ResultType definition).

+    RecordHttpResponseOrErrorCode(kUmaHashResponseMetricName, status,
+                                  response_code);
     fetcher.reset(request_.release());
 
     if (request_type_ == UPDATE_REQUEST ||
         request_type_ == BACKUP_UPDATE_REQUEST) {
       if (!fetcher.get()) {
         // We've timed out waiting for an update response, so we've cancelled
         // the update request and scheduled a new one. Ignore this response.
         return;
       }
 
@@ skipping 513 matching lines @@
 SafeBrowsingProtocolManager::FullHashDetails::FullHashDetails(
     FullHashCallback callback,
     bool is_download)
     : callback(callback), is_download(is_download) {}
 
 SafeBrowsingProtocolManager::FullHashDetails::~FullHashDetails() {}
 
 SafeBrowsingProtocolManagerDelegate::~SafeBrowsingProtocolManagerDelegate() {}
 
 }  // namespace safe_browsing