From 322a564f49d9c995cfffbaabd3d8c5d5aa326e86 Mon Sep 17 00:00:00 2001 From: "decurtis@webrtc.org" Date: Tue, 3 Feb 2015 22:09:37 +0000 Subject: [PATCH] Fix datachannel stats id and timestamp. Makes the id now be "datachannel_#####" where '####' is the id number for the datachannel. Adds a timestamp to the data channel reports. Implements unit tests to verify that the timestamp is set correctly. BUG=1805 R=juberti@webrtc.org Review URL: https://webrtc-codereview.appspot.com/33119004 Cr-Commit-Position: refs/heads/master@{#8236} git-svn-id: http://webrtc.googlecode.com/svn/trunk@8236 4adac7df-926f-26a2-2b94-8c16560cd09d --- talk/app/webrtc/statscollector.cc | 14 ++--- talk/app/webrtc/statscollector.h | 3 + talk/app/webrtc/statscollector_unittest.cc | 68 +++++++++++++++------- talk/app/webrtc/statstypes.cc | 28 ++++++++- talk/app/webrtc/statstypes.h | 3 + 5 files changed, 85 insertions(+), 31 deletions(-) diff --git a/talk/app/webrtc/statscollector.cc b/talk/app/webrtc/statscollector.cc index 125e52cc2e..499df667b9 100644 --- a/talk/app/webrtc/statscollector.cc +++ b/talk/app/webrtc/statscollector.cc @@ -56,10 +56,6 @@ const char* STATSREPORT_ADAPTER_TYPE_WIFI = "wlan"; const char* STATSREPORT_ADAPTER_TYPE_WWAN = "wwan"; const char* STATSREPORT_ADAPTER_TYPE_VPN = "vpn"; -double GetTimeNow() { - return rtc::Timing::WallTimeNow() * rtc::kNumMillisecsPerSec; -} - bool GetTransportIdFromProxy(const cricket::ProxyTransportMap& map, const std::string& proxy, std::string* transport) { @@ -379,6 +375,10 @@ StatsCollector::~StatsCollector() { ASSERT(session_->signaling_thread()->IsCurrent()); } +double StatsCollector::GetTimeNow() { + return rtc::Timing::WallTimeNow() * rtc::kNumMillisecsPerSec; +} + // Adds a MediaStream with tracks that can be used as a |selector| in a call // to GetStats. void StatsCollector::AddStream(MediaStreamInterface* stream) { @@ -830,10 +830,10 @@ void StatsCollector::ExtractDataInfo() { for (const auto& dc : session_->mediastream_signaling()->sctp_data_channels()) { - rtc::scoped_ptr id( - StatsReport::NewTypedId(StatsReport::kStatsReportTypeDataChannel, - dc->label())); + rtc::scoped_ptr id(StatsReport::NewTypedIntId( + StatsReport::kStatsReportTypeDataChannel, dc->id())); StatsReport* report = reports_.ReplaceOrAddNew(id.Pass()); + report->set_timestamp(stats_gathering_started_); report->AddValue(StatsReport::kStatsValueNameLabel, dc->label()); report->AddValue(StatsReport::kStatsValueNameDataChannelId, dc->id()); report->AddValue(StatsReport::kStatsValueNameProtocol, dc->protocol()); diff --git a/talk/app/webrtc/statscollector.h b/talk/app/webrtc/statscollector.h index 499da44da8..a4e42c9d0c 100644 --- a/talk/app/webrtc/statscollector.h +++ b/talk/app/webrtc/statscollector.h @@ -97,6 +97,9 @@ class StatsCollector { private: friend class StatsCollectorTest; + // Overridden in unit tests to fake timing. + virtual double GetTimeNow(); + bool CopySelectedReports(const std::string& selector, StatsReports* reports); // Helper method for AddCertificateReports. diff --git a/talk/app/webrtc/statscollector_unittest.cc b/talk/app/webrtc/statscollector_unittest.cc index b8c983b842..96975c2836 100644 --- a/talk/app/webrtc/statscollector_unittest.cc +++ b/talk/app/webrtc/statscollector_unittest.cc @@ -455,6 +455,20 @@ void InitVoiceReceiverInfo(cricket::VoiceReceiverInfo* voice_receiver_info) { voice_receiver_info->expand_rate = 121; } +class StatsCollectorForTest : public webrtc::StatsCollector { + public: + explicit StatsCollectorForTest(WebRtcSession* session) : + StatsCollector(session), time_now_(19477) { + } + + double GetTimeNow() override { + return time_now_; + } + + private: + double time_now_; +}; + class StatsCollectorTest : public testing::Test { protected: StatsCollectorTest() @@ -615,7 +629,7 @@ class StatsCollectorTest : public testing::Test { const std::vector& local_ders, const rtc::FakeSSLCertificate& remote_cert, const std::vector& remote_ders) { - webrtc::StatsCollector stats(&session_); + StatsCollectorForTest stats(&session_); StatsReports reports; // returned values. @@ -718,12 +732,22 @@ TEST_F(StatsCollectorTest, ExtractDataInfo) { config.id = id; signaling_.AddDataChannel(DataChannel::Create( &data_channel_provider_, cricket::DCT_SCTP, label, config)); - webrtc::StatsCollector stats(&session_); + StatsCollectorForTest stats(&session_); stats.UpdateStats(PeerConnectionInterface::kStatsOutputLevelStandard); StatsReports reports; stats.GetStats(NULL, &reports); + + const StatsReport* report = + FindNthReportByType(reports, StatsReport::kStatsReportTypeDataChannel, 1); + + scoped_ptr reportId = StatsReport::NewTypedIntId( + StatsReport::kStatsReportTypeDataChannel, id); + + EXPECT_TRUE(reportId->Equals(report->id())); + + EXPECT_EQ(stats.GetTimeNow(), report->timestamp()); EXPECT_EQ(label, ExtractStatsValue(StatsReport::kStatsReportTypeDataChannel, reports, StatsReport::kStatsValueNameLabel)); @@ -741,7 +765,7 @@ TEST_F(StatsCollectorTest, ExtractDataInfo) { // This test verifies that 64-bit counters are passed successfully. TEST_F(StatsCollectorTest, BytesCounterHandles64Bits) { - webrtc::StatsCollector stats(&session_); + StatsCollectorForTest stats(&session_); MockVideoMediaChannel* media_channel = new MockVideoMediaChannel(); cricket::VideoChannel video_channel(rtc::Thread::Current(), @@ -775,7 +799,7 @@ TEST_F(StatsCollectorTest, BytesCounterHandles64Bits) { // Test that BWE information is reported via stats. TEST_F(StatsCollectorTest, BandwidthEstimationInfoIsReported) { - webrtc::StatsCollector stats(&session_); + StatsCollectorForTest stats(&session_); MockVideoMediaChannel* media_channel = new MockVideoMediaChannel(); cricket::VideoChannel video_channel(rtc::Thread::Current(), @@ -820,7 +844,7 @@ TEST_F(StatsCollectorTest, BandwidthEstimationInfoIsReported) { // This test verifies that an object of type "googSession" always // exists in the returned stats. TEST_F(StatsCollectorTest, SessionObjectExists) { - webrtc::StatsCollector stats(&session_); + StatsCollectorForTest stats(&session_); StatsReports reports; // returned values. EXPECT_CALL(session_, video_channel()).WillRepeatedly(ReturnNull()); @@ -835,7 +859,7 @@ TEST_F(StatsCollectorTest, SessionObjectExists) { // This test verifies that only one object of type "googSession" exists // in the returned stats. TEST_F(StatsCollectorTest, OnlyOneSessionObjectExists) { - webrtc::StatsCollector stats(&session_); + StatsCollectorForTest stats(&session_); StatsReports reports; // returned values. EXPECT_CALL(session_, video_channel()).WillRepeatedly(ReturnNull()); @@ -854,7 +878,7 @@ TEST_F(StatsCollectorTest, OnlyOneSessionObjectExists) { // This test verifies that the empty track report exists in the returned stats // without calling StatsCollector::UpdateStats. TEST_F(StatsCollectorTest, TrackObjectExistsWithoutUpdateStats) { - webrtc::StatsCollector stats(&session_); + StatsCollectorForTest stats(&session_); MockVideoMediaChannel* media_channel = new MockVideoMediaChannel(); cricket::VideoChannel video_channel(rtc::Thread::Current(), @@ -878,7 +902,7 @@ TEST_F(StatsCollectorTest, TrackObjectExistsWithoutUpdateStats) { // This test verifies that the empty track report exists in the returned stats // when StatsCollector::UpdateStats is called with ssrc stats. TEST_F(StatsCollectorTest, TrackAndSsrcObjectExistAfterUpdateSsrcStats) { - webrtc::StatsCollector stats(&session_); + StatsCollectorForTest stats(&session_); MockVideoMediaChannel* media_channel = new MockVideoMediaChannel(); cricket::VideoChannel video_channel(rtc::Thread::Current(), @@ -934,7 +958,7 @@ TEST_F(StatsCollectorTest, TrackAndSsrcObjectExistAfterUpdateSsrcStats) { // This test verifies that an SSRC object has the identifier of a Transport // stats object, and that this transport stats object exists in stats. TEST_F(StatsCollectorTest, TransportObjectLinkedFromSsrcObject) { - webrtc::StatsCollector stats(&session_); + StatsCollectorForTest stats(&session_); // Ignore unused callback (logspam). EXPECT_CALL(session_, GetTransport(_)) @@ -995,7 +1019,7 @@ TEST_F(StatsCollectorTest, TransportObjectLinkedFromSsrcObject) { // This test verifies that a remote stats object will not be created for // an outgoing SSRC where remote stats are not returned. TEST_F(StatsCollectorTest, RemoteSsrcInfoIsAbsent) { - webrtc::StatsCollector stats(&session_); + StatsCollectorForTest stats(&session_); MockVideoMediaChannel* media_channel = new MockVideoMediaChannel(); // The content_name known by the video channel. @@ -1019,7 +1043,7 @@ TEST_F(StatsCollectorTest, RemoteSsrcInfoIsAbsent) { // This test verifies that a remote stats object will be created for // an outgoing SSRC where stats are returned. TEST_F(StatsCollectorTest, RemoteSsrcInfoIsPresent) { - webrtc::StatsCollector stats(&session_); + StatsCollectorForTest stats(&session_); // Ignore unused callback (logspam). EXPECT_CALL(session_, GetTransport(_)) @@ -1062,13 +1086,13 @@ TEST_F(StatsCollectorTest, RemoteSsrcInfoIsPresent) { const StatsReport* remote_report = FindNthReportByType(reports, StatsReport::kStatsReportTypeRemoteSsrc, 1); EXPECT_FALSE(remote_report == NULL); - EXPECT_NE(0, remote_report->timestamp()); + EXPECT_EQ(12345.678, remote_report->timestamp()); } // This test verifies that the empty track report exists in the returned stats // when StatsCollector::UpdateStats is called with ssrc stats. TEST_F(StatsCollectorTest, ReportsFromRemoteTrack) { - webrtc::StatsCollector stats(&session_); + StatsCollectorForTest stats(&session_); MockVideoMediaChannel* media_channel = new MockVideoMediaChannel(); cricket::VideoChannel video_channel(rtc::Thread::Current(), @@ -1114,7 +1138,7 @@ TEST_F(StatsCollectorTest, ReportsFromRemoteTrack) { // This test verifies the Ice Candidate report should contain the correct // information from local/remote candidates. TEST_F(StatsCollectorTest, IceCandidateReport) { - webrtc::StatsCollector stats(&session_); + StatsCollectorForTest stats(&session_); StatsReports reports; // returned values. @@ -1244,7 +1268,7 @@ TEST_F(StatsCollectorTest, ChainlessCertificateReportsCreated) { // This test verifies that the stats are generated correctly when no // transport is present. TEST_F(StatsCollectorTest, NoTransport) { - webrtc::StatsCollector stats(&session_); + StatsCollectorForTest stats(&session_); StatsReports reports; // returned values. @@ -1291,7 +1315,7 @@ TEST_F(StatsCollectorTest, NoTransport) { // This test verifies that the stats are generated correctly when the transport // does not have any certificates. TEST_F(StatsCollectorTest, NoCertificates) { - webrtc::StatsCollector stats(&session_); + StatsCollectorForTest stats(&session_); StatsReports reports; // returned values. @@ -1360,7 +1384,7 @@ TEST_F(StatsCollectorTest, UnsupportedDigestIgnored) { // Verifies the correct optons are passed to the VideoMediaChannel when using // verbose output level. TEST_F(StatsCollectorTest, StatsOutputLevelVerbose) { - webrtc::StatsCollector stats(&session_); + StatsCollectorForTest stats(&session_); MockVideoMediaChannel* media_channel = new MockVideoMediaChannel(); cricket::VideoChannel video_channel(rtc::Thread::Current(), @@ -1406,7 +1430,7 @@ TEST_F(StatsCollectorTest, StatsOutputLevelVerbose) { // This test verifies that a local stats object can get statistics via // AudioTrackInterface::GetStats() method. TEST_F(StatsCollectorTest, GetStatsFromLocalAudioTrack) { - webrtc::StatsCollector stats(&session_); + StatsCollectorForTest stats(&session_); // Ignore unused callback (logspam). EXPECT_CALL(session_, GetTransport(_)) @@ -1440,7 +1464,7 @@ TEST_F(StatsCollectorTest, GetStatsFromLocalAudioTrack) { // This test verifies that audio receive streams populate stats reports // correctly. TEST_F(StatsCollectorTest, GetStatsFromRemoteStream) { - webrtc::StatsCollector stats(&session_); + StatsCollectorForTest stats(&session_); // Ignore unused callback (logspam). EXPECT_CALL(session_, GetTransport(_)) @@ -1467,7 +1491,7 @@ TEST_F(StatsCollectorTest, GetStatsFromRemoteStream) { // This test verifies that a local stats object won't update its statistics // after a RemoveLocalAudioTrack() call. TEST_F(StatsCollectorTest, GetStatsAfterRemoveAudioStream) { - webrtc::StatsCollector stats(&session_); + StatsCollectorForTest stats(&session_); // Ignore unused callback (logspam). EXPECT_CALL(session_, GetTransport(_)) @@ -1525,7 +1549,7 @@ TEST_F(StatsCollectorTest, GetStatsAfterRemoveAudioStream) { // This test verifies that when ongoing and incoming audio tracks are using // the same ssrc, they populate stats reports correctly. TEST_F(StatsCollectorTest, LocalAndRemoteTracksWithSameSsrc) { - webrtc::StatsCollector stats(&session_); + StatsCollectorForTest stats(&session_); // Ignore unused callback (logspam). EXPECT_CALL(session_, GetTransport(_)) @@ -1608,7 +1632,7 @@ TEST_F(StatsCollectorTest, LocalAndRemoteTracksWithSameSsrc) { // TODO(xians): Figure out if it is possible to encapsulate the setup and // avoid duplication of code in test cases. TEST_F(StatsCollectorTest, TwoLocalTracksWithSameSsrc) { - webrtc::StatsCollector stats(&session_); + StatsCollectorForTest stats(&session_); // Ignore unused callback (logspam). EXPECT_CALL(session_, GetTransport(_)) diff --git a/talk/app/webrtc/statstypes.cc b/talk/app/webrtc/statstypes.cc index a9f30e5922..31ae740d7e 100644 --- a/talk/app/webrtc/statstypes.cc +++ b/talk/app/webrtc/statstypes.cc @@ -76,7 +76,6 @@ class BandwidthEstimationId : public StatsReport::Id { class TypedId : public StatsReport::Id { public: - static const char kSeparator = '_'; TypedId(StatsReport::StatsType type, const std::string& id) : StatsReport::Id(type), id_(id) {} @@ -93,6 +92,26 @@ class TypedId : public StatsReport::Id { const std::string id_; }; +class TypedIntId : public StatsReport::Id { + public: + TypedIntId(StatsReport::StatsType type, int id) + : StatsReport::Id(type), id_(id) {} + + bool Equals(const Id& other) const override { + return Id::Equals(other) && + static_cast(other).id_ == id_; + } + + std::string ToString() const override { + return std::string(InternalTypeToString(type_)) + + kSeparator + + rtc::ToString(id_); + } + + protected: + const int id_; +}; + class IdWithDirection : public TypedId { public: IdWithDirection(StatsReport::StatsType type, const std::string& id, @@ -106,7 +125,7 @@ class IdWithDirection : public TypedId { std::string ToString() const override { std::string ret(TypedId::ToString()); - ret += '_'; + ret += kSeparator; ret += direction_ == StatsReport::kSend ? "send" : "recv"; return ret; } @@ -436,6 +455,11 @@ scoped_ptr StatsReport::NewTypedId( return scoped_ptr(new TypedId(type, id)).Pass(); } +// static +scoped_ptr StatsReport::NewTypedIntId(StatsType type, int id) { + return scoped_ptr(new TypedIntId(type, id)).Pass(); +} + // static scoped_ptr StatsReport::NewIdWithDirection( StatsType type, const std::string& id, StatsReport::Direction direction) { diff --git a/talk/app/webrtc/statstypes.h b/talk/app/webrtc/statstypes.h index 8f132dbdfd..a3abc2e994 100644 --- a/talk/app/webrtc/statstypes.h +++ b/talk/app/webrtc/statstypes.h @@ -229,6 +229,8 @@ class StatsReport { protected: Id(StatsType type); // Only meant for derived classes. const StatsType type_; + + static const char kSeparator = '_'; }; struct Value { @@ -254,6 +256,7 @@ class StatsReport { // Factory functions for various types of stats IDs. static rtc::scoped_ptr NewBandwidthEstimationId(); static rtc::scoped_ptr NewTypedId(StatsType type, const std::string& id); + static rtc::scoped_ptr NewTypedIntId(StatsType type, int id); static rtc::scoped_ptr NewIdWithDirection( StatsType type, const std::string& id, Direction direction); static rtc::scoped_ptr NewCandidateId(bool local, const std::string& id);