From 13f4c896d507b116e5f035b135ef21ebe0c7069c Mon Sep 17 00:00:00 2001 From: Qingsi Wang Date: Tue, 10 Jul 2018 20:53:10 +0000 Subject: [PATCH] Revert "Replace the usage of MetricsObserverInterface by RTC_HISTOGRAM_*." This reverts commit 870bca1f418a1abf445169a638a61f9a649d557f. Reason for revert: it breaks internal tests and builds Original change's description: > Replace the usage of MetricsObserverInterface by RTC_HISTOGRAM_*. > > We now use RTC_HISTOGRAM_* macros in system_wrappers/include/metrics.h > to report the metrics in pc/ and p2p/ that are currently been reported > using MetricsObserverInterface. > > TBR=tommi@webrtc.org > > Bug: webrtc:9409 > Change-Id: I47c9975402293c72250203fa1ec19eb1668766f6 > Reviewed-on: https://webrtc-review.googlesource.com/83782 > Commit-Queue: Qingsi Wang > Reviewed-by: Harald Alvestrand > Reviewed-by: Taylor (left Google) > Reviewed-by: Steve Anton > Cr-Commit-Position: refs/heads/master@{#23914} TBR=steveanton@webrtc.org,deadbeef@webrtc.org,hta@webrtc.org,tommi@webrtc.org Change-Id: I1afd92d44f3b8cf3ae9aa6e6daa9a3a272e8097f No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: webrtc:9409 Reviewed-on: https://webrtc-review.googlesource.com/88040 Reviewed-by: Qingsi Wang Commit-Queue: Qingsi Wang Cr-Commit-Position: refs/heads/master@{#23916} --- api/BUILD.gn | 21 +++ api/fakemetricsobserver.cc | 87 +++++++++ api/fakemetricsobserver.h | 56 ++++++ api/umametrics.cc | 21 +++ api/umametrics.h | 4 +- p2p/BUILD.gn | 3 +- p2p/base/p2ptransportchannel.cc | 9 +- p2p/base/p2ptransportchannel_unittest.cc | 99 ++++++----- p2p/base/regatheringcontroller_unittest.cc | 1 + p2p/client/basicportallocator.cc | 11 +- p2p/client/basicportallocator_unittest.cc | 18 -- pc/BUILD.gn | 4 +- pc/peerconnection.cc | 198 +++++++++++---------- pc/peerconnection.h | 3 +- pc/peerconnection_histogram_unittest.cc | 51 +++--- pc/peerconnection_integrationtest.cc | 94 ++++++---- pc/peerconnection_rtp_unittest.cc | 62 +++---- pc/peerconnectionwrapper.cc | 8 + pc/peerconnectionwrapper.h | 6 + pc/srtpsession.cc | 12 +- pc/srtpsession_unittest.cc | 41 ++--- rtc_base/BUILD.gn | 1 - rtc_base/sslstreamadapter.h | 2 - rtc_base/unittest_main.cc | 2 - system_wrappers/include/metrics.h | 3 - 25 files changed, 510 insertions(+), 307 deletions(-) create mode 100644 api/fakemetricsobserver.cc create mode 100644 api/fakemetricsobserver.h create mode 100644 api/umametrics.cc diff --git a/api/BUILD.gn b/api/BUILD.gn index b6caeaa6876..71c2ec39bce 100644 --- a/api/BUILD.gn +++ b/api/BUILD.gn @@ -86,6 +86,7 @@ rtc_static_library("libjingle_peerconnection_api") { "statstypes.cc", "statstypes.h", "turncustomizer.h", + "umametrics.cc", "umametrics.h", "videosourceproxy.h", ] @@ -439,6 +440,26 @@ if (rtc_include_tests) { ] } + rtc_source_set("fakemetricsobserver") { + testonly = true + sources = [ + "fakemetricsobserver.cc", + "fakemetricsobserver.h", + ] + deps = [ + "../media:rtc_media_base", + "../rtc_base:checks", + "../rtc_base:rtc_base_approved", + ] + if (!build_with_chromium && is_clang) { + # Suppress warnings from the Chromium Clang plugin (bugs.webrtc.org/163). + suppressed_configs += [ "//build/config/clang:find_bad_constructs" ] + } + if (!build_with_mozilla) { + deps += [ ":libjingle_peerconnection_api" ] + } + } + rtc_source_set("rtc_api_unittests") { testonly = true diff --git a/api/fakemetricsobserver.cc b/api/fakemetricsobserver.cc new file mode 100644 index 00000000000..cd8de392a31 --- /dev/null +++ b/api/fakemetricsobserver.cc @@ -0,0 +1,87 @@ +/* + * Copyright 2015 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. + */ + +#include "api/fakemetricsobserver.h" +#include "rtc_base/checks.h" + +namespace webrtc { + +FakeMetricsObserver::FakeMetricsObserver() { + Reset(); +} + +void FakeMetricsObserver::Reset() { + RTC_DCHECK(thread_checker_.CalledOnValidThread()); + counters_.clear(); + memset(histogram_samples_, 0, sizeof(histogram_samples_)); +} + +void FakeMetricsObserver::IncrementEnumCounter( + PeerConnectionEnumCounterType type, + int counter, + int counter_max) { + RTC_DCHECK(thread_checker_.CalledOnValidThread()); + if (counters_.size() <= static_cast(type)) { + counters_.resize(type + 1); + } + auto& counters = counters_[type]; + ++counters[counter]; +} + +void FakeMetricsObserver::AddHistogramSample(PeerConnectionMetricsName type, + int value) { + RTC_DCHECK(thread_checker_.CalledOnValidThread()); + RTC_DCHECK_EQ(histogram_samples_[type], 0); + histogram_samples_[type] = value; +} + +int FakeMetricsObserver::GetEnumCounter(PeerConnectionEnumCounterType type, + int counter) const { + RTC_DCHECK(thread_checker_.CalledOnValidThread()); + if (counters_.size() <= static_cast(type)) { + return 0; + } + const auto& it = counters_[type].find(counter); + if (it == counters_[type].end()) { + return 0; + } + return it->second; +} + +int FakeMetricsObserver::GetHistogramSample( + PeerConnectionMetricsName type) const { + RTC_DCHECK(thread_checker_.CalledOnValidThread()); + return histogram_samples_[type]; +} + +bool FakeMetricsObserver::ExpectOnlySingleEnumCount( + PeerConnectionEnumCounterType type, + int counter) const { + RTC_DCHECK(thread_checker_.CalledOnValidThread()); + if (counters_.size() <= static_cast(type)) { + // If a counter has not been allocated then there has been no call to + // |IncrementEnumCounter| so all the values are 0. + return false; + } + bool pass = true; + if (GetEnumCounter(type, counter) != 1) { + RTC_LOG(LS_ERROR) << "Expected single count for counter: " << counter; + pass = false; + } + for (const auto& entry : counters_[type]) { + if (entry.first != counter && entry.second > 0) { + RTC_LOG(LS_ERROR) << "Expected no count for counter: " << entry.first; + pass = false; + } + } + return pass; +} + +} // namespace webrtc diff --git a/api/fakemetricsobserver.h b/api/fakemetricsobserver.h new file mode 100644 index 00000000000..1f5b7043b2f --- /dev/null +++ b/api/fakemetricsobserver.h @@ -0,0 +1,56 @@ +/* + * Copyright 2015 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 API_FAKEMETRICSOBSERVER_H_ +#define API_FAKEMETRICSOBSERVER_H_ + +#include +#include +#include + +#include "api/peerconnectioninterface.h" +#include "rtc_base/thread_checker.h" + +namespace webrtc { + +class FakeMetricsObserver : public MetricsObserverInterface { + public: + FakeMetricsObserver(); + void Reset(); + + void IncrementEnumCounter(PeerConnectionEnumCounterType, + int counter, + int counter_max) override; + void AddHistogramSample(PeerConnectionMetricsName type, int value) override; + + // Accessors to be used by the tests. + int GetEnumCounter(PeerConnectionEnumCounterType type, int counter) const; + int GetHistogramSample(PeerConnectionMetricsName type) const; + + // Returns true if and only if there is a count of 1 for the given counter and + // a count of 0 for all other counters of the given enum type. + bool ExpectOnlySingleEnumCount(PeerConnectionEnumCounterType type, + int counter) const; + + protected: + ~FakeMetricsObserver() {} + + private: + rtc::ThreadChecker thread_checker_; + // The vector contains maps for each counter type. In the map, it's a mapping + // from individual counter to its count, such that it's memory efficient when + // comes to sparse enum types, like the SSL ciphers in the IANA registry. + std::vector> counters_; + int histogram_samples_[kPeerConnectionMetricsName_Max]; +}; + +} // namespace webrtc + +#endif // API_FAKEMETRICSOBSERVER_H_ diff --git a/api/umametrics.cc b/api/umametrics.cc new file mode 100644 index 00000000000..d5f2bb62b27 --- /dev/null +++ b/api/umametrics.cc @@ -0,0 +1,21 @@ +/* + * Copyright 2017 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. + */ + +#include "api/umametrics.h" + +namespace webrtc { + +void MetricsObserverInterface::IncrementSparseEnumCounter( + PeerConnectionEnumCounterType type, + int counter) { + IncrementEnumCounter(type, counter, 0 /* Ignored */); +} + +} // namespace webrtc diff --git a/api/umametrics.h b/api/umametrics.h index b999b649584..081b515c7c9 100644 --- a/api/umametrics.h +++ b/api/umametrics.h @@ -176,13 +176,13 @@ class MetricsObserverInterface : public rtc::RefCountInterface { // number after the highest counter. virtual void IncrementEnumCounter(PeerConnectionEnumCounterType type, int counter, - int counter_max) = 0; + int counter_max) {} // This is used to handle sparse counters like SSL cipher suites. // TODO(guoweis): Remove the implementation once the dependency's interface // definition is updated. virtual void IncrementSparseEnumCounter(PeerConnectionEnumCounterType type, - int counter) = 0; + int counter); virtual void AddHistogramSample(PeerConnectionMetricsName type, int value) = 0; diff --git a/p2p/BUILD.gn b/p2p/BUILD.gn index 4763001f499..a2f51608d4d 100644 --- a/p2p/BUILD.gn +++ b/p2p/BUILD.gn @@ -91,7 +91,6 @@ rtc_static_library("rtc_p2p") { "../rtc_base:safe_minmax", "../rtc_base:stringutils", "../system_wrappers:field_trial_api", - "../system_wrappers:metrics_api", "//third_party/abseil-cpp/absl/memory", "//third_party/abseil-cpp/absl/types:optional", ] @@ -172,13 +171,13 @@ if (rtc_include_tests) { deps = [ ":p2p_test_utils", ":rtc_p2p", + "../api:fakemetricsobserver", "../api:ortc_api", "../rtc_base:checks", "../rtc_base:rtc_base", "../rtc_base:rtc_base_approved", "../rtc_base:rtc_base_tests_utils", "../rtc_base:stringutils", - "../system_wrappers:metrics_default", "../test:test_support", "//testing/gtest", "//third_party/abseil-cpp/absl/memory", diff --git a/p2p/base/p2ptransportchannel.cc b/p2p/base/p2ptransportchannel.cc index 67bcdfa56cc..dca8d6374c1 100644 --- a/p2p/base/p2ptransportchannel.cc +++ b/p2p/base/p2ptransportchannel.cc @@ -28,7 +28,6 @@ #include "rtc_base/stringencode.h" #include "rtc_base/timeutils.h" #include "system_wrappers/include/field_trial.h" -#include "system_wrappers/include/metrics.h" namespace { @@ -676,7 +675,7 @@ void P2PTransportChannel::MaybeStartGathering() { SignalGatheringState(this); } - if (!allocator_sessions_.empty()) { + if (metrics_observer_ && !allocator_sessions_.empty()) { IceRestartState state; if (writable()) { state = IceRestartState::CONNECTED; @@ -685,9 +684,9 @@ void P2PTransportChannel::MaybeStartGathering() { } else { state = IceRestartState::DISCONNECTED; } - RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.IceRestartState", - static_cast(state), - static_cast(IceRestartState::MAX_VALUE)); + metrics_observer_->IncrementEnumCounter( + webrtc::kEnumCounterIceRestart, static_cast(state), + static_cast(IceRestartState::MAX_VALUE)); } // Time for a new allocator. diff --git a/p2p/base/p2ptransportchannel_unittest.cc b/p2p/base/p2ptransportchannel_unittest.cc index 1c3ae9ab778..cbf8b250e58 100644 --- a/p2p/base/p2ptransportchannel_unittest.cc +++ b/p2p/base/p2ptransportchannel_unittest.cc @@ -13,6 +13,7 @@ #include #include "absl/memory/memory.h" +#include "api/fakemetricsobserver.h" #include "p2p/base/fakeportallocator.h" #include "p2p/base/icetransportinternal.h" #include "p2p/base/p2ptransportchannel.h" @@ -36,7 +37,6 @@ #include "rtc_base/ssladapter.h" #include "rtc_base/thread.h" #include "rtc_base/virtualsocketserver.h" -#include "system_wrappers/include/metrics_default.h" namespace { @@ -207,10 +207,15 @@ class P2PTransportChannelTestBase : public testing::Test, ep1_.allocator_.reset( CreateBasicPortAllocator(&ep1_.network_manager_, stun_servers, kTurnUdpIntAddr, rtc::SocketAddress())); + ep1_.metrics_observer_ = + new rtc::RefCountedObject(); + ep1_.allocator_->SetMetricsObserver(ep1_.metrics_observer_); ep2_.allocator_.reset( CreateBasicPortAllocator(&ep2_.network_manager_, stun_servers, kTurnUdpIntAddr, rtc::SocketAddress())); - webrtc::metrics::Reset(); + ep2_.metrics_observer_ = + new rtc::RefCountedObject(); + ep2_.allocator_->SetMetricsObserver(ep2_.metrics_observer_); } protected: @@ -277,7 +282,7 @@ class P2PTransportChannelTestBase : public testing::Test, Candidates candidates; }; - struct Endpoint : public sigslot::has_slots<> { + struct Endpoint { Endpoint() : role_(ICEROLE_UNKNOWN), tiebreaker_(0), @@ -308,15 +313,10 @@ class P2PTransportChannelTestBase : public testing::Test, allocator_->set_allow_tcp_listen(allow_tcp_listen); } - void OnIceRegathering(PortAllocatorSession*, IceRegatheringReason reason) { - ++ice_regathering_counter_[reason]; - } - - int GetIceRegatheringCountForReason(IceRegatheringReason reason) { - return ice_regathering_counter_[reason]; - } - rtc::FakeNetworkManager network_manager_; + // |metrics_observer_| should outlive |allocator_| as the former may be + // used by the latter. + rtc::scoped_refptr metrics_observer_; std::unique_ptr allocator_; ChannelData cd1_; ChannelData cd2_; @@ -326,7 +326,6 @@ class P2PTransportChannelTestBase : public testing::Test, bool save_candidates_; std::vector> saved_candidates_; bool ready_to_send_ = false; - std::map ice_regathering_counter_; }; ChannelData* GetChannelData(rtc::PacketTransportInternal* transport) { @@ -354,14 +353,12 @@ class P2PTransportChannelTestBase : public testing::Test, ice_ep1_cd1_ch, ice_ep2_cd1_ch)); ep2_.cd1_.ch_.reset(CreateChannel(1, ICE_CANDIDATE_COMPONENT_DEFAULT, ice_ep2_cd1_ch, ice_ep1_cd1_ch)); + ep1_.cd1_.ch_->SetMetricsObserver(ep1_.metrics_observer_); + ep2_.cd1_.ch_->SetMetricsObserver(ep2_.metrics_observer_); ep1_.cd1_.ch_->SetIceConfig(ep1_config); ep2_.cd1_.ch_->SetIceConfig(ep2_config); ep1_.cd1_.ch_->MaybeStartGathering(); ep2_.cd1_.ch_->MaybeStartGathering(); - ep1_.cd1_.ch_->allocator_session()->SignalIceRegathering.connect( - &ep1_, &Endpoint::OnIceRegathering); - ep2_.cd1_.ch_->allocator_session()->SignalIceRegathering.connect( - &ep2_, &Endpoint::OnIceRegathering); } void CreateChannels() { @@ -444,6 +441,9 @@ class P2PTransportChannelTestBase : public testing::Test, BasicPortAllocator* GetAllocator(int endpoint) { return GetEndpoint(endpoint)->allocator_.get(); } + webrtc::FakeMetricsObserver* GetMetricsObserver(int endpoint) { + return GetEndpoint(endpoint)->metrics_observer_; + } void AddAddress(int endpoint, const SocketAddress& addr) { GetEndpoint(endpoint)->network_manager_.AddInterface(addr); } @@ -1266,15 +1266,15 @@ TEST_F(P2PTransportChannelTest, TestUMAIceRestartWhileDisconnected) { ep1_ch1()->SetIceParameters(kIceParams[2]); ep1_ch1()->SetRemoteIceParameters(kIceParams[3]); ep1_ch1()->MaybeStartGathering(); - EXPECT_EQ(1, webrtc::metrics::NumEvents( - "WebRTC.PeerConnection.IceRestartState", + EXPECT_EQ(1, GetMetricsObserver(0)->GetEnumCounter( + webrtc::kEnumCounterIceRestart, static_cast(IceRestartState::DISCONNECTED))); ep2_ch1()->SetIceParameters(kIceParams[3]); ep2_ch1()->SetRemoteIceParameters(kIceParams[2]); ep2_ch1()->MaybeStartGathering(); - EXPECT_EQ(2, webrtc::metrics::NumEvents( - "WebRTC.PeerConnection.IceRestartState", + EXPECT_EQ(1, GetMetricsObserver(1)->GetEnumCounter( + webrtc::kEnumCounterIceRestart, static_cast(IceRestartState::DISCONNECTED))); DestroyChannels(); @@ -1295,15 +1295,15 @@ TEST_F(P2PTransportChannelTest, TestUMAIceRestartWhileConnected) { ep1_ch1()->SetIceParameters(kIceParams[2]); ep1_ch1()->SetRemoteIceParameters(kIceParams[3]); ep1_ch1()->MaybeStartGathering(); - EXPECT_EQ(1, webrtc::metrics::NumEvents( - "WebRTC.PeerConnection.IceRestartState", + EXPECT_EQ(1, GetMetricsObserver(0)->GetEnumCounter( + webrtc::kEnumCounterIceRestart, static_cast(IceRestartState::CONNECTED))); ep2_ch1()->SetIceParameters(kIceParams[3]); ep2_ch1()->SetRemoteIceParameters(kIceParams[2]); ep2_ch1()->MaybeStartGathering(); - EXPECT_EQ(2, webrtc::metrics::NumEvents( - "WebRTC.PeerConnection.IceRestartState", + EXPECT_EQ(1, GetMetricsObserver(1)->GetEnumCounter( + webrtc::kEnumCounterIceRestart, static_cast(IceRestartState::CONNECTED))); DestroyChannels(); @@ -1321,15 +1321,15 @@ TEST_F(P2PTransportChannelTest, TestUMAIceRestartWhileConnecting) { ep1_ch1()->SetIceParameters(kIceParams[2]); ep1_ch1()->SetRemoteIceParameters(kIceParams[3]); ep1_ch1()->MaybeStartGathering(); - EXPECT_EQ(1, webrtc::metrics::NumEvents( - "WebRTC.PeerConnection.IceRestartState", + EXPECT_EQ(1, GetMetricsObserver(0)->GetEnumCounter( + webrtc::kEnumCounterIceRestart, static_cast(IceRestartState::CONNECTING))); ep2_ch1()->SetIceParameters(kIceParams[3]); ep2_ch1()->SetRemoteIceParameters(kIceParams[2]); ep2_ch1()->MaybeStartGathering(); - EXPECT_EQ(2, webrtc::metrics::NumEvents( - "WebRTC.PeerConnection.IceRestartState", + EXPECT_EQ(1, GetMetricsObserver(1)->GetEnumCounter( + webrtc::kEnumCounterIceRestart, static_cast(IceRestartState::CONNECTING))); DestroyChannels(); @@ -1355,10 +1355,12 @@ TEST_F(P2PTransportChannelTest, // Adding address in ep1 will trigger continual gathering. AddAddress(0, kAlternateAddrs[0]); - EXPECT_EQ_SIMULATED_WAIT(1, - GetEndpoint(0)->GetIceRegatheringCountForReason( - IceRegatheringReason::NETWORK_CHANGE), - kDefaultTimeout, clock); + EXPECT_EQ_SIMULATED_WAIT( + 1, + GetMetricsObserver(0)->GetEnumCounter( + webrtc::kEnumCounterIceRegathering, + static_cast(IceRegatheringReason::NETWORK_CHANGE)), + kDefaultTimeout, clock); ep2_ch1()->SetIceParameters(kIceParams[3]); ep2_ch1()->SetRemoteIceParameters(kIceParams[2]); @@ -1367,8 +1369,9 @@ TEST_F(P2PTransportChannelTest, AddAddress(1, kAlternateAddrs[1]); SIMULATED_WAIT(false, kDefaultTimeout, clock); // ep2 has not enabled continual gathering. - EXPECT_EQ(0, GetEndpoint(1)->GetIceRegatheringCountForReason( - IceRegatheringReason::NETWORK_CHANGE)); + EXPECT_EQ(0, GetMetricsObserver(1)->GetEnumCounter( + webrtc::kEnumCounterIceRegathering, + static_cast(IceRegatheringReason::NETWORK_CHANGE))); DestroyChannels(); } @@ -1396,13 +1399,12 @@ TEST_F(P2PTransportChannelTest, // Timeout value such that all connections are deleted. const int kNetworkFailureTimeout = 35000; SIMULATED_WAIT(false, kNetworkFailureTimeout, clock); - EXPECT_LE(1, GetEndpoint(0)->GetIceRegatheringCountForReason( - IceRegatheringReason::NETWORK_FAILURE)); - EXPECT_LE(1, webrtc::metrics::NumEvents( - "WebRTC.PeerConnection.IceRegatheringReason", + EXPECT_LE(1, GetMetricsObserver(0)->GetEnumCounter( + webrtc::kEnumCounterIceRegathering, + static_cast(IceRegatheringReason::NETWORK_FAILURE))); + EXPECT_EQ(0, GetMetricsObserver(1)->GetEnumCounter( + webrtc::kEnumCounterIceRegathering, static_cast(IceRegatheringReason::NETWORK_FAILURE))); - EXPECT_EQ(0, GetEndpoint(1)->GetIceRegatheringCountForReason( - IceRegatheringReason::NETWORK_FAILURE)); DestroyChannels(); } @@ -1431,14 +1433,13 @@ TEST_F(P2PTransportChannelTest, TestIceRegatherOnAllNetworksContinual) { const int kNetworkGatherDuration = 11000; SIMULATED_WAIT(false, kNetworkGatherDuration, clock); // Expect regathering to happen 5 times in 11s with 2s interval. - EXPECT_LE(5, GetEndpoint(0)->GetIceRegatheringCountForReason( - IceRegatheringReason::OCCASIONAL_REFRESH)); - EXPECT_LE(5, webrtc::metrics::NumEvents( - "WebRTC.PeerConnection.IceRegatheringReason", + EXPECT_LE(5, GetMetricsObserver(0)->GetEnumCounter( + webrtc::kEnumCounterIceRegathering, static_cast(IceRegatheringReason::OCCASIONAL_REFRESH))); // Expect no regathering if continual gathering not configured. - EXPECT_EQ(0, GetEndpoint(1)->GetIceRegatheringCountForReason( - IceRegatheringReason::OCCASIONAL_REFRESH)); + EXPECT_EQ(0, GetMetricsObserver(1)->GetEnumCounter( + webrtc::kEnumCounterIceRegathering, + static_cast(IceRegatheringReason::OCCASIONAL_REFRESH))); DestroyChannels(); } @@ -1483,8 +1484,10 @@ class P2PTransportRegatherAllNetworksTest : public P2PTransportChannelTest { const int kWaitRegather = kRegatherInterval * kNumRegathers + kRegatherInterval / 2; SIMULATED_WAIT(false, kWaitRegather, clock); - EXPECT_EQ(kNumRegathers, GetEndpoint(0)->GetIceRegatheringCountForReason( - IceRegatheringReason::OCCASIONAL_REFRESH)); + EXPECT_EQ(kNumRegathers, + GetMetricsObserver(0)->GetEnumCounter( + webrtc::kEnumCounterIceRegathering, + static_cast(IceRegatheringReason::OCCASIONAL_REFRESH))); const Connection* new_selected = ep1_ch1()->selected_connection(); diff --git a/p2p/base/regatheringcontroller_unittest.cc b/p2p/base/regatheringcontroller_unittest.cc index 6ae64e8658d..18b23e05362 100644 --- a/p2p/base/regatheringcontroller_unittest.cc +++ b/p2p/base/regatheringcontroller_unittest.cc @@ -13,6 +13,7 @@ #include #include +#include "api/fakemetricsobserver.h" #include "p2p/base/fakeportallocator.h" #include "p2p/base/mockicetransport.h" #include "p2p/base/p2pconstants.h" diff --git a/p2p/client/basicportallocator.cc b/p2p/client/basicportallocator.cc index eb14a377588..4335ddaad4b 100644 --- a/p2p/client/basicportallocator.cc +++ b/p2p/client/basicportallocator.cc @@ -16,6 +16,7 @@ #include #include +#include "api/umametrics.h" #include "p2p/base/basicpacketsocketfactory.h" #include "p2p/base/port.h" #include "p2p/base/relayport.h" @@ -27,7 +28,6 @@ #include "rtc_base/helpers.h" #include "rtc_base/ipaddress.h" #include "rtc_base/logging.h" -#include "system_wrappers/include/metrics.h" using rtc::CreateRandomId; @@ -196,6 +196,9 @@ void BasicPortAllocator::Construct() { void BasicPortAllocator::OnIceRegathering(PortAllocatorSession* session, IceRegatheringReason reason) { + if (!metrics_observer()) { + return; + } // If the session has not been taken by an active channel, do not report the // metric. for (auto& allocator_session : pooled_sessions()) { @@ -204,9 +207,9 @@ void BasicPortAllocator::OnIceRegathering(PortAllocatorSession* session, } } - RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.IceRegatheringReason", - static_cast(reason), - static_cast(IceRegatheringReason::MAX_VALUE)); + metrics_observer()->IncrementEnumCounter( + webrtc::kEnumCounterIceRegathering, static_cast(reason), + static_cast(IceRegatheringReason::MAX_VALUE)); } BasicPortAllocator::~BasicPortAllocator() { diff --git a/p2p/client/basicportallocator_unittest.cc b/p2p/client/basicportallocator_unittest.cc index d5fc0bd2543..d995eb18dee 100644 --- a/p2p/client/basicportallocator_unittest.cc +++ b/p2p/client/basicportallocator_unittest.cc @@ -34,7 +34,6 @@ #include "rtc_base/ssladapter.h" #include "rtc_base/thread.h" #include "rtc_base/virtualsocketserver.h" -#include "system_wrappers/include/metrics_default.h" using rtc::IPAddress; using rtc::SocketAddress; @@ -2369,21 +2368,4 @@ TEST_F(BasicPortAllocatorTest, expected_stun_keepalive_interval); } -TEST_F(BasicPortAllocatorTest, IceRegatheringMetricsLoggedWhenNetworkChanges) { - // Only test local ports to simplify test. - ResetWithNoServersOrNat(); - AddInterface(kClientAddr, "test_net0"); - ASSERT_TRUE(CreateSession(ICE_CANDIDATE_COMPONENT_RTP)); - session_->StartGettingPorts(); - EXPECT_TRUE_SIMULATED_WAIT(candidate_allocation_done_, - kDefaultAllocationTimeout, fake_clock); - candidate_allocation_done_ = false; - AddInterface(kClientAddr2, "test_net1"); - EXPECT_TRUE_SIMULATED_WAIT(candidate_allocation_done_, - kDefaultAllocationTimeout, fake_clock); - EXPECT_EQ(1, webrtc::metrics::NumEvents( - "WebRTC.PeerConnection.IceRegatheringReason", - static_cast(IceRegatheringReason::NETWORK_CHANGE))); -} - } // namespace cricket diff --git a/pc/BUILD.gn b/pc/BUILD.gn index 329d66730f6..195d18518db 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn @@ -206,7 +206,6 @@ rtc_static_library("peerconnection") { "../stats", "../system_wrappers", "../system_wrappers:field_trial_api", - "../system_wrappers:metrics_api", "//third_party/abseil-cpp/absl/memory", "//third_party/abseil-cpp/absl/types:optional", ] @@ -297,6 +296,7 @@ if (rtc_include_tests) { ":rtc_pc", ":rtc_pc_base", "../api:array_view", + "../api:fakemetricsobserver", "../api:libjingle_peerconnection_api", "../call:rtp_interfaces", "../logging:rtc_event_log_api", @@ -334,6 +334,7 @@ if (rtc_include_tests) { ] deps = [ ":pc_test_utils", + "../api:fakemetricsobserver", "../api:libjingle_peerconnection_api", "../api:libjingle_peerconnection_test_api", "../api:rtc_stats_api", @@ -493,6 +494,7 @@ if (rtc_include_tests) { ":pc_test_utils", "..:webrtc_common", "../api:callfactory_api", + "../api:fakemetricsobserver", "../api:libjingle_peerconnection_test_api", "../api:rtc_stats_api", "../api/audio_codecs:audio_codecs_api", diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index ea5d9228ef1..89f35c2131d 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -52,7 +52,6 @@ #include "rtc_base/trace_event.h" #include "system_wrappers/include/clock.h" #include "system_wrappers/include/field_trial.h" -#include "system_wrappers/include/metrics.h" using cricket::ContentInfo; using cricket::ContentInfos; @@ -384,10 +383,15 @@ bool MediaSectionsHaveSameCount(const SessionDescription& desc1, return desc1.contents().size() == desc2.contents().size(); } -void NoteKeyProtocolAndMedia(KeyExchangeProtocolType protocol_type, - cricket::MediaType media_type) { - RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.KeyProtocol", protocol_type, - kEnumCounterKeyProtocolMax); +void NoteKeyProtocolAndMedia( + KeyExchangeProtocolType protocol_type, + cricket::MediaType media_type, + rtc::scoped_refptr uma_observer) { + if (!uma_observer) + return; + uma_observer->IncrementEnumCounter(webrtc::kEnumCounterKeyProtocol, + protocol_type, + webrtc::kEnumCounterKeyProtocolMax); static const std::map, KeyExchangeProtocolMedia> proto_media_counter_map = { @@ -406,8 +410,9 @@ void NoteKeyProtocolAndMedia(KeyExchangeProtocolType protocol_type, auto it = proto_media_counter_map.find({protocol_type, media_type}); if (it != proto_media_counter_map.end()) { - RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.KeyProtocolByMedia", - it->second, kEnumCounterKeyProtocolMediaTypeMax); + uma_observer->IncrementEnumCounter(webrtc::kEnumCounterKeyProtocolMediaType, + it->second, + kEnumCounterKeyProtocolMediaTypeMax); } } @@ -417,7 +422,9 @@ void NoteKeyProtocolAndMedia(KeyExchangeProtocolType protocol_type, // needs a ufrag and pwd. Mismatches, such as replying with a DTLS fingerprint // to SDES keys, will be caught in JsepTransport negotiation, and backstopped // by Channel's |srtp_required| check. -RTCError VerifyCrypto(const SessionDescription* desc, bool dtls_enabled) { +RTCError VerifyCrypto(const SessionDescription* desc, + bool dtls_enabled, + rtc::scoped_refptr uma_observer) { const cricket::ContentGroup* bundle = desc->GetGroupByName(cricket::GROUP_TYPE_BUNDLE); for (const cricket::ContentInfo& content_info : desc->contents()) { @@ -427,7 +434,8 @@ RTCError VerifyCrypto(const SessionDescription* desc, bool dtls_enabled) { // Note what media is used with each crypto protocol, for all sections. NoteKeyProtocolAndMedia(dtls_enabled ? webrtc::kEnumCounterKeyProtocolDtls : webrtc::kEnumCounterKeyProtocolSdes, - content_info.media_description()->type()); + content_info.media_description()->type(), + uma_observer); const std::string& mid = content_info.name; if (bundle && bundle->HasContentName(mid) && mid != *(bundle->FirstContentName())) { @@ -931,16 +939,6 @@ bool PeerConnection::Initialize( NoteUsageEvent(UsageEvent::TURN_SERVER_ADDED); } - // Send information about IPv4/IPv6 status. - PeerConnectionAddressFamilyCounter address_family; - if (port_allocator_flags_ & cricket::PORTALLOCATOR_ENABLE_IPV6) { - address_family = kPeerConnection_IPv6; - } else { - address_family = kPeerConnection_IPv4; - } - RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.IPMetrics", address_family, - kPeerConnectionAddressFamilyCounter_Max); - const PeerConnectionFactoryInterface::Options& options = factory_->options(); // RFC 3264: The numeric value of the session id and version in the @@ -3061,6 +3059,18 @@ void PeerConnection::RegisterUMAObserver(UMAObserver* observer) { network_thread()->Invoke( RTC_FROM_HERE, rtc::Bind(&PeerConnection::SetMetricObserver_n, this, observer)); + // Send information about IPv4/IPv6 status. + if (uma_observer_) { + if (port_allocator_flags_ & cricket::PORTALLOCATOR_ENABLE_IPV6) { + uma_observer_->IncrementEnumCounter( + kEnumCounterAddressFamily, kPeerConnection_IPv6, + kPeerConnectionAddressFamilyCounter_Max); + } else { + uma_observer_->IncrementEnumCounter( + kEnumCounterAddressFamily, kPeerConnection_IPv4, + kPeerConnectionAddressFamilyCounter_Max); + } + } } void PeerConnection::SetMetricObserver_n(UMAObserver* observer) { @@ -5284,7 +5294,9 @@ void PeerConnection::OnTransportControllerConnectionState( } SetIceConnectionState(PeerConnectionInterface::kIceConnectionCompleted); NoteUsageEvent(UsageEvent::ICE_STATE_CONNECTED); - ReportTransportStats(); + if (metrics_observer()) { + ReportTransportStats(); + } break; default: RTC_NOTREACHED(); @@ -5336,9 +5348,11 @@ void PeerConnection::OnTransportControllerCandidatesRemoved( void PeerConnection::OnTransportControllerDtlsHandshakeError( rtc::SSLHandshakeError error) { - RTC_HISTOGRAM_ENUMERATION( - "WebRTC.PeerConnection.DtlsHandshakeError", static_cast(error), - static_cast(rtc::SSLHandshakeError::MAX_VALUE)); + if (metrics_observer()) { + metrics_observer()->IncrementEnumCounter( + webrtc::kEnumCounterDtlsHandshakeError, static_cast(error), + static_cast(rtc::SSLHandshakeError::MAX_VALUE)); + } } void PeerConnection::EnableSending() { @@ -5776,7 +5790,8 @@ RTCError PeerConnection::ValidateSessionDescription( std::string crypto_error; if (webrtc_session_desc_factory_->SdesPolicy() == cricket::SEC_REQUIRED || dtls_enabled_) { - RTCError crypto_error = VerifyCrypto(sdesc->description(), dtls_enabled_); + RTCError crypto_error = + VerifyCrypto(sdesc->description(), dtls_enabled_, uma_observer_); if (!crypto_error.ok()) { return crypto_error; } @@ -5903,6 +5918,9 @@ std::string PeerConnection::GetSessionErrorMsg() { void PeerConnection::ReportSdpFormatReceived( const SessionDescriptionInterface& remote_offer) { + if (!uma_observer_) { + return; + } int num_audio_mlines = 0; int num_video_mlines = 0; int num_audio_tracks = 0; @@ -5927,8 +5945,8 @@ void PeerConnection::ReportSdpFormatReceived( } else if (num_audio_tracks > 0 || num_video_tracks > 0) { format = kSdpFormatReceivedSimple; } - RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.SdpFormatReceived", format, - kSdpFormatReceivedMax); + uma_observer_->IncrementEnumCounter(kEnumCounterSdpFormatReceived, format, + kSdpFormatReceivedMax); } void PeerConnection::NoteUsageEvent(UsageEvent event) { @@ -5938,33 +5956,42 @@ void PeerConnection::NoteUsageEvent(UsageEvent event) { void PeerConnection::ReportUsagePattern() const { RTC_DLOG(LS_INFO) << "Usage signature is " << usage_event_accumulator_; - RTC_HISTOGRAM_ENUMERATION_SPARSE("WebRTC.PeerConnection.UsagePattern", - usage_event_accumulator_, - static_cast(UsageEvent::MAX_VALUE)); + if (uma_observer_) { + uma_observer_->IncrementSparseEnumCounter(kEnumCounterUsagePattern, + usage_event_accumulator_); + } } void PeerConnection::ReportNegotiatedSdpSemantics( const SessionDescriptionInterface& answer) { - SdpSemanticNegotiated semantics_negotiated; + if (!uma_observer_) { + return; + } switch (answer.description()->msid_signaling()) { case 0: - semantics_negotiated = kSdpSemanticNegotiatedNone; + uma_observer_->IncrementEnumCounter(kEnumCounterSdpSemanticNegotiated, + kSdpSemanticNegotiatedNone, + kSdpSemanticNegotiatedMax); break; case cricket::kMsidSignalingMediaSection: - semantics_negotiated = kSdpSemanticNegotiatedUnifiedPlan; + uma_observer_->IncrementEnumCounter(kEnumCounterSdpSemanticNegotiated, + kSdpSemanticNegotiatedUnifiedPlan, + kSdpSemanticNegotiatedMax); break; case cricket::kMsidSignalingSsrcAttribute: - semantics_negotiated = kSdpSemanticNegotiatedPlanB; + uma_observer_->IncrementEnumCounter(kEnumCounterSdpSemanticNegotiated, + kSdpSemanticNegotiatedPlanB, + kSdpSemanticNegotiatedMax); break; case cricket::kMsidSignalingMediaSection | cricket::kMsidSignalingSsrcAttribute: - semantics_negotiated = kSdpSemanticNegotiatedMixed; + uma_observer_->IncrementEnumCounter(kEnumCounterSdpSemanticNegotiated, + kSdpSemanticNegotiatedMixed, + kSdpSemanticNegotiatedMax); break; default: RTC_NOTREACHED(); } - RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.SdpSemanticNegotiated", - semantics_negotiated, kSdpSemanticNegotiatedMax); } // We need to check the local/remote description for the Transport instead of @@ -6058,6 +6085,7 @@ void PeerConnection::ReportTransportStats() { // for IPv4 and IPv6. void PeerConnection::ReportBestConnectionState( const cricket::TransportStats& stats) { + RTC_DCHECK(metrics_observer()); for (const cricket::TransportChannelStats& channel_stats : stats.channel_stats) { for (const cricket::ConnectionInfo& connection_info : @@ -6066,6 +6094,7 @@ void PeerConnection::ReportBestConnectionState( continue; } + PeerConnectionEnumCounterType type = kPeerConnectionEnumCounterMax; const cricket::Candidate& local = connection_info.local_candidate; const cricket::Candidate& remote = connection_info.remote_candidate; @@ -6073,26 +6102,26 @@ void PeerConnection::ReportBestConnectionState( if (local.protocol() == cricket::TCP_PROTOCOL_NAME || (local.type() == RELAY_PORT_TYPE && local.relay_protocol() == cricket::TCP_PROTOCOL_NAME)) { - RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.CandidatePairType_TCP", - GetIceCandidatePairCounter(local, remote), - kIceCandidatePairMax); + type = kEnumCounterIceCandidatePairTypeTcp; } else if (local.protocol() == cricket::UDP_PROTOCOL_NAME) { - RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.CandidatePairType_UDP", - GetIceCandidatePairCounter(local, remote), - kIceCandidatePairMax); + type = kEnumCounterIceCandidatePairTypeUdp; } else { RTC_CHECK(0); } + metrics_observer()->IncrementEnumCounter( + type, GetIceCandidatePairCounter(local, remote), + kIceCandidatePairMax); // Increment the counter for IP type. if (local.address().family() == AF_INET) { - RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.IPMetrics", - kBestConnections_IPv4, - kPeerConnectionAddressFamilyCounter_Max); + metrics_observer()->IncrementEnumCounter( + kEnumCounterAddressFamily, kBestConnections_IPv4, + kPeerConnectionAddressFamilyCounter_Max); + } else if (local.address().family() == AF_INET6) { - RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.IPMetrics", - kBestConnections_IPv6, - kPeerConnectionAddressFamilyCounter_Max); + metrics_observer()->IncrementEnumCounter( + kEnumCounterAddressFamily, kBestConnections_IPv6, + kPeerConnectionAddressFamilyCounter_Max); } else { RTC_CHECK(0); } @@ -6105,6 +6134,7 @@ void PeerConnection::ReportBestConnectionState( void PeerConnection::ReportNegotiatedCiphers( const cricket::TransportStats& stats, const std::set& media_types) { + RTC_DCHECK(metrics_observer()); if (!dtls_enabled_ || stats.channel_stats.empty()) { return; } @@ -6116,53 +6146,33 @@ void PeerConnection::ReportNegotiatedCiphers( return; } - if (srtp_crypto_suite != rtc::SRTP_INVALID_CRYPTO_SUITE) { - for (cricket::MediaType media_type : media_types) { - switch (media_type) { - case cricket::MEDIA_TYPE_AUDIO: - RTC_HISTOGRAM_ENUMERATION_SPARSE( - "WebRTC.PeerConnection.SrtpCryptoSuite.Audio", srtp_crypto_suite, - rtc::SRTP_CRYPTO_SUITE_MAX_VALUE); - break; - case cricket::MEDIA_TYPE_VIDEO: - RTC_HISTOGRAM_ENUMERATION_SPARSE( - "WebRTC.PeerConnection.SrtpCryptoSuite.Video", srtp_crypto_suite, - rtc::SRTP_CRYPTO_SUITE_MAX_VALUE); - break; - case cricket::MEDIA_TYPE_DATA: - RTC_HISTOGRAM_ENUMERATION_SPARSE( - "WebRTC.PeerConnection.SrtpCryptoSuite.Data", srtp_crypto_suite, - rtc::SRTP_CRYPTO_SUITE_MAX_VALUE); - break; - default: - RTC_NOTREACHED(); - continue; - } + for (cricket::MediaType media_type : media_types) { + PeerConnectionEnumCounterType srtp_counter_type; + PeerConnectionEnumCounterType ssl_counter_type; + switch (media_type) { + case cricket::MEDIA_TYPE_AUDIO: + srtp_counter_type = kEnumCounterAudioSrtpCipher; + ssl_counter_type = kEnumCounterAudioSslCipher; + break; + case cricket::MEDIA_TYPE_VIDEO: + srtp_counter_type = kEnumCounterVideoSrtpCipher; + ssl_counter_type = kEnumCounterVideoSslCipher; + break; + case cricket::MEDIA_TYPE_DATA: + srtp_counter_type = kEnumCounterDataSrtpCipher; + ssl_counter_type = kEnumCounterDataSslCipher; + break; + default: + RTC_NOTREACHED(); + continue; } - } - - if (ssl_cipher_suite != rtc::TLS_NULL_WITH_NULL_NULL) { - for (cricket::MediaType media_type : media_types) { - switch (media_type) { - case cricket::MEDIA_TYPE_AUDIO: - RTC_HISTOGRAM_ENUMERATION_SPARSE( - "WebRTC.PeerConnection.SslCipherSuite.Audio", ssl_cipher_suite, - rtc::SSL_CIPHER_SUITE_MAX_VALUE); - break; - case cricket::MEDIA_TYPE_VIDEO: - RTC_HISTOGRAM_ENUMERATION_SPARSE( - "WebRTC.PeerConnection.SslCipherSuite.Video", ssl_cipher_suite, - rtc::SSL_CIPHER_SUITE_MAX_VALUE); - break; - case cricket::MEDIA_TYPE_DATA: - RTC_HISTOGRAM_ENUMERATION_SPARSE( - "WebRTC.PeerConnection.SslCipherSuite.Data", ssl_cipher_suite, - rtc::SSL_CIPHER_SUITE_MAX_VALUE); - break; - default: - RTC_NOTREACHED(); - continue; - } + if (srtp_crypto_suite != rtc::SRTP_INVALID_CRYPTO_SUITE) { + metrics_observer()->IncrementSparseEnumCounter(srtp_counter_type, + srtp_crypto_suite); + } + if (ssl_cipher_suite != rtc::TLS_NULL_WITH_NULL_NULL) { + metrics_observer()->IncrementSparseEnumCounter(ssl_counter_type, + ssl_cipher_suite); } } } diff --git a/pc/peerconnection.h b/pc/peerconnection.h index 44b66c36583..7d7315d4e4a 100644 --- a/pc/peerconnection.h +++ b/pc/peerconnection.h @@ -66,8 +66,7 @@ class PeerConnection : public PeerConnectionInternal, CANDIDATE_COLLECTED = 0x80, REMOTE_CANDIDATE_ADDED = 0x100, ICE_STATE_CONNECTED = 0x200, - CLOSE_CALLED = 0x400, - MAX_VALUE = 0x800, + CLOSE_CALLED = 0x400 }; explicit PeerConnection(PeerConnectionFactory* factory, diff --git a/pc/peerconnection_histogram_unittest.cc b/pc/peerconnection_histogram_unittest.cc index 64c4cfe897d..b9983727d9a 100644 --- a/pc/peerconnection_histogram_unittest.cc +++ b/pc/peerconnection_histogram_unittest.cc @@ -11,6 +11,7 @@ #include #include "absl/memory/memory.h" +#include "api/fakemetricsobserver.h" #include "api/peerconnectionproxy.h" #include "media/base/fakemediaengine.h" #include "pc/mediasession.h" @@ -21,7 +22,6 @@ #include "pc/test/fakesctptransport.h" #include "rtc_base/gunit.h" #include "rtc_base/virtualsocketserver.h" -#include "system_wrappers/include/metrics_default.h" namespace webrtc { @@ -127,7 +127,6 @@ class PeerConnectionUsageHistogramTest : public ::testing::Test { PeerConnectionUsageHistogramTest() : vss_(new rtc::VirtualSocketServer()), main_(vss_.get()) { - webrtc::metrics::Reset(); } WrapperPtr CreatePeerConnection() { @@ -176,12 +175,14 @@ class PeerConnectionUsageHistogramTest : public ::testing::Test { TEST_F(PeerConnectionUsageHistogramTest, UsageFingerprintHistogramFromTimeout) { auto pc = CreatePeerConnectionWithImmediateReport(); + // Register UMA observer before signaling begins. + rtc::scoped_refptr caller_observer = + new rtc::RefCountedObject(); + pc->GetInternalPeerConnection()->RegisterUMAObserver(caller_observer); int expected_fingerprint = MakeUsageFingerprint({}); - ASSERT_TRUE_WAIT( - 1u == webrtc::metrics::NumSamples("WebRTC.PeerConnection.UsagePattern"), - kDefaultTimeout); - EXPECT_EQ(1, webrtc::metrics::NumEvents("WebRTC.PeerConnection.UsagePattern", - expected_fingerprint)); + ASSERT_TRUE_WAIT(caller_observer->ExpectOnlySingleEnumCount( + webrtc::kEnumCounterUsagePattern, expected_fingerprint), + kDefaultTimeout); } #ifndef WEBRTC_ANDROID @@ -192,6 +193,9 @@ TEST_F(PeerConnectionUsageHistogramTest, UsageFingerprintHistogramFromTimeout) { TEST_F(PeerConnectionUsageHistogramTest, FingerprintAudioVideo) { auto caller = CreatePeerConnection(); auto callee = CreatePeerConnection(); + // Register UMA observer before signaling begins. + auto caller_observer = caller->RegisterFakeMetricsObserver(); + auto callee_observer = callee->RegisterFakeMetricsObserver(); caller->AddAudioTrack("audio"); caller->AddVideoTrack("video"); caller->PrepareToExchangeCandidates(callee.get()); @@ -209,16 +213,19 @@ TEST_F(PeerConnectionUsageHistogramTest, FingerprintAudioVideo) { PeerConnection::UsageEvent::REMOTE_CANDIDATE_ADDED, PeerConnection::UsageEvent::ICE_STATE_CONNECTED, PeerConnection::UsageEvent::CLOSE_CALLED}); - EXPECT_EQ(2, - webrtc::metrics::NumSamples("WebRTC.PeerConnection.UsagePattern")); - EXPECT_EQ(2, webrtc::metrics::NumEvents("WebRTC.PeerConnection.UsagePattern", - expected_fingerprint)); + EXPECT_TRUE(caller_observer->ExpectOnlySingleEnumCount( + webrtc::kEnumCounterUsagePattern, expected_fingerprint)); + EXPECT_TRUE(callee_observer->ExpectOnlySingleEnumCount( + webrtc::kEnumCounterUsagePattern, expected_fingerprint)); } #ifdef HAVE_SCTP TEST_F(PeerConnectionUsageHistogramTest, FingerprintDataOnly) { auto caller = CreatePeerConnection(); auto callee = CreatePeerConnection(); + // Register UMA observer before signaling begins. + auto caller_observer = caller->RegisterFakeMetricsObserver(); + auto callee_observer = callee->RegisterFakeMetricsObserver(); caller->CreateDataChannel("foodata"); caller->PrepareToExchangeCandidates(callee.get()); ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); @@ -233,10 +240,10 @@ TEST_F(PeerConnectionUsageHistogramTest, FingerprintDataOnly) { PeerConnection::UsageEvent::REMOTE_CANDIDATE_ADDED, PeerConnection::UsageEvent::ICE_STATE_CONNECTED, PeerConnection::UsageEvent::CLOSE_CALLED}); - EXPECT_EQ(2, - webrtc::metrics::NumSamples("WebRTC.PeerConnection.UsagePattern")); - EXPECT_EQ(2, webrtc::metrics::NumEvents("WebRTC.PeerConnection.UsagePattern", - expected_fingerprint)); + EXPECT_TRUE(caller_observer->ExpectOnlySingleEnumCount( + webrtc::kEnumCounterUsagePattern, expected_fingerprint)); + EXPECT_TRUE(callee_observer->ExpectOnlySingleEnumCount( + webrtc::kEnumCounterUsagePattern, expected_fingerprint)); } #endif // HAVE_SCTP #endif // WEBRTC_ANDROID @@ -252,15 +259,14 @@ TEST_F(PeerConnectionUsageHistogramTest, FingerprintStunTurn) { configuration.servers.push_back(server); auto caller = CreatePeerConnection(configuration); ASSERT_TRUE(caller); + auto caller_observer = caller->RegisterFakeMetricsObserver(); caller->pc()->Close(); int expected_fingerprint = MakeUsageFingerprint({PeerConnection::UsageEvent::STUN_SERVER_ADDED, PeerConnection::UsageEvent::TURN_SERVER_ADDED, PeerConnection::UsageEvent::CLOSE_CALLED}); - EXPECT_EQ(1, - webrtc::metrics::NumSamples("WebRTC.PeerConnection.UsagePattern")); - EXPECT_EQ(1, webrtc::metrics::NumEvents("WebRTC.PeerConnection.UsagePattern", - expected_fingerprint)); + EXPECT_TRUE(caller_observer->ExpectOnlySingleEnumCount( + webrtc::kEnumCounterUsagePattern, expected_fingerprint)); } TEST_F(PeerConnectionUsageHistogramTest, FingerprintStunTurnInReconfiguration) { @@ -274,6 +280,7 @@ TEST_F(PeerConnectionUsageHistogramTest, FingerprintStunTurnInReconfiguration) { configuration.servers.push_back(server); auto caller = CreatePeerConnection(); ASSERT_TRUE(caller); + auto caller_observer = caller->RegisterFakeMetricsObserver(); RTCError error; caller->pc()->SetConfiguration(configuration, &error); ASSERT_TRUE(error.ok()); @@ -282,10 +289,8 @@ TEST_F(PeerConnectionUsageHistogramTest, FingerprintStunTurnInReconfiguration) { MakeUsageFingerprint({PeerConnection::UsageEvent::STUN_SERVER_ADDED, PeerConnection::UsageEvent::TURN_SERVER_ADDED, PeerConnection::UsageEvent::CLOSE_CALLED}); - EXPECT_EQ(1, - webrtc::metrics::NumSamples("WebRTC.PeerConnection.UsagePattern")); - EXPECT_EQ(1, webrtc::metrics::NumEvents("WebRTC.PeerConnection.UsagePattern", - expected_fingerprint)); + EXPECT_TRUE(caller_observer->ExpectOnlySingleEnumCount( + webrtc::kEnumCounterUsagePattern, expected_fingerprint)); } } // namespace webrtc diff --git a/pc/peerconnection_integrationtest.cc b/pc/peerconnection_integrationtest.cc index 525eba35b31..ed5db1f585f 100644 --- a/pc/peerconnection_integrationtest.cc +++ b/pc/peerconnection_integrationtest.cc @@ -24,6 +24,7 @@ #include "api/audio_codecs/builtin_audio_decoder_factory.h" #include "api/audio_codecs/builtin_audio_encoder_factory.h" +#include "api/fakemetricsobserver.h" #include "api/mediastreaminterface.h" #include "api/peerconnectioninterface.h" #include "api/peerconnectionproxy.h" @@ -62,7 +63,6 @@ #include "rtc_base/numerics/safe_conversions.h" #include "rtc_base/testcertificateverifier.h" #include "rtc_base/virtualsocketserver.h" -#include "system_wrappers/include/metrics_default.h" #include "test/gmock.h" using cricket::ContentInfo; @@ -1106,7 +1106,6 @@ class PeerConnectionIntegrationBaseTest : public testing::Test { worker_thread_->SetName("PCWorkerThread", this); RTC_CHECK(network_thread_->Start()); RTC_CHECK(worker_thread_->Start()); - webrtc::metrics::Reset(); } ~PeerConnectionIntegrationBaseTest() { @@ -1514,17 +1513,20 @@ class PeerConnectionIntegrationBaseTest : public testing::Test { int expected_cipher_suite) { ASSERT_TRUE(CreatePeerConnectionWrappersWithOptions(caller_options, callee_options)); + rtc::scoped_refptr caller_observer = + new rtc::RefCountedObject(); + caller()->pc()->RegisterUMAObserver(caller_observer); ConnectFakeSignaling(); caller()->AddAudioVideoTracks(); callee()->AddAudioVideoTracks(); caller()->CreateAndSetAndSignalOffer(); - ASSERT_TRUE_WAIT(DtlsConnected(), kDefaultTimeout); + ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout); EXPECT_EQ_WAIT(rtc::SrtpCryptoSuiteToName(expected_cipher_suite), caller()->OldGetStats()->SrtpCipher(), kDefaultTimeout); - // TODO(bugs.webrtc.org/9456): Fix it. - EXPECT_EQ(1, webrtc::metrics::NumEvents( - "WebRTC.PeerConnection.SrtpCryptoSuite.Audio", - expected_cipher_suite)); + EXPECT_EQ( + 1, caller_observer->GetEnumCounter(webrtc::kEnumCounterAudioSrtpCipher, + expected_cipher_suite)); + caller()->pc()->RegisterUMAObserver(nullptr); } void TestGcmNegotiationUsesCipherSuite(bool local_gcm_enabled, @@ -1694,6 +1696,9 @@ TEST_P(PeerConnectionIntegrationTest, DtmfSenderObserver) { TEST_P(PeerConnectionIntegrationTest, EndToEndCallWithDtls) { ASSERT_TRUE(CreatePeerConnectionWrappers()); ConnectFakeSignaling(); + rtc::scoped_refptr caller_observer = + new rtc::RefCountedObject(); + caller()->pc()->RegisterUMAObserver(caller_observer); // Do normal offer/answer and wait for some frames to be received in each // direction. @@ -1704,10 +1709,12 @@ TEST_P(PeerConnectionIntegrationTest, EndToEndCallWithDtls) { MediaExpectations media_expectations; media_expectations.ExpectBidirectionalAudioAndVideo(); ASSERT_TRUE(ExpectNewFrames(media_expectations)); - EXPECT_LE(2, webrtc::metrics::NumEvents("WebRTC.PeerConnection.KeyProtocol", - webrtc::kEnumCounterKeyProtocolDtls)); - EXPECT_EQ(0, webrtc::metrics::NumEvents("WebRTC.PeerConnection.KeyProtocol", - webrtc::kEnumCounterKeyProtocolSdes)); + EXPECT_LE( + 1, caller_observer->GetEnumCounter(webrtc::kEnumCounterKeyProtocol, + webrtc::kEnumCounterKeyProtocolDtls)); + EXPECT_EQ( + 0, caller_observer->GetEnumCounter(webrtc::kEnumCounterKeyProtocol, + webrtc::kEnumCounterKeyProtocolSdes)); } // Uses SDES instead of DTLS for key agreement. @@ -1716,6 +1723,9 @@ TEST_P(PeerConnectionIntegrationTest, EndToEndCallWithSdes) { sdes_config.enable_dtls_srtp.emplace(false); ASSERT_TRUE(CreatePeerConnectionWrappersWithConfig(sdes_config, sdes_config)); ConnectFakeSignaling(); + rtc::scoped_refptr caller_observer = + new rtc::RefCountedObject(); + caller()->pc()->RegisterUMAObserver(caller_observer); // Do normal offer/answer and wait for some frames to be received in each // direction. @@ -1726,10 +1736,12 @@ TEST_P(PeerConnectionIntegrationTest, EndToEndCallWithSdes) { MediaExpectations media_expectations; media_expectations.ExpectBidirectionalAudioAndVideo(); ASSERT_TRUE(ExpectNewFrames(media_expectations)); - EXPECT_LE(2, webrtc::metrics::NumEvents("WebRTC.PeerConnection.KeyProtocol", - webrtc::kEnumCounterKeyProtocolSdes)); - EXPECT_EQ(0, webrtc::metrics::NumEvents("WebRTC.PeerConnection.KeyProtocol", - webrtc::kEnumCounterKeyProtocolDtls)); + EXPECT_LE( + 1, caller_observer->GetEnumCounter(webrtc::kEnumCounterKeyProtocol, + webrtc::kEnumCounterKeyProtocolSdes)); + EXPECT_EQ( + 0, caller_observer->GetEnumCounter(webrtc::kEnumCounterKeyProtocol, + webrtc::kEnumCounterKeyProtocolDtls)); } // Tests that the GetRemoteAudioSSLCertificate method returns the remote DTLS @@ -2731,19 +2743,22 @@ TEST_P(PeerConnectionIntegrationTest, Dtls10CipherStatsAndUmaMetrics) { ASSERT_TRUE(CreatePeerConnectionWrappersWithOptions(dtls_10_options, dtls_10_options)); ConnectFakeSignaling(); + // Register UMA observer before signaling begins. + rtc::scoped_refptr caller_observer = + new rtc::RefCountedObject(); + caller()->pc()->RegisterUMAObserver(caller_observer); caller()->AddAudioVideoTracks(); callee()->AddAudioVideoTracks(); caller()->CreateAndSetAndSignalOffer(); - ASSERT_TRUE_WAIT(DtlsConnected(), kDefaultTimeout); + ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout); EXPECT_TRUE_WAIT(rtc::SSLStreamAdapter::IsAcceptableCipher( caller()->OldGetStats()->DtlsCipher(), rtc::KT_DEFAULT), kDefaultTimeout); EXPECT_EQ_WAIT(rtc::SrtpCryptoSuiteToName(kDefaultSrtpCryptoSuite), caller()->OldGetStats()->SrtpCipher(), kDefaultTimeout); - // TODO(bugs.webrtc.org/9456): Fix it. - EXPECT_EQ(1, webrtc::metrics::NumEvents( - "WebRTC.PeerConnection.SrtpCryptoSuite.Audio", - kDefaultSrtpCryptoSuite)); + EXPECT_EQ(1, + caller_observer->GetEnumCounter(webrtc::kEnumCounterAudioSrtpCipher, + kDefaultSrtpCryptoSuite)); } // Test getting cipher stats and UMA metrics when DTLS 1.2 is negotiated. @@ -2753,19 +2768,22 @@ TEST_P(PeerConnectionIntegrationTest, Dtls12CipherStatsAndUmaMetrics) { ASSERT_TRUE(CreatePeerConnectionWrappersWithOptions(dtls_12_options, dtls_12_options)); ConnectFakeSignaling(); + // Register UMA observer before signaling begins. + rtc::scoped_refptr caller_observer = + new rtc::RefCountedObject(); + caller()->pc()->RegisterUMAObserver(caller_observer); caller()->AddAudioVideoTracks(); callee()->AddAudioVideoTracks(); caller()->CreateAndSetAndSignalOffer(); - ASSERT_TRUE_WAIT(DtlsConnected(), kDefaultTimeout); + ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout); EXPECT_TRUE_WAIT(rtc::SSLStreamAdapter::IsAcceptableCipher( caller()->OldGetStats()->DtlsCipher(), rtc::KT_DEFAULT), kDefaultTimeout); EXPECT_EQ_WAIT(rtc::SrtpCryptoSuiteToName(kDefaultSrtpCryptoSuite), caller()->OldGetStats()->SrtpCipher(), kDefaultTimeout); - // TODO(bugs.webrtc.org/9456): Fix it. - EXPECT_EQ(1, webrtc::metrics::NumEvents( - "WebRTC.PeerConnection.SrtpCryptoSuite.Audio", - kDefaultSrtpCryptoSuite)); + EXPECT_EQ(1, + caller_observer->GetEnumCounter(webrtc::kEnumCounterAudioSrtpCipher, + kDefaultSrtpCryptoSuite)); } // Test that DTLS 1.0 can be used if the caller supports DTLS 1.2 and the @@ -3484,15 +3502,19 @@ TEST_P(PeerConnectionIntegrationIceStatesTest, VerifyBestConnection) { SetUpNetworkInterfaces(); caller()->AddAudioVideoTracks(); callee()->AddAudioVideoTracks(); + + rtc::scoped_refptr metrics_observer( + new rtc::RefCountedObject()); + caller()->pc()->RegisterUMAObserver(metrics_observer.get()); + caller()->CreateAndSetAndSignalOffer(); ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout); - // TODO(bugs.webrtc.org/9456): Fix it. - const int num_best_ipv4 = webrtc::metrics::NumEvents( - "WebRTC.PeerConnection.IPMetrics", webrtc::kBestConnections_IPv4); - const int num_best_ipv6 = webrtc::metrics::NumEvents( - "WebRTC.PeerConnection.IPMetrics", webrtc::kBestConnections_IPv6); + const int num_best_ipv4 = metrics_observer->GetEnumCounter( + webrtc::kEnumCounterAddressFamily, webrtc::kBestConnections_IPv4); + const int num_best_ipv6 = metrics_observer->GetEnumCounter( + webrtc::kEnumCounterAddressFamily, webrtc::kBestConnections_IPv6); if (TestIPv6()) { // When IPv6 is enabled, we should prefer an IPv6 connection over an IPv4 // connection. @@ -3503,12 +3525,12 @@ TEST_P(PeerConnectionIntegrationIceStatesTest, VerifyBestConnection) { EXPECT_EQ(0, num_best_ipv6); } - EXPECT_EQ(0, webrtc::metrics::NumEvents( - "WebRTC.PeerConnection.CandidatePairType_UDP", - webrtc::kIceCandidatePairHostHost)); - EXPECT_EQ(1, webrtc::metrics::NumEvents( - "WebRTC.PeerConnection.CandidatePairType_UDP", - webrtc::kIceCandidatePairHostPublicHostPublic)); + EXPECT_EQ(0, metrics_observer->GetEnumCounter( + webrtc::kEnumCounterIceCandidatePairTypeUdp, + webrtc::kIceCandidatePairHostHost)); + EXPECT_EQ(1, metrics_observer->GetEnumCounter( + webrtc::kEnumCounterIceCandidatePairTypeUdp, + webrtc::kIceCandidatePairHostPublicHostPublic)); } constexpr uint32_t kFlagsIPv4NoStun = cricket::PORTALLOCATOR_DISABLE_TCP | diff --git a/pc/peerconnection_rtp_unittest.cc b/pc/peerconnection_rtp_unittest.cc index df1b1eef1b2..50fe0dc6c8a 100644 --- a/pc/peerconnection_rtp_unittest.cc +++ b/pc/peerconnection_rtp_unittest.cc @@ -17,6 +17,7 @@ #include "api/jsep.h" #include "api/mediastreaminterface.h" #include "api/peerconnectioninterface.h" +#include "api/umametrics.h" #include "api/video_codecs/builtin_video_decoder_factory.h" #include "api/video_codecs/builtin_video_encoder_factory.h" #include "pc/mediasession.h" @@ -31,7 +32,6 @@ #include "rtc_base/refcountedobject.h" #include "rtc_base/scoped_ref_ptr.h" #include "rtc_base/thread.h" -#include "system_wrappers/include/metrics_default.h" #include "test/gmock.h" // This file contains tests for RTP Media API-related behavior of @@ -77,9 +77,7 @@ class PeerConnectionRtpBaseTest : public testing::Test { CreateBuiltinVideoEncoderFactory(), CreateBuiltinVideoDecoderFactory(), nullptr /* audio_mixer */, - nullptr /* audio_processing */)) { - webrtc::metrics::Reset(); - } + nullptr /* audio_processing */)) {} std::unique_ptr CreatePeerConnection() { return CreatePeerConnection(RTCConfiguration()); @@ -1371,6 +1369,7 @@ TEST_F(PeerConnectionMsidSignalingTest, UnifiedPlanTalkingToOurself) { caller->AddAudioTrack("caller_audio"); auto callee = CreatePeerConnectionWithUnifiedPlan(); callee->AddAudioTrack("callee_audio"); + auto caller_observer = caller->RegisterFakeMetricsObserver(); ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); @@ -1385,11 +1384,8 @@ TEST_F(PeerConnectionMsidSignalingTest, UnifiedPlanTalkingToOurself) { EXPECT_EQ(cricket::kMsidSignalingMediaSection, answer->description()->msid_signaling()); // Check that this is counted correctly - EXPECT_EQ(2, webrtc::metrics::NumSamples( - "WebRTC.PeerConnection.SdpSemanticNegotiated")); - EXPECT_EQ(2, webrtc::metrics::NumEvents( - "WebRTC.PeerConnection.SdpSemanticNegotiated", - kSdpSemanticNegotiatedUnifiedPlan)); + EXPECT_TRUE(caller_observer->ExpectOnlySingleEnumCount( + kEnumCounterSdpSemanticNegotiated, kSdpSemanticNegotiatedUnifiedPlan)); } TEST_F(PeerConnectionMsidSignalingTest, PlanBOfferToUnifiedPlanAnswer) { @@ -1474,14 +1470,12 @@ TEST_F(SdpFormatReceivedTest, DataChannelOnlyIsReportedAsNoTracks) { auto caller = CreatePeerConnectionWithUnifiedPlan(); caller->CreateDataChannel("dc"); auto callee = CreatePeerConnectionWithUnifiedPlan(); + auto callee_metrics = callee->RegisterFakeMetricsObserver(); ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); - // Note that only the callee does ReportSdpFormatReceived. - EXPECT_EQ(1, webrtc::metrics::NumSamples( - "WebRTC.PeerConnection.SdpFormatReceived")); - EXPECT_EQ( - 1, webrtc::metrics::NumEvents("WebRTC.PeerConnection.SdpFormatReceived", - kSdpFormatReceivedNoTracks)); + + EXPECT_TRUE(callee_metrics->ExpectOnlySingleEnumCount( + kEnumCounterSdpFormatReceived, kSdpFormatReceivedNoTracks)); } #endif // HAVE_SCTP @@ -1490,28 +1484,24 @@ TEST_F(SdpFormatReceivedTest, SimpleUnifiedPlanIsReportedAsSimple) { caller->AddAudioTrack("audio"); caller->AddVideoTrack("video"); auto callee = CreatePeerConnectionWithPlanB(); + auto callee_metrics = callee->RegisterFakeMetricsObserver(); ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); - // Note that only the callee does ReportSdpFormatReceived. - EXPECT_EQ(1, webrtc::metrics::NumSamples( - "WebRTC.PeerConnection.SdpFormatReceived")); - EXPECT_EQ( - 1, webrtc::metrics::NumEvents("WebRTC.PeerConnection.SdpFormatReceived", - kSdpFormatReceivedSimple)); + + EXPECT_TRUE(callee_metrics->ExpectOnlySingleEnumCount( + kEnumCounterSdpFormatReceived, kSdpFormatReceivedSimple)); } TEST_F(SdpFormatReceivedTest, SimplePlanBIsReportedAsSimple) { auto caller = CreatePeerConnectionWithPlanB(); caller->AddVideoTrack("video"); // Video only. auto callee = CreatePeerConnectionWithUnifiedPlan(); + auto callee_metrics = callee->RegisterFakeMetricsObserver(); ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); - EXPECT_EQ(1, webrtc::metrics::NumSamples( - "WebRTC.PeerConnection.SdpFormatReceived")); - EXPECT_EQ( - 1, webrtc::metrics::NumEvents("WebRTC.PeerConnection.SdpFormatReceived", - kSdpFormatReceivedSimple)); + EXPECT_TRUE(callee_metrics->ExpectOnlySingleEnumCount( + kEnumCounterSdpFormatReceived, kSdpFormatReceivedSimple)); } TEST_F(SdpFormatReceivedTest, ComplexUnifiedIsReportedAsComplexUnifiedPlan) { @@ -1520,14 +1510,12 @@ TEST_F(SdpFormatReceivedTest, ComplexUnifiedIsReportedAsComplexUnifiedPlan) { caller->AddAudioTrack("audio2"); caller->AddVideoTrack("video"); auto callee = CreatePeerConnectionWithPlanB(); + auto callee_metrics = callee->RegisterFakeMetricsObserver(); ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); - // Note that only the callee does ReportSdpFormatReceived. - EXPECT_EQ(1, webrtc::metrics::NumSamples( - "WebRTC.PeerConnection.SdpFormatReceived")); - EXPECT_EQ( - 1, webrtc::metrics::NumEvents("WebRTC.PeerConnection.SdpFormatReceived", - kSdpFormatReceivedComplexUnifiedPlan)); + + EXPECT_TRUE(callee_metrics->ExpectOnlySingleEnumCount( + kEnumCounterSdpFormatReceived, kSdpFormatReceivedComplexUnifiedPlan)); } TEST_F(SdpFormatReceivedTest, ComplexPlanBIsReportedAsComplexPlanB) { @@ -1535,17 +1523,15 @@ TEST_F(SdpFormatReceivedTest, ComplexPlanBIsReportedAsComplexPlanB) { caller->AddVideoTrack("video1"); caller->AddVideoTrack("video2"); auto callee = CreatePeerConnectionWithUnifiedPlan(); + auto callee_metrics = callee->RegisterFakeMetricsObserver(); // This fails since Unified Plan cannot set a session description with // multiple "Plan B tracks" in the same media section. But we still expect the // SDP Format to be recorded. ASSERT_FALSE(callee->SetRemoteDescription(caller->CreateOffer())); - // Note that only the callee does ReportSdpFormatReceived. - EXPECT_EQ(1, webrtc::metrics::NumSamples( - "WebRTC.PeerConnection.SdpFormatReceived")); - EXPECT_EQ( - 1, webrtc::metrics::NumEvents("WebRTC.PeerConnection.SdpFormatReceived", - kSdpFormatReceivedComplexPlanB)); + + EXPECT_TRUE(callee_metrics->ExpectOnlySingleEnumCount( + kEnumCounterSdpFormatReceived, kSdpFormatReceivedComplexPlanB)); } // Sender setups in a call. diff --git a/pc/peerconnectionwrapper.cc b/pc/peerconnectionwrapper.cc index 5006337f875..0d19cf3f75c 100644 --- a/pc/peerconnectionwrapper.cc +++ b/pc/peerconnectionwrapper.cc @@ -320,4 +320,12 @@ PeerConnectionWrapper::GetStats() { return callback->report(); } +rtc::scoped_refptr +PeerConnectionWrapper::RegisterFakeMetricsObserver() { + RTC_DCHECK(!fake_metrics_observer_); + fake_metrics_observer_ = new rtc::RefCountedObject(); + pc_->RegisterUMAObserver(fake_metrics_observer_); + return fake_metrics_observer_; +} + } // namespace webrtc diff --git a/pc/peerconnectionwrapper.h b/pc/peerconnectionwrapper.h index 436460e8729..f7de67ecc8a 100644 --- a/pc/peerconnectionwrapper.h +++ b/pc/peerconnectionwrapper.h @@ -16,6 +16,7 @@ #include #include +#include "api/fakemetricsobserver.h" #include "api/peerconnectioninterface.h" #include "pc/test/mockpeerconnectionobservers.h" #include "rtc_base/function_view.h" @@ -170,6 +171,10 @@ class PeerConnectionWrapper { // report. If GetStats() fails, this method returns null and fails the test. rtc::scoped_refptr GetStats(); + // Creates a new FakeMetricsObserver and registers it with the PeerConnection + // as the UMA observer. + rtc::scoped_refptr RegisterFakeMetricsObserver(); + private: std::unique_ptr CreateSdp( rtc::FunctionView fn, @@ -180,6 +185,7 @@ class PeerConnectionWrapper { rtc::scoped_refptr pc_factory_; std::unique_ptr observer_; rtc::scoped_refptr pc_; + rtc::scoped_refptr fake_metrics_observer_; }; } // namespace webrtc diff --git a/pc/srtpsession.cc b/pc/srtpsession.cc index 28349adb2ac..3b33f85c345 100644 --- a/pc/srtpsession.cc +++ b/pc/srtpsession.cc @@ -139,7 +139,11 @@ bool SrtpSession::UnprotectRtp(void* p, int in_len, int* out_len) { int err = srtp_unprotect(session_, p, out_len); if (err != srtp_err_status_ok) { RTC_LOG(LS_WARNING) << "Failed to unprotect SRTP packet, err=" << err; - RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.SrtpUnprotectError", + if (metrics_observer_) { + metrics_observer_->IncrementSparseEnumCounter( + webrtc::kEnumCounterSrtpUnprotectError, err); + } + RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.UnprotectSrtpError", static_cast(err), kSrtpErrorCodeBoundary); return false; } @@ -157,7 +161,11 @@ bool SrtpSession::UnprotectRtcp(void* p, int in_len, int* out_len) { int err = srtp_unprotect_rtcp(session_, p, out_len); if (err != srtp_err_status_ok) { RTC_LOG(LS_WARNING) << "Failed to unprotect SRTCP packet, err=" << err; - RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.SrtcpUnprotectError", + if (metrics_observer_) { + metrics_observer_->IncrementSparseEnumCounter( + webrtc::kEnumCounterSrtcpUnprotectError, err); + } + RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.UnprotectSrtcpError", static_cast(err), kSrtpErrorCodeBoundary); return false; } diff --git a/pc/srtpsession_unittest.cc b/pc/srtpsession_unittest.cc index b1bc9f06760..66e1ceabb18 100644 --- a/pc/srtpsession_unittest.cc +++ b/pc/srtpsession_unittest.cc @@ -13,21 +13,20 @@ #include #include "absl/memory/memory.h" +#include "api/fakemetricsobserver.h" #include "media/base/fakertp.h" #include "pc/srtptestutil.h" #include "rtc_base/gunit.h" #include "rtc_base/sslstreamadapter.h" // For rtc::SRTP_* -#include "system_wrappers/include/metrics_default.h" #include "third_party/libsrtp/include/srtp.h" namespace rtc { +using webrtc::FakeMetricsObserver; + std::vector kEncryptedHeaderExtensionIds; class SrtpSessionTest : public testing::Test { - public: - SrtpSessionTest() { webrtc::metrics::Reset(); } - protected: virtual void SetUp() { rtp_len_ = sizeof(kPcmuFrame); @@ -137,6 +136,9 @@ TEST_F(SrtpSessionTest, TestGetSendStreamPacketIndex) { // Test that we fail to unprotect if someone tampers with the RTP/RTCP paylaods. TEST_F(SrtpSessionTest, TestTamperReject) { + rtc::scoped_refptr metrics_observer( + new rtc::RefCountedObject()); + s2_.SetMetricsObserver(metrics_observer); int out_len; EXPECT_TRUE(s1_.SetSend(SRTP_AES128_CM_SHA1_80, kTestKey1, kTestKeyLen, kEncryptedHeaderExtensionIds)); @@ -147,38 +149,29 @@ TEST_F(SrtpSessionTest, TestTamperReject) { rtp_packet_[0] = 0x12; rtcp_packet_[1] = 0x34; EXPECT_FALSE(s2_.UnprotectRtp(rtp_packet_, rtp_len_, &out_len)); - EXPECT_EQ(1, webrtc::metrics::NumSamples( - "WebRTC.PeerConnection.SrtpUnprotectError")); - EXPECT_EQ( - 1, webrtc::metrics::NumEvents("WebRTC.PeerConnection.SrtpUnprotectError", - srtp_err_status_bad_param)); + EXPECT_TRUE(metrics_observer->ExpectOnlySingleEnumCount( + webrtc::kEnumCounterSrtpUnprotectError, srtp_err_status_bad_param)); EXPECT_FALSE(s2_.UnprotectRtcp(rtcp_packet_, rtcp_len_, &out_len)); - EXPECT_EQ(1, webrtc::metrics::NumSamples( - "WebRTC.PeerConnection.SrtcpUnprotectError")); - EXPECT_EQ( - 1, webrtc::metrics::NumEvents("WebRTC.PeerConnection.SrtcpUnprotectError", - srtp_err_status_auth_fail)); + EXPECT_TRUE(metrics_observer->ExpectOnlySingleEnumCount( + webrtc::kEnumCounterSrtcpUnprotectError, srtp_err_status_auth_fail)); } // Test that we fail to unprotect if the payloads are not authenticated. TEST_F(SrtpSessionTest, TestUnencryptReject) { + rtc::scoped_refptr metrics_observer( + new rtc::RefCountedObject()); + s2_.SetMetricsObserver(metrics_observer); int out_len; EXPECT_TRUE(s1_.SetSend(SRTP_AES128_CM_SHA1_80, kTestKey1, kTestKeyLen, kEncryptedHeaderExtensionIds)); EXPECT_TRUE(s2_.SetRecv(SRTP_AES128_CM_SHA1_80, kTestKey1, kTestKeyLen, kEncryptedHeaderExtensionIds)); EXPECT_FALSE(s2_.UnprotectRtp(rtp_packet_, rtp_len_, &out_len)); - EXPECT_EQ(1, webrtc::metrics::NumSamples( - "WebRTC.PeerConnection.SrtpUnprotectError")); - EXPECT_EQ( - 1, webrtc::metrics::NumEvents("WebRTC.PeerConnection.SrtpUnprotectError", - srtp_err_status_auth_fail)); + EXPECT_TRUE(metrics_observer->ExpectOnlySingleEnumCount( + webrtc::kEnumCounterSrtpUnprotectError, srtp_err_status_auth_fail)); EXPECT_FALSE(s2_.UnprotectRtcp(rtcp_packet_, rtcp_len_, &out_len)); - EXPECT_EQ(1, webrtc::metrics::NumSamples( - "WebRTC.PeerConnection.SrtcpUnprotectError")); - EXPECT_EQ( - 1, webrtc::metrics::NumEvents("WebRTC.PeerConnection.SrtcpUnprotectError", - srtp_err_status_cant_check)); + EXPECT_TRUE(metrics_observer->ExpectOnlySingleEnumCount( + webrtc::kEnumCounterSrtcpUnprotectError, srtp_err_status_cant_check)); } // Test that we fail when using buffers that are too small. diff --git a/rtc_base/BUILD.gn b/rtc_base/BUILD.gn index 18d6897e3cb..4f4cdf1f610 100644 --- a/rtc_base/BUILD.gn +++ b/rtc_base/BUILD.gn @@ -1066,7 +1066,6 @@ if (rtc_include_tests) { ":rtc_base_approved", ":rtc_base_tests_utils", "../system_wrappers:field_trial_default", - "../system_wrappers:metrics_default", "../test:field_trial", "../test:fileutils", "../test:test_support", diff --git a/rtc_base/sslstreamadapter.h b/rtc_base/sslstreamadapter.h index 2d4e19f9b50..827dc455625 100644 --- a/rtc_base/sslstreamadapter.h +++ b/rtc_base/sslstreamadapter.h @@ -22,7 +22,6 @@ namespace rtc { // Constants for SSL profile. const int TLS_NULL_WITH_NULL_NULL = 0; -const int SSL_CIPHER_SUITE_MAX_VALUE = 0xFFFF; // Constants for SRTP profiles. const int SRTP_INVALID_CRYPTO_SUITE = 0; @@ -38,7 +37,6 @@ const int SRTP_AEAD_AES_128_GCM = 0x0007; #ifndef SRTP_AEAD_AES_256_GCM const int SRTP_AEAD_AES_256_GCM = 0x0008; #endif -const int SRTP_CRYPTO_SUITE_MAX_VALUE = 0xFFFF; // Names of SRTP profiles listed above. // 128-bit AES with 80-bit SHA-1 HMAC. diff --git a/rtc_base/unittest_main.cc b/rtc_base/unittest_main.cc index 0b5a39da555..df9622bd94d 100644 --- a/rtc_base/unittest_main.cc +++ b/rtc_base/unittest_main.cc @@ -20,7 +20,6 @@ #include "rtc_base/ssladapter.h" #include "rtc_base/sslstreamadapter.h" #include "system_wrappers/include/field_trial_default.h" -#include "system_wrappers/include/metrics_default.h" #include "test/field_trial.h" #include "test/testsupport/fileutils.h" @@ -82,7 +81,6 @@ int main(int argc, char* argv[]) { // InitFieldTrialsFromString stores the char*, so the char array must outlive // the application. webrtc::field_trial::InitFieldTrialsFromString(FLAG_force_fieldtrials); - webrtc::metrics::Enable(); #if defined(WEBRTC_WIN) if (!FLAG_default_error_handlers) { diff --git a/system_wrappers/include/metrics.h b/system_wrappers/include/metrics.h index 99b8194f1aa..13ed2c936d2 100644 --- a/system_wrappers/include/metrics.h +++ b/system_wrappers/include/metrics.h @@ -127,9 +127,6 @@ // Histogram for enumerators (evenly spaced buckets). // |boundary| should be above the max enumerator sample. -// -// TODO(qingsi): Refactor the default implementation given by RtcHistogram, -// which is already sparse, and remove the boundary argument from the macro. #define RTC_HISTOGRAM_ENUMERATION_SPARSE(name, sample, boundary) \ RTC_HISTOGRAM_COMMON_BLOCK( \ name, sample, \