Add histogram for HTTP response codes

net/http/http_response_headers.cc

 // Copyright (c) 2010 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.
 
 // The rules for header parsing were borrowed from Firefox:
 // http://lxr.mozilla.org/seamonkey/source/netwerk/protocol/http/src/nsHttpResponseHead.cpp
 // The rules for parsing content-types were also borrowed from Firefox:
 // http://lxr.mozilla.org/mozilla/source/netwerk/base/src/nsURLHelper.cpp#834
 
 #include "net/http/http_response_headers.h"
 
 #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"
 #include "base/time.h"
 #include "net/base/escape.h"
 #include "net/http/http_util.h"
 
 using base::Time;
 using base::TimeDelta;
 
@@ skipping 53 matching lines @@
 
 bool ShouldUpdateHeader(const std::string::const_iterator& name_begin,
                         const std::string::const_iterator& name_end) {
   for (size_t i = 0; i < arraysize(kNonUpdatedHeaders); ++i) {
     if (LowerCaseEqualsASCII(name_begin, name_end, kNonUpdatedHeaders[i]))
       return false;
   }
   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 {
   // A header "continuation" contains only a subsequent value for the
   // preceding header.  (Header values are comma separated.)
   bool is_continuation() const { return name_begin == name_end; }
 
   std::string::const_iterator name_begin;
   std::string::const_iterator name_end;
   std::string::const_iterator value_begin;
   std::string::const_iterator value_end;
 };
 
 //-----------------------------------------------------------------------------
 
 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)
     : response_code_(-1) {
   std::string raw_input;
   if (pickle.ReadString(iter, &raw_input))
     Parse(raw_input);
 }
 
 void HttpResponseHeaders::Persist(Pickle* pickle, PersistOptions options) {
@@ skipping 1091 matching lines @@
   // We have all the values; let's verify that they make sense for a 206
   // response.
   if (*first_byte_position < 0 || *last_byte_position < 0 ||
       *instance_length < 0 || *instance_length - 1 < *last_byte_position)
     return false;
 
   return true;
 }
 
 }  // namespace net