Reland "[Structured metrics] Add metric collection and upload logic."

This is a reland of 2fe77415

Original change's description:
> [Structured metrics] Add metric collection and upload logic.
> 
> This is the final CL for the core of the structured metrics system. It
> ties together the KeyData class (for managing keys and computing hashes)
> with the metrics provider, and implements logic to:
> 
>  1. Hash and store incoming structured metrics events.
>  2. Provide stored metrics events for upload.
> 
> It also adds integration tests for the system, checking that an event
> created through the public API is correctly prepared for upload.
> 
> Other misc changes:
> 
>  - Some minor refactoring of event_base.{h,cc} and equivalent changes
>    to events_template.py, to fix some bad style.
>  - Added a missing RemoveObserver call to the metrics provider dtor.
> 
> Bug: 1016655
> Change-Id: I3daae2ba7927742d26db8d5750538584bf13b94f
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1975386
> Commit-Queue: Tony Yeoman <tby@chromium.org>
> Reviewed-by: Andrew Moylan <amoylan@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#735748}

Bug: 1016655
Change-Id: I83e69de7a9b865d35036b714f068cfda3c423b76
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2026913Reviewed-by: Andrew Moylan <amoylan@chromium.org>
Commit-Queue: Tony Yeoman <tby@chromium.org>
Cr-Commit-Position: refs/heads/master@{#736590}

components/metrics/structured/BUILD.gn

@@ -42,6 +42,7 @@ source_set("unit_tests") {
     "//base/test:test_support",
     "//components/prefs",
     "//testing/gtest",
+    "//tools/metrics/structured:structured_events",
   ]
 }
 

components/metrics/structured/event_base.cc

@@ -9,7 +9,8 @@
 namespace metrics {
 namespace structured {
 
-EventBase::EventBase() = default;
+EventBase::EventBase(uint64_t event_name_hash)
+    : event_name_hash_(event_name_hash) {}
 EventBase::EventBase(const EventBase& other) = default;
 EventBase::~EventBase() = default;
 
@@ -17,6 +18,18 @@ void EventBase::Record() {
   Recorder::GetInstance()->Record(std::move(*this));
 }
 
+void EventBase::AddStringMetric(uint64_t name_hash, const std::string& value) {
+  Metric metric(name_hash, MetricType::kString);
+  metric.string_value = value;
+  metrics_.push_back(metric);
+}
+
+void EventBase::AddIntMetric(uint64_t name_hash, int value) {
+  Metric metric(name_hash, MetricType::kInt);
+  metric.int_value = value;
+  metrics_.push_back(metric);
+}
+
 EventBase::Metric::Metric(uint64_t name_hash, MetricType type)
     : name_hash(name_hash), type(type) {}
 EventBase::Metric::~Metric() = default;

components/metrics/structured/event_base.h

@@ -19,17 +19,10 @@ class EventBase {
   EventBase(const EventBase& other);
   virtual ~EventBase();
 
-  // Finalizes the event and sends it for recording. After this call, the event
-  // is left in an invalid state and should not be used further.
-  void Record();
-
- protected:
-  EventBase();
-
   // Specifies which value type a Metric object holds.
   enum class MetricType {
-    STRING = 0,
-    INT = 1,
+    kString = 0,
+    kInt = 1,
   };
 
   // Stores all information about a single metric: name hash, value, and a
@@ -53,18 +46,22 @@ class EventBase {
     int int_value;
   };
 
-  void AddStringMetric(uint64_t name_hash, const std::string& value) {
-    Metric metric(name_hash, MetricType::STRING);
-    metric.string_value = value;
-    metrics_.push_back(metric);
-  }
+  // Finalizes the event and sends it for recording. After this call, the event
+  // is left in an invalid state and should not be used further.
+  void Record();
+
+  std::vector<Metric> metrics() const { return metrics_; }
+
+  uint64_t name_hash() const { return event_name_hash_; }
+
+ protected:
+  explicit EventBase(uint64_t event_name_hash);
+
+  void AddStringMetric(uint64_t name_hash, const std::string& value);
 
-  void AddIntMetric(uint64_t name_hash, int value) {
-    Metric metric(name_hash, MetricType::INT);
-    metric.int_value = value;
-    metrics_.push_back(metric);
-  }
+  void AddIntMetric(uint64_t name_hash, int value);
 
+ private:
   // First 8 bytes of the MD5 hash of the event name, as defined in
   // structured.xml. This is calculated by tools/metrics/structured/codegen.py.
   uint64_t event_name_hash_;

components/metrics/structured/structured_metrics_provider.cc

@@ -4,10 +4,15 @@
 
 #include "components/metrics/structured/structured_metrics_provider.h"
 
+#include <utility>
+
 #include "base/message_loop/message_loop_current.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/values.h"
 #include "components/metrics/structured/event_base.h"
 #include "components/prefs/json_pref_store.h"
+#include "components/prefs/writeable_pref_store.h"
+#include "third_party/metrics_proto/chrome_user_metrics_extension.pb.h"
 
 namespace metrics {
 namespace structured {
@@ -29,6 +34,9 @@ StructuredMetricsProvider::StructuredMetricsProvider() = default;
 StructuredMetricsProvider::~StructuredMetricsProvider() {
   if (storage_)
     storage_->RemoveObserver(this);
+  if (recording_enabled_)
+    Recorder::GetInstance()->RemoveObserver(this);
+  DCHECK(!IsInObserverList());
 }
 
 StructuredMetricsProvider::PrefStoreErrorDelegate::PrefStoreErrorDelegate() =
@@ -49,7 +57,37 @@ void StructuredMetricsProvider::OnRecord(const EventBase& event) {
   if (!recording_enabled_ || !initialized_)
     return;
 
-  // TODO(crbug.com/1016655): Add logic for hashing an event.
+  // Make a list of metrics.
+  base::Value metrics(base::Value::Type::LIST);
+  for (const auto& metric : event.metrics()) {
+    base::Value name_value(base::Value::Type::DICTIONARY);
+    name_value.SetStringKey("name", base::NumberToString(metric.name_hash));
+
+    if (metric.type == EventBase::MetricType::kString) {
+      // Store hashed values as strings, because the JSON parser only retains 53
+      // bits of precision for ints. This would corrupt the hashes.
+      name_value.SetStringKey(
+          "value",
+          base::NumberToString(key_data_->HashForEventMetric(
+              event.name_hash(), metric.name_hash, metric.string_value)));
+    } else if (metric.type == EventBase::MetricType::kInt) {
+      name_value.SetIntKey("value", metric.int_value);
+    }
+
+    metrics.Append(std::move(name_value));
+  }
+
+  // Create an event value containing the metrics and the event name hash.
+  base::Value event_value(base::Value::Type::DICTIONARY);
+  event_value.SetStringKey("name", base::NumberToString(event.name_hash()));
+  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));
 }
 
 void StructuredMetricsProvider::OnProfileAdded(
@@ -70,7 +108,15 @@ void StructuredMetricsProvider::OnInitializationCompleted(const bool success) {
   if (!success)
     return;
   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() {
@@ -85,14 +131,76 @@ void StructuredMetricsProvider::OnRecordingDisabled() {
   if (recording_enabled_)
     Recorder::GetInstance()->RemoveObserver(this);
   recording_enabled_ = false;
-  // TODO(crbug.com/1016655): Ensure cache of unsent logs is cleared. Launch
-  // blocking.
+
+  // Clear the cache of unsent logs.
+  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();
 }
 
 void StructuredMetricsProvider::ProvideCurrentSessionData(
     ChromeUserMetricsExtension* uma_proto) {
   DCHECK(base::MessageLoopCurrentForUI::IsSet());
-  // TODO(crbug.com/1016655): Add logic for uploading stored events.
+
+  // TODO(crbug.com/1016655): Memory usage UMA metrics for unsent logs would be
+  // 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.";
+  }
+
+  for (const auto& event : events->GetList()) {
+    auto* event_proto = uma_proto->add_structured_event();
+
+    uint64_t event_name_hash;
+    if (!base::StringToUint64(event.FindKey("name")->GetString(),
+                              &event_name_hash)) {
+      // TODO(crbug.com/1016655): We shouldn't get imperfect string conversions,
+      // but it's not impossible if there's a problematic update to
+      // structured.xml. Log an error to UMA in this case.
+      continue;
+    }
+    event_proto->set_event_name_hash(event_name_hash);
+    event_proto->set_profile_event_id(key_data_->UserEventId(event_name_hash));
+
+    for (const auto& metric : event.FindKey("metrics")->GetList()) {
+      auto* metric_proto = event_proto->add_metrics();
+      uint64_t name_hash;
+      if (!base::StringToUint64(metric.FindKey("name")->GetString(),
+                                &name_hash)) {
+        // TODO(crbug.com/1016655): Log an error to UMA. Same case as previous
+        // todo.
+        continue;
+      }
+      metric_proto->set_name_hash(name_hash);
+
+      const auto* value = metric.FindKey("value");
+      if (value->is_string()) {
+        uint64_t hmac;
+        if (!base::StringToUint64(value->GetString(), &hmac)) {
+          // TODO(crbug.com/1016655): Log an error to UMA. Same case as previous
+          // todo.
+          continue;
+        }
+        metric_proto->set_value_hmac(hmac);
+      } else if (value->is_int()) {
+        metric_proto->set_value_int64(value->GetInt());
+      }
+    }
+  }
+
+  // Clear the reported events.
+  events->ClearList();
+}
+
+void StructuredMetricsProvider::CommitPendingWriteForTest() {
+  storage_->CommitPendingWrite();
 }
 
 }  // namespace structured

components/metrics/structured/structured_metrics_provider.h

@@ -13,6 +13,7 @@
 #include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
 #include "components/metrics/metrics_provider.h"
+#include "components/metrics/structured/key_data.h"
 #include "components/metrics/structured/recorder.h"
 #include "components/prefs/persistent_pref_store.h"
 #include "components/prefs/pref_store.h"
@@ -108,6 +109,10 @@ class StructuredMetricsProvider : public metrics::MetricsProvider,
   void OnInitializationCompleted(bool success) override;
   void OnPrefValueChanged(const std::string& key) override {}
 
+  // Makes the |storage_| PrefStore flush to disk. Used for flushing any
+  // modified but not-yet-written data to disk during unit tests.
+  void CommitPendingWriteForTest();
+
   // Beyond this number of logging events between successive calls to
   // ProvideCurrentSessionData, we stop recording events.
   static int kMaxEventsPerUpload;
@@ -137,8 +142,19 @@ class StructuredMetricsProvider : public metrics::MetricsProvider,
   // On-device storage within the user's cryptohome for keys and unsent logs.
   // This is constructed as part of initialization and is guaranteed to be
   // initialized if |initialized_| is true.
+  //
+  // 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
   scoped_refptr<JsonPrefStore> storage_;
 
+  // Storage for all event's keys, and hashing logic for values. This stores
+  // keys on-disk using the |storage_| JsonPrefStore.
+  std::unique_ptr<internal::KeyData> key_data_;
+
   base::WeakPtrFactory<StructuredMetricsProvider> weak_factory_{this};
 };
 

components/metrics/structured/structured_metrics_provider_unittest.cc

@@ -5,38 +5,139 @@
 #include "components/metrics/structured/structured_metrics_provider.h"
 
 #include "base/files/file_path.h"
+#include "base/files/important_file_writer.h"
 #include "base/files/scoped_temp_dir.h"
+#include "base/run_loop.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/test/task_environment.h"
 #include "base/values.h"
 #include "components/metrics/structured/event_base.h"
 #include "components/metrics/structured/recorder.h"
 #include "components/prefs/json_pref_store.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/metrics_proto/chrome_user_metrics_extension.pb.h"
+#include "tools/metrics/structured/structured_events.h"
 
 namespace metrics {
 namespace structured {
 
+namespace {
+
+// These event and metric names are used for testing.
+// - event: TestEventOne
+//   - metric: TestMetricOne
+//   - metric: TestMetricTwo
+// - event: TestsEventTwo
+//   - metric: TestMetricThree
+
+// To test that the right values are calculated for hashed metrics, we need to
+// set up some fake keys that we know the output hashes for. kKeyData contains
+// the JSON for a simple structured_metrics.json file with keys for the test
+// events.
+// TODO(crbug.com/1016655): Once custom rotation periods have been implemented,
+// change the large constants to 0.
+constexpr char kKeyData[] = R"({
+  "keys":{
+    "15619026293081468407":{
+      "rotation_period":1000000,
+      "last_rotation":1000000,
+      "key":"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+    },
+    "15791833939776536363":{
+      "rotation_period":1000000,
+      "last_rotation":1000000,
+      "key":"bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"
+    }
+  }
+})";
+
+// The name hash of "TestEventOne".
+constexpr uint64_t kEventOneHash = UINT64_C(15619026293081468407);
+// The name hash of "TestEventTwo".
+constexpr uint64_t kEventTwoHash = UINT64_C(15791833939776536363);
+
+// The name hash of "TestMetricOne".
+constexpr uint64_t kMetricOneHash = UINT64_C(637929385654885975);
+// The name hash of "TestMetricTwo".
+constexpr uint64_t kMetricTwoHash = UINT64_C(14083999144141567134);
+// The name hash of "TestMetricThree".
+constexpr uint64_t kMetricThreeHash = UINT64_C(13469300759843809564);
+
+// The hex-encoded first 8 bytes of SHA256("aaa...a")
+constexpr char kKeyOneId[] = "3BA3F5F43B926026";
+// The hex-encoded first 8 bytes of SHA256("bbb...b")
+constexpr char kKeyTwoId[] = "BDB339768BC5E4FE";
+
+// Test values.
+constexpr char kValueOne[] = "value one";
+constexpr char kValueTwo[] = "value two";
+
+std::string HashToHex(const uint64_t hash) {
+  return base::HexEncode(&hash, sizeof(uint64_t));
+}
+
+}  // namespace
+
 class StructuredMetricsProviderTest : public testing::Test {
  protected:
   void SetUp() override {
     ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
-
     storage_ = new JsonPrefStore(temp_dir_.GetPath().Append("storage.json"));
-    provider_ = std::make_unique<StructuredMetricsProvider>();
+    Recorder::GetInstance()->SetUiTaskRunner(
+        task_environment_.GetMainThreadTaskRunner());
   }
 
   base::FilePath TempDirPath() { return temp_dir_.GetPath(); }
 
   void Wait() { task_environment_.RunUntilIdle(); }
 
-  void BeginInit() { return provider_->OnProfileAdded(TempDirPath()); }
+  void WriteTestingKeys() {
+    CHECK(base::ImportantFileWriter::WriteFileAtomically(
+        TempDirPath().Append("structured_metrics.json"), kKeyData,
+        "StructuredMetricsProviderTest"));
+  }
+
+  // Simulates the three external events that the structure metrics system cares
+  // about: the metrics service initializing and enabling its providers, and a
+  // user logging in.
+  void Init() {
+    // Create the provider, normally done by the ChromeMetricsServiceClient.
+    provider_ = std::make_unique<StructuredMetricsProvider>();
+    // Enable recording, normally done after the metrics service has checked
+    // consent allows recording.
+    provider_->OnRecordingEnabled();
+    // Add a profile, normally done by the ChromeMetricsServiceClient after a
+    // user logs in.
+    provider_->OnProfileAdded(TempDirPath());
+    Wait();
+  }
 
   bool is_initialized() { return provider_->initialized_; }
   bool is_recording_enabled() { return provider_->recording_enabled_; }
 
- private:
+  void OnRecordingEnabled() { provider_->OnRecordingEnabled(); }
+
+  void OnRecordingDisabled() { provider_->OnRecordingDisabled(); }
+
+  void OnProfileAdded(const base::FilePath& path) {
+    provider_->OnProfileAdded(path);
+  }
+
+  void CommitPendingWrite() {
+    provider_->CommitPendingWriteForTest();
+    Wait();
+  }
+
+  ChromeUserMetricsExtension GetProvidedEvents() {
+    ChromeUserMetricsExtension uma_proto;
+    provider_->ProvideCurrentSessionData(&uma_proto);
+    return uma_proto;
+  }
+
+ protected:
   std::unique_ptr<StructuredMetricsProvider> provider_;
 
+ private:
   base::test::TaskEnvironment task_environment_{
       base::test::TaskEnvironment::MainThreadType::UI,
       base::test::TaskEnvironment::ThreadPoolExecutionMode::QUEUED};
@@ -44,14 +145,205 @@ class StructuredMetricsProviderTest : public testing::Test {
   scoped_refptr<JsonPrefStore> storage_;
 };
 
+// Simple test to ensure initialization works correctly in the case of a
+// first-time run.
 TEST_F(StructuredMetricsProviderTest, ProviderInitializesFromBlankSlate) {
-  BeginInit();
+  Init();
+  EXPECT_TRUE(is_initialized());
+  EXPECT_TRUE(is_recording_enabled());
+}
+
+// Ensure a call to OnRecordingDisabled prevents reporting.
+TEST_F(StructuredMetricsProviderTest, EventsNotReportedWhenRecordingDisabled) {
+  Init();
+  OnRecordingDisabled();
+  events::TestEventOne().SetTestMetricTwo(1).Record();
+  EXPECT_EQ(GetProvidedEvents().structured_event_size(), 0);
+}
+
+// Ensure that, if recording is disabled part-way through initialization, the
+// initialization still completes correctly, but recording is correctly set to
+// disabled.
+TEST_F(StructuredMetricsProviderTest, RecordingDisabledDuringInitialization) {
+  provider_ = std::make_unique<StructuredMetricsProvider>();
+
+  OnProfileAdded(TempDirPath());
+  OnRecordingDisabled();
+  EXPECT_FALSE(is_initialized());
+  EXPECT_FALSE(is_recording_enabled());
+
+  Wait();
+  EXPECT_TRUE(is_initialized());
+  EXPECT_FALSE(is_recording_enabled());
+}
+
+// Ensure that recording is disabled until explicitly enabled with a call to
+// OnRecordingEnabled.
+TEST_F(StructuredMetricsProviderTest, RecordingDisabledByDefault) {
+  provider_ = std::make_unique<StructuredMetricsProvider>();
+
+  OnProfileAdded(TempDirPath());
   Wait();
   EXPECT_TRUE(is_initialized());
+  EXPECT_FALSE(is_recording_enabled());
+
+  OnRecordingEnabled();
+  EXPECT_TRUE(is_recording_enabled());
+}
+
+TEST_F(StructuredMetricsProviderTest, RecordedEventAppearsInReport) {
+  Init();
+
+  events::TestEventOne()
+      .SetTestMetricOne("a string")
+      .SetTestMetricTwo(12345)
+      .Record();
+  events::TestEventOne()
+      .SetTestMetricOne("a string")
+      .SetTestMetricTwo(12345)
+      .Record();
+  events::TestEventOne()
+      .SetTestMetricOne("a string")
+      .SetTestMetricTwo(12345)
+      .Record();
+
+  EXPECT_EQ(GetProvidedEvents().structured_event_size(), 3);
+}
+
+TEST_F(StructuredMetricsProviderTest, EventsReportedCorrectly) {
+  WriteTestingKeys();
+  Init();
+
+  events::TestEventOne()
+      .SetTestMetricOne(kValueOne)
+      .SetTestMetricTwo(12345)
+      .Record();
+  events::TestEventTwo().SetTestMetricThree(kValueTwo).Record();
+
+  const auto uma = GetProvidedEvents();
+  ASSERT_EQ(uma.structured_event_size(), 2);
+
+  {  // First event
+    const auto& event = uma.structured_event(0);
+    EXPECT_EQ(event.event_name_hash(), kEventOneHash);
+    EXPECT_EQ(HashToHex(event.profile_event_id()), kKeyOneId);
+    ASSERT_EQ(event.metrics_size(), 2);
+
+    {  // First metric
+      const auto& metric = event.metrics(0);
+      EXPECT_EQ(metric.name_hash(), kMetricOneHash);
+      EXPECT_EQ(HashToHex(metric.value_hmac()),
+                // Value of HMAC_256("aaa...a", concat(hex(kMetricOneHash),
+                // "value one"))
+                "8C2469269D142715");
+    }
+
+    {  // Second metric
+      const auto& metric = event.metrics(1);
+      EXPECT_EQ(metric.name_hash(), kMetricTwoHash);
+      EXPECT_EQ(metric.value_int64(), 12345);
+    }
+  }
+
+  {  // Second event
+    const auto& event = uma.structured_event(1);
+    EXPECT_EQ(event.event_name_hash(), kEventTwoHash);
+    EXPECT_EQ(HashToHex(event.profile_event_id()), kKeyTwoId);
+    ASSERT_EQ(event.metrics_size(), 1);
+
+    {  // First metric
+      const auto& metric = event.metrics(0);
+      EXPECT_EQ(metric.name_hash(), kMetricThreeHash);
+      EXPECT_EQ(HashToHex(metric.value_hmac()),
+                // Value of HMAC_256("bbb...b", concat(hex(kMetricOneHash),
+                // "value three"))
+                "86F0169868588DC7");
+    }
+  }
+}
+
+// Test that a call to ProvideCurrentSessionData clears the provided events from
+// the cache, and a subsequent call does not return those events again.
+TEST_F(StructuredMetricsProviderTest, EventsClearedAfterReport) {
+  Init();
+
+  events::TestEventOne().SetTestMetricTwo(1).Record();
+  events::TestEventOne().SetTestMetricTwo(2).Record();
+  // Should provide both the previous events.
+  EXPECT_EQ(GetProvidedEvents().structured_event_size(), 2);
+
+  // But the previous events shouldn't appear in the second report.
+  EXPECT_EQ(GetProvidedEvents().structured_event_size(), 0);
+
+  events::TestEventOne().SetTestMetricTwo(3).Record();
+  // The third request should only contain the third event.
+  EXPECT_EQ(GetProvidedEvents().structured_event_size(), 1);
+}
+
+// Test that events recorded in one session are correctly persisted and are
+// uploaded in the first report from a subsequent session.
+TEST_F(StructuredMetricsProviderTest, EventsFromPreviousSessionAreReported) {
+  // Start first session and record one event.
+  Init();
+  events::TestEventOne().SetTestMetricTwo(1234).Record();
+
+  // Write events to disk, then destroy the provider.
+  CommitPendingWrite();
+  provider_.reset();
+
+  // Start a second session and ensure the event is reported.
+  Init();
+  const auto uma = GetProvidedEvents();
+  ASSERT_EQ(uma.structured_event_size(), 1);
+  ASSERT_EQ(uma.structured_event(0).metrics_size(), 1);
+  EXPECT_EQ(uma.structured_event(0).metrics(0).value_int64(), 1234);
+}
+
+// Test that events reported at various stages before and during initialization
+// are ignored (and don't cause a crash).
+TEST_F(StructuredMetricsProviderTest, EventsNotRecordedBeforeInitialization) {
+  // Manually create and initialize the provider, adding recording calls between
+  // each step. All of these events should be ignored.
+  events::TestEventOne().SetTestMetricTwo(1).Record();
+  provider_ = std::make_unique<StructuredMetricsProvider>();
+  events::TestEventOne().SetTestMetricTwo(1).Record();
+  OnRecordingEnabled();
+  events::TestEventOne().SetTestMetricTwo(1).Record();
+  OnProfileAdded(TempDirPath());
+  // This one should still fail even though all of the initialization calls are
+  // done, because the provider hasn't finished loading the keys from disk.
+  events::TestEventOne().SetTestMetricTwo(1).Record();
+  Wait();
+  EXPECT_EQ(GetProvidedEvents().structured_event_size(), 0);
 }
 
-// TODO(crbug.com/1016655): Add more tests once codegen for EventBase subclasses
-// has been merged.
+// Ensure a call to OnRecordingDisabled not only prevents the reporting of new
+// events, but also clears the cache of any existing events that haven't yet
+// been reported.
+TEST_F(StructuredMetricsProviderTest,
+       ExistingEventsClearedWhenRecordingDisabled) {
+  Init();
+  events::TestEventOne().SetTestMetricTwo(1).Record();
+  events::TestEventOne().SetTestMetricTwo(1).Record();
+  OnRecordingDisabled();
+  events::TestEventOne().SetTestMetricTwo(1).Record();
+  EXPECT_EQ(GetProvidedEvents().structured_event_size(), 0);
+}
+
+// Ensure that recording and reporting is re-enabled after recording is disabled
+// and then enabled again.
+TEST_F(StructuredMetricsProviderTest, ReportingResumesWhenEnabled) {
+  Init();
+  events::TestEventOne().SetTestMetricTwo(1).Record();
+  events::TestEventOne().SetTestMetricTwo(1).Record();
+  OnRecordingDisabled();
+  events::TestEventOne().SetTestMetricTwo(1).Record();
+
+  OnRecordingEnabled();
+  events::TestEventOne().SetTestMetricTwo(1).Record();
+  events::TestEventOne().SetTestMetricTwo(1).Record();
+  EXPECT_EQ(GetProvidedEvents().structured_event_size(), 2);
+}
 
 }  // namespace structured
 }  // namespace metrics

tools/metrics/structured/events_template.py

@@ -82,7 +82,8 @@ namespace events {{\
 
 IMPL_EVENT_TEMPLATE = """
 
-{event.name}::{event.name}() {{}}
+{event.name}::{event.name}() :
+  ::metrics::structured::EventBase(kEventNameHash) {{}}
 {event.name}::~{event.name}() = default;\
 {metric_code}\
 """

tools/metrics/structured/structured.xml

@@ -24,7 +24,7 @@
   <summary>
     Event for unit testing, do not use.
   </summary>
-  <metric name="TestMetricOne" kind="hashed-string">
+  <metric name="TestMetricThree" kind="hashed-string">
     <summary>
       A per-user keyed hashed value.
     </summary>