[Structured metrics] Change on-disk event format

Events are currently stored in this format:

{"events":[{event1}, {event2}, {event3}], "keys":{...}}

But we are about to create new categories of event, related to whether
they are associated with the UMA client ID or not. This requires we
store two separate event lists on-disk. This will look like:

{"events":{
  "independent":[{event1}, {event2}, {event3}],
  "associated":[....]},
 "keys":{...}}

This CL:

- refactors the logic for getting the events list, creating
  GetEventsList.
- adds logic to migrate from the old storage format to the new.

Bug: 1148168
Change-Id: I1d828a4e44b2e483b908a53d5d47c241f3603b8b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2532164
Commit-Queue: Tony Yeoman <tby@chromium.org>
Reviewed-by: Rachel Wong <wrong@chromium.org>
Cr-Commit-Position: refs/heads/master@{#827666}

components/metrics/structured/event_base.h

@@ -19,6 +19,16 @@ class EventBase {
   EventBase(const EventBase& other);
   virtual ~EventBase();
 
+  // Specifies the type of identifier attached to an event.
+  enum class IdentifierType {
+    // Events are attached to a per-event (or per-project) id.
+    kProjectId = 0,
+    // Events are attached to the UMA client_id.
+    kUmaId = 1,
+    // Events are attached to no id.
+    kUnidentified = 2,
+  };
+
   // Specifies which value type a Metric object holds.
   enum class MetricType {
     kString = 0,

components/metrics/structured/structured_metrics_provider.cc

@@ -9,8 +9,6 @@
 #include "base/logging.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/task/current_thread.h"
-#include "base/values.h"
-#include "components/metrics/structured/event_base.h"
 #include "components/metrics/structured/histogram_util.h"
 #include "components/prefs/json_pref_store.h"
 #include "components/prefs/writeable_pref_store.h"
@@ -23,6 +21,9 @@ namespace {
 using ::metrics::ChromeUserMetricsExtension;
 using PrefReadError = ::PersistentPrefStore::PrefReadError;
 
+constexpr char kAssociatedEventsKey[] = "associated";
+constexpr char kIndependentEventsKey[] = "independent";
+
 }  // namespace
 
 int StructuredMetricsProvider::kMaxEventsPerUpload = 100;
@@ -50,6 +51,51 @@ void StructuredMetricsProvider::PrefStoreErrorDelegate::OnError(
   LogPrefReadError(error);
 }
 
+base::Value* StructuredMetricsProvider::GetEventsList(
+    EventBase::IdentifierType type) {
+  // Ensure the events key exists. The "events" key was a list of event objects,
+  // and is now a dict of lists. Migrate to the new layout if needed.
+  base::Value* events = nullptr;
+  if (!storage_->GetMutableValue("events", &events)) {
+    storage_->SetValue(
+        "events", std::make_unique<base::Value>(base::Value::Type::DICTIONARY),
+        WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS);
+    storage_->GetMutableValue("events", &events);
+  } else if (events->is_list()) {
+    auto new_events =
+        std::make_unique<base::Value>(base::Value::Type::DICTIONARY);
+    // All existing events have previously been associated with the UMA
+    // client_id, so move them to the associated events list.
+    new_events->SetKey(kAssociatedEventsKey, events->Clone());
+    storage_->SetValue("events", std::move(new_events),
+                       WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS);
+    storage_->GetMutableValue("events", &events);
+  }
+
+  DCHECK(events->is_dict());
+
+  // Choose the key for |type|, ensure the list Value actually exists, and
+  // return it.
+  base::StringPiece list_key;
+  switch (type) {
+    case EventBase::IdentifierType::kUmaId:
+      list_key = base::StringPiece(kAssociatedEventsKey);
+      break;
+    case EventBase::IdentifierType::kProjectId:
+    case EventBase::IdentifierType::kUnidentified:
+      list_key = base::StringPiece(kIndependentEventsKey);
+      break;
+  }
+
+  base::Value* events_list = events->FindKey(list_key);
+  if (events_list) {
+    return events_list;
+  } else {
+    base::Value events_list(base::Value::Type::LIST);
+    return events->SetKey(list_key, std::move(events_list));
+  }
+}
+
 void StructuredMetricsProvider::OnRecord(const EventBase& event) {
   // Records the information in |event|, to be logged to UMA on the next call to
   // ProvideCurrentSessionData. Should only be called from the browser UI
@@ -98,12 +144,10 @@ void StructuredMetricsProvider::OnRecord(const EventBase& event) {
   event_value.SetKey("metrics", std::move(metrics));
 
   // Add the event to |storage_|.
-  base::Value* events;
-  if (!storage_->GetMutableValue("events", &events)) {
-    LOG(DFATAL) << "Events key does not exist in pref store.";
-  }
-
-  events->Append(std::move(event_value));
+  // TODO(crbug.com/1016655): Choose the event list based on the identifier type
+  // of the event subclass.
+  GetEventsList(EventBase::IdentifierType::kUmaId)
+      ->Append(std::move(event_value));
 }
 
 void StructuredMetricsProvider::OnProfileAdded(
@@ -126,13 +170,6 @@ void StructuredMetricsProvider::OnInitializationCompleted(const bool success) {
   DCHECK(!storage_->ReadOnly());
   key_data_ = std::make_unique<internal::KeyData>(storage_.get());
   initialized_ = true;
-
-  // Ensure the "events" key exists.
-  if (!storage_->GetValue("events", nullptr)) {
-    storage_->SetValue("events",
-                       std::make_unique<base::Value>(base::Value::Type::LIST),
-                       WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS);
-  }
 }
 
 void StructuredMetricsProvider::OnRecordingEnabled() {
@@ -148,14 +185,22 @@ void StructuredMetricsProvider::OnRecordingDisabled() {
     Recorder::GetInstance()->RemoveObserver(this);
   recording_enabled_ = false;
 
-  // Clear the cache of unsent logs.
+  // Clear the cache of unsent logs. Either |storage_| or its "events" key can
+  // be nullptr if OnRecordingDisabled is called before initialization is
+  // complete. In that case, there are no cached events to clear. See the class
+  // comment in the header for more details on the initialization process.
+  //
+  // "events" was migrated from a list to a dict of lists. Handle both cases in
+  // case recording is disabled before initialization can complete the
+  // migration.
   base::Value* events = nullptr;
-  // Either |storage_| or its "events" key can be nullptr if OnRecordingDisabled
-  // is called before initialization is complete. In that case, there are no
-  // cached events to clear. See the class comment in the header for more
-  // details on the initialization process.
-  if (storage_ && storage_->GetMutableValue("events", &events))
-    events->ClearList();
+  if (storage_ && storage_->GetMutableValue("events", &events)) {
+    if (events->is_list()) {
+      events->ClearList();
+    } else if (events->is_dict()) {
+      events->DictClear();
+    }
+  }
 }
 
 void StructuredMetricsProvider::ProvideCurrentSessionData(
@@ -168,11 +213,7 @@ void StructuredMetricsProvider::ProvideCurrentSessionData(
   // useful as a canary for performance issues. base::Value::EstimateMemoryUsage
   // perhaps.
 
-  base::Value* events = nullptr;
-  if (!storage_->GetMutableValue("events", &events)) {
-    LOG(DFATAL) << "Events key does not exist in pref store.";
-  }
-
+  base::Value* events = GetEventsList(EventBase::IdentifierType::kUmaId);
   for (const auto& event : events->GetList()) {
     auto* event_proto = uma_proto->add_structured_event();
 

components/metrics/structured/structured_metrics_provider.h

@@ -12,7 +12,9 @@
 #include "base/files/file_path.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
+#include "base/values.h"
 #include "components/metrics/metrics_provider.h"
+#include "components/metrics/structured/event_base.h"
 #include "components/metrics/structured/key_data.h"
 #include "components/metrics/structured/recorder.h"
 #include "components/prefs/persistent_pref_store.h"
@@ -95,6 +97,8 @@ class StructuredMetricsProvider : public metrics::MetricsProvider,
     void OnError(PersistentPrefStore::PrefReadError error) override;
   };
 
+  base::Value* GetEventsList(EventBase::IdentifierType type);
+
   // metrics::MetricsProvider:
   void OnRecordingEnabled() override;
   void OnRecordingDisabled() override;
@@ -146,9 +150,12 @@ class StructuredMetricsProvider : public metrics::MetricsProvider,
   // For details of key storage, see key_data.h
   //
   // Unsent logs are stored in hashed, ready-to-upload form in the structure:
-  //  logs[i].event
-  //         .metrics[j].name
-  //                    .value
+  //
+  //  events.<events_list>[i].metrics[j].name
+  //                                    .value
+  //
+  // The <events_list> key is either "associated" or "independent", for storing
+  // events that are or aren't associated with the UMA client_id.
   scoped_refptr<JsonPrefStore> storage_;
 
   // Storage for all event's keys, and hashing logic for values. This stores

components/metrics/structured/structured_metrics_provider_unittest.cc

@@ -5,8 +5,10 @@
 #include "components/metrics/structured/structured_metrics_provider.h"
 
 #include "base/files/file_path.h"
+#include "base/files/file_util.h"
 #include "base/files/important_file_writer.h"
 #include "base/files/scoped_temp_dir.h"
+#include "base/json/json_reader.h"
 #include "base/run_loop.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/test/metrics/histogram_tester.h"
@@ -424,5 +426,45 @@ TEST_F(StructuredMetricsProviderTest,
   EXPECT_EQ(GetProvidedEvents().structured_event_size(), 0);
 }
 
+// Ensure an old structured_metrics.json file correctly migrates to the new
+// format
+TEST_F(StructuredMetricsProviderTest, MigrateEventsKey) {
+  const auto json_path = TempDirPath().Append("structured_metrics.json");
+
+  // Write a json file with the old format.
+  const std::string old_json = R"({
+    "events":[
+      {"id":"some_id",
+       "metrics":[{
+          "name":"some_name",
+          "value":"some_value"}]}]
+  })";
+  CHECK(base::ImportantFileWriter::WriteFileAtomically(
+      TempDirPath().Append("structured_metrics.json"), old_json,
+      "StructuredMetricsProviderTest"));
+
+  // Initialize and trigger a migration by recording an event.
+  Init();
+  events::TestEventOne().SetTestMetricTwo(1).Record();
+  CommitPendingWrite();
+
+  // Check that the new format has the structure:
+  // {"events": {"associated": [{...}, {...}]}}
+  std::string new_json;
+  ASSERT_TRUE(base::ReadFileToString(json_path, &new_json));
+  const auto value = base::JSONReader::Read(new_json);
+  ASSERT_TRUE(value.has_value());
+
+  const auto* events = value.value().FindKey("events");
+  ASSERT_TRUE(events != nullptr);
+  ASSERT_TRUE(events->is_dict());
+  ASSERT_EQ(events->DictSize(), 1U);
+
+  const auto* associated = events->FindKey("associated");
+  ASSERT_TRUE(associated != nullptr);
+  ASSERT_TRUE(associated->is_list());
+  ASSERT_EQ(associated->GetList().size(), 2U);
+}
+
 }  // namespace structured
 }  // namespace metrics