Add instrumentation to track the IceEndpointType.

talk/app/webrtc/webrtcsession.cc

Index: talk/app/webrtc/webrtcsession.cc
diff --git a/talk/app/webrtc/webrtcsession.cc b/talk/app/webrtc/webrtcsession.cc
index 2533328968945e4c2c67a2694e95ae8d19c288ea..4416706a906159925c0eda392c2a0480be47fc84 100644
--- a/talk/app/webrtc/webrtcsession.cc
+++ b/talk/app/webrtc/webrtcsession.cc
@@ -57,6 +57,12 @@ using cricket::MediaContentDescription;
 using cricket::SessionDescription;
 using cricket::TransportInfo;
 
+using cricket::LOCAL_PORT_TYPE;

[pthatcher1, 2015/08/18 18:06:23]

I think you could do 

using HOST = cricket::LOCAL_PORT_TYPE;
using SRFLX = cricket::STUN_PORT_TYPE;
using RELAY = cricket::RELAY_PORT_TYPE;
using PRFLX = cricket::PRFLX_PORT_TYPE;

And that might save you some ugliness in the macro.

[guoweis_webrtc, 2015/08/18 22:33:57]

doesn't alias only work for type? 

[pthatcher1, 2015/08/19 02:47:52]

Yeah, you're right.  I thought it was more powerful than that in c++11.  I was
wrong.

+using cricket::STUN_PORT_TYPE;
+using cricket::RELAY_PORT_TYPE;
+using cricket::PRFLX_PORT_TYPE;
+using webrtc::IceCandidatePairType;
+
 namespace webrtc {
 
 // Error messages
@@ -83,6 +89,33 @@ const char kDtlsSetupFailureRtcp[] =
     "Couldn't set up DTLS-SRTP on RTCP channel.";
 const int kMaxUnsignalledRecvStreams = 20;
 
+#define CANDIDATE_PAIR_TYPE_ENTRY(local_type, remote_type, endpoint_type) \
+  if (local.type() == local_type##_PORT_TYPE &&                           \
+      remote.type() == remote_type##_PORT_TYPE) {                         \
+    return endpoint_type;                                                 \
+  }
+
+IceCandidatePairType GetIceCandidatePairCounter(
+    const cricket::Candidate& local,
+    const cricket::Candidate& remote) {
+  CANDIDATE_PAIR_TYPE_ENTRY(LOCAL, LOCAL, kIceCandidatePair_Host_Host);

[pthatcher1, 2015/08/18 18:06:22]

I think removing the macro and just doing this might be better:

auto l = local.type();
auto r = remote.type();

if (l == HOST && r == HOST) return kIceCandidatePair_Host_Host;
if (l == HOST && r == RELAY) return kIceCandidatePair_Host_Relay;
...

+  CANDIDATE_PAIR_TYPE_ENTRY(LOCAL, STUN, kIceCandidatePair_Host_Srflx);
+  CANDIDATE_PAIR_TYPE_ENTRY(LOCAL, RELAY, kIceCandidatePair_Host_Relay);
+  CANDIDATE_PAIR_TYPE_ENTRY(LOCAL, PRFLX, kIceCandidatePair_Host_Prflx);
+  CANDIDATE_PAIR_TYPE_ENTRY(STUN, LOCAL, kIceCandidatePair_Srflx_Host);
+  CANDIDATE_PAIR_TYPE_ENTRY(STUN, STUN, kIceCandidatePair_Srflx_Srflx);
+  CANDIDATE_PAIR_TYPE_ENTRY(STUN, RELAY, kIceCandidatePair_Srflx_Relay);
+  CANDIDATE_PAIR_TYPE_ENTRY(STUN, PRFLX, kIceCandidatePair_Srflx_Prflx);
+  CANDIDATE_PAIR_TYPE_ENTRY(RELAY, LOCAL, kIceCandidatePair_Relay_Host);
+  CANDIDATE_PAIR_TYPE_ENTRY(RELAY, STUN, kIceCandidatePair_Relay_Srflx);
+  CANDIDATE_PAIR_TYPE_ENTRY(RELAY, RELAY, kIceCandidatePair_Relay_Relay);
+  CANDIDATE_PAIR_TYPE_ENTRY(RELAY, PRFLX, kIceCandidatePair_Relay_Prflx);
+  CANDIDATE_PAIR_TYPE_ENTRY(PRFLX, LOCAL, kIceCandidatePair_Prflx_Host);
+  CANDIDATE_PAIR_TYPE_ENTRY(PRFLX, STUN, kIceCandidatePair_Prflx_Srflx);
+  CANDIDATE_PAIR_TYPE_ENTRY(PRFLX, RELAY, kIceCandidatePair_Prflx_Relay);
+  return kIceCandidatePair_Max;
+}
+
 // Compares |answer| against |offer|. Comparision is done
 // for number of m-lines in answer against offer. If matches true will be
 // returned otherwise false.
@@ -1916,14 +1949,45 @@ void WebRtcSession::ReportBestConnectionState(
       if (!it_info->best_connection) {
         continue;
       }
+
+      PeerConnectionEnumCounterType type = kPeerConnectionEnumCounter_Max;
+      int counter = -1;
+
+      // Increment the counter for IceCandidatePairType.

[pthatcher1, 2015/08/18 18:06:23]

Having an

const Candidate& local = it_info->local_candidate();

Would make the rest a lot more readable.

[guoweis_webrtc, 2015/08/18 22:33:57]

Done.

+      if (it_info->local_candidate.protocol() == cricket::TCP_PROTOCOL_NAME ||

[pthatcher1, 2015/08/18 18:06:22]

Might as well add a "using TCP = cricket::TCP_PROTOCOL_NAME" as well.

+          (it_info->local_candidate.type() == RELAY_PORT_TYPE &&
+           it_info->local_candidate.relay_protocol() ==
+               cricket::TCP_PROTOCOL_NAME)) {
+        type = kPeerConnectionEnumCounter_IceCandidatePairTypeTcp;
+      } else if (it_info->local_candidate.protocol() ==
+                 cricket::UDP_PROTOCOL_NAME) {

[pthatcher1, 2015/08/18 18:06:23]

And "using UDP = circket::UDP_PROTOCOL_NAME"

+        type = kPeerConnectionEnumCounter_IceCandidatePairTypeUdp;
+      } else {
+        DCHECK(0);

[pthatcher1, 2015/08/18 18:06:22]

Should we record a kPeerConnectionEnumCounter_Unknown?  That way we'd know if
this is happening in production (even though it shouldn't and the number should
be 0, at least we could verify).  

[guoweis_webrtc, 2015/08/18 22:33:57]

This is too specific and I don't think UMA is designed for that.

[pthatcher1, 2015/08/19 02:47:52]

Sorry, I meant kPeerConnectionEnumCounter_IceCandidatePairTypeUnknown.

Counters for UDP/TCP/Unknown

[guoweis_webrtc, 2015/08/19 18:25:52]

What's the goal here? To catch a bug? I don't think UMA is designed to catch
bugs like this - see the content in histograms.xml.

[pthatcher2, 2015/08/19 18:39:41]

The goal was to measure how often we screwed up the measurement and hit that
DCHECK(0) in production.  Because release builds will just silently eat that. 
If you're sure that will never happen, you don't need the DCHECK.  If you're not
sure, then you might want to know how often it happens in production.  

+      }
+      metrics_observer_->IncrementEnumCounter(
+          type, GetIceCandidatePairCounter(it_info->local_candidate,
+                                           it_info->remote_candidate),
+          kIceCandidatePair_Max);
+
+      // Increment the counter for IP type.
       if (it_info->local_candidate.address().family() == AF_INET) {
+        counter = kBestConnections_IPv4;
+        // TODO(guoweis): Remove this once IncrementEnumCounter implemented for
+        // PeerConnectionMetrics.
         metrics_observer_->IncrementCounter(kBestConnections_IPv4);
       } else if (it_info->local_candidate.address().family() ==
                  AF_INET6) {
+        counter = kBestConnections_IPv6;
+        // TODO(guoweis): Remove this.
         metrics_observer_->IncrementCounter(kBestConnections_IPv6);
       } else {
         RTC_NOTREACHED();
       }
+      metrics_observer_->IncrementEnumCounter(
+          kPeerConnectionEnumCounter_AddressFamily, counter,
+          kPeerConnectionAddressFamilyCounter_Max);

[pthatcher1, 2015/08/18 18:06:23]

Instead of setting counter in two places and then calling
metrics_observer_->IncrementEnumCounter once, can you just call
metrics_observer_->IncrementEnumCounter in two places?  I think that would be
more readable.

[guoweis_webrtc, 2015/08/18 22:33:57]

Done.

+
       return;
     }
   }