Add metrics::Samples to facilitate easier testing

Currently, tests that verify metrics use a combination of
metrics::NumSamples and metrics::NumEvents to assert which samples
were recorded and how many times they were recorded. This means
that a comprehensive tests has n + 1 assertions for n distinct
samples.

The new metrics::Samples function returns a map of sample --> num
events which can be asserted against using gmock matchers,
achieving better coverage and better test failure messages in just
one line.

Bug: None
Change-Id: I07d4a766654cfc04e414b77b6de02927683a361f
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/125486
Reviewed-by: Karl Wiberg <kwiberg@webrtc.org>
Commit-Queue: Steve Anton <steveanton@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26974}

pc/peer_connection_rtp_unittest.cc

@@ -64,6 +64,7 @@ namespace webrtc {
 
 using RTCConfiguration = PeerConnectionInterface::RTCConfiguration;
 using ::testing::ElementsAre;
+using ::testing::Pair;
 using ::testing::UnorderedElementsAre;
 using ::testing::Values;
 
@@ -1606,11 +1607,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(
+  EXPECT_THAT(metrics::Samples("WebRTC.PeerConnection.SdpSemanticNegotiated"),
-                   "WebRTC.PeerConnection.SdpSemanticNegotiated"));
+              ElementsAre(Pair(kSdpSemanticNegotiatedUnifiedPlan, 2)));
-  EXPECT_EQ(2, webrtc::metrics::NumEvents(
-                   "WebRTC.PeerConnection.SdpSemanticNegotiated",
-                   kSdpSemanticNegotiatedUnifiedPlan));
 }
 
 TEST_F(PeerConnectionMsidSignalingTest, PlanBOfferToUnifiedPlanAnswer) {
@@ -1698,11 +1696,8 @@ TEST_F(SdpFormatReceivedTest, DataChannelOnlyIsReportedAsNoTracks) {
 
   ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer()));
   // Note that only the callee does ReportSdpFormatReceived.
-  EXPECT_EQ(1, webrtc::metrics::NumSamples(
+  EXPECT_THAT(metrics::Samples("WebRTC.PeerConnection.SdpFormatReceived"),
-                   "WebRTC.PeerConnection.SdpFormatReceived"));
+              ElementsAre(Pair(kSdpFormatReceivedNoTracks, 1)));
-  EXPECT_EQ(
-      1, webrtc::metrics::NumEvents("WebRTC.PeerConnection.SdpFormatReceived",
-                                    kSdpFormatReceivedNoTracks));
 }
 #endif  // HAVE_SCTP
 
@@ -1714,11 +1709,8 @@ TEST_F(SdpFormatReceivedTest, SimpleUnifiedPlanIsReportedAsSimple) {
 
   ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer()));
   // Note that only the callee does ReportSdpFormatReceived.
-  EXPECT_EQ(1, webrtc::metrics::NumSamples(
+  EXPECT_THAT(metrics::Samples("WebRTC.PeerConnection.SdpFormatReceived"),
-                   "WebRTC.PeerConnection.SdpFormatReceived"));
+              ElementsAre(Pair(kSdpFormatReceivedSimple, 1)));
-  EXPECT_EQ(
-      1, webrtc::metrics::NumEvents("WebRTC.PeerConnection.SdpFormatReceived",
-                                    kSdpFormatReceivedSimple));
 }
 
 TEST_F(SdpFormatReceivedTest, SimplePlanBIsReportedAsSimple) {
@@ -1727,12 +1719,9 @@ TEST_F(SdpFormatReceivedTest, SimplePlanBIsReportedAsSimple) {
   auto callee = CreatePeerConnectionWithUnifiedPlan();
 
   ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer()));
-
+  // Note that only the callee does ReportSdpFormatReceived.
-  EXPECT_EQ(1, webrtc::metrics::NumSamples(
+  EXPECT_THAT(metrics::Samples("WebRTC.PeerConnection.SdpFormatReceived"),
-                   "WebRTC.PeerConnection.SdpFormatReceived"));
+              ElementsAre(Pair(kSdpFormatReceivedSimple, 1)));
-  EXPECT_EQ(
-      1, webrtc::metrics::NumEvents("WebRTC.PeerConnection.SdpFormatReceived",
-                                    kSdpFormatReceivedSimple));
 }
 
 TEST_F(SdpFormatReceivedTest, ComplexUnifiedIsReportedAsComplexUnifiedPlan) {
@@ -1744,11 +1733,8 @@ TEST_F(SdpFormatReceivedTest, ComplexUnifiedIsReportedAsComplexUnifiedPlan) {
 
   ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer()));
   // Note that only the callee does ReportSdpFormatReceived.
-  EXPECT_EQ(1, webrtc::metrics::NumSamples(
+  EXPECT_THAT(metrics::Samples("WebRTC.PeerConnection.SdpFormatReceived"),
-                   "WebRTC.PeerConnection.SdpFormatReceived"));
+              ElementsAre(Pair(kSdpFormatReceivedComplexUnifiedPlan, 1)));
-  EXPECT_EQ(
-      1, webrtc::metrics::NumEvents("WebRTC.PeerConnection.SdpFormatReceived",
-                                    kSdpFormatReceivedComplexUnifiedPlan));
 }
 
 TEST_F(SdpFormatReceivedTest, ComplexPlanBIsReportedAsComplexPlanB) {
@@ -1762,11 +1748,8 @@ TEST_F(SdpFormatReceivedTest, ComplexPlanBIsReportedAsComplexPlanB) {
   // SDP Format to be recorded.
   ASSERT_FALSE(callee->SetRemoteDescription(caller->CreateOffer()));
   // Note that only the callee does ReportSdpFormatReceived.
-  EXPECT_EQ(1, webrtc::metrics::NumSamples(
+  EXPECT_THAT(metrics::Samples("WebRTC.PeerConnection.SdpFormatReceived"),
-                   "WebRTC.PeerConnection.SdpFormatReceived"));
+              ElementsAre(Pair(kSdpFormatReceivedComplexPlanB, 1)));
-  EXPECT_EQ(
-      1, webrtc::metrics::NumEvents("WebRTC.PeerConnection.SdpFormatReceived",
-                                    kSdpFormatReceivedComplexPlanB));
 }
 
 // Sender setups in a call.

system_wrappers/include/metrics.h

@@ -309,6 +309,10 @@ int NumSamples(const std::string& name);
 // Returns the minimum sample value (or -1 if the histogram has no samples).
 int MinSample(const std::string& name);
 
+// Returns a map with keys the samples with at least one event and values the
+// number of events for that sample.
+std::map<int, int> Samples(const std::string& name);
+
 }  // namespace metrics
 }  // namespace webrtc
 

system_wrappers/source/metrics.cc

@@ -88,6 +88,11 @@ class RtcHistogram {
     return (info_.samples.empty()) ? -1 : info_.samples.begin()->first;
   }
 
+  std::map<int, int> Samples() const {
+    rtc::CritScope cs(&crit_);
+    return info_.samples;
+  }
+
  private:
   rtc::CriticalSection crit_;
   const int min_;
@@ -162,6 +167,12 @@ class RtcHistogramMap {
     return (it == map_.end()) ? -1 : it->second->MinSample();
   }
 
+  std::map<int, int> Samples(const std::string& name) const {
+    rtc::CritScope cs(&crit_);
+    const auto& it = map_.find(name);
+    return (it == map_.end()) ? std::map<int, int>() : it->second->Samples();
+  }
+
  private:
   rtc::CriticalSection crit_;
   std::map<std::string, std::unique_ptr<RtcHistogram>> map_
@@ -307,5 +318,10 @@ int MinSample(const std::string& name) {
   return map ? map->MinSample(name) : -1;
 }
 
+std::map<int, int> Samples(const std::string& name) {
+  RtcHistogramMap* map = GetMap();
+  return map ? map->Samples(name) : std::map<int, int>();
+}
+
 }  // namespace metrics
 }  // namespace webrtc

system_wrappers/source/metrics_unittest.cc

@@ -9,8 +9,13 @@
  */
 
 #include "system_wrappers/include/metrics.h"
+#include "test/gmock.h"
 #include "test/gtest.h"
 
+using ::testing::ElementsAre;
+using ::testing::IsEmpty;
+using ::testing::Pair;
+
 namespace webrtc {
 namespace {
 const int kSample = 22;
@@ -34,6 +39,7 @@ class MetricsTest : public ::testing::Test {
 TEST_F(MetricsTest, InitiallyNoSamples) {
   EXPECT_EQ(0, metrics::NumSamples("NonExisting"));
   EXPECT_EQ(0, metrics::NumEvents("NonExisting", kSample));
+  EXPECT_THAT(metrics::Samples("NonExisting"), IsEmpty());
 }
 
 TEST_F(MetricsTest, RtcHistogramPercent_AddSample) {
@@ -41,6 +47,7 @@ TEST_F(MetricsTest, RtcHistogramPercent_AddSample) {
   RTC_HISTOGRAM_PERCENTAGE(kName, kSample);
   EXPECT_EQ(1, metrics::NumSamples(kName));
   EXPECT_EQ(1, metrics::NumEvents(kName, kSample));
+  EXPECT_THAT(metrics::Samples(kName), ElementsAre(Pair(kSample, 1)));
 }
 
 TEST_F(MetricsTest, RtcHistogramEnumeration_AddSample) {
@@ -48,6 +55,7 @@ TEST_F(MetricsTest, RtcHistogramEnumeration_AddSample) {
   RTC_HISTOGRAM_ENUMERATION(kName, kSample, kSample + 1);
   EXPECT_EQ(1, metrics::NumSamples(kName));
   EXPECT_EQ(1, metrics::NumEvents(kName, kSample));
+  EXPECT_THAT(metrics::Samples(kName), ElementsAre(Pair(kSample, 1)));
 }
 
 TEST_F(MetricsTest, RtcHistogramBoolean_AddSample) {
@@ -56,6 +64,7 @@ TEST_F(MetricsTest, RtcHistogramBoolean_AddSample) {
   RTC_HISTOGRAM_BOOLEAN(kName, kSample);
   EXPECT_EQ(1, metrics::NumSamples(kName));
   EXPECT_EQ(1, metrics::NumEvents(kName, kSample));
+  EXPECT_THAT(metrics::Samples(kName), ElementsAre(Pair(kSample, 1)));
 }
 
 TEST_F(MetricsTest, RtcHistogramCountsSparse_AddSample) {
@@ -63,6 +72,7 @@ TEST_F(MetricsTest, RtcHistogramCountsSparse_AddSample) {
   RTC_HISTOGRAM_COUNTS_SPARSE_100(kName, kSample);
   EXPECT_EQ(1, metrics::NumSamples(kName));
   EXPECT_EQ(1, metrics::NumEvents(kName, kSample));
+  EXPECT_THAT(metrics::Samples(kName), ElementsAre(Pair(kSample, 1)));
 }
 
 TEST_F(MetricsTest, RtcHistogramCounts_AddSample) {
@@ -70,16 +80,20 @@ TEST_F(MetricsTest, RtcHistogramCounts_AddSample) {
   RTC_HISTOGRAM_COUNTS_100(kName, kSample);
   EXPECT_EQ(1, metrics::NumSamples(kName));
   EXPECT_EQ(1, metrics::NumEvents(kName, kSample));
+  EXPECT_THAT(metrics::Samples(kName), ElementsAre(Pair(kSample, 1)));
 }
 
 TEST_F(MetricsTest, RtcHistogramCounts_AddMultipleSamples) {
   const std::string kName = "Counts200";
   const int kNumSamples = 10;
+  std::map<int, int> samples;
   for (int i = 1; i <= kNumSamples; ++i) {
     RTC_HISTOGRAM_COUNTS_200(kName, i);
     EXPECT_EQ(1, metrics::NumEvents(kName, i));
     EXPECT_EQ(i, metrics::NumSamples(kName));
+    samples[i] = 1;
   }
+  EXPECT_EQ(samples, metrics::Samples(kName));
 }
 
 TEST_F(MetricsTest, RtcHistogramsCounts_AddSample) {
@@ -92,6 +106,9 @@ TEST_F(MetricsTest, RtcHistogramsCounts_AddSample) {
   EXPECT_EQ(1, metrics::NumEvents("Name1", kSample + 0));
   EXPECT_EQ(1, metrics::NumEvents("Name2", kSample + 1));
   EXPECT_EQ(1, metrics::NumEvents("Name3", kSample + 2));
+  EXPECT_THAT(metrics::Samples("Name1"), ElementsAre(Pair(kSample + 0, 1)));
+  EXPECT_THAT(metrics::Samples("Name2"), ElementsAre(Pair(kSample + 1, 1)));
+  EXPECT_THAT(metrics::Samples("Name3"), ElementsAre(Pair(kSample + 2, 1)));
 }
 
 #if RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID)
@@ -107,6 +124,8 @@ TEST_F(MetricsTest, RtcHistogramSparse_NonConstantNameWorks) {
   AddSparseSample("Sparse2", kSample);
   EXPECT_EQ(1, metrics::NumSamples("Sparse1"));
   EXPECT_EQ(1, metrics::NumSamples("Sparse2"));
+  EXPECT_THAT(metrics::Samples("Sparse1"), ElementsAre(Pair(kSample, 1)));
+  EXPECT_THAT(metrics::Samples("Sparse2"), ElementsAre(Pair(kSample, 1)));
 }
 
 }  // namespace webrtc