Make ClientSocketPool histograms static so that they work properly.

Also change their names so that they appear all together on the histograms
page.

BUG=43375
TEST=none

Review URL: http://codereview.chromium.org/2029004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@47843 0039d316-1c4b-4281-b951-d872f2087c98

12 files changed, 156 insertions, 58 deletions

diff --git a/net/socket/client_socket_handle.cc b/net/socket/client_socket_handle.cc
index 87212b1..5964239 100644
--- a/net/socket/client_socket_handle.cc
+++ b/net/socket/client_socket_handle.cc
@@ -9,6 +9,7 @@
 #include "base/logging.h"
 #include "net/base/net_errors.h"
 #include "net/socket/client_socket_pool.h"
+#include "net/socket/client_socket_pool_histograms.h"
 
 namespace net {
 
@@ -75,40 +76,21 @@ void ClientSocketHandle::HandleInitCompletion(int result) {
   }
   setup_time_ = base::TimeTicks::Now() - init_time_;
 
-  // TODO(vandebo): bug 43375: The strings passed to HISTOGRAM macros should NOT
-  // vary, as the macro snapshots the name into a static.  I've temporarilly
-  // made this code use statics so that the HISTOGRAM macro will not DCHECK (and
-  // this also makes the current semantics a tiny bit more clear).
-  static std::string metric = "Net." + pool_->name() + "SocketType";
-  UMA_HISTOGRAM_ENUMERATION(metric, reuse_type(), NUM_TYPES);
+  scoped_refptr<ClientSocketPoolHistograms> histograms = pool_->histograms();
+  histograms->AddSocketType(reuse_type());
   switch (reuse_type()) {
-    case ClientSocketHandle::UNUSED: {
-      static std::string metric = "Net." + pool_->name() + "SocketRequestTime";
-      UMA_HISTOGRAM_CLIPPED_TIMES(metric, setup_time(),
-                                  base::TimeDelta::FromMilliseconds(1),
-                                  base::TimeDelta::FromMinutes(10), 100);
+    case ClientSocketHandle::UNUSED:
+      histograms->AddRequestTime(setup_time());
       break;
-    }
-    case ClientSocketHandle::UNUSED_IDLE: {
-      static std::string metric = "Net." + pool_->name() +
-          "SocketIdleTimeBeforeNextUse_UnusedSocket";
-      UMA_HISTOGRAM_CUSTOM_TIMES(metric, idle_time(),
-                                 base::TimeDelta::FromMilliseconds(1),
-                                 base::TimeDelta::FromMinutes(6), 100);
+    case ClientSocketHandle::UNUSED_IDLE:
+      histograms->AddUnusedIdleTime(idle_time());
       break;
-    }
-    case ClientSocketHandle::REUSED_IDLE: {
-      static std::string metric = "Net." + pool_->name() +
-          "SocketIdleTimeBeforeNextUse_ReusedSocket";
-      UMA_HISTOGRAM_CUSTOM_TIMES(metric, idle_time(),
-                                 base::TimeDelta::FromMilliseconds(1),
-                                 base::TimeDelta::FromMinutes(6), 100);
+    case ClientSocketHandle::REUSED_IDLE:
+      histograms->AddReusedIdleTime(idle_time());
       break;
-    }
-    default: {
+    default:
       NOTREACHED();
       break;
-    }
   }
 }
 
diff --git a/net/socket/client_socket_pool.h b/net/socket/client_socket_pool.h
index fb77ac4..c58e72d 100644
--- a/net/socket/client_socket_pool.h
+++ b/net/socket/client_socket_pool.h
@@ -19,6 +19,7 @@ namespace net {
 
 class ClientSocket;
 class ClientSocketHandle;
+class ClientSocketPoolHistograms;
 
 // A ClientSocketPool is used to restrict the number of sockets open at a time.
 // It also maintains a list of idle persistent sockets.
@@ -86,8 +87,9 @@ class ClientSocketPool : public base::RefCounted<ClientSocketPool> {
   // Returns the maximum amount of time to wait before retrying a connect.
   static const int kMaxConnectRetryIntervalMs = 250;
 
-  // The name of this pool, i.e. TCP, SOCKS.
-  virtual const std::string& name() const = 0;
+  // The set of histograms specific to this pool.  We can't use the standard
+  // UMA_HISTOGRAM_* macros because they are callsite static.
+  virtual scoped_refptr<ClientSocketPoolHistograms> histograms() const = 0;
 
  protected:
   ClientSocketPool() {}
diff --git a/net/socket/client_socket_pool_base.h b/net/socket/client_socket_pool_base.h
index 89bdb78..db408a2 100644
--- a/net/socket/client_socket_pool_base.h
+++ b/net/socket/client_socket_pool_base.h
@@ -504,12 +504,12 @@ class ClientSocketPoolBase {
   ClientSocketPoolBase(
       int max_sockets,
       int max_sockets_per_group,
-      const std::string& name,
+      const scoped_refptr<ClientSocketPoolHistograms>& histograms,
       base::TimeDelta unused_idle_socket_timeout,
       base::TimeDelta used_idle_socket_timeout,
       ConnectJobFactory* connect_job_factory,
       NetworkChangeNotifier* network_change_notifier)
-      : name_(name),
+      : histograms_(histograms),
         helper_(new internal::ClientSocketPoolBaseHelper(
           max_sockets, max_sockets_per_group,
           unused_idle_socket_timeout, used_idle_socket_timeout,
@@ -576,7 +576,9 @@ class ClientSocketPoolBase {
     return helper_->ConnectionTimeout();
   }
 
-  const std::string& name() const { return name_; }
+  scoped_refptr<ClientSocketPoolHistograms> histograms() const {
+    return histograms_;
+  }
 
   void enable_backup_jobs() { helper_->enable_backup_jobs(); };
 
@@ -614,8 +616,8 @@ class ClientSocketPoolBase {
     const scoped_ptr<ConnectJobFactory> connect_job_factory_;
   };
 
-  // Name of this pool.
-  const std::string name_;
+  // Histograms for the pool
+  const scoped_refptr<ClientSocketPoolHistograms> histograms_;
 
   // One might ask why ClientSocketPoolBaseHelper is also refcounted if its
   // containing ClientSocketPool is already refcounted.  The reason is because
diff --git a/net/socket/client_socket_pool_base_unittest.cc b/net/socket/client_socket_pool_base_unittest.cc
index f21ee55..b33da2e 100644
--- a/net/socket/client_socket_pool_base_unittest.cc
+++ b/net/socket/client_socket_pool_base_unittest.cc
@@ -17,6 +17,7 @@
 #include "net/socket/client_socket.h"
 #include "net/socket/client_socket_factory.h"
 #include "net/socket/client_socket_handle.h"
+#include "net/socket/client_socket_pool_histograms.h"
 #include "net/socket/socket_test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -282,11 +283,11 @@ class TestClientSocketPool : public ClientSocketPool {
   TestClientSocketPool(
       int max_sockets,
       int max_sockets_per_group,
-      const std::string& name,
+      const scoped_refptr<ClientSocketPoolHistograms>& histograms,
       base::TimeDelta unused_idle_socket_timeout,
       base::TimeDelta used_idle_socket_timeout,
       TestClientSocketPoolBase::ConnectJobFactory* connect_job_factory)
-      : base_(max_sockets, max_sockets_per_group, name,
+      : base_(max_sockets, max_sockets_per_group, histograms,
               unused_idle_socket_timeout, used_idle_socket_timeout,
               connect_job_factory, NULL) {}
 
@@ -332,7 +333,9 @@ class TestClientSocketPool : public ClientSocketPool {
     return base_.ConnectionTimeout();
   }
 
-  virtual const std::string& name() const { return base_.name(); }
+  virtual scoped_refptr<ClientSocketPoolHistograms> histograms() const {
+    return base_.histograms();
+  }
 
   const TestClientSocketPoolBase* base() const { return &base_; }
 
@@ -400,7 +403,8 @@ class TestConnectJobDelegate : public ConnectJob::Delegate {
 
 class ClientSocketPoolBaseTest : public ClientSocketPoolTest {
  protected:
-  ClientSocketPoolBaseTest() {}
+  ClientSocketPoolBaseTest()
+      : histograms_(new ClientSocketPoolHistograms("ClientSocketPoolTest")) {}
 
   void CreatePool(int max_sockets, int max_sockets_per_group) {
     CreatePoolWithIdleTimeouts(
@@ -418,7 +422,7 @@ class ClientSocketPoolBaseTest : public ClientSocketPoolTest {
     connect_job_factory_ = new TestConnectJobFactory(&client_socket_factory_);
     pool_ = new TestClientSocketPool(max_sockets,
                                      max_sockets_per_group,
-                                     "IdleTimeoutTestPool",
+                                     histograms_,
                                      unused_idle_socket_timeout,
                                      used_idle_socket_timeout,
                                      connect_job_factory_);
@@ -451,6 +455,7 @@ class ClientSocketPoolBaseTest : public ClientSocketPoolTest {
   MockClientSocketFactory client_socket_factory_;
   TestConnectJobFactory* connect_job_factory_;
   scoped_refptr<TestClientSocketPool> pool_;
+  scoped_refptr<ClientSocketPoolHistograms> histograms_;
 };
 
 // Helper function which explicitly specifies the template parameters, since
@@ -1506,7 +1511,7 @@ TEST_F(ClientSocketPoolBaseTest, SocketLimitReleasingSockets) {
               InitHandle(req_b[i]->handle(), "b", LOWEST, req_b[i].get(), pool_,
                          BoundNetLog()));
   }
-  
+
   // Make 4 pending requests, 2 per group.
 
   for (int i = 2; i < 4; ++i) {
diff --git a/net/socket/client_socket_pool_histograms.cc b/net/socket/client_socket_pool_histograms.cc
new file mode 100644
index 0000000..769257b
--- /dev/null
+++ b/net/socket/client_socket_pool_histograms.cc
@@ -0,0 +1,56 @@
+// 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.
+
+#include "net/socket/client_socket_pool_histograms.h"
+
+#include <string>
+
+#include "base/histogram.h"
+#include "net/socket/client_socket_handle.h"
+
+namespace net {
+
+ClientSocketPoolHistograms::ClientSocketPoolHistograms(
+    const std::string& pool_name) {
+  // UMA_HISTOGRAM_ENUMERATION
+  socket_type_ = LinearHistogram::FactoryGet("Net.SocketType_" + pool_name, 1,
+      ClientSocketHandle::NUM_TYPES, ClientSocketHandle::NUM_TYPES + 1,
+      Histogram::kUmaTargetedHistogramFlag);
+  // UMA_HISTOGRAM_CUSTOM_TIMES
+  request_time_ = Histogram::FactoryGet(
+      "Net.SocketRequestTime_" + pool_name,
+      base::TimeDelta::FromMilliseconds(1),
+      base::TimeDelta::FromMinutes(10),
+      100, Histogram::kUmaTargetedHistogramFlag);
+  // UMA_HISTOGRAM_CUSTOM_TIMES
+  unused_idle_time_ = Histogram::FactoryGet(
+      "Net.SocketIdleTimeBeforeNextUse_UnusedSocket_" + pool_name,
+      base::TimeDelta::FromMilliseconds(1),
+      base::TimeDelta::FromMinutes(6),
+      100, Histogram::kUmaTargetedHistogramFlag);
+  // UMA_HISTOGRAM_CUSTOM_TIMES
+  reused_idle_time_ = Histogram::FactoryGet(
+      "Net.SocketIdleTimeBeforeNextUse_ReusedSocket_" + pool_name,
+      base::TimeDelta::FromMilliseconds(1),
+      base::TimeDelta::FromMinutes(6),
+      100, Histogram::kUmaTargetedHistogramFlag);
+}
+
+void ClientSocketPoolHistograms::AddSocketType(int type) const {
+  socket_type_->Add(type);
+}
+
+void ClientSocketPoolHistograms::AddRequestTime(base::TimeDelta time) const {
+  request_time_->AddTime(time);
+}
+
+void ClientSocketPoolHistograms::AddUnusedIdleTime(base::TimeDelta time) const {
+  unused_idle_time_->AddTime(time);
+}
+
+void ClientSocketPoolHistograms::AddReusedIdleTime(base::TimeDelta time) const {
+  reused_idle_time_->AddTime(time);
+}
+
+}  // namespace net
diff --git a/net/socket/client_socket_pool_histograms.h b/net/socket/client_socket_pool_histograms.h
new file mode 100644
index 0000000..1aea112
--- /dev/null
+++ b/net/socket/client_socket_pool_histograms.h
@@ -0,0 +1,37 @@
+// 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.
+
+#ifndef NET_SOCKET_CLIENT_SOCKET_POOL_HISTOGRAMS_H_
+#define NET_SOCKET_CLIENT_SOCKET_POOL_HISTOGRAMS_H_
+
+#include <string>
+
+#include "base/histogram.h"
+#include "base/ref_counted.h"
+
+namespace net {
+
+class ClientSocketPoolHistograms
+    : public base::RefCounted<ClientSocketPoolHistograms> {
+ public:
+  ClientSocketPoolHistograms(const std::string& pool_name);
+
+  void AddSocketType(int socket_reuse_type) const;
+  void AddRequestTime(base::TimeDelta time) const;
+  void AddUnusedIdleTime(base::TimeDelta time) const;
+  void AddReusedIdleTime(base::TimeDelta time) const;
+
+ private:
+  friend class base::RefCounted<ClientSocketPoolHistograms>;
+  ~ClientSocketPoolHistograms() {}
+
+  scoped_refptr<Histogram> socket_type_;
+  scoped_refptr<Histogram> request_time_;
+  scoped_refptr<Histogram> unused_idle_time_;
+  scoped_refptr<Histogram> reused_idle_time_;
+};
+
+}  // namespace net
+
+#endif  // NET_SOCKET_CLIENT_SOCKET_POOL_HISTOGRAMS_H_
diff --git a/net/socket/socks_client_socket_pool.cc b/net/socket/socks_client_socket_pool.cc
index b98f9fd..452ce25 100644
--- a/net/socket/socks_client_socket_pool.cc
+++ b/net/socket/socks_client_socket_pool.cc
@@ -159,11 +159,11 @@ SOCKSClientSocketPool::SOCKSConnectJobFactory::ConnectionTimeout() const {
 SOCKSClientSocketPool::SOCKSClientSocketPool(
     int max_sockets,
     int max_sockets_per_group,
-    const std::string& name,
+    const scoped_refptr<ClientSocketPoolHistograms>& histograms,
     const scoped_refptr<HostResolver>& host_resolver,
     const scoped_refptr<TCPClientSocketPool>& tcp_pool,
     NetworkChangeNotifier* network_change_notifier)
-    : base_(max_sockets, max_sockets_per_group, name,
+    : base_(max_sockets, max_sockets_per_group, histograms,
             base::TimeDelta::FromSeconds(kUnusedIdleSocketTimeout),
             base::TimeDelta::FromSeconds(kUsedIdleSocketTimeout),
             new SOCKSConnectJobFactory(tcp_pool, host_resolver),
diff --git a/net/socket/socks_client_socket_pool.h b/net/socket/socks_client_socket_pool.h
index a786dc1..ea162245 100644
--- a/net/socket/socks_client_socket_pool.h
+++ b/net/socket/socks_client_socket_pool.h
@@ -15,6 +15,7 @@
 #include "net/base/host_resolver.h"
 #include "net/proxy/proxy_server.h"
 #include "net/socket/client_socket_pool_base.h"
+#include "net/socket/client_socket_pool_histograms.h"
 #include "net/socket/client_socket_pool.h"
 #include "net/socket/tcp_client_socket_pool.h"
 
@@ -107,7 +108,7 @@ class SOCKSClientSocketPool : public ClientSocketPool {
   SOCKSClientSocketPool(
       int max_sockets,
       int max_sockets_per_group,
-      const std::string& name,
+      const scoped_refptr<ClientSocketPoolHistograms>& histograms,
       const scoped_refptr<HostResolver>& host_resolver,
       const scoped_refptr<TCPClientSocketPool>& tcp_pool,
       NetworkChangeNotifier* network_change_notifier);
@@ -141,7 +142,9 @@ class SOCKSClientSocketPool : public ClientSocketPool {
     return base_.ConnectionTimeout();
   }
 
-  virtual const std::string& name() const { return base_.name(); };
+  virtual scoped_refptr<ClientSocketPoolHistograms> histograms() const {
+    return base_.histograms();
+  };
 
  protected:
   virtual ~SOCKSClientSocketPool();
diff --git a/net/socket/socks_client_socket_pool_unittest.cc b/net/socket/socks_client_socket_pool_unittest.cc
index 6072ad3..27cb027 100644
--- a/net/socket/socks_client_socket_pool_unittest.cc
+++ b/net/socket/socks_client_socket_pool_unittest.cc
@@ -15,6 +15,7 @@
 #include "net/base/test_completion_callback.h"
 #include "net/socket/client_socket_factory.h"
 #include "net/socket/client_socket_handle.h"
+#include "net/socket/client_socket_pool_histograms.h"
 #include "net/socket/socket_test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -82,10 +83,10 @@ class MockTCPClientSocketPool : public TCPClientSocketPool {
   };
 
   MockTCPClientSocketPool(int max_sockets, int max_sockets_per_group,
-                          const std::string& name,
-                          ClientSocketFactory* socket_factory,
-                          NetworkChangeNotifier* network_change_notifier)
-      : TCPClientSocketPool(max_sockets, max_sockets_per_group, name,
+      const scoped_refptr<ClientSocketPoolHistograms>& histograms,
+      ClientSocketFactory* socket_factory,
+      NetworkChangeNotifier* network_change_notifier)
+      : TCPClientSocketPool(max_sockets, max_sockets_per_group, histograms,
                             NULL, NULL, network_change_notifier),
         client_socket_factory_(socket_factory),
         release_count_(0),
@@ -169,15 +170,17 @@ class SOCKSClientSocketPoolTest : public ClientSocketPoolTest {
   SOCKSClientSocketPoolTest()
       : ignored_tcp_socket_params_(
             HostPortPair("proxy", 80), MEDIUM, GURL(), false),
+        tcp_histograms_(new ClientSocketPoolHistograms("MockTCP")),
         tcp_socket_pool_(new MockTCPClientSocketPool(
-            kMaxSockets, kMaxSocketsPerGroup, "MockTCP",
+            kMaxSockets, kMaxSocketsPerGroup, tcp_histograms_,
             &tcp_client_socket_factory_, &tcp_notifier_)),
         ignored_socket_params_(ignored_tcp_socket_params_, true,
                                HostPortPair("host", 80),
                                MEDIUM, GURL()),
+        socks_histograms_(new ClientSocketPoolHistograms("SOCKSUnitTest")),
         pool_(new SOCKSClientSocketPool(
-            kMaxSockets, kMaxSocketsPerGroup, "SOCKSUnitTest", NULL,
-            tcp_socket_pool_.get(), &socks_notifier_)) {
+            kMaxSockets, kMaxSocketsPerGroup, socks_histograms_, NULL,
+            tcp_socket_pool_, &socks_notifier_)) {
   }
 
   int StartRequest(const std::string& group_name, RequestPriority priority) {
@@ -186,11 +189,13 @@ class SOCKSClientSocketPoolTest : public ClientSocketPoolTest {
   }
 
   TCPSocketParams ignored_tcp_socket_params_;
+  scoped_refptr<ClientSocketPoolHistograms> tcp_histograms_;
   MockClientSocketFactory tcp_client_socket_factory_;
   MockNetworkChangeNotifier tcp_notifier_;
   scoped_refptr<MockTCPClientSocketPool> tcp_socket_pool_;
 
   SOCKSSocketParams ignored_socket_params_;
+  scoped_refptr<ClientSocketPoolHistograms> socks_histograms_;
   MockNetworkChangeNotifier socks_notifier_;
   scoped_refptr<SOCKSClientSocketPool> pool_;
 };
diff --git a/net/socket/tcp_client_socket_pool.cc b/net/socket/tcp_client_socket_pool.cc
index 74841cf..833b36e 100644
--- a/net/socket/tcp_client_socket_pool.cc
+++ b/net/socket/tcp_client_socket_pool.cc
@@ -175,11 +175,11 @@ base::TimeDelta
 TCPClientSocketPool::TCPClientSocketPool(
     int max_sockets,
     int max_sockets_per_group,
-    const std::string& name,
+    const scoped_refptr<ClientSocketPoolHistograms>& histograms,
     HostResolver* host_resolver,
     ClientSocketFactory* client_socket_factory,
     NetworkChangeNotifier* network_change_notifier)
-    : base_(max_sockets, max_sockets_per_group, name,
+    : base_(max_sockets, max_sockets_per_group, histograms,
             base::TimeDelta::FromSeconds(kUnusedIdleSocketTimeout),
             base::TimeDelta::FromSeconds(kUsedIdleSocketTimeout),
             new TCPConnectJobFactory(client_socket_factory, host_resolver),
diff --git a/net/socket/tcp_client_socket_pool.h b/net/socket/tcp_client_socket_pool.h
index 30158f6..355bd2a 100644
--- a/net/socket/tcp_client_socket_pool.h
+++ b/net/socket/tcp_client_socket_pool.h
@@ -15,6 +15,7 @@
 #include "net/base/host_port_pair.h"
 #include "net/base/host_resolver.h"
 #include "net/socket/client_socket_pool_base.h"
+#include "net/socket/client_socket_pool_histograms.h"
 #include "net/socket/client_socket_pool.h"
 
 namespace net {
@@ -114,7 +115,7 @@ class TCPClientSocketPool : public ClientSocketPool {
   TCPClientSocketPool(
       int max_sockets,
       int max_sockets_per_group,
-      const std::string& name,
+      const scoped_refptr<ClientSocketPoolHistograms>& histograms,
       HostResolver* host_resolver,
       ClientSocketFactory* client_socket_factory,
       NetworkChangeNotifier* network_change_notifier);
@@ -149,7 +150,9 @@ class TCPClientSocketPool : public ClientSocketPool {
     return base_.ConnectionTimeout();
   }
 
-  virtual const std::string& name() const { return base_.name(); }
+  virtual scoped_refptr<ClientSocketPoolHistograms> histograms() const {
+    return base_.histograms();
+  }
 
  protected:
   virtual ~TCPClientSocketPool();
diff --git a/net/socket/tcp_client_socket_pool_unittest.cc b/net/socket/tcp_client_socket_pool_unittest.cc
index e6d435a..e0dc09f 100644
--- a/net/socket/tcp_client_socket_pool_unittest.cc
+++ b/net/socket/tcp_client_socket_pool_unittest.cc
@@ -14,6 +14,7 @@
 #include "net/socket/client_socket.h"
 #include "net/socket/client_socket_factory.h"
 #include "net/socket/client_socket_handle.h"
+#include "net/socket/client_socket_pool_histograms.h"
 #include "net/socket/socket_test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -264,10 +265,11 @@ class TCPClientSocketPoolTest : public ClientSocketPoolTest {
   TCPClientSocketPoolTest()
       : ignored_socket_params_(
             HostPortPair("ignored", 80), MEDIUM, GURL(), false),
+        histograms_(new ClientSocketPoolHistograms("TCPUnitTest")),
         host_resolver_(new MockHostResolver),
         pool_(new TCPClientSocketPool(kMaxSockets,
                                       kMaxSocketsPerGroup,
-                                      "TCPUnitTest",
+                                      histograms_,
                                       host_resolver_,
                                       &client_socket_factory_,
                                       &notifier_)) {
@@ -279,6 +281,7 @@ class TCPClientSocketPoolTest : public ClientSocketPoolTest {
   }
 
   TCPSocketParams ignored_socket_params_;
+  scoped_refptr<ClientSocketPoolHistograms> histograms_;
   scoped_refptr<MockHostResolver> host_resolver_;
   MockClientSocketFactory client_socket_factory_;
   MockNetworkChangeNotifier notifier_;
@@ -740,7 +743,7 @@ TEST_F(TCPClientSocketPoolTest, BackupSocketConnect) {
 
     pool_ = new TCPClientSocketPool(kMaxSockets,
                                     kMaxSocketsPerGroup,
-                                    "TCPUnitTest",
+                                    histograms_,
                                     host_resolver_,
                                     &client_socket_factory_,
                                     NULL);