Skip to content

GitLab

T
tangled
Commit b1d8bf5d authored by Maksim Ivanov's avatar Maksim Ivanov Committed by Commit Bot
Browse files
Options

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

This reverts commit 4b79ed37.

Reason for revert: Causes crashes on Chrome OS - see https://crbug.com/1047222.

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}

TBR=amoylan@chromium.org,tby@chromium.org

Change-Id: I06ea1c7935600f26a7afbd2ec81a4c0f11277a70
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1016655
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2030968Reviewed-by: default avatarMaksim Ivanov <emaxx@chromium.org>
Commit-Queue: Maksim Ivanov <emaxx@chromium.org>
Cr-Commit-Position: refs/heads/master@{#736890}
parent d4843952
Expand all Hide whitespace changes
Inline Side-by-side
Showing with 33 additions and 459 deletions
components/metrics/structured/event_base.cc
View file @ b1d8bf5d
......@@ -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
View file @ b1d8bf5d
......@@ -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
View file @ b1d8bf5d
......@@ -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,37 +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;
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): Add logic for hashing an event.
}
void StructuredMetricsProvider::OnProfileAdded(
......@@ -108,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() {
......@@ -131,76 +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;
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();
// TODO(crbug.com/1016655): Add logic for uploading stored events.
}
} // namespace structured
......
components/metrics/structured/structured_metrics_provider.h
View file @ b1d8bf5d
......@@ -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
View file @ b1d8bf5d
......@@ -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
View file @ b1d8bf5d
......@@ -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>
......
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