diff --git a/api/BUILD.gn b/api/BUILD.gn index 71c2ec39bc..b6caeaa687 100644 --- a/api/BUILD.gn +++ b/api/BUILD.gn @@ -86,7 +86,6 @@ rtc_static_library("libjingle_peerconnection_api") { "statstypes.cc", "statstypes.h", "turncustomizer.h", - "umametrics.cc", "umametrics.h", "videosourceproxy.h", ] @@ -440,26 +439,6 @@ 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 deleted file mode 100644 index cd8de392a3..0000000000 --- a/api/fakemetricsobserver.cc +++ /dev/null @@ -1,87 +0,0 @@ -/* - * 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 deleted file mode 100644 index 1f5b7043b2..0000000000 --- a/api/fakemetricsobserver.h +++ /dev/null @@ -1,56 +0,0 @@ -/* - * 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 deleted file mode 100644 index d5f2bb62b2..0000000000 --- a/api/umametrics.cc +++ /dev/null @@ -1,21 +0,0 @@ -/* - * 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 081b515c7c..b999b64958 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) {} + int counter_max) = 0; // 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); + int counter) = 0; virtual void AddHistogramSample(PeerConnectionMetricsName type, int value) = 0; diff --git a/p2p/BUILD.gn b/p2p/BUILD.gn index a2f51608d4..4763001f49 100644 --- a/p2p/BUILD.gn +++ b/p2p/BUILD.gn @@ -91,6 +91,7 @@ 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", ] @@ -171,13 +172,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 dca8d6374c..67bcdfa56c 100644 --- a/p2p/base/p2ptransportchannel.cc +++ b/p2p/base/p2ptransportchannel.cc @@ -28,6 +28,7 @@ #include "rtc_base/stringencode.h" #include "rtc_base/timeutils.h" #include "system_wrappers/include/field_trial.h" +#include "system_wrappers/include/metrics.h" namespace { @@ -675,7 +676,7 @@ void P2PTransportChannel::MaybeStartGathering() { SignalGatheringState(this); } - if (metrics_observer_ && !allocator_sessions_.empty()) { + if (!allocator_sessions_.empty()) { IceRestartState state; if (writable()) { state = IceRestartState::CONNECTED; @@ -684,9 +685,9 @@ void P2PTransportChannel::MaybeStartGathering() { } else { state = IceRestartState::DISCONNECTED; } - metrics_observer_->IncrementEnumCounter( - webrtc::kEnumCounterIceRestart, static_cast(state), - static_cast(IceRestartState::MAX_VALUE)); + RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.IceRestartState", + 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 cbf8b250e5..1c3ae9ab77 100644 --- a/p2p/base/p2ptransportchannel_unittest.cc +++ b/p2p/base/p2ptransportchannel_unittest.cc @@ -13,7 +13,6 @@ #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" @@ -37,6 +36,7 @@ #include "rtc_base/ssladapter.h" #include "rtc_base/thread.h" #include "rtc_base/virtualsocketserver.h" +#include "system_wrappers/include/metrics_default.h" namespace { @@ -207,15 +207,10 @@ 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())); - ep2_.metrics_observer_ = - new rtc::RefCountedObject(); - ep2_.allocator_->SetMetricsObserver(ep2_.metrics_observer_); + webrtc::metrics::Reset(); } protected: @@ -282,7 +277,7 @@ class P2PTransportChannelTestBase : public testing::Test, Candidates candidates; }; - struct Endpoint { + struct Endpoint : public sigslot::has_slots<> { Endpoint() : role_(ICEROLE_UNKNOWN), tiebreaker_(0), @@ -313,10 +308,15 @@ 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,6 +326,7 @@ 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) { @@ -353,12 +354,14 @@ 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() { @@ -441,9 +444,6 @@ 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, GetMetricsObserver(0)->GetEnumCounter( - webrtc::kEnumCounterIceRestart, + EXPECT_EQ(1, webrtc::metrics::NumEvents( + "WebRTC.PeerConnection.IceRestartState", static_cast(IceRestartState::DISCONNECTED))); ep2_ch1()->SetIceParameters(kIceParams[3]); ep2_ch1()->SetRemoteIceParameters(kIceParams[2]); ep2_ch1()->MaybeStartGathering(); - EXPECT_EQ(1, GetMetricsObserver(1)->GetEnumCounter( - webrtc::kEnumCounterIceRestart, + EXPECT_EQ(2, webrtc::metrics::NumEvents( + "WebRTC.PeerConnection.IceRestartState", 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, GetMetricsObserver(0)->GetEnumCounter( - webrtc::kEnumCounterIceRestart, + EXPECT_EQ(1, webrtc::metrics::NumEvents( + "WebRTC.PeerConnection.IceRestartState", static_cast(IceRestartState::CONNECTED))); ep2_ch1()->SetIceParameters(kIceParams[3]); ep2_ch1()->SetRemoteIceParameters(kIceParams[2]); ep2_ch1()->MaybeStartGathering(); - EXPECT_EQ(1, GetMetricsObserver(1)->GetEnumCounter( - webrtc::kEnumCounterIceRestart, + EXPECT_EQ(2, webrtc::metrics::NumEvents( + "WebRTC.PeerConnection.IceRestartState", 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, GetMetricsObserver(0)->GetEnumCounter( - webrtc::kEnumCounterIceRestart, + EXPECT_EQ(1, webrtc::metrics::NumEvents( + "WebRTC.PeerConnection.IceRestartState", static_cast(IceRestartState::CONNECTING))); ep2_ch1()->SetIceParameters(kIceParams[3]); ep2_ch1()->SetRemoteIceParameters(kIceParams[2]); ep2_ch1()->MaybeStartGathering(); - EXPECT_EQ(1, GetMetricsObserver(1)->GetEnumCounter( - webrtc::kEnumCounterIceRestart, + EXPECT_EQ(2, webrtc::metrics::NumEvents( + "WebRTC.PeerConnection.IceRestartState", static_cast(IceRestartState::CONNECTING))); DestroyChannels(); @@ -1355,12 +1355,10 @@ TEST_F(P2PTransportChannelTest, // Adding address in ep1 will trigger continual gathering. AddAddress(0, kAlternateAddrs[0]); - EXPECT_EQ_SIMULATED_WAIT( - 1, - GetMetricsObserver(0)->GetEnumCounter( - webrtc::kEnumCounterIceRegathering, - static_cast(IceRegatheringReason::NETWORK_CHANGE)), - kDefaultTimeout, clock); + EXPECT_EQ_SIMULATED_WAIT(1, + GetEndpoint(0)->GetIceRegatheringCountForReason( + IceRegatheringReason::NETWORK_CHANGE), + kDefaultTimeout, clock); ep2_ch1()->SetIceParameters(kIceParams[3]); ep2_ch1()->SetRemoteIceParameters(kIceParams[2]); @@ -1369,9 +1367,8 @@ TEST_F(P2PTransportChannelTest, AddAddress(1, kAlternateAddrs[1]); SIMULATED_WAIT(false, kDefaultTimeout, clock); // ep2 has not enabled continual gathering. - EXPECT_EQ(0, GetMetricsObserver(1)->GetEnumCounter( - webrtc::kEnumCounterIceRegathering, - static_cast(IceRegatheringReason::NETWORK_CHANGE))); + EXPECT_EQ(0, GetEndpoint(1)->GetIceRegatheringCountForReason( + IceRegatheringReason::NETWORK_CHANGE)); DestroyChannels(); } @@ -1399,12 +1396,13 @@ TEST_F(P2PTransportChannelTest, // Timeout value such that all connections are deleted. const int kNetworkFailureTimeout = 35000; SIMULATED_WAIT(false, kNetworkFailureTimeout, clock); - EXPECT_LE(1, GetMetricsObserver(0)->GetEnumCounter( - webrtc::kEnumCounterIceRegathering, - static_cast(IceRegatheringReason::NETWORK_FAILURE))); - EXPECT_EQ(0, GetMetricsObserver(1)->GetEnumCounter( - webrtc::kEnumCounterIceRegathering, + EXPECT_LE(1, GetEndpoint(0)->GetIceRegatheringCountForReason( + IceRegatheringReason::NETWORK_FAILURE)); + EXPECT_LE(1, webrtc::metrics::NumEvents( + "WebRTC.PeerConnection.IceRegatheringReason", static_cast(IceRegatheringReason::NETWORK_FAILURE))); + EXPECT_EQ(0, GetEndpoint(1)->GetIceRegatheringCountForReason( + IceRegatheringReason::NETWORK_FAILURE)); DestroyChannels(); } @@ -1433,13 +1431,14 @@ 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, GetMetricsObserver(0)->GetEnumCounter( - webrtc::kEnumCounterIceRegathering, + EXPECT_LE(5, GetEndpoint(0)->GetIceRegatheringCountForReason( + IceRegatheringReason::OCCASIONAL_REFRESH)); + EXPECT_LE(5, webrtc::metrics::NumEvents( + "WebRTC.PeerConnection.IceRegatheringReason", static_cast(IceRegatheringReason::OCCASIONAL_REFRESH))); // Expect no regathering if continual gathering not configured. - EXPECT_EQ(0, GetMetricsObserver(1)->GetEnumCounter( - webrtc::kEnumCounterIceRegathering, - static_cast(IceRegatheringReason::OCCASIONAL_REFRESH))); + EXPECT_EQ(0, GetEndpoint(1)->GetIceRegatheringCountForReason( + IceRegatheringReason::OCCASIONAL_REFRESH)); DestroyChannels(); } @@ -1484,10 +1483,8 @@ class P2PTransportRegatherAllNetworksTest : public P2PTransportChannelTest { const int kWaitRegather = kRegatherInterval * kNumRegathers + kRegatherInterval / 2; SIMULATED_WAIT(false, kWaitRegather, clock); - EXPECT_EQ(kNumRegathers, - GetMetricsObserver(0)->GetEnumCounter( - webrtc::kEnumCounterIceRegathering, - static_cast(IceRegatheringReason::OCCASIONAL_REFRESH))); + EXPECT_EQ(kNumRegathers, GetEndpoint(0)->GetIceRegatheringCountForReason( + 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 18b23e0536..6ae64e8658 100644 --- a/p2p/base/regatheringcontroller_unittest.cc +++ b/p2p/base/regatheringcontroller_unittest.cc @@ -13,7 +13,6 @@ #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 4335ddaad4..eb14a37758 100644 --- a/p2p/client/basicportallocator.cc +++ b/p2p/client/basicportallocator.cc @@ -16,7 +16,6 @@ #include #include -#include "api/umametrics.h" #include "p2p/base/basicpacketsocketfactory.h" #include "p2p/base/port.h" #include "p2p/base/relayport.h" @@ -28,6 +27,7 @@ #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,9 +196,6 @@ 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()) { @@ -207,9 +204,9 @@ void BasicPortAllocator::OnIceRegathering(PortAllocatorSession* session, } } - metrics_observer()->IncrementEnumCounter( - webrtc::kEnumCounterIceRegathering, static_cast(reason), - static_cast(IceRegatheringReason::MAX_VALUE)); + RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.IceRegatheringReason", + 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 d995eb18de..b3e52ffec5 100644 --- a/p2p/client/basicportallocator_unittest.cc +++ b/p2p/client/basicportallocator_unittest.cc @@ -34,6 +34,7 @@ #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; @@ -168,6 +169,7 @@ class BasicPortAllocatorTestBase : public testing::Test, kRelaySslTcpIntAddr)); allocator_->Initialize(); allocator_->set_step_delay(kMinimumStepDelay); + webrtc::metrics::Reset(); } void AddInterface(const SocketAddress& addr) { @@ -2368,4 +2370,21 @@ 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 195d18518d..329d66730f 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn @@ -206,6 +206,7 @@ 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", ] @@ -296,7 +297,6 @@ 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,7 +334,6 @@ if (rtc_include_tests) { ] deps = [ ":pc_test_utils", - "../api:fakemetricsobserver", "../api:libjingle_peerconnection_api", "../api:libjingle_peerconnection_test_api", "../api:rtc_stats_api", @@ -494,7 +493,6 @@ 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 89f35c2131..ea5d9228ef 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -52,6 +52,7 @@ #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; @@ -383,15 +384,10 @@ bool MediaSectionsHaveSameCount(const SessionDescription& desc1, return desc1.contents().size() == desc2.contents().size(); } -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); +void NoteKeyProtocolAndMedia(KeyExchangeProtocolType protocol_type, + cricket::MediaType media_type) { + RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.KeyProtocol", protocol_type, + kEnumCounterKeyProtocolMax); static const std::map, KeyExchangeProtocolMedia> proto_media_counter_map = { @@ -410,9 +406,8 @@ void NoteKeyProtocolAndMedia( auto it = proto_media_counter_map.find({protocol_type, media_type}); if (it != proto_media_counter_map.end()) { - uma_observer->IncrementEnumCounter(webrtc::kEnumCounterKeyProtocolMediaType, - it->second, - kEnumCounterKeyProtocolMediaTypeMax); + RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.KeyProtocolByMedia", + it->second, kEnumCounterKeyProtocolMediaTypeMax); } } @@ -422,9 +417,7 @@ void NoteKeyProtocolAndMedia( // 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, - rtc::scoped_refptr uma_observer) { +RTCError VerifyCrypto(const SessionDescription* desc, bool dtls_enabled) { const cricket::ContentGroup* bundle = desc->GetGroupByName(cricket::GROUP_TYPE_BUNDLE); for (const cricket::ContentInfo& content_info : desc->contents()) { @@ -434,8 +427,7 @@ RTCError VerifyCrypto(const SessionDescription* desc, // Note what media is used with each crypto protocol, for all sections. NoteKeyProtocolAndMedia(dtls_enabled ? webrtc::kEnumCounterKeyProtocolDtls : webrtc::kEnumCounterKeyProtocolSdes, - content_info.media_description()->type(), - uma_observer); + content_info.media_description()->type()); const std::string& mid = content_info.name; if (bundle && bundle->HasContentName(mid) && mid != *(bundle->FirstContentName())) { @@ -939,6 +931,16 @@ 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 @@ -3059,18 +3061,6 @@ 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) { @@ -5294,9 +5284,7 @@ void PeerConnection::OnTransportControllerConnectionState( } SetIceConnectionState(PeerConnectionInterface::kIceConnectionCompleted); NoteUsageEvent(UsageEvent::ICE_STATE_CONNECTED); - if (metrics_observer()) { - ReportTransportStats(); - } + ReportTransportStats(); break; default: RTC_NOTREACHED(); @@ -5348,11 +5336,9 @@ void PeerConnection::OnTransportControllerCandidatesRemoved( void PeerConnection::OnTransportControllerDtlsHandshakeError( rtc::SSLHandshakeError error) { - if (metrics_observer()) { - metrics_observer()->IncrementEnumCounter( - webrtc::kEnumCounterDtlsHandshakeError, static_cast(error), - static_cast(rtc::SSLHandshakeError::MAX_VALUE)); - } + RTC_HISTOGRAM_ENUMERATION( + "WebRTC.PeerConnection.DtlsHandshakeError", static_cast(error), + static_cast(rtc::SSLHandshakeError::MAX_VALUE)); } void PeerConnection::EnableSending() { @@ -5790,8 +5776,7 @@ 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_, uma_observer_); + RTCError crypto_error = VerifyCrypto(sdesc->description(), dtls_enabled_); if (!crypto_error.ok()) { return crypto_error; } @@ -5918,9 +5903,6 @@ 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; @@ -5945,8 +5927,8 @@ void PeerConnection::ReportSdpFormatReceived( } else if (num_audio_tracks > 0 || num_video_tracks > 0) { format = kSdpFormatReceivedSimple; } - uma_observer_->IncrementEnumCounter(kEnumCounterSdpFormatReceived, format, - kSdpFormatReceivedMax); + RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.SdpFormatReceived", format, + kSdpFormatReceivedMax); } void PeerConnection::NoteUsageEvent(UsageEvent event) { @@ -5956,42 +5938,33 @@ void PeerConnection::NoteUsageEvent(UsageEvent event) { void PeerConnection::ReportUsagePattern() const { RTC_DLOG(LS_INFO) << "Usage signature is " << usage_event_accumulator_; - if (uma_observer_) { - uma_observer_->IncrementSparseEnumCounter(kEnumCounterUsagePattern, - usage_event_accumulator_); - } + RTC_HISTOGRAM_ENUMERATION_SPARSE("WebRTC.PeerConnection.UsagePattern", + usage_event_accumulator_, + static_cast(UsageEvent::MAX_VALUE)); } void PeerConnection::ReportNegotiatedSdpSemantics( const SessionDescriptionInterface& answer) { - if (!uma_observer_) { - return; - } + SdpSemanticNegotiated semantics_negotiated; switch (answer.description()->msid_signaling()) { case 0: - uma_observer_->IncrementEnumCounter(kEnumCounterSdpSemanticNegotiated, - kSdpSemanticNegotiatedNone, - kSdpSemanticNegotiatedMax); + semantics_negotiated = kSdpSemanticNegotiatedNone; break; case cricket::kMsidSignalingMediaSection: - uma_observer_->IncrementEnumCounter(kEnumCounterSdpSemanticNegotiated, - kSdpSemanticNegotiatedUnifiedPlan, - kSdpSemanticNegotiatedMax); + semantics_negotiated = kSdpSemanticNegotiatedUnifiedPlan; break; case cricket::kMsidSignalingSsrcAttribute: - uma_observer_->IncrementEnumCounter(kEnumCounterSdpSemanticNegotiated, - kSdpSemanticNegotiatedPlanB, - kSdpSemanticNegotiatedMax); + semantics_negotiated = kSdpSemanticNegotiatedPlanB; break; case cricket::kMsidSignalingMediaSection | cricket::kMsidSignalingSsrcAttribute: - uma_observer_->IncrementEnumCounter(kEnumCounterSdpSemanticNegotiated, - kSdpSemanticNegotiatedMixed, - kSdpSemanticNegotiatedMax); + semantics_negotiated = kSdpSemanticNegotiatedMixed; 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 @@ -6085,7 +6058,6 @@ 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 : @@ -6094,7 +6066,6 @@ void PeerConnection::ReportBestConnectionState( continue; } - PeerConnectionEnumCounterType type = kPeerConnectionEnumCounterMax; const cricket::Candidate& local = connection_info.local_candidate; const cricket::Candidate& remote = connection_info.remote_candidate; @@ -6102,26 +6073,26 @@ void PeerConnection::ReportBestConnectionState( if (local.protocol() == cricket::TCP_PROTOCOL_NAME || (local.type() == RELAY_PORT_TYPE && local.relay_protocol() == cricket::TCP_PROTOCOL_NAME)) { - type = kEnumCounterIceCandidatePairTypeTcp; + RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.CandidatePairType_TCP", + GetIceCandidatePairCounter(local, remote), + kIceCandidatePairMax); } else if (local.protocol() == cricket::UDP_PROTOCOL_NAME) { - type = kEnumCounterIceCandidatePairTypeUdp; + RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.CandidatePairType_UDP", + GetIceCandidatePairCounter(local, remote), + kIceCandidatePairMax); } else { RTC_CHECK(0); } - metrics_observer()->IncrementEnumCounter( - type, GetIceCandidatePairCounter(local, remote), - kIceCandidatePairMax); // Increment the counter for IP type. if (local.address().family() == AF_INET) { - metrics_observer()->IncrementEnumCounter( - kEnumCounterAddressFamily, kBestConnections_IPv4, - kPeerConnectionAddressFamilyCounter_Max); - + RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.IPMetrics", + kBestConnections_IPv4, + kPeerConnectionAddressFamilyCounter_Max); } else if (local.address().family() == AF_INET6) { - metrics_observer()->IncrementEnumCounter( - kEnumCounterAddressFamily, kBestConnections_IPv6, - kPeerConnectionAddressFamilyCounter_Max); + RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.IPMetrics", + kBestConnections_IPv6, + kPeerConnectionAddressFamilyCounter_Max); } else { RTC_CHECK(0); } @@ -6134,7 +6105,6 @@ 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; } @@ -6146,33 +6116,53 @@ void PeerConnection::ReportNegotiatedCiphers( return; } - 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 (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; + } } - 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); + } + + 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; + } } } } diff --git a/pc/peerconnection.h b/pc/peerconnection.h index 7d7315d4e4..44b66c3658 100644 --- a/pc/peerconnection.h +++ b/pc/peerconnection.h @@ -66,7 +66,8 @@ class PeerConnection : public PeerConnectionInternal, CANDIDATE_COLLECTED = 0x80, REMOTE_CANDIDATE_ADDED = 0x100, ICE_STATE_CONNECTED = 0x200, - CLOSE_CALLED = 0x400 + CLOSE_CALLED = 0x400, + MAX_VALUE = 0x800, }; explicit PeerConnection(PeerConnectionFactory* factory, diff --git a/pc/peerconnection_histogram_unittest.cc b/pc/peerconnection_histogram_unittest.cc index b9983727d9..64c4cfe897 100644 --- a/pc/peerconnection_histogram_unittest.cc +++ b/pc/peerconnection_histogram_unittest.cc @@ -11,7 +11,6 @@ #include #include "absl/memory/memory.h" -#include "api/fakemetricsobserver.h" #include "api/peerconnectionproxy.h" #include "media/base/fakemediaengine.h" #include "pc/mediasession.h" @@ -22,6 +21,7 @@ #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,6 +127,7 @@ class PeerConnectionUsageHistogramTest : public ::testing::Test { PeerConnectionUsageHistogramTest() : vss_(new rtc::VirtualSocketServer()), main_(vss_.get()) { + webrtc::metrics::Reset(); } WrapperPtr CreatePeerConnection() { @@ -175,14 +176,12 @@ 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(caller_observer->ExpectOnlySingleEnumCount( - webrtc::kEnumCounterUsagePattern, expected_fingerprint), - kDefaultTimeout); + ASSERT_TRUE_WAIT( + 1u == webrtc::metrics::NumSamples("WebRTC.PeerConnection.UsagePattern"), + kDefaultTimeout); + EXPECT_EQ(1, webrtc::metrics::NumEvents("WebRTC.PeerConnection.UsagePattern", + expected_fingerprint)); } #ifndef WEBRTC_ANDROID @@ -193,9 +192,6 @@ 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()); @@ -213,19 +209,16 @@ TEST_F(PeerConnectionUsageHistogramTest, FingerprintAudioVideo) { PeerConnection::UsageEvent::REMOTE_CANDIDATE_ADDED, PeerConnection::UsageEvent::ICE_STATE_CONNECTED, PeerConnection::UsageEvent::CLOSE_CALLED}); - EXPECT_TRUE(caller_observer->ExpectOnlySingleEnumCount( - webrtc::kEnumCounterUsagePattern, expected_fingerprint)); - EXPECT_TRUE(callee_observer->ExpectOnlySingleEnumCount( - webrtc::kEnumCounterUsagePattern, expected_fingerprint)); + EXPECT_EQ(2, + webrtc::metrics::NumSamples("WebRTC.PeerConnection.UsagePattern")); + EXPECT_EQ(2, webrtc::metrics::NumEvents("WebRTC.PeerConnection.UsagePattern", + 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())); @@ -240,10 +233,10 @@ TEST_F(PeerConnectionUsageHistogramTest, FingerprintDataOnly) { PeerConnection::UsageEvent::REMOTE_CANDIDATE_ADDED, PeerConnection::UsageEvent::ICE_STATE_CONNECTED, PeerConnection::UsageEvent::CLOSE_CALLED}); - EXPECT_TRUE(caller_observer->ExpectOnlySingleEnumCount( - webrtc::kEnumCounterUsagePattern, expected_fingerprint)); - EXPECT_TRUE(callee_observer->ExpectOnlySingleEnumCount( - webrtc::kEnumCounterUsagePattern, expected_fingerprint)); + EXPECT_EQ(2, + webrtc::metrics::NumSamples("WebRTC.PeerConnection.UsagePattern")); + EXPECT_EQ(2, webrtc::metrics::NumEvents("WebRTC.PeerConnection.UsagePattern", + expected_fingerprint)); } #endif // HAVE_SCTP #endif // WEBRTC_ANDROID @@ -259,14 +252,15 @@ 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_TRUE(caller_observer->ExpectOnlySingleEnumCount( - webrtc::kEnumCounterUsagePattern, expected_fingerprint)); + EXPECT_EQ(1, + webrtc::metrics::NumSamples("WebRTC.PeerConnection.UsagePattern")); + EXPECT_EQ(1, webrtc::metrics::NumEvents("WebRTC.PeerConnection.UsagePattern", + expected_fingerprint)); } TEST_F(PeerConnectionUsageHistogramTest, FingerprintStunTurnInReconfiguration) { @@ -280,7 +274,6 @@ 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()); @@ -289,8 +282,10 @@ TEST_F(PeerConnectionUsageHistogramTest, FingerprintStunTurnInReconfiguration) { MakeUsageFingerprint({PeerConnection::UsageEvent::STUN_SERVER_ADDED, PeerConnection::UsageEvent::TURN_SERVER_ADDED, PeerConnection::UsageEvent::CLOSE_CALLED}); - EXPECT_TRUE(caller_observer->ExpectOnlySingleEnumCount( - webrtc::kEnumCounterUsagePattern, expected_fingerprint)); + EXPECT_EQ(1, + webrtc::metrics::NumSamples("WebRTC.PeerConnection.UsagePattern")); + EXPECT_EQ(1, webrtc::metrics::NumEvents("WebRTC.PeerConnection.UsagePattern", + expected_fingerprint)); } } // namespace webrtc diff --git a/pc/peerconnection_integrationtest.cc b/pc/peerconnection_integrationtest.cc index ed5db1f585..525eba35b3 100644 --- a/pc/peerconnection_integrationtest.cc +++ b/pc/peerconnection_integrationtest.cc @@ -24,7 +24,6 @@ #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" @@ -63,6 +62,7 @@ #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,6 +1106,7 @@ class PeerConnectionIntegrationBaseTest : public testing::Test { worker_thread_->SetName("PCWorkerThread", this); RTC_CHECK(network_thread_->Start()); RTC_CHECK(worker_thread_->Start()); + webrtc::metrics::Reset(); } ~PeerConnectionIntegrationBaseTest() { @@ -1513,20 +1514,17 @@ 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(SignalingStateStable(), kDefaultTimeout); + ASSERT_TRUE_WAIT(DtlsConnected(), kDefaultTimeout); EXPECT_EQ_WAIT(rtc::SrtpCryptoSuiteToName(expected_cipher_suite), caller()->OldGetStats()->SrtpCipher(), kDefaultTimeout); - EXPECT_EQ( - 1, caller_observer->GetEnumCounter(webrtc::kEnumCounterAudioSrtpCipher, - expected_cipher_suite)); - caller()->pc()->RegisterUMAObserver(nullptr); + // TODO(bugs.webrtc.org/9456): Fix it. + EXPECT_EQ(1, webrtc::metrics::NumEvents( + "WebRTC.PeerConnection.SrtpCryptoSuite.Audio", + expected_cipher_suite)); } void TestGcmNegotiationUsesCipherSuite(bool local_gcm_enabled, @@ -1696,9 +1694,6 @@ 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. @@ -1709,12 +1704,10 @@ TEST_P(PeerConnectionIntegrationTest, EndToEndCallWithDtls) { MediaExpectations media_expectations; media_expectations.ExpectBidirectionalAudioAndVideo(); ASSERT_TRUE(ExpectNewFrames(media_expectations)); - EXPECT_LE( - 1, caller_observer->GetEnumCounter(webrtc::kEnumCounterKeyProtocol, - webrtc::kEnumCounterKeyProtocolDtls)); - EXPECT_EQ( - 0, caller_observer->GetEnumCounter(webrtc::kEnumCounterKeyProtocol, - webrtc::kEnumCounterKeyProtocolSdes)); + EXPECT_LE(2, webrtc::metrics::NumEvents("WebRTC.PeerConnection.KeyProtocol", + webrtc::kEnumCounterKeyProtocolDtls)); + EXPECT_EQ(0, webrtc::metrics::NumEvents("WebRTC.PeerConnection.KeyProtocol", + webrtc::kEnumCounterKeyProtocolSdes)); } // Uses SDES instead of DTLS for key agreement. @@ -1723,9 +1716,6 @@ 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. @@ -1736,12 +1726,10 @@ TEST_P(PeerConnectionIntegrationTest, EndToEndCallWithSdes) { MediaExpectations media_expectations; media_expectations.ExpectBidirectionalAudioAndVideo(); ASSERT_TRUE(ExpectNewFrames(media_expectations)); - EXPECT_LE( - 1, caller_observer->GetEnumCounter(webrtc::kEnumCounterKeyProtocol, - webrtc::kEnumCounterKeyProtocolSdes)); - EXPECT_EQ( - 0, caller_observer->GetEnumCounter(webrtc::kEnumCounterKeyProtocol, - webrtc::kEnumCounterKeyProtocolDtls)); + EXPECT_LE(2, webrtc::metrics::NumEvents("WebRTC.PeerConnection.KeyProtocol", + webrtc::kEnumCounterKeyProtocolSdes)); + EXPECT_EQ(0, webrtc::metrics::NumEvents("WebRTC.PeerConnection.KeyProtocol", + webrtc::kEnumCounterKeyProtocolDtls)); } // Tests that the GetRemoteAudioSSLCertificate method returns the remote DTLS @@ -2743,22 +2731,19 @@ 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(SignalingStateStable(), kDefaultTimeout); + ASSERT_TRUE_WAIT(DtlsConnected(), kDefaultTimeout); EXPECT_TRUE_WAIT(rtc::SSLStreamAdapter::IsAcceptableCipher( caller()->OldGetStats()->DtlsCipher(), rtc::KT_DEFAULT), kDefaultTimeout); EXPECT_EQ_WAIT(rtc::SrtpCryptoSuiteToName(kDefaultSrtpCryptoSuite), caller()->OldGetStats()->SrtpCipher(), kDefaultTimeout); - EXPECT_EQ(1, - caller_observer->GetEnumCounter(webrtc::kEnumCounterAudioSrtpCipher, - kDefaultSrtpCryptoSuite)); + // TODO(bugs.webrtc.org/9456): Fix it. + EXPECT_EQ(1, webrtc::metrics::NumEvents( + "WebRTC.PeerConnection.SrtpCryptoSuite.Audio", + kDefaultSrtpCryptoSuite)); } // Test getting cipher stats and UMA metrics when DTLS 1.2 is negotiated. @@ -2768,22 +2753,19 @@ 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(SignalingStateStable(), kDefaultTimeout); + ASSERT_TRUE_WAIT(DtlsConnected(), kDefaultTimeout); EXPECT_TRUE_WAIT(rtc::SSLStreamAdapter::IsAcceptableCipher( caller()->OldGetStats()->DtlsCipher(), rtc::KT_DEFAULT), kDefaultTimeout); EXPECT_EQ_WAIT(rtc::SrtpCryptoSuiteToName(kDefaultSrtpCryptoSuite), caller()->OldGetStats()->SrtpCipher(), kDefaultTimeout); - EXPECT_EQ(1, - caller_observer->GetEnumCounter(webrtc::kEnumCounterAudioSrtpCipher, - kDefaultSrtpCryptoSuite)); + // TODO(bugs.webrtc.org/9456): Fix it. + EXPECT_EQ(1, webrtc::metrics::NumEvents( + "WebRTC.PeerConnection.SrtpCryptoSuite.Audio", + kDefaultSrtpCryptoSuite)); } // Test that DTLS 1.0 can be used if the caller supports DTLS 1.2 and the @@ -3502,19 +3484,15 @@ 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); - 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); + // 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); if (TestIPv6()) { // When IPv6 is enabled, we should prefer an IPv6 connection over an IPv4 // connection. @@ -3525,12 +3503,12 @@ TEST_P(PeerConnectionIntegrationIceStatesTest, VerifyBestConnection) { EXPECT_EQ(0, num_best_ipv6); } - EXPECT_EQ(0, metrics_observer->GetEnumCounter( - webrtc::kEnumCounterIceCandidatePairTypeUdp, - webrtc::kIceCandidatePairHostHost)); - EXPECT_EQ(1, metrics_observer->GetEnumCounter( - webrtc::kEnumCounterIceCandidatePairTypeUdp, - webrtc::kIceCandidatePairHostPublicHostPublic)); + EXPECT_EQ(0, webrtc::metrics::NumEvents( + "WebRTC.PeerConnection.CandidatePairType_UDP", + webrtc::kIceCandidatePairHostHost)); + EXPECT_EQ(1, webrtc::metrics::NumEvents( + "WebRTC.PeerConnection.CandidatePairType_UDP", + 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 50fe0dc6c8..df1b1eef1b 100644 --- a/pc/peerconnection_rtp_unittest.cc +++ b/pc/peerconnection_rtp_unittest.cc @@ -17,7 +17,6 @@ #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" @@ -32,6 +31,7 @@ #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,7 +77,9 @@ class PeerConnectionRtpBaseTest : public testing::Test { CreateBuiltinVideoEncoderFactory(), CreateBuiltinVideoDecoderFactory(), nullptr /* audio_mixer */, - nullptr /* audio_processing */)) {} + nullptr /* audio_processing */)) { + webrtc::metrics::Reset(); + } std::unique_ptr CreatePeerConnection() { return CreatePeerConnection(RTCConfiguration()); @@ -1369,7 +1371,6 @@ 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())); @@ -1384,8 +1385,11 @@ TEST_F(PeerConnectionMsidSignalingTest, UnifiedPlanTalkingToOurself) { EXPECT_EQ(cricket::kMsidSignalingMediaSection, answer->description()->msid_signaling()); // Check that this is counted correctly - EXPECT_TRUE(caller_observer->ExpectOnlySingleEnumCount( - kEnumCounterSdpSemanticNegotiated, kSdpSemanticNegotiatedUnifiedPlan)); + EXPECT_EQ(2, webrtc::metrics::NumSamples( + "WebRTC.PeerConnection.SdpSemanticNegotiated")); + EXPECT_EQ(2, webrtc::metrics::NumEvents( + "WebRTC.PeerConnection.SdpSemanticNegotiated", + kSdpSemanticNegotiatedUnifiedPlan)); } TEST_F(PeerConnectionMsidSignalingTest, PlanBOfferToUnifiedPlanAnswer) { @@ -1470,12 +1474,14 @@ TEST_F(SdpFormatReceivedTest, DataChannelOnlyIsReportedAsNoTracks) { auto caller = CreatePeerConnectionWithUnifiedPlan(); caller->CreateDataChannel("dc"); auto callee = CreatePeerConnectionWithUnifiedPlan(); - auto callee_metrics = callee->RegisterFakeMetricsObserver(); ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); - - EXPECT_TRUE(callee_metrics->ExpectOnlySingleEnumCount( - kEnumCounterSdpFormatReceived, kSdpFormatReceivedNoTracks)); + // 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)); } #endif // HAVE_SCTP @@ -1484,24 +1490,28 @@ TEST_F(SdpFormatReceivedTest, SimpleUnifiedPlanIsReportedAsSimple) { caller->AddAudioTrack("audio"); caller->AddVideoTrack("video"); auto callee = CreatePeerConnectionWithPlanB(); - auto callee_metrics = callee->RegisterFakeMetricsObserver(); ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); - - EXPECT_TRUE(callee_metrics->ExpectOnlySingleEnumCount( - kEnumCounterSdpFormatReceived, kSdpFormatReceivedSimple)); + // 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)); } 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_TRUE(callee_metrics->ExpectOnlySingleEnumCount( - kEnumCounterSdpFormatReceived, kSdpFormatReceivedSimple)); + EXPECT_EQ(1, webrtc::metrics::NumSamples( + "WebRTC.PeerConnection.SdpFormatReceived")); + EXPECT_EQ( + 1, webrtc::metrics::NumEvents("WebRTC.PeerConnection.SdpFormatReceived", + kSdpFormatReceivedSimple)); } TEST_F(SdpFormatReceivedTest, ComplexUnifiedIsReportedAsComplexUnifiedPlan) { @@ -1510,12 +1520,14 @@ TEST_F(SdpFormatReceivedTest, ComplexUnifiedIsReportedAsComplexUnifiedPlan) { caller->AddAudioTrack("audio2"); caller->AddVideoTrack("video"); auto callee = CreatePeerConnectionWithPlanB(); - auto callee_metrics = callee->RegisterFakeMetricsObserver(); ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); - - EXPECT_TRUE(callee_metrics->ExpectOnlySingleEnumCount( - kEnumCounterSdpFormatReceived, kSdpFormatReceivedComplexUnifiedPlan)); + // 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)); } TEST_F(SdpFormatReceivedTest, ComplexPlanBIsReportedAsComplexPlanB) { @@ -1523,15 +1535,17 @@ 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())); - - EXPECT_TRUE(callee_metrics->ExpectOnlySingleEnumCount( - kEnumCounterSdpFormatReceived, kSdpFormatReceivedComplexPlanB)); + // 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)); } // Sender setups in a call. diff --git a/pc/peerconnectionwrapper.cc b/pc/peerconnectionwrapper.cc index 0d19cf3f75..5006337f87 100644 --- a/pc/peerconnectionwrapper.cc +++ b/pc/peerconnectionwrapper.cc @@ -320,12 +320,4 @@ 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 f7de67ecc8..436460e872 100644 --- a/pc/peerconnectionwrapper.h +++ b/pc/peerconnectionwrapper.h @@ -16,7 +16,6 @@ #include #include -#include "api/fakemetricsobserver.h" #include "api/peerconnectioninterface.h" #include "pc/test/mockpeerconnectionobservers.h" #include "rtc_base/function_view.h" @@ -171,10 +170,6 @@ 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, @@ -185,7 +180,6 @@ 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 3b33f85c34..28349adb2a 100644 --- a/pc/srtpsession.cc +++ b/pc/srtpsession.cc @@ -139,11 +139,7 @@ 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; - if (metrics_observer_) { - metrics_observer_->IncrementSparseEnumCounter( - webrtc::kEnumCounterSrtpUnprotectError, err); - } - RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.UnprotectSrtpError", + RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.SrtpUnprotectError", static_cast(err), kSrtpErrorCodeBoundary); return false; } @@ -161,11 +157,7 @@ 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; - if (metrics_observer_) { - metrics_observer_->IncrementSparseEnumCounter( - webrtc::kEnumCounterSrtcpUnprotectError, err); - } - RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.UnprotectSrtcpError", + RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.SrtcpUnprotectError", static_cast(err), kSrtpErrorCodeBoundary); return false; } diff --git a/pc/srtpsession_unittest.cc b/pc/srtpsession_unittest.cc index 66e1ceabb1..b1bc9f0676 100644 --- a/pc/srtpsession_unittest.cc +++ b/pc/srtpsession_unittest.cc @@ -13,20 +13,21 @@ #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); @@ -136,9 +137,6 @@ 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)); @@ -149,29 +147,38 @@ TEST_F(SrtpSessionTest, TestTamperReject) { rtp_packet_[0] = 0x12; rtcp_packet_[1] = 0x34; EXPECT_FALSE(s2_.UnprotectRtp(rtp_packet_, rtp_len_, &out_len)); - EXPECT_TRUE(metrics_observer->ExpectOnlySingleEnumCount( - webrtc::kEnumCounterSrtpUnprotectError, srtp_err_status_bad_param)); + EXPECT_EQ(1, webrtc::metrics::NumSamples( + "WebRTC.PeerConnection.SrtpUnprotectError")); + EXPECT_EQ( + 1, webrtc::metrics::NumEvents("WebRTC.PeerConnection.SrtpUnprotectError", + srtp_err_status_bad_param)); EXPECT_FALSE(s2_.UnprotectRtcp(rtcp_packet_, rtcp_len_, &out_len)); - EXPECT_TRUE(metrics_observer->ExpectOnlySingleEnumCount( - webrtc::kEnumCounterSrtcpUnprotectError, srtp_err_status_auth_fail)); + EXPECT_EQ(1, webrtc::metrics::NumSamples( + "WebRTC.PeerConnection.SrtcpUnprotectError")); + EXPECT_EQ( + 1, webrtc::metrics::NumEvents("WebRTC.PeerConnection.SrtcpUnprotectError", + 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_TRUE(metrics_observer->ExpectOnlySingleEnumCount( - webrtc::kEnumCounterSrtpUnprotectError, srtp_err_status_auth_fail)); + EXPECT_EQ(1, webrtc::metrics::NumSamples( + "WebRTC.PeerConnection.SrtpUnprotectError")); + EXPECT_EQ( + 1, webrtc::metrics::NumEvents("WebRTC.PeerConnection.SrtpUnprotectError", + srtp_err_status_auth_fail)); EXPECT_FALSE(s2_.UnprotectRtcp(rtcp_packet_, rtcp_len_, &out_len)); - EXPECT_TRUE(metrics_observer->ExpectOnlySingleEnumCount( - webrtc::kEnumCounterSrtcpUnprotectError, srtp_err_status_cant_check)); + EXPECT_EQ(1, webrtc::metrics::NumSamples( + "WebRTC.PeerConnection.SrtcpUnprotectError")); + EXPECT_EQ( + 1, webrtc::metrics::NumEvents("WebRTC.PeerConnection.SrtcpUnprotectError", + 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 4f4cdf1f61..18d6897e3c 100644 --- a/rtc_base/BUILD.gn +++ b/rtc_base/BUILD.gn @@ -1066,6 +1066,7 @@ 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 827dc45562..2d4e19f9b5 100644 --- a/rtc_base/sslstreamadapter.h +++ b/rtc_base/sslstreamadapter.h @@ -22,6 +22,7 @@ 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; @@ -37,6 +38,7 @@ 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 df9622bd94..0b5a39da55 100644 --- a/rtc_base/unittest_main.cc +++ b/rtc_base/unittest_main.cc @@ -20,6 +20,7 @@ #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" @@ -81,6 +82,7 @@ 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 13ed2c936d..99b8194f1a 100644 --- a/system_wrappers/include/metrics.h +++ b/system_wrappers/include/metrics.h @@ -127,6 +127,9 @@ // 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, \