Add histogram for HTTP response codes

net/http/http_response_headers.cc

Index: net/http/http_response_headers.cc
diff --git a/net/http/http_response_headers.cc b/net/http/http_response_headers.cc
index 85df8d5e15f3c390a663dfad6eecb6857564ee59..fad4521a20314785ae16a58a7ff4bf2d3d2ac97a 100644
--- a/net/http/http_response_headers.cc
+++ b/net/http/http_response_headers.cc
@@ -12,6 +12,7 @@
 #include <algorithm>
 
 #include "base/logging.h"
+#include "base/metrics/histogram.h"
 #include "base/pickle.h"
 #include "base/string_number_conversions.h"
 #include "base/string_util.h"
@@ -85,6 +86,35 @@ bool ShouldUpdateHeader(const std::string::const_iterator& name_begin,
   return true;
 }
 
+// Functions for histogram initialization.  The code 0 is put in the
+// response map to track response codes that are invalid.
+// TODO(gavinp): Greatly prune the collected codes once we learn which
+// ares are not sent in practice, to reduce upload size & memory use.

[wtc, 2011/01/26 19:03:11]

Typo: remove 'ares', or change it to 'ones'

[gavinp, 2011/01/27 00:04:36]

Done.

+
+enum { kHistogramMinHttpResponseCode = 100,
+       kHistogramMaxHttpResponseCode = 599,

[wtc, 2011/01/26 19:03:11]

The formatting is still wrong.  It should be:

enum {
    kHistogramMinHttpResponseCode = 100,
    kHistogramMaxHttpResponseCode = 599,
};

[jar (doing other things), 2011/01/26 21:33:14]

nit: On Chrome, we use MACRO_STYLE_NAMING for enums (contrary to google3).

see: https://sites.google.com/a/chromium.org/dev/developers/coding-style

[gavinp, 2011/01/27 00:04:36]

Done.

[gavinp, 2011/01/27 00:04:36]

Done.

+};
+
+std::vector<int> GetAllHttpResponseCodes() {
+  std::vector<int> codes;
+  codes.reserve(
+      kHistogramMaxHttpResponseCode - kHistogramMinHttpResponseCode + 2);
+  // The 0 histogram value is used to return results that are not in the 
+  // range [kHistogramMinHttpResponseCode, kHistogramMaxHttpResponseCode].

[wtc, 2011/01/26 19:03:11]

I see that you already explained the 0 histogram value at lines 89-90 above.
So you may remove this comment I asked you to add.

[gavinp, 2011/01/27 00:04:36]

Done.

+  codes.push_back(0);
+  for (int i = kHistogramMinHttpResponseCode;
+       i <= kHistogramMaxHttpResponseCode; ++i)
+    codes.push_back(i);
+  return codes;
+}
+
+int MapHttpResponseCode(int code) {
+  if (kHistogramMinHttpResponseCode <= code &&
+      code <= kHistogramMaxHttpResponseCode)
+    return code;
+  return 0;
+}
+
 }  // namespace
 
 struct HttpResponseHeaders::ParsedHeader {
@@ -103,6 +133,18 @@ struct HttpResponseHeaders::ParsedHeader {
 HttpResponseHeaders::HttpResponseHeaders(const std::string& raw_input)
     : response_code_(-1) {
   Parse(raw_input);
+
+  // The most important thing to do with this histogram is find out the
+  // existence of unusual HTTP response codes.  As it happens right now,
+  // there aren't double-constructions of response headers using this
+  // constructor, so our counts should also be accurate, without
+  // instantiating the histogram in two places.

[wtc, 2011/01/26 19:03:11]

You should also explain why the other constructors don't call this histogram.

I would not do the histogram in a HttpResponseHeaders constructor because
this side effect is not obvious, but this may be a matter of personal taste.

Nit: don't abbreviate "def'n".

[gavinp, 2011/01/27 00:04:36]

Done.

+  UMA_HISTOGRAM_CUSTOM_ENUMERATION("Net.HttpResponseCode",
+                                   MapHttpResponseCode(response_code_),
+                                   // Note the third argument is only 
+                                   // evaluated once, see macro def'n
+                                   // for details.
+                                   GetAllHttpResponseCodes());
 }
 
 HttpResponseHeaders::HttpResponseHeaders(const Pickle& pickle, void** iter)