From 76829d7c3d3bf62ceafef0114a155404c02d5a7e Mon Sep 17 00:00:00 2001
From: Harald Alvestrand <hta@webrtc.org>
Date: Wed, 18 Jul 2018 23:24:36 +0200
Subject: [PATCH] Add UMA metric for ICE candidate addition outcome

Bug: webrtc:9532
Change-Id: I58af94c03f5bbf25db2b558a8fe1ae53634fb99f
Reviewed-on: https://webrtc-review.googlesource.com/89063
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Steve Anton <steveanton@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#24032}
---
 api/umametrics.h                  | 13 +++++++++++++
 pc/peerconnection.cc              | 21 +++++++++++++++++++--
 pc/peerconnection_ice_unittest.cc | 10 +++++++++-
 3 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/api/umametrics.h b/api/umametrics.h
index b999b64958..d6c86ddab2 100644
--- a/api/umametrics.h
+++ b/api/umametrics.h
@@ -169,6 +169,19 @@ enum SdpFormatReceived {
   kSdpFormatReceivedMax
 };
 
+// Metric for counting the outcome of adding an ICE candidate
+enum AddIceCandidateResult {
+  kAddIceCandidateSuccess,
+  kAddIceCandidateFailClosed,
+  kAddIceCandidateFailNoRemoteDescription,
+  kAddIceCandidateFailNullCandidate,
+  kAddIceCandidateFailNotValid,
+  kAddIceCandidateFailNotReady,
+  kAddIceCandidateFailInAddition,
+  kAddIceCandidateFailNotUsable,
+  kAddIceCandidateMax
+};
+
 class MetricsObserverInterface : public rtc::RefCountInterface {
  public:
   // |type| is the type of the enum counter to be incremented. |counter|
diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc
index 90ee340c85..06058e107a 100644
--- a/pc/peerconnection.cc
+++ b/pc/peerconnection.cc
@@ -411,6 +411,11 @@ void NoteKeyProtocolAndMedia(KeyExchangeProtocolType protocol_type,
   }
 }
 
+void NoteAddIceCandidateResult(int result) {
+  RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.AddIceCandidate", result,
+                            kAddIceCandidateMax);
+}
+
 // Checks that each non-rejected content has SDES crypto keys or a DTLS
 // fingerprint, unless it's in a BUNDLE group, in which case only the
 // BUNDLE-tag section (first media section/description in the BUNDLE group)
@@ -3004,37 +3009,49 @@ bool PeerConnection::AddIceCandidate(
   TRACE_EVENT0("webrtc", "PeerConnection::AddIceCandidate");
   if (IsClosed()) {
     RTC_LOG(LS_ERROR) << "AddIceCandidate: PeerConnection is closed.";
+    NoteAddIceCandidateResult(kAddIceCandidateFailClosed);
     return false;
   }
 
   if (!remote_description()) {
     RTC_LOG(LS_ERROR) << "AddIceCandidate: ICE candidates can't be added "
                          "without any remote session description.";
+    NoteAddIceCandidateResult(kAddIceCandidateFailNoRemoteDescription);
     return false;
   }
 
   if (!ice_candidate) {
     RTC_LOG(LS_ERROR) << "AddIceCandidate: Candidate is null.";
+    NoteAddIceCandidateResult(kAddIceCandidateFailNullCandidate);
     return false;
   }
 
   bool valid = false;
   bool ready = ReadyToUseRemoteCandidate(ice_candidate, nullptr, &valid);
   if (!valid) {
+    NoteAddIceCandidateResult(kAddIceCandidateFailNotValid);
     return false;
   }
 
   // Add this candidate to the remote session description.
   if (!mutable_remote_description()->AddCandidate(ice_candidate)) {
     RTC_LOG(LS_ERROR) << "AddIceCandidate: Candidate cannot be used.";
+    NoteAddIceCandidateResult(kAddIceCandidateFailInAddition);
     return false;
   }
-  NoteUsageEvent(UsageEvent::REMOTE_CANDIDATE_ADDED);
 
   if (ready) {
-    return UseCandidate(ice_candidate);
+    bool result = UseCandidate(ice_candidate);
+    if (result) {
+      NoteUsageEvent(UsageEvent::REMOTE_CANDIDATE_ADDED);
+      NoteAddIceCandidateResult(kAddIceCandidateSuccess);
+    } else {
+      NoteAddIceCandidateResult(kAddIceCandidateFailNotUsable);
+    }
+    return result;
   } else {
     RTC_LOG(LS_INFO) << "AddIceCandidate: Not ready to use candidate.";
+    NoteAddIceCandidateResult(kAddIceCandidateFailNotReady);
     return true;
   }
 }
diff --git a/pc/peerconnection_ice_unittest.cc b/pc/peerconnection_ice_unittest.cc
index fcd5face58..656ff22497 100644
--- a/pc/peerconnection_ice_unittest.cc
+++ b/pc/peerconnection_ice_unittest.cc
@@ -28,6 +28,7 @@
 #include "rtc_base/fakenetwork.h"
 #include "rtc_base/gunit.h"
 #include "rtc_base/virtualsocketserver.h"
+#include "system_wrappers/include/metrics_default.h"
 
 namespace webrtc {
 
@@ -246,7 +247,9 @@ class PeerConnectionIceTest
     : public PeerConnectionIceBaseTest,
       public ::testing::WithParamInterface<SdpSemantics> {
  protected:
-  PeerConnectionIceTest() : PeerConnectionIceBaseTest(GetParam()) {}
+  PeerConnectionIceTest() : PeerConnectionIceBaseTest(GetParam()) {
+    webrtc::metrics::Reset();
+  }
 };
 
 ::testing::AssertionResult AssertCandidatesEqual(const char* a_expr,
@@ -415,6 +418,11 @@ TEST_P(PeerConnectionIceTest, CannotAddCandidateWhenRemoteDescriptionNotSet) {
   caller->CreateOfferAndSetAsLocal();
 
   EXPECT_FALSE(caller->pc()->AddIceCandidate(&jsep_candidate));
+  EXPECT_EQ(
+      2, webrtc::metrics::NumSamples("WebRTC.PeerConnection.AddIceCandidate"));
+  EXPECT_EQ(
+      2, webrtc::metrics::NumEvents("WebRTC.PeerConnection.AddIceCandidate",
+                                    kAddIceCandidateFailNoRemoteDescription));
 }
 
 TEST_P(PeerConnectionIceTest, CannotAddCandidateWhenPeerConnectionClosed) {