Better determination of Symmetric NAT

webrtc/p2p/stunprober/main.cc

Index: webrtc/p2p/stunprober/main.cc
diff --git a/webrtc/p2p/stunprober/main.cc b/webrtc/p2p/stunprober/main.cc
index 762b6da6d79b895085d3a288b2eef52e796ce135..0358e87f319fff20b145348aa5bd7b44c804fdd8 100644
--- a/webrtc/p2p/stunprober/main.cc
+++ b/webrtc/p2p/stunprober/main.cc
@@ -38,6 +38,8 @@ using stunprober::ServerSocketInterface;
 using stunprober::SocketFactory;
 using stunprober::TaskRunner;
 
+using NATTYPE = stunprober::StunProber::NATTYPE;
+
 DEFINE_bool(help, false, "Prints this message");
 DEFINE_int(interval, 10, "Interval of consecutive stun pings in milliseconds");
 DEFINE_bool(shared_socket, false, "Share socket mode for different remote IPs");
@@ -118,6 +120,21 @@ std::string HistogramName(bool behind_nat,
   return std::string(output);
 }
 
+const char* PrintNATType(NATTYPE type) {
+  switch (type) {
+    case NATTYPE::NATTYPE_NO_NAT:

[pthatcher2, 2015/06/05 22:39:41]

Do you really need "NATTYPE::" Prefixed everywhere?  I don't think you do.

[pthatcher2, 2015/06/05 22:39:41]

NATTYPE_NONE?

[guoweis_webrtc, 2015/06/07 17:29:39]

Done.

[guoweis_webrtc, 2015/06/07 17:29:39]

Done.

+      return "not behind NAT";
+    case NATTYPE::NATTYPE_NAT_TYPE_UNKNOWN:

[pthatcher2, 2015/06/05 22:39:41]

Why not NATTYPE_UNKNOWN?

[guoweis_webrtc, 2015/06/07 17:29:39]

Done.

+      return "unknown NAT type";
+    case NATTYPE::NATTYPE_SYM_NAT:

[pthatcher2, 2015/06/05 22:39:41]

NATTYPE_SYMMETRIC?

[guoweis_webrtc, 2015/06/07 17:29:39]

Done.

+      return "Symmetric NAT";
+    case NATTYPE::NATTYPE_NON_SYM_NAT:

[pthatcher2, 2015/06/05 22:39:41]

NATTYPE_NON_SYMMETRIC?

[guoweis_webrtc, 2015/06/07 17:29:40]

Done.

+      return "Non-Symmetric NAT";

[pthatcher2, 2015/06/05 22:39:41]

Please capitalize all of them or none of them.

[guoweis_webrtc, 2015/06/07 17:29:40]

Done.

+    default:
+      return "unknown";

[pthatcher2, 2015/06/05 22:39:41]

Is this the same as NATTYPE_UNKNOWN?

[guoweis_webrtc, 2015/06/07 17:29:39]

It's invalid. Updated.

+  }
+}
+
 void PrintStats(StunProber* prober) {
   StunProber::Stats stats;
   if (!prober->GetStats(&stats)) {
@@ -130,24 +147,23 @@ void PrintStats(StunProber* prober) {
   LOG(LS_INFO) << "Responses received: " << stats.num_response_received;
   LOG(LS_INFO) << "Target interval (ns): " << stats.target_request_interval_ns;
   LOG(LS_INFO) << "Actual interval (ns): " << stats.actual_request_interval_ns;
-  LOG(LS_INFO) << "Behind NAT: " << stats.behind_nat;
-  if (stats.behind_nat) {
-    LOG(LS_INFO) << "NAT is symmetrical: " << (stats.srflx_addrs.size() > 1);
-  }
+  LOG(LS_INFO) << "NAT Type: " << PrintNATType(stats.nat_type);
   LOG(LS_INFO) << "Host IP: " << stats.host_ip;
   LOG(LS_INFO) << "Server-reflexive ips: ";
   for (auto& ip : stats.srflx_addrs) {
     LOG(LS_INFO) << "\t" << ip;
   }
 
-  std::string histogram_name = HistogramName(
-      stats.behind_nat, FLAG_shared_socket, FLAG_interval, "SuccessPercent");
+  std::string histogram_name =
+      HistogramName((stats.nat_type > NATTYPE::NATTYPE_NO_NAT),

[pthatcher2, 2015/06/05 22:39:41]

What if it's NATTYPE_UNKNOWN?

[guoweis_webrtc, 2015/06/07 17:29:40]

Remove the code. Histogram doesn't make sense here.

+                    FLAG_shared_socket, FLAG_interval, "SuccessPercent");
 
   LOG(LS_INFO) << "Histogram '" << histogram_name.c_str()
                << "' = " << stats.success_percent;
 
-  histogram_name = HistogramName(stats.behind_nat, FLAG_shared_socket,
-                                 FLAG_interval, "ResponseLatency");
+  histogram_name =
+      HistogramName((stats.nat_type > NATTYPE::NATTYPE_NO_NAT),

[pthatcher2, 2015/06/05 22:39:41]

You could avoid duplication with 

bool behind_nat = stats.nat_type > NATTYPE_UNKNOWN.

Or reuse the "is_behind_nat" function.

[guoweis_webrtc, 2015/06/07 17:29:39]

Remove the code. Histogram doesn't make sense here.

+                    FLAG_shared_socket, FLAG_interval, "ResponseLatency");
 
   LOG(LS_INFO) << "Histogram '" << histogram_name.c_str()
                << "' = " << stats.average_rtt_ms << " ms";