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

This reverts commit 2fe77415.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 735748 as the
culprit for failures in the build cycles as shown on:
https://analysis.chromium.org/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtLzJmZTc3NDE1NjI3YjAzZGEwYWYwZWRhZjA3Y2ZmOWNjY2MzYTI4MzYM

Sample Failed Build: https://ci.chromium.org/b/8890045091910242336

Sample Failed Step: compile

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}


Change-Id: I49a0897dee3a1a5df3b07f60abdf53da74c568c1
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1016655
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2024575
Cr-Commit-Position: refs/heads/master@{#735755}

components/metrics/structured/BUILD.gn

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

components/metrics/structured/event_base.cc

@@ -9,8 +9,7 @@
 namespace metrics {
 namespace structured {
 
-EventBase::EventBase(uint64_t event_name_hash)
-    : event_name_hash_(event_name_hash) {}
+EventBase::EventBase() = default;
 EventBase::EventBase(const EventBase& other) = default;
 EventBase::~EventBase() = default;
 
@@ -18,18 +17,6 @@ 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,10 +19,17 @@ 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 {
-    kString = 0,
-    kInt = 1,
+    STRING = 0,
+    INT = 1,
   };
 
   // Stores all information about a single metric: name hash, value, and a
@@ -46,22 +53,18 @@ class EventBase {
     int int_value;
   };
 
-  // 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 AddStringMetric(uint64_t name_hash, const std::string& value) {
+    Metric metric(name_hash, MetricType::STRING);
+    metric.string_value = value;
+    metrics_.push_back(metric);
+  }
 
-  void AddIntMetric(uint64_t name_hash, int value);
+  void AddIntMetric(uint64_t name_hash, int value) {
+    Metric metric(name_hash, MetricType::INT);
+    metric.int_value = value;
+    metrics_.push_back(metric);
+  }
 
- 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,15 +4,10 @@
 
 #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 {
@@ -34,9 +29,6 @@ StructuredMetricsProvider::StructuredMetricsProvider() = default;
 StructuredMetricsProvider::~StructuredMetricsProvider() {
   if (storage_)
     storage_->RemoveObserver(this);
-  if (recording_enabled_)
-    Recorder::GetInstance()->RemoveObserver(this);
-  DCHECK(!IsInObserverList());
 }
 
 StructuredMetricsProvider::PrefStoreErrorDelegate::PrefStoreErrorDelegate() =
@@ -57,35 +49,7 @@ void StructuredMetricsProvider::OnRecord(const EventBase& event) {
   if (!recording_enabled_ || !initialized_)
     return;
 
-  // 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;
-  DCHECK(storage_->GetMutableValue("events", &events));
-  events->Append(std::move(event_value));
+  // TODO(crbug.com/1016655): Add logic for hashing an event.
 }
 
 void StructuredMetricsProvider::OnProfileAdded(
@@ -106,15 +70,7 @@ 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() {
@@ -129,60 +85,14 @@ void StructuredMetricsProvider::OnRecordingDisabled() {
   if (recording_enabled_)
     Recorder::GetInstance()->RemoveObserver(this);
   recording_enabled_ = false;
-
-  // 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();
+  // TODO(crbug.com/1016655): Ensure cache of unsent logs is cleared. Launch
+  // blocking.
 }
 
 void StructuredMetricsProvider::ProvideCurrentSessionData(
     ChromeUserMetricsExtension* uma_proto) {
   DCHECK(base::MessageLoopCurrentForUI::IsSet());
-
-  // 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;
-  DCHECK(storage_->GetMutableValue("events", &events));
-  for (const auto& event : events->GetList()) {
-    auto* event_proto = uma_proto->add_structured_event();
-
-    uint64_t event_name_hash;
-    DCHECK(base::StringToUint64(event.FindKey("name")->GetString(),
-                                &event_name_hash));
-    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;
-      DCHECK(base::StringToUint64(metric.FindKey("name")->GetString(),
-                                  &name_hash));
-      metric_proto->set_name_hash(name_hash);
-
-      const auto* value = metric.FindKey("value");
-      if (value->is_string()) {
-        uint64_t hmac;
-        DCHECK(base::StringToUint64(value->GetString(), &hmac));
-        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();
+  // TODO(crbug.com/1016655): Add logic for uploading stored events.
 }
 
 }  // namespace structured

components/metrics/structured/structured_metrics_provider.h

@@ -13,7 +13,6 @@
 #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"
@@ -109,10 +108,6 @@ 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;
@@ -142,19 +137,8 @@ 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,8 +82,7 @@ namespace events {{\
 
 IMPL_EVENT_TEMPLATE = """
 
-{event.name}::{event.name}() :
-  ::metrics::structured::EventBase(kEventNameHash) {{}}
+{event.name}::{event.name}() {{}}
 {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="TestMetricThree" kind="hashed-string">
+  <metric name="TestMetricOne" kind="hashed-string">
     <summary>
       A per-user keyed hashed value.
     </summary>