Add histograms to NetworkChangeNotifier to identify ways to improve interface

net/base/network_change_notifier.cc

Index: net/base/network_change_notifier.cc
diff --git a/net/base/network_change_notifier.cc b/net/base/network_change_notifier.cc
index 8c5f4461d96b66189d8de1c1a95fa7ac002830a6..0bee27b6f5dfc4255b8aaf9e9db2b5253c4f8ef0 100644
--- a/net/base/network_change_notifier.cc
+++ b/net/base/network_change_notifier.cc
@@ -4,7 +4,10 @@
 
 #include "net/base/network_change_notifier.h"
 #include "net/base/network_change_notifier_factory.h"
+#include "base/metrics/histogram.h"
 #include "build/build_config.h"
+#include "googleurl/src/gurl.h"
+#include "net/base/net_util.h"
 #if defined(OS_WIN)
 #include "net/base/network_change_notifier_win.h"
 #elif defined(OS_LINUX) && !defined(OS_CHROMEOS)
@@ -35,6 +38,111 @@ class MockNetworkChangeNotifier : public NetworkChangeNotifier {
 
 }  // namespace
 
+// The main observer class that records UMAs for network events.
+class HistogramWatcher
+    : public NetworkChangeNotifier::ConnectionTypeObserver,
+      public NetworkChangeNotifier::IPAddressObserver,
+      public NetworkChangeNotifier::DNSObserver {
+ public:
+  HistogramWatcher()
+      : last_ip_address_change_(base::TimeTicks::Now()),
+        last_connection_change_(base::TimeTicks::Now()),
+        last_dns_change_(base::TimeTicks::Now()),
+        last_connection_type_(NetworkChangeNotifier::CONNECTION_UNKNOWN),
+        offline_packets_received_(0) {}
+
+  // Registers our three Observer implementations.  This is called from the
+  // network thread so that our Observer implementations are also called
+  // from the network thread.  This avoids multi-threaded race conditions
+  // because the only other interface, |NotifyDataReceived| is also
+  // only called from the network thread.
+  void InitHistogramWatcher();

[szym, 2012/08/08 20:41:17]

You could use base::NonThreadSafe to enforce the thread requirements. Just call
DetachFromThread in ~HistogramWatcher to avoid its complaints about destruction
order.

[pauljensen, 2012/08/13 15:42:04]

As we discussed this would add so much complexity as to outweigh the benefit of
the checks.

+
+  virtual ~HistogramWatcher() {}
+
+  // NetworkChangeNotifier::IPAddressObserver implementation.
+  virtual void OnIPAddressChanged() OVERRIDE {
+    UMA_HISTOGRAM_MEDIUM_TIMES("NCN.IPAddressChange",
+                               SinceLast(&last_ip_address_change_));

[szym, 2012/08/08 20:41:17]

I'm a bit uneasy about using a function call with side effects as a macro
parameter.

[pauljensen, 2012/08/13 15:42:04]

I don't like it either.  I wish the histograms were done without macros.  The
histogram macros seem to be done right (they're so widespread I'd hope so)
though and only evaluate the sample argument once.

+  }
+
+  // NetworkChangeNotifier::ConnectionTypeObserver implementation.
+  virtual void OnConnectionTypeChanged(
+      NetworkChangeNotifier::ConnectionType type) OVERRIDE {
+    if (type != NetworkChangeNotifier::CONNECTION_NONE) {
+      UMA_HISTOGRAM_MEDIUM_TIMES("NCN.OnlineChange",
+                                 SinceLast(&last_connection_change_));
+
+      if (offline_packets_received_) {
+        if ((last_connection_change_ - last_offline_packet_received_) <
+            base::TimeDelta::FromSeconds(5)) {
+          // we can compare this sum with the sum of NCN.OfflineDataRecv

[szym, 2012/08/08 20:41:17]

nit: Format like a sentence, but maybe just remove.

[pauljensen, 2012/08/13 15:42:04]

Done.

+          UMA_HISTOGRAM_COUNTS_10000(
+              "NCN.OfflineDataRecvAny5sBeforeOnline",
+              offline_packets_received_);
+        }
+
+        UMA_HISTOGRAM_MEDIUM_TIMES("NCN.OfflineDataRecvUntilOnline",
+                                   last_connection_change_ -
+                                       last_offline_packet_received_);
+      }
+    } else {
+      UMA_HISTOGRAM_MEDIUM_TIMES("NCN.OfflineChange",
+                                 SinceLast(&last_connection_change_));
+    }
+
+    offline_packets_received_ = 0;
+    last_connection_type_ = type;
+  }
+
+  // NetworkChangeNotifier::DNSObserver implementation.
+  virtual void OnDNSChanged(unsigned detail) OVERRIDE {
+    if (detail == NetworkChangeNotifier::CHANGE_DNS_SETTINGS) {
+      UMA_HISTOGRAM_MEDIUM_TIMES("NCN.DNSConfigChange",
+                                 SinceLast(&last_dns_change_));
+    }
+  }
+
+  // Record histogram data whenever we receive a packet but think we're
+  // offline.  Should only be called from the network thread.
+  void NotifyDataReceived(const GURL& source) {
+    if (last_connection_type_ != NetworkChangeNotifier::CONNECTION_NONE ||
+        IsLocalhost(source.host()) ||
+        !(source.SchemeIs("http") || source.SchemeIs("https")))

[szym, 2012/08/08 20:41:17]

nit: This might need braces.

[pauljensen, 2012/08/13 15:42:04]

Done.

+      return;
+    base::TimeTicks current_time = base::TimeTicks::Now();
+    UMA_HISTOGRAM_MEDIUM_TIMES("NCN.OfflineDataRecv",
+                               current_time - last_connection_change_);
+    offline_packets_received_++;
+    last_offline_packet_received_ = current_time;
+  }
+
+ private:
+  base::TimeTicks last_ip_address_change_;
+  base::TimeTicks last_connection_change_;
+  base::TimeTicks last_dns_change_;
+  base::TimeTicks last_offline_packet_received_;
+  NetworkChangeNotifier::ConnectionType last_connection_type_;
+  int32 offline_packets_received_;
+
+  static base::TimeDelta SinceLast(base::TimeTicks *last_time);

[szym, 2012/08/08 20:41:17]

Method declarations should precede field declarations, but 
this could also be put in the unnamed namespace above it all.

[pauljensen, 2012/08/13 15:42:04]

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(HistogramWatcher);
+};
+
+base::TimeDelta HistogramWatcher::SinceLast(base::TimeTicks *last_time) {

[szym, 2012/08/08 20:41:17]

Why is this implemented here rather than inline as everything else?

[pauljensen, 2012/08/13 15:42:04]

Done.

+  base::TimeTicks current_time = base::TimeTicks::Now();
+  base::TimeDelta delta = current_time - *last_time;
+  *last_time = current_time;
+  return delta;
+}
+
+void HistogramWatcher::InitHistogramWatcher() {

[szym, 2012/08/08 20:41:17]

ditto: why implemented out-of-line?

[pauljensen, 2012/08/13 15:42:04]

Done.

+  NetworkChangeNotifier::AddConnectionTypeObserver(this);
+  NetworkChangeNotifier::AddIPAddressObserver(this);
+  NetworkChangeNotifier::AddDNSObserver(this);
+}
+
 NetworkChangeNotifier::~NetworkChangeNotifier() {
   DCHECK_EQ(this, g_network_change_notifier);
   g_network_change_notifier = NULL;
@@ -84,6 +192,20 @@ NetworkChangeNotifier::GetConnectionType() {
       CONNECTION_UNKNOWN;
 }
 
+// static
+void NetworkChangeNotifier::NotifyDataReceived(const GURL& source) {
+  if (!g_network_change_notifier)
+    return;
+  g_network_change_notifier->histogram_watcher_->NotifyDataReceived(source);
+}
+
+// static
+void NetworkChangeNotifier::InitHistogramWatcher() {
+  if (!g_network_change_notifier)
+    return;
+  g_network_change_notifier->histogram_watcher_->InitHistogramWatcher();
+}
+
 #if defined(OS_LINUX)
 // static
 const internal::AddressTrackerLinux*
@@ -162,6 +284,7 @@ NetworkChangeNotifier::NetworkChangeNotifier()
       watching_dns_(false) {
   DCHECK(!g_network_change_notifier);
   g_network_change_notifier = this;
+  histogram_watcher_.reset(new HistogramWatcher());

[szym, 2012/08/08 20:41:17]

nit: Put this in the initializer list.

[pauljensen, 2012/08/13 15:42:04]

It must appear after |g_network_change_notifier = this;| which unfortunately
cannot be moved to the initializer list without removing the
|DCHECK(!g_network_change_notifier)| line which does server a purpose.

 }
 
 #if defined(OS_LINUX)