Add UMA metrics for ICE regathering reasons.

webrtc/p2p/base/portallocator.h

Index: webrtc/p2p/base/portallocator.h
diff --git a/webrtc/p2p/base/portallocator.h b/webrtc/p2p/base/portallocator.h
index 8befbadcc73981375d22ba23b321c417fbb2cd21..3e7591f6d40f56e7cd15b3e8906b1c58152549a7 100644
--- a/webrtc/p2p/base/portallocator.h
+++ b/webrtc/p2p/base/portallocator.h
@@ -23,6 +23,10 @@
 #include "webrtc/base/sigslot.h"
 #include "webrtc/base/thread.h"
 
+namespace webrtc {
+class MetricsObserverInterface;
+}
+
 namespace cricket {
 
 // PortAllocator is responsible for allocating Port types for a given
@@ -72,6 +76,16 @@ enum {
   PORTALLOCATOR_DISABLE_COSTLY_NETWORKS = 0x2000,
 };
 
+// Defines various reasons that have caused ICE regathering.
+enum class IceRegatheringReason {
+  ICE_RESTART_WHEN_CONNECTED,
+  ICE_RESTART_WHEN_CONNECTING,
+  ICE_RESTART_WHEN_DISCONNECTED,
+  CONTINUAL_GATHERING_BY_NETWORK_CHANGE,
+  CONTINUAL_GATHERING_BY_NETWORK_FAILURE,
+  MAX_VALUE
+};
+
 const uint32_t kDefaultPortAllocatorFlags = 0;
 
 const uint32_t kDefaultStepDelay = 1000;  // 1 sec step delay.
@@ -216,6 +230,9 @@ class PortAllocatorSession : public sigslot::has_slots<> {
       SignalCandidatesRemoved;
   sigslot::signal1<PortAllocatorSession*> SignalCandidatesAllocationDone;
 
+  sigslot::signal2<PortAllocatorSession*, IceRegatheringReason>
+      SignalIceRegatheringReason;
+
   virtual uint32_t generation() { return generation_; }
   virtual void set_generation(uint32_t generation) { generation_ = generation; }
   sigslot::signal1<PortAllocatorSession*> SignalDestroyed;
@@ -370,6 +387,10 @@ class PortAllocator : public sigslot::has_slots<> {
   const std::string& origin() const { return origin_; }
   void set_origin(const std::string& origin) { origin_ = origin; }
 
+  void set_metrics_observer(webrtc::MetricsObserverInterface* observer) {
+    metrics_observer_ = observer;
+  }

[Taylor Brandstetter, 2016/10/03 20:33:35]

It seems confusing that the metrics observer is set on the base class, but only
used on the derived class, even though every derived class is expected to do the
same thing. It's also odd that the enum is defined here, but only two values are
relevant to the PortAllocator itself.

If the P2PTransportChannel had a direct pointer to the metrics observer, I think
it would be better if PortAllocatorSession just had two signals: "regathering
due to network change" and "regathering due to failed networks". Then
P2PTransportChannel could listen to the signals and increment the stats itself.

[honghaiz3, 2016/10/04 01:03:55]

The first part, really just follows a few other members in this class where they
were set in the base but used in the derived class. I think the reason is
because the derived class is expected to the same thing as you said. Plus each
derived class has its own control on whether and what observer to set. For
example, we set it the fake metrics observer in the tests. 

Regarding the second part, I think it is not uncommon to define some common
enums in a base class and use them elsewhere including other classes. At least
we defined a lot of constants in transport.h which are then used in many places.

For this particular enum list, some of them will be generated in
BasicPortAllocator and some in p2ptransportchannel. As I have responded
elsewhere, it is simpler to keep the observer pointer in the PortAllocator. 
If you really want to use only the defined enums in BasicPortAllocator, I can
create a separate enum and pass that from p2ptransportchannel to
basicportallocator and translate that enum to the one defined in portallocator,
although I do not see the point of doing that instead of directly passing the
enum defined in portallocator. 

[Taylor Brandstetter, 2016/10/04 02:42:32]

The enum definition doesn't matter too much. What I'm more concerned about is
the confusing relationship between PortAllocator, BasicPortAllocator,
MetricsObserverInterface and P2PTransportChannel. Mostly, that
P2PTransportChannel shouldn't be directly calling a signal of another class, and
relying on that signal to increment a stat. An alternative would be to add a
method such as "RecordIceRegatheringReason", but it's still an odd roundabout
approach. The PortAllocator becomes responsible for recording metrics for
another class.

If the main reason for this design is that it's easier to pass the
MetricsObserverInterface from PeerConnection->PortAllocator as opposed to
WebRtcSession->TransportController->P2PTransportChannel, I don't think that's a
strong enough reason.

[honghaiz3, 2016/10/04 04:06:42]

This is what I was doing in the second patch. 
In fact we can pass from PortAllocator to P2PTransportChannel directly. We then
have the following options: 
1. Pass the allocator from WebRTCSession->TransportController->(perhaps also
transport and DtlsTransportChannel)->P2PTransportChannel
2. Then why not the following simpler approach?
 record  allocator->metric_observer in p2ptransportchannel 
3. Or could we just use allocator->metric_observer() every time?
4. Or should we wrap the method of reporting the events by doing something like
allocator->Report...

[Taylor Brandstetter, 2016/10/04 17:44:15]

I'd still prefer #1, because I don't think it should be PortAllocator's
responsibility to assist other classes in recording metrics. The approach won't
work if we need to record a metric for something that doesn't have a pointer to
a PortAllocator.

I think the approaches that make sense are:

1. Bubble events up to WebRtcSession (what we've been doing until now).
2. Trickle the metric observer down to the things that need it.

And I think it makes sense to start taking the "trickle down" approach (like is
used for RtcEventLog), because it requires a lot less boilerplate code. And
although it means more objects have a pointer to a shared object (the metric
observer), this is fine since it's only used for logging.

[honghaiz3, 2016/10/05 05:01:53]

Done.

+
  protected:
   virtual PortAllocatorSession* CreateSessionInternal(
       const std::string& content_name,
@@ -377,6 +398,10 @@ class PortAllocator : public sigslot::has_slots<> {
       const std::string& ice_ufrag,
       const std::string& ice_pwd) = 0;
 
+  webrtc::MetricsObserverInterface* metrics_observer() {
+    return metrics_observer_;
+  }
+
   uint32_t flags_;
   std::string agent_;
   rtc::ProxyInfo proxy_;
@@ -397,6 +422,8 @@ class PortAllocator : public sigslot::has_slots<> {
   int allocated_pooled_session_count_ = 0;
   std::deque<std::unique_ptr<PortAllocatorSession>> pooled_sessions_;
   bool prune_turn_ports_ = false;
+
+  webrtc::MetricsObserverInterface* metrics_observer_ = nullptr;
 };
 
 }  // namespace cricket