Add UMA metrics for ICE regathering reasons.

webrtc/p2p/base/portallocator.h

 /*
  *  Copyright 2004 The WebRTC Project Authors. All rights reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #ifndef WEBRTC_P2P_BASE_PORTALLOCATOR_H_
 #define WEBRTC_P2P_BASE_PORTALLOCATOR_H_
 
 #include <deque>
 #include <memory>
 #include <string>
 #include <vector>
 
 #include "webrtc/p2p/base/port.h"
 #include "webrtc/p2p/base/portinterface.h"
 #include "webrtc/base/helpers.h"
 #include "webrtc/base/proxyinfo.h"
 #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
 // P2PSocket. It also handles port freeing.
 //
 // Clients can override this class to control port allocation, including
 // what kinds of ports are allocated.
 
 enum {
   // Disable local UDP ports. This doesn't impact how we connect to relay
@@ skipping 29 matching lines @@
   // high cost. So if both Wi-Fi and cellular networks exist, gather only on the
   // Wi-Fi network. If a network type is "unknown", it has a cost lower than
   // cellular but higher than Wi-Fi/Ethernet. So if an unknown network exists,
   // cellular networks will not be used to gather candidates and if a Wi-Fi
   // network is present, "unknown" networks will not be usd to gather
   // candidates. Doing so ensures that even if a cellular network type was not
   // detected initially, it would not be used if a Wi-Fi network is present.
   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.
 // As per RFC 5245 Appendix B.1, STUN transactions need to be paced at certain
 // internal. Less than 20ms is not acceptable. We choose 50ms as our default.
 const uint32_t kMinimumStepDelay = 50;
 
 // CF = CANDIDATE FILTER
 enum {
   CF_NONE = 0x0,
@@ skipping 124 matching lines @@
   sigslot::signal2<PortAllocatorSession*, const std::vector<PortInterface*>&>
       SignalPortsPruned;
   sigslot::signal2<PortAllocatorSession*,
                    const std::vector<Candidate>&> SignalCandidatesReady;
   // Candidates should be signaled to be removed when the port that generated
   // the candidates is removed.
   sigslot::signal2<PortAllocatorSession*, const std::vector<Candidate>&>
       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;
 
  protected:
   // This method is called when a pooled session (which doesn't have these
   // properties initially) is returned by PortAllocator::TakePooledSession,
   // and the content name, component, and ICE ufrag/pwd are updated.
   //
   // A subclass may need to override this method to perform additional actions,
@@ skipping 134 matching lines @@
   void set_candidate_filter(uint32_t filter) {
     candidate_filter_ = filter;
   }
 
   bool prune_turn_ports() const { return prune_turn_ports_; }
 
   // Gets/Sets the Origin value used for WebRTC STUN requests.
   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,
       int component,
       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_;
   int min_port_;
   int max_port_;
   uint32_t step_delay_;
   bool allow_tcp_listen_;
   uint32_t candidate_filter_;
   std::string origin_;
 
  private:
   ServerAddresses stun_servers_;
   std::vector<RelayServerConfig> turn_servers_;
   // The last size passed into SetConfiguration.
   int target_pooled_session_count_ = 0;
   // This variable represents the total number of pooled sessions
   // both owned by this class and taken by TakePooledSession.
   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
 
 #endif  // WEBRTC_P2P_BASE_PORTALLOCATOR_H_