Commit 2fe77415 authored by tby's avatar tby Committed by Commit Bot

[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: default avatarAndrew Moylan <amoylan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#735748}
parent 24e79495
......@@ -42,6 +42,7 @@ source_set("unit_tests") {
"//base/test:test_support",
"//components/prefs",
"//testing/gtest",
"//tools/metrics/structured:structured_events",
]
}
......
......@@ -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;
......
......@@ -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_;
......
......@@ -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,35 @@ 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;
DCHECK(storage_->GetMutableValue("events", &events));
events->Append(std::move(event_value));
}
void StructuredMetricsProvider::OnProfileAdded(
......@@ -70,7 +106,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 +129,60 @@ 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;
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();
}
} // namespace structured
......
......@@ -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};
};
......
......@@ -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}\
"""
......
......@@ -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>
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment