Add instrumentation to track the IceEndpointType.

talk/app/webrtc/webrtcsession.cc

 /*
  * libjingle
  * Copyright 2012 Google Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are met:
  *
  *  1. Redistributions of source code must retain the above copyright notice,
  *     this list of conditions and the following disclaimer.
  *  2. Redistributions in binary form must reproduce the above copyright notice,
@@ skipping 39 matching lines @@
 #include "webrtc/base/stringencode.h"
 #include "webrtc/base/stringutils.h"
 #include "webrtc/p2p/base/portallocator.h"
 
 using cricket::ContentInfo;
 using cricket::ContentInfos;
 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
 const char kBundleWithoutRtcpMux[] = "RTCP-MUX must be enabled when BUNDLE "
                                      "is enabled.";
 const char kCreateChannelFailed[] = "Failed to create channels.";
 const char kInvalidCandidates[] = "Description contains invalid candidates.";
 const char kInvalidSdp[] = "Invalid session description.";
 const char kMlineMismatch[] =
     "Offer and answer descriptions m-lines are not matching. Rejecting answer.";
 const char kPushDownTDFailed[] =
     "Failed to push down transport description:";
 const char kSdpWithoutDtlsFingerprint[] =
     "Called with SDP without DTLS fingerprint.";
 const char kSdpWithoutSdesCrypto[] =
     "Called with SDP without SDES crypto.";
 const char kSdpWithoutIceUfragPwd[] =
     "Called with SDP without ice-ufrag and ice-pwd.";
 const char kSessionError[] = "Session error code: ";
 const char kSessionErrorDesc[] = "Session error description: ";
 const char kDtlsSetupFailureRtp[] =
     "Couldn't set up DTLS-SRTP on RTP channel.";
 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.
 static bool VerifyMediaDescriptions(
     const SessionDescription* answer, const SessionDescription* offer) {
   if (offer->contents().size() != answer->contents().size())
     return false;
 
   for (size_t i = 0; i < offer->contents().size(); ++i) {
     if ((offer->contents()[i].name) != answer->contents()[i].name) {
@@ skipping 1813 matching lines @@
   DCHECK(metrics_observer_ != NULL);
   for (cricket::TransportChannelStatsList::const_iterator it =
          stats.channel_stats.begin();
        it != stats.channel_stats.end(); ++it) {
     for (cricket::ConnectionInfos::const_iterator it_info =
            it->connection_infos.begin();
          it_info != it->connection_infos.end(); ++it_info) {
       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;
     }
   }
 }
 
 void WebRtcSession::ReportNegotiatedCiphers(
     const cricket::TransportStats& stats) {
   DCHECK(metrics_observer_ != NULL);
   if (!dtls_enabled_ || stats.channel_stats.empty()) {
     return;
@@ skipping 23 matching lines @@
 
   if (!srtp_cipher.empty()) {
     metrics_observer_->AddHistogramSample(srtp_name, srtp_cipher);
   }
   if (!ssl_cipher.empty()) {
     metrics_observer_->AddHistogramSample(ssl_name, ssl_cipher);
   }
 }
 
 }  // namespace webrtc