Add UMA for the kernel's TCP RTT estimate.

net/socket/tcp_client_socket.cc

Index: net/socket/tcp_client_socket.cc
diff --git a/net/socket/tcp_client_socket.cc b/net/socket/tcp_client_socket.cc
index a1c1fadb85e2b8ba0d01c600c760536f7e206eeb..6ab5da7e7ed29c46303fdbec69cb7639cf684fce 100644
--- a/net/socket/tcp_client_socket.cc
+++ b/net/socket/tcp_client_socket.cc
@@ -6,7 +6,9 @@
 
 #include "base/callback_helpers.h"
 #include "base/logging.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/profiler/scoped_tracker.h"
+#include "base/time/time.h"
 #include "net/base/io_buffer.h"
 #include "net/base/ip_endpoint.h"
 #include "net/base/net_errors.h"
@@ -38,6 +40,7 @@ TCPClientSocket::TCPClientSocket(scoped_ptr<TCPSocket> connected_socket,
 }
 
 TCPClientSocket::~TCPClientSocket() {
+  EmitTCPMetricsHistogramsOnSocketClose();
 }
 
 int TCPClientSocket::Bind(const IPEndPoint& address) {
@@ -178,6 +181,7 @@ int TCPClientSocket::DoConnectComplete(int result) {
 }
 
 void TCPClientSocket::Disconnect() {
+  EmitTCPMetricsHistogramsOnSocketClose();

[mmenke, 2015/07/07 19:53:49]

Suggest a couple things:

1) Rename this to EmitTCPMetricsHistograms (The "socket" is implied, and it's
not clear what "close" means).
2) Move this to DoDisconnect.  We probably won't get anything in the other
disconnect case, but the Disconnect / DoDisconnect split makes me very nervous
about regressions.
3) Call Disconnection on destruction.  We'll be doing a little extra work, but
it looks pretty negligible to me.

[Bryan McQuade, 2015/07/07 21:03:32]

I changed to EmitTCPMetricsHistogramsOnDisconnect. I think it's important to
capture in the signature that the metric being recorded in the current
implementation only want to be recorded at disconnect time (the metric is named
Net.TcpRtt.AtDisconnect). Happy to remove the 'OnDisconnect' part of the method
name but I worry that it makes it easier for someone modifying this code in the
future to start calling the method at other locations, and inadvertently logging
Net.TcpRtt.AtDisconnect at places other than disconnection.
Done. I noticed that DoDisconnect also gets called from
TCPClientSocket::DoConnectComplete with the comment " Close whatever partially
connected socket we currently have.". I wasn't sure if it was desirable to try
to emit histograms for such a socket. What do you think?
Done. Note that Disconnect is virtual, so if TCPClientSocket is subclassed and
Disconnect overridden, the overridden implementation won't get called when the
derived class destructs (unless the derived class also calls Disconnect in its
destructor...). Not sure if this is a concern or not. I could also call the
non-virtual DoDisconnect in the destructor. Let me know which you prefer.

   DoDisconnect();
   current_address_index_ = -1;
   bind_address_.reset();
@@ -350,4 +354,16 @@ int TCPClientSocket::OpenSocket(AddressFamily family) {
   return OK;
 }
 
+void TCPClientSocket::EmitTCPMetricsHistogramsOnSocketClose() {
+  if (previously_disconnected_) {

[Alexei Svitkine (slow), 2015/07/07 18:50:56]

Nit: No {}'s for 1-line bodies.

[Bryan McQuade, 2015/07/07 19:35:13]

Done.

[mmenke, 2015/07/07 19:53:49]

Is this needed?  Seems weird to exclude the second value it a socket was
previously disconnected and then reused (Something I'd rather not allow, but we
do).

I think what you're concerned about is a socket being disconnected and then us
gathering measurements again in the destructor, but GetEstimatedRoundTripTime()
already returns false in that case (Since socket_->Close() will have been
called, which nulls out the...erm...socket_'s socket_).

[Bryan McQuade, 2015/07/07 21:03:32]

Removed.

+    return;
+  }
+  base::TimeDelta rtt;
+  if (socket_ && socket_->GetEstimatedRoundTripTime(&rtt)) {

[mmenke, 2015/07/07 19:53:49]

socket_ can't be NULL (It's set to a non-NULL value on construction, and never
reset).

[Bryan McQuade, 2015/07/07 21:03:32]

Done.

+    UMA_HISTOGRAM_CUSTOM_TIMES("Net.TcpRttAtSocketClose", rtt,

[Alexei Svitkine (slow), 2015/07/07 18:50:56]

Nit: Optional suggestion:

Rename to Net.TcpRtt.AtSocketClose.

That way, if you want to log this value at other intervals, it's easy to add
another suffix while keeping the names more readable.

(Change the xml name too if you make this change.)

[Bryan McQuade, 2015/07/07 19:35:13]

Great suggestion, thanks! Done.

+                               base::TimeDelta::FromMilliseconds(1),
+                               base::TimeDelta::FromMinutes(10), 100);

[Alexei Svitkine (slow), 2015/07/07 18:50:56]

Nit: Why do you need 100 buckets instead of the usual 50?

In fact, maybe the default UMA_HISTOGRAM_MEDIUM_TIMES() could work here? The
difference is it has a max of 3mins instead of 10. Do you care much about the
buckets between 3mins and 10mins?

[Bryan McQuade, 2015/07/07 19:35:13]

I chose this bucketing strategy to match the similar Net.TCP_Connection_Latency
histogram. We'll want to compare the histograms so I thought it best to use the
same bucketing strategy.

https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/transpo...

+  }
+}
+
 }  // namespace net