[NTP Snippets] Add metric for fetch error cases

components/ntp_snippets/ntp_snippets_fetcher.cc

 // Copyright 2016 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 "components/ntp_snippets/ntp_snippets_fetcher.h"
 
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
+#include "base/metrics/sparse_histogram.h"
 #include "base/path_service.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/task_runner_util.h"
 #include "google_apis/google_api_keys.h"
 #include "net/base/load_flags.h"
 #include "net/http/http_request_headers.h"
 #include "net/http/http_response_headers.h"
 #include "net/http/http_status_code.h"
@@ skipping 94 matching lines @@
   url_fetcher_->Start();
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // URLFetcherDelegate overrides
 void NTPSnippetsFetcher::OnURLFetchComplete(const URLFetcher* source) {
   DCHECK_EQ(url_fetcher_.get(), source);
 
   std::string message;
   const URLRequestStatus& status = source->GetStatus();
+
   if (!status.is_success()) {
+    UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.Snippets.FailedRequestErrorCode",
+                                -status.error());

[Marc Treib, 2016/05/02 14:41:22]

I assume the minus is because the error codes tend to be negative - are we sure
that's always the case? URLRequestStatus doesn't seem to say anything to that
effect?

[mastiz, 2016/05/02 15:11:44]

Although it's not explicitly documented, URLRequestStatus' constructor has a
DCHECK that verifies it's zero or negative. In fact, there's various histograms
that make this assumption.

[Marc Treib, 2016/05/02 15:20:56]

Alright then. Maybe worth a comment?

+
     message = base::StringPrintf(kStatusMessageURLRequestErrorFormat,
                                  status.error());
-  } else if (source->GetResponseCode() != net::HTTP_OK) {
-    message = base::StringPrintf(kStatusMessageHTTPErrorFormat,
-                                 source->GetResponseCode());
+  } else {
+    UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.Snippets.FetchResponseCode",
+                                source->GetResponseCode());

[Marc Treib, 2016/05/02 14:41:22]

So this one is recorded on success too, which probably makes sense, but doesn't
match the CL description.

[mastiz, 2016/05/02 15:11:44]

You're right, the description was outdated, thanks for pointing out.

+
+    if (source->GetResponseCode() != net::HTTP_OK) {
+      message = base::StringPrintf(kStatusMessageHTTPErrorFormat,
+                                   source->GetResponseCode());
+    }
   }
 
   std::string response;
   if (!message.empty()) {
     DLOG(WARNING) << message << " while trying to download "
                   << source->GetURL().spec();
 
   } else {
     bool stores_result_to_string = source->GetResponseAsString(&response);
     DCHECK(stores_result_to_string);
   }
 
   callback_list_.Notify(response, message);
 }
 
 }  // namespace ntp_snippets