Group Ukm Recordings into a struct.

This both helps organize the members of UkmRecorderImpl a bit, and
also simplifies the code for Purge(), so that it doesn't need to know
about all of the different fields. This should help make it harder to
accidently add a new field and not delete it during a Purge().

Change-Id: I49558468cfb491c34ef76d2c2801aae72ddc8364
Reviewed-on: https://chromium-review.googlesource.com/1054142
Commit-Queue: Steven Holte <holte@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573526}

components/ukm/debug/ukm_debug_data_extractor.cc

@@ -62,11 +62,11 @@ std::string UkmDebugDataExtractor::GetHTMLData(UkmService* ukm_service) {
 
     const auto& decode_map = ukm_service->decode_map_;
     std::map<SourceId, SourceData> source_data;
-    for (const auto& kv : ukm_service->sources_) {
+    for (const auto& kv : ukm_service->recordings_.sources) {
       source_data[kv.first].source = kv.second.get();
     }
 
-    for (const auto& v : ukm_service->entries_) {
+    for (const auto& v : ukm_service->recordings_.entries) {
       source_data[v->source_id].entries.push_back(v.get());
     }
 

components/ukm/ukm_recorder_impl.cc

@@ -210,7 +210,19 @@ void UkmRecorderImpl::CreateFallbackSamplingTrial(
       trial.get());
 }
 
-void UkmRecorderImpl::SourceCounts::Reset() {
+UkmRecorderImpl::EventAggregate::EventAggregate() = default;
+UkmRecorderImpl::EventAggregate::~EventAggregate() = default;
+
+UkmRecorderImpl::Recordings::Recordings() = default;
+UkmRecorderImpl::Recordings& UkmRecorderImpl::Recordings::operator=(
+    Recordings&&) = default;
+UkmRecorderImpl::Recordings::~Recordings() = default;
+
+void UkmRecorderImpl::Recordings::Reset() {
+  *this = Recordings();
+}
+
+void UkmRecorderImpl::Recordings::SourceCounts::Reset() {
   *this = SourceCounts();
 }
 
@@ -232,11 +244,7 @@ void UkmRecorderImpl::DisableSamplingForTesting() {
 
 void UkmRecorderImpl::Purge() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  sources_.clear();
-  source_counts_.Reset();
-  carryover_urls_whitelist_.clear();
-  entries_.clear();
-  event_aggregations_.clear();
+  recordings_.Reset();
 }
 
 void UkmRecorderImpl::SetIsWebstoreExtensionCallback(
@@ -248,19 +256,19 @@ void UkmRecorderImpl::StoreRecordingsInReport(Report* report) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   std::set<SourceId> ids_seen;
-  for (const auto& entry : entries_) {
+  for (const auto& entry : recordings_.entries) {
     Entry* proto_entry = report->add_entries();
     StoreEntryProto(*entry, proto_entry);
     ids_seen.insert(entry->source_id);
   }
 
   std::unordered_set<std::string> url_whitelist;
-  carryover_urls_whitelist_.swap(url_whitelist);
-  AppendWhitelistedUrls(sources_, &url_whitelist);
+  recordings_.carryover_urls_whitelist.swap(url_whitelist);
+  AppendWhitelistedUrls(recordings_.sources, &url_whitelist);
 
   std::vector<std::unique_ptr<UkmSource>> unsent_sources;
   int unmatched_sources = 0;
-  for (auto& kv : sources_) {
+  for (auto& kv : recordings_.sources) {
     // If the source id is not whitelisted, don't send it unless it has
     // associated entries and the URL matches a URL of a whitelisted source.
     // Note: If ShouldRestrictToWhitelistedSourceIds() is true, this logic will
@@ -284,7 +292,7 @@ void UkmRecorderImpl::StoreRecordingsInReport(Report* report) {
     if (!ShouldRecordInitialUrl())
       proto_source->clear_initial_url();
   }
-  for (const auto& event_and_aggregate : event_aggregations_) {
+  for (const auto& event_and_aggregate : recordings_.event_aggregations) {
     if (event_and_aggregate.second.metrics.empty())
       continue;
     const EventAggregate& event_aggregate = event_and_aggregate.second;
@@ -326,23 +334,25 @@ void UkmRecorderImpl::StoreRecordingsInReport(Report* report) {
   }
 
   UMA_HISTOGRAM_COUNTS_1000("UKM.Sources.SerializedCount",
-                            sources_.size() - unsent_sources.size());
-  UMA_HISTOGRAM_COUNTS_100000("UKM.Entries.SerializedCount2", entries_.size());
+                            recordings_.sources.size() - unsent_sources.size());
+  UMA_HISTOGRAM_COUNTS_100000("UKM.Entries.SerializedCount2",
+                              recordings_.entries.size());
   UMA_HISTOGRAM_COUNTS_1000("UKM.Sources.UnsentSourcesCount",
                             unsent_sources.size());
 
   Report::SourceCounts* source_counts_proto = report->mutable_source_counts();
-  source_counts_proto->set_observed(source_counts_.observed);
+  source_counts_proto->set_observed(recordings_.source_counts.observed);
   source_counts_proto->set_navigation_sources(
-      source_counts_.navigation_sources);
+      recordings_.source_counts.navigation_sources);
   source_counts_proto->set_unmatched_sources(unmatched_sources);
   source_counts_proto->set_deferred_sources(unsent_sources.size());
-  source_counts_proto->set_carryover_sources(source_counts_.carryover_sources);
+  source_counts_proto->set_carryover_sources(
+      recordings_.source_counts.carryover_sources);
 
-  sources_.clear();
-  source_counts_.Reset();
-  entries_.clear();
-  event_aggregations_.clear();
+  recordings_.sources.clear();
+  recordings_.source_counts.Reset();
+  recordings_.entries.clear();
+  recordings_.event_aggregations.clear();
 
   // Keep at most |max_kept_sources|, prioritizing most-recent entries (by
   // creation time).
@@ -361,11 +371,12 @@ void UkmRecorderImpl::StoreRecordingsInReport(Report* report) {
   for (auto& source : unsent_sources) {
     // We already matched these sources against the URL whitelist.
     // Re-whitelist them for the next report.
-    carryover_urls_whitelist_.insert(source->url().spec());
-    sources_.emplace(source->id(), std::move(source));
+    recordings_.carryover_urls_whitelist.insert(source->url().spec());
+    recordings_.sources.emplace(source->id(), std::move(source));
   }
-  UMA_HISTOGRAM_COUNTS_1000("UKM.Sources.KeptSourcesCount", sources_.size());
-  source_counts_.carryover_sources = sources_.size();
+  UMA_HISTOGRAM_COUNTS_1000("UKM.Sources.KeptSourcesCount",
+                            recordings_.sources.size());
+  recordings_.source_counts.carryover_sources = recordings_.sources.size();
 }
 
 bool UkmRecorderImpl::ShouldRestrictToWhitelistedSourceIds() const {
@@ -377,9 +388,6 @@ bool UkmRecorderImpl::ShouldRestrictToWhitelistedEntries() const {
   return true;
 }
 
-UkmRecorderImpl::EventAggregate::EventAggregate() = default;
-UkmRecorderImpl::EventAggregate::~EventAggregate() = default;
-
 void UkmRecorderImpl::UpdateSourceURL(SourceId source_id,
                                       const GURL& unsanitized_url) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -400,9 +408,9 @@ void UkmRecorderImpl::UpdateSourceURL(SourceId source_id,
     return;
   }
 
-  source_counts_.observed++;
+  recordings_.source_counts.observed++;
   if (GetSourceIdType(source_id) == SourceIdType::NAVIGATION_ID)
-    source_counts_.navigation_sources++;
+    recordings_.source_counts.navigation_sources++;
 
   GURL url = SanitizeURL(unsanitized_url);
 
@@ -430,16 +438,17 @@ void UkmRecorderImpl::UpdateSourceURL(SourceId source_id,
   // Update the pre-existing source if there is any. This happens when the
   // initial URL is different from the committed URL for the same source, e.g.,
   // when there is redirection.
-  if (base::ContainsKey(sources_, source_id)) {
-    sources_[source_id]->UpdateUrl(url);
+  if (base::ContainsKey(recordings_.sources, source_id)) {
+    recordings_.sources[source_id]->UpdateUrl(url);
     return;
   }
 
-  if (sources_.size() >= GetMaxSources()) {
+  if (recordings_.sources.size() >= GetMaxSources()) {
     RecordDroppedSource(DroppedDataReason::MAX_HIT);
     return;
   }
-  sources_.emplace(source_id, std::make_unique<UkmSource>(source_id, url));
+  recordings_.sources.emplace(source_id,
+                              std::make_unique<UkmSource>(source_id, url));
 }
 
 void UkmRecorderImpl::UpdateAppURL(SourceId source_id, const GURL& url) {
@@ -460,7 +469,8 @@ void UkmRecorderImpl::AddEntry(mojom::UkmEntryPtr entry) {
     return;
   }
 
-  EventAggregate& event_aggregate = event_aggregations_[entry->event_hash];
+  EventAggregate& event_aggregate =
+      recordings_.event_aggregations[entry->event_hash];
   event_aggregate.total_count++;
   for (const auto& metric : entry->metrics) {
     MetricAggregate& aggregate = event_aggregate.metrics[metric.first];
@@ -496,7 +506,7 @@ void UkmRecorderImpl::AddEntry(mojom::UkmEntryPtr entry) {
     return;
   }
 
-  if (entries_.size() >= GetMaxEntries()) {
+  if (recordings_.entries.size() >= GetMaxEntries()) {
     RecordDroppedEntry(DroppedDataReason::MAX_HIT);
     event_aggregate.dropped_due_to_limits++;
     for (auto& metric : entry->metrics)
@@ -504,7 +514,7 @@ void UkmRecorderImpl::AddEntry(mojom::UkmEntryPtr entry) {
     return;
   }
 
-  entries_.push_back(std::move(entry));
+  recordings_.entries.push_back(std::move(entry));
 }
 
 void UkmRecorderImpl::LoadExperimentSamplingInfo() {

components/ukm/ukm_recorder_impl.h

@@ -74,10 +74,12 @@ class UkmRecorderImpl : public UkmRecorder {
   void StoreRecordingsInReport(Report* report);
 
   const std::map<SourceId, std::unique_ptr<UkmSource>>& sources() const {
-    return sources_;
+    return recordings_.sources;
   }
 
-  const std::vector<mojom::UkmEntryPtr>& entries() const { return entries_; }
+  const std::vector<mojom::UkmEntryPtr>& entries() const {
+    return recordings_.entries;
+  }
 
   // UkmRecorder:
   void UpdateSourceURL(SourceId source_id, const GURL& url) override;
@@ -132,16 +134,6 @@ class UkmRecorderImpl : public UkmRecorder {
   // Callback for checking extension IDs.
   IsWebstoreExtensionCallback is_webstore_extension_callback_;
 
-  // Contains newly added sources and entries of UKM metrics which periodically
-  // get serialized and cleared by StoreRecordingsInReport().
-  std::map<SourceId, std::unique_ptr<UkmSource>> sources_;
-  std::vector<mojom::UkmEntryPtr> entries_;
-
-  // URLs of sources that matched a whitelist url, but were not included in
-  // the report generated by the last log rotation because we haven't seen any
-  // events for that source yet.
-  std::unordered_set<std::string> carryover_urls_whitelist_;
-
   // Map from hashes to entry and metric names.
   ukm::builders::DecodeMap decode_map_;
 
@@ -152,22 +144,45 @@ class UkmRecorderImpl : public UkmRecorder {
   int default_sampling_rate_ = 0;
   base::flat_map<uint64_t, int> event_sampling_rates_;
 
-  // Aggregate information for collected event metrics.
-  std::map<uint64_t, EventAggregate> event_aggregations_;
-
-  // Aggregated counters about Sources recorded in the current log.
-  struct SourceCounts {
-    // Count of URLs recorded for all sources.
-    size_t observed = 0;
-    // Count of URLs recorded for all SourceIdType::NAVIGATION_ID Sources.
-    size_t navigation_sources = 0;
-    // Sources carried over (not recorded) from a previous logging rotation.
-    size_t carryover_sources = 0;
+  // Contains data from various recordings which periodically get serialized
+  // and cleared by StoreRecordingsInReport() and may be Purged().
+  struct Recordings {
+    Recordings();
+    Recordings& operator=(Recordings&&);
+    ~Recordings();
+
+    // Data captured by UpdateSourceUrl().
+    std::map<SourceId, std::unique_ptr<UkmSource>> sources;
+
+    // Data captured by AddEntry().
+    std::vector<mojom::UkmEntryPtr> entries;
+
+    // URLs of sources that matched a whitelist url, but were not included in
+    // the report generated by the last log rotation because we haven't seen any
+    // events for that source yet.
+    std::unordered_set<std::string> carryover_urls_whitelist;
+
+    // Aggregate information for collected event metrics.
+    std::map<uint64_t, EventAggregate> event_aggregations;
+
+    // Aggregated counters about Sources recorded in the current log.
+    struct SourceCounts {
+      // Count of URLs recorded for all sources.
+      size_t observed = 0;
+      // Count of URLs recorded for all SourceIdType::NAVIGATION_ID Sources.
+      size_t navigation_sources = 0;
+      // Sources carried over (not recorded) from a previous logging rotation.
+      size_t carryover_sources = 0;
+
+      // Resets all of the data.
+      void Reset();
+    };
+    SourceCounts source_counts;
 
     // Resets all of the data.
     void Reset();
   };
-  SourceCounts source_counts_;
+  Recordings recordings_;
 
   SEQUENCE_CHECKER(sequence_checker_);
 };

components/ukm/ukm_service_unittest.cc

@@ -222,6 +222,27 @@ TEST_F(UkmServiceTest, PersistAndPurge) {
   EXPECT_EQ(GetPersistedLogCount(), 0);
 }
 
+TEST_F(UkmServiceTest, Purge) {
+  UkmService service(&prefs_, &client_,
+                     true /* restrict_to_whitelisted_entries */);
+  TestRecordingHelper recorder(&service);
+  EXPECT_EQ(GetPersistedLogCount(), 0);
+  service.Initialize();
+  task_runner_->RunUntilIdle();
+  service.EnableRecording(/*extensions=*/false);
+  service.EnableReporting();
+
+  // Record some data
+  auto id = GetWhitelistedSourceId(0);
+  recorder.UpdateSourceURL(id, GURL("https://google.com/foobar1"));
+  TestEvent1(id).Record(&service);
+
+  // Purge should delete data, so there shouldn't be anything left to upload.
+  service.Purge();
+  service.Flush();
+  EXPECT_EQ(0, GetPersistedLogCount());
+}
+
 TEST_F(UkmServiceTest, SourceSerialization) {
   UkmService service(&prefs_, &client_,
                      true /* restrict_to_whitelisted_entries */);
@@ -557,6 +578,7 @@ TEST_F(UkmServiceTest, PurgeMidUpload) {
   task_runner_->RunUntilIdle();
   service.EnableRecording(/*extensions=*/false);
   service.EnableReporting();
+
   auto id = GetWhitelistedSourceId(0);
   recorder.UpdateSourceURL(id, GURL("https://google.com/foobar1"));
   // Should init, generate a log, and start an upload.