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

PS1 is the original reland, PS2 is the re-reland, including:

 1. A fix for the original problem in structured_metrics_provider.cc
 2. A test covering that case into s_m_p_unittest.cc
 3. A deleted include that's no longer necessary after another CL.

This is a reland of 4b79ed37

Original change's description:
> 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/+/2026913
> Reviewed-by: Andrew Moylan <amoylan@chromium.org>
> Commit-Queue: Tony Yeoman <tby@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#736590}

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

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,78 @@ 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.
+  if (!recording_enabled_ || !initialized_)
+    return;
+
+  // 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};
 };
 

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>