Persistent metrics files that have an embedded profile don't need to
be sent during startup. Instead, send them as independent logs
once the browser is already running.
BUG=695880
Review-Url: https://codereview.chromium.org/2918533003
Cr-Commit-Position: refs/heads/master@{#478359}
diff --git a/chrome/browser/metrics/chrome_metrics_service_client.cc b/chrome/browser/metrics/chrome_metrics_service_client.cc
index 9d24cd6d..699bee1e 100644
--- a/chrome/browser/metrics/chrome_metrics_service_client.cc
+++ b/chrome/browser/metrics/chrome_metrics_service_client.cc
@@ -177,6 +177,7 @@
bool metrics_reporting_enabled,
const base::FilePath& dir,
base::StringPiece metrics_name,
+ metrics::FileMetricsProvider::SourceAssociation association,
scoped_refptr<base::TaskRunner> task_runner,
metrics::FileMetricsProvider* file_metrics_provider) {
base::FilePath metrics_file;
@@ -188,7 +189,7 @@
file_metrics_provider->RegisterSource(
metrics_file,
metrics::FileMetricsProvider::SOURCE_HISTOGRAMS_ATOMIC_FILE,
- metrics::FileMetricsProvider::ASSOCIATE_PREVIOUS_RUN, metrics_name);
+ association, metrics_name);
} else {
// When metrics reporting is not enabled, any existing file should be
// deleted in order to preserve user privacy.
@@ -223,7 +224,8 @@
metrics_reporting_enabled && (send_unreported == "yes");
RegisterOrRemovePreviousRunMetricsFile(
report_previous_persistent_histograms, user_data_dir,
- ChromeMetricsServiceClient::kBrowserMetricsName, task_runner,
+ ChromeMetricsServiceClient::kBrowserMetricsName,
+ metrics::FileMetricsProvider::ASSOCIATE_INTERNAL_PROFILE, task_runner,
file_metrics_provider.get());
// Register the Crashpad metrics files.
@@ -231,8 +233,10 @@
// cleanly.
RegisterOrRemovePreviousRunMetricsFile(
metrics_reporting_enabled, user_data_dir,
- kCrashpadHistogramAllocatorName, task_runner,
- file_metrics_provider.get());
+ kCrashpadHistogramAllocatorName,
+ metrics::FileMetricsProvider::
+ ASSOCIATE_INTERNAL_PROFILE_OR_PREVIOUS_RUN,
+ task_runner, file_metrics_provider.get());
if (metrics_reporting_enabled) {
base::FilePath active_path;
base::GlobalHistogramAllocator::ConstructFilePaths(
diff --git a/components/metrics/file_metrics_provider.cc b/components/metrics/file_metrics_provider.cc
index d9a6a04..61297c6e 100644
--- a/components/metrics/file_metrics_provider.cc
+++ b/components/metrics/file_metrics_provider.cc
@@ -20,6 +20,7 @@
#include "base/time/time.h"
#include "components/metrics/metrics_pref_names.h"
#include "components/metrics/metrics_service.h"
+#include "components/metrics/persistent_system_profile.h"
#include "components/prefs/pref_registry_simple.h"
#include "components/prefs/pref_service.h"
@@ -70,6 +71,19 @@
}
};
+enum EmbeddedProfileResult : int {
+ EMBEDDED_PROFILE_ATTEMPT,
+ EMBEDDED_PROFILE_FOUND,
+ EMBEDDED_PROFILE_FALLBACK,
+ EMBEDDED_PROFILE_DROPPED,
+ EMBEDDED_PROFILE_ACTION_MAX
+};
+
+void RecordEmbeddedProfileResult(EmbeddedProfileResult result) {
+ UMA_HISTOGRAM_ENUMERATION("UMA.FileMetricsProvider.EmbeddedProfileResult",
+ result, EMBEDDED_PROFILE_ACTION_MAX);
+}
+
void DeleteFileWhenPossible(const base::FilePath& path) {
// Open (with delete) and then immediately close the file by going out of
// scope. This is the only cross-platform safe way to delete a file that may
@@ -84,12 +98,16 @@
// This structure stores all the information about the sources being monitored
// and their current reporting state.
struct FileMetricsProvider::SourceInfo {
- SourceInfo(SourceType source_type) : type(source_type) {}
+ SourceInfo(SourceType source_type, SourceAssociation source_association)
+ : type(source_type), association(source_association) {}
~SourceInfo() {}
// How to access this source (file/dir, atomic/active).
const SourceType type;
+ // With what run this source is associated.
+ const SourceAssociation association;
+
// Where on disk the directory is located. This will only be populated when
// a directory is being monitored.
base::FilePath directory;
@@ -136,7 +154,7 @@
// Ensure that kSourceOptions has been filled for this type.
DCHECK_GT(arraysize(kSourceOptions), static_cast<size_t>(type));
- std::unique_ptr<SourceInfo> source(new SourceInfo(type));
+ std::unique_ptr<SourceInfo> source(new SourceInfo(type, source_association));
source->prefs_key = prefs_key.as_string();
switch (source->type) {
@@ -161,9 +179,11 @@
switch (source_association) {
case ASSOCIATE_CURRENT_RUN:
+ case ASSOCIATE_INTERNAL_PROFILE:
sources_to_check_.push_back(std::move(source));
break;
case ASSOCIATE_PREVIOUS_RUN:
+ case ASSOCIATE_INTERNAL_PROFILE_OR_PREVIOUS_RUN:
DCHECK_EQ(SOURCE_HISTOGRAMS_ATOMIC_FILE, source->type);
sources_for_previous_run_.push_back(std::move(source));
break;
@@ -250,6 +270,30 @@
}
// static
+void FileMetricsProvider::FinishedWithSource(SourceInfo* source,
+ AccessResult result) {
+ // Different source types require different post-processing.
+ switch (source->type) {
+ case SOURCE_HISTOGRAMS_ATOMIC_FILE:
+ case SOURCE_HISTOGRAMS_ATOMIC_DIR:
+ // Done with this file so delete the allocator and its owned file.
+ source->allocator.reset();
+ // Remove the file if has been recorded. This prevents them from
+ // accumulating or also being recorded by different instances of
+ // the browser.
+ if (result == ACCESS_RESULT_SUCCESS ||
+ result == ACCESS_RESULT_NOT_MODIFIED) {
+ DeleteFileWhenPossible(source->path);
+ }
+ break;
+ case SOURCE_HISTOGRAMS_ACTIVE_FILE:
+ // Keep the allocator open so it doesn't have to be re-mapped each
+ // time. This also allows the contents to be merged on-demand.
+ break;
+ }
+}
+
+// static
void FileMetricsProvider::CheckAndMergeMetricSourcesOnTaskRunner(
SourceInfoList* sources) {
// This method has all state information passed in |sources| and is intended
@@ -264,31 +308,19 @@
"UMA.FileMetricsProvider.AccessResult", result, ACCESS_RESULT_MAX);
}
+ // Metrics associated with internal profiles have to be fetched directly
+ // so just keep the mapping for use by the main thread.
+ if (source->association == ASSOCIATE_INTERNAL_PROFILE)
+ continue;
+
// Mapping was successful. Merge it.
if (result == ACCESS_RESULT_SUCCESS) {
MergeHistogramDeltasFromSource(source.get());
DCHECK(source->read_complete);
}
- // Different source types require different post-processing.
- switch (source->type) {
- case SOURCE_HISTOGRAMS_ATOMIC_FILE:
- case SOURCE_HISTOGRAMS_ATOMIC_DIR:
- // Done with this file so delete the allocator and its owned file.
- source->allocator.reset();
- // Remove the file if has been recorded. This prevents them from
- // accumulating or also being recorded by different instances of
- // the browser.
- if (result == ACCESS_RESULT_SUCCESS ||
- result == ACCESS_RESULT_NOT_MODIFIED) {
- base::DeleteFile(source->path, /*recursive=*/false);
- }
- break;
- case SOURCE_HISTOGRAMS_ACTIVE_FILE:
- // Keep the allocator open so it doesn't have to be re-mapped each
- // time. This also allows the contents to be merged on-demand.
- break;
- }
+ // All done with this source.
+ FinishedWithSource(source.get(), result);
}
}
@@ -297,8 +329,11 @@
// static
FileMetricsProvider::AccessResult FileMetricsProvider::CheckAndMapMetricSource(
SourceInfo* source) {
- DCHECK(!source->allocator);
+ // If source was read, clean up after it.
+ if (source->read_complete)
+ FinishedWithSource(source, ACCESS_RESULT_SUCCESS);
source->read_complete = false;
+ DCHECK(!source->allocator);
// If the source is a directory, look for files within it.
if (!source->directory.empty() && !LocateNextFileInDirectory(source))
@@ -437,10 +472,16 @@
RecordSourceAsRead(source);
did_read = true;
}
- if (source->allocator)
- sources_mapped_.splice(sources_mapped_.end(), *checked, temp);
- else
+ if (source->allocator) {
+ if (source->association == ASSOCIATE_INTERNAL_PROFILE) {
+ sources_with_profile_.splice(sources_with_profile_.end(), *checked,
+ temp);
+ } else {
+ sources_mapped_.splice(sources_mapped_.end(), *checked, temp);
+ }
+ } else {
sources_to_check_.splice(sources_to_check_.end(), *checked, temp);
+ }
}
// If a read was done, schedule another one immediately. In the case of a
@@ -487,6 +528,39 @@
sources_for_previous_run_.clear();
}
+bool FileMetricsProvider::ProvideIndependentMetrics(
+ SystemProfileProto* system_profile_proto,
+ base::HistogramSnapshotManager* snapshot_manager) {
+ DCHECK(thread_checker_.CalledOnValidThread());
+
+ while (!sources_with_profile_.empty()) {
+ SourceInfo* source = sources_with_profile_.begin()->get();
+ DCHECK(source->allocator);
+
+ bool success = false;
+ RecordEmbeddedProfileResult(EMBEDDED_PROFILE_ATTEMPT);
+ if (PersistentSystemProfile::GetSystemProfile(
+ *source->allocator->memory_allocator(), system_profile_proto)) {
+ RecordHistogramSnapshotsFromSource(snapshot_manager, source);
+ success = true;
+ RecordEmbeddedProfileResult(EMBEDDED_PROFILE_FOUND);
+ } else {
+ RecordEmbeddedProfileResult(EMBEDDED_PROFILE_DROPPED);
+ }
+
+ // Regardless of whether this source was successfully recorded, it is never
+ // read again.
+ source->read_complete = true;
+ RecordSourceAsRead(source);
+ sources_to_check_.splice(sources_to_check_.end(), sources_with_profile_,
+ sources_with_profile_.begin());
+ if (success)
+ return true;
+ }
+
+ return false;
+}
+
bool FileMetricsProvider::HasInitialStabilityMetrics() {
DCHECK(thread_checker_.CalledOnValidThread());
@@ -521,6 +595,21 @@
RecordSourceAsRead(source);
DeleteFileAsync(source->path);
sources_for_previous_run_.erase(temp);
+ continue;
+ }
+
+ DCHECK(source->allocator);
+
+ // If the source should be associated with an existing internal profile,
+ // move it to |sources_with_profile_| for later upload.
+ if (source->association == ASSOCIATE_INTERNAL_PROFILE_OR_PREVIOUS_RUN) {
+ if (PersistentSystemProfile::HasSystemProfile(
+ *source->allocator->memory_allocator())) {
+ RecordEmbeddedProfileResult(EMBEDDED_PROFILE_ATTEMPT);
+ RecordEmbeddedProfileResult(EMBEDDED_PROFILE_FALLBACK);
+ sources_with_profile_.splice(sources_with_profile_.end(),
+ sources_for_previous_run_, temp);
+ }
}
}
diff --git a/components/metrics/file_metrics_provider.h b/components/metrics/file_metrics_provider.h
index 43d4d30..a19577b 100644
--- a/components/metrics/file_metrics_provider.h
+++ b/components/metrics/file_metrics_provider.h
@@ -80,6 +80,19 @@
// This is important when metrics are dumped as part of a crash of the
// previous run. This can only be used with FILE_HISTOGRAMS_ATOMIC.
ASSOCIATE_PREVIOUS_RUN,
+
+ // Associates the metrics in the file with the a profile embedded in the
+ // same file. The reporting will take place at a convenient time after
+ // startup when the browser is otherwise idle. If there is no embedded
+ // system profile, these metrics will be lost.
+ ASSOCIATE_INTERNAL_PROFILE,
+
+ // Like above but fall back to ASSOCIATE_PREVIOUS_RUN if there is no
+ // embedded profile. This has a small cost during startup as that is
+ // when previous-run metrics are sent so the file has be checked at
+ // that time even though actual transfer will be delayed if an
+ // embedded profile is found.
+ ASSOCIATE_INTERNAL_PROFILE_OR_PREVIOUS_RUN,
};
FileMetricsProvider(const scoped_refptr<base::TaskRunner>& task_runner,
@@ -147,6 +160,9 @@
// be internally updated to indicate the next file to be read.
static bool LocateNextFileInDirectory(SourceInfo* source);
+ // Handles the completion of a source.
+ static void FinishedWithSource(SourceInfo* source, AccessResult result);
+
// Checks a list of sources (on a task-runner allowed to do I/O) and merge
// any data found within them.
static void CheckAndMergeMetricSourcesOnTaskRunner(SourceInfoList* sources);
@@ -175,8 +191,11 @@
// Updates the persistent state information to show a source as being read.
void RecordSourceAsRead(SourceInfo* source);
- // metrics::MetricsDataProvider:
+ // metrics::MetricsProvider:
void OnDidCreateMetricsLog() override;
+ bool ProvideIndependentMetrics(
+ SystemProfileProto* system_profile_proto,
+ base::HistogramSnapshotManager* snapshot_manager) override;
bool HasInitialStabilityMetrics() override;
void RecordInitialHistogramSnapshots(
base::HistogramSnapshotManager* snapshot_manager) override;
@@ -193,6 +212,9 @@
// A list of currently active sources to be merged when required.
SourceInfoList sources_mapped_;
+ // A list of currently active sources to be merged when required.
+ SourceInfoList sources_with_profile_;
+
// A list of sources for a previous run. These are held separately because
// they are not subject to the periodic background checking that handles
// metrics for the current run.
diff --git a/components/metrics/file_metrics_provider_unittest.cc b/components/metrics/file_metrics_provider_unittest.cc
index 9781cea..2b6fbdb 100644
--- a/components/metrics/file_metrics_provider_unittest.cc
+++ b/components/metrics/file_metrics_provider_unittest.cc
@@ -4,6 +4,8 @@
#include "components/metrics/file_metrics_provider.h"
+#include <functional>
+
#include "base/files/file_util.h"
#include "base/files/memory_mapped_file.h"
#include "base/files/scoped_temp_dir.h"
@@ -20,6 +22,8 @@
#include "base/threading/thread_task_runner_handle.h"
#include "base/time/time.h"
#include "components/metrics/metrics_pref_names.h"
+#include "components/metrics/persistent_system_profile.h"
+#include "components/metrics/proto/system_profile.pb.h"
#include "components/prefs/pref_registry_simple.h"
#include "components/prefs/testing_pref_service.h"
#include "testing/gtest/include/gtest/gtest.h"
@@ -118,6 +122,13 @@
provider()->MergeHistogramDeltas();
}
+ bool ProvideIndependentMetrics(
+ SystemProfileProto* profile_proto,
+ base::HistogramSnapshotManager* snapshot_manager) {
+ return provider()->ProvideIndependentMetrics(profile_proto,
+ snapshot_manager);
+ }
+
void RecordInitialHistogramSnapshots(
base::HistogramSnapshotManager* snapshot_manager) {
provider()->RecordInitialHistogramSnapshots(snapshot_manager);
@@ -176,7 +187,10 @@
}
std::unique_ptr<base::PersistentHistogramAllocator>
- CreateMetricsFileWithHistograms(int histogram_count) {
+ CreateMetricsFileWithHistograms(
+ int histogram_count,
+ const std::function<void(base::PersistentHistogramAllocator*)>&
+ callback) {
// Get this first so it isn't created inside the persistent allocator.
base::GlobalHistogramAllocator::GetCreateHistogramResultHistogram();
@@ -188,10 +202,18 @@
std::unique_ptr<base::PersistentHistogramAllocator> histogram_allocator =
base::GlobalHistogramAllocator::ReleaseForTesting();
+ callback(histogram_allocator.get());
+
WriteMetricsFile(metrics_file(), histogram_allocator.get());
return histogram_allocator;
}
+ std::unique_ptr<base::PersistentHistogramAllocator>
+ CreateMetricsFileWithHistograms(int histogram_count) {
+ return CreateMetricsFileWithHistograms(
+ histogram_count, [](base::PersistentHistogramAllocator* allocator) {});
+ }
+
base::HistogramBase* GetCreatedHistogram(int index) {
DCHECK_GT(kMaxCreateHistograms, index);
return created_histograms_[index];
@@ -411,7 +433,7 @@
kMetricsName);
// Record embedded snapshots via snapshot-manager.
- HasInitialStabilityMetrics();
+ ASSERT_TRUE(HasInitialStabilityMetrics());
RunTasks();
{
HistogramFlattenerDeltaRecorder flattener;
@@ -435,4 +457,144 @@
EXPECT_TRUE(base::PathExists(metrics_file()));
}
+TEST_P(FileMetricsProviderTest, AccessEmbeddedProfileMetricsWithoutProfile) {
+ ASSERT_FALSE(PathExists(metrics_file()));
+ CreateMetricsFileWithHistograms(2);
+
+ // Register the file and allow the "checker" task to run.
+ ASSERT_TRUE(PathExists(metrics_file()));
+ provider()->RegisterSource(
+ metrics_file(), FileMetricsProvider::SOURCE_HISTOGRAMS_ATOMIC_FILE,
+ FileMetricsProvider::ASSOCIATE_INTERNAL_PROFILE, kMetricsName);
+
+ // Record embedded snapshots via snapshot-manager.
+ OnDidCreateMetricsLog();
+ RunTasks();
+ {
+ HistogramFlattenerDeltaRecorder flattener;
+ base::HistogramSnapshotManager snapshot_manager(&flattener);
+ SystemProfileProto profile;
+
+ // A read of metrics with internal profiles should return nothing.
+ EXPECT_FALSE(ProvideIndependentMetrics(&profile, &snapshot_manager));
+ }
+ EXPECT_TRUE(base::PathExists(metrics_file()));
+ OnDidCreateMetricsLog();
+ RunTasks();
+ EXPECT_FALSE(base::PathExists(metrics_file()));
+}
+
+TEST_P(FileMetricsProviderTest, AccessEmbeddedProfileMetricsWithProfile) {
+ ASSERT_FALSE(PathExists(metrics_file()));
+ CreateMetricsFileWithHistograms(
+ 2, [](base::PersistentHistogramAllocator* allocator) {
+ SystemProfileProto profile_proto;
+ SystemProfileProto::FieldTrial* trial = profile_proto.add_field_trial();
+ trial->set_name_id(123);
+ trial->set_group_id(456);
+
+ PersistentSystemProfile persistent_profile;
+ persistent_profile.RegisterPersistentAllocator(
+ allocator->memory_allocator());
+ persistent_profile.SetSystemProfile(profile_proto);
+ });
+
+ // Register the file and allow the "checker" task to run.
+ ASSERT_TRUE(PathExists(metrics_file()));
+ provider()->RegisterSource(
+ metrics_file(), FileMetricsProvider::SOURCE_HISTOGRAMS_ATOMIC_FILE,
+ FileMetricsProvider::ASSOCIATE_INTERNAL_PROFILE, kMetricsName);
+
+ // Record embedded snapshots via snapshot-manager.
+ OnDidCreateMetricsLog();
+ RunTasks();
+ {
+ HistogramFlattenerDeltaRecorder flattener;
+ base::HistogramSnapshotManager snapshot_manager(&flattener);
+ RecordInitialHistogramSnapshots(&snapshot_manager);
+ EXPECT_EQ(0U, flattener.GetRecordedDeltaHistogramNames().size());
+
+ // A read of metrics with internal profiles should return one result.
+ SystemProfileProto profile;
+ EXPECT_TRUE(ProvideIndependentMetrics(&profile, &snapshot_manager));
+ EXPECT_FALSE(ProvideIndependentMetrics(&profile, &snapshot_manager));
+ }
+ EXPECT_TRUE(base::PathExists(metrics_file()));
+ OnDidCreateMetricsLog();
+ RunTasks();
+ EXPECT_FALSE(base::PathExists(metrics_file()));
+}
+
+TEST_P(FileMetricsProviderTest, AccessEmbeddedFallbackMetricsWithoutProfile) {
+ ASSERT_FALSE(PathExists(metrics_file()));
+ CreateMetricsFileWithHistograms(2);
+
+ // Register the file and allow the "checker" task to run.
+ ASSERT_TRUE(PathExists(metrics_file()));
+ provider()->RegisterSource(
+ metrics_file(), FileMetricsProvider::SOURCE_HISTOGRAMS_ATOMIC_FILE,
+ FileMetricsProvider::ASSOCIATE_INTERNAL_PROFILE_OR_PREVIOUS_RUN,
+ kMetricsName);
+
+ // Record embedded snapshots via snapshot-manager.
+ ASSERT_TRUE(HasInitialStabilityMetrics());
+ RunTasks();
+ {
+ HistogramFlattenerDeltaRecorder flattener;
+ base::HistogramSnapshotManager snapshot_manager(&flattener);
+ RecordInitialHistogramSnapshots(&snapshot_manager);
+ EXPECT_EQ(2U, flattener.GetRecordedDeltaHistogramNames().size());
+
+ // A read of metrics with internal profiles should return nothing.
+ SystemProfileProto profile;
+ EXPECT_FALSE(ProvideIndependentMetrics(&profile, &snapshot_manager));
+ }
+ EXPECT_TRUE(base::PathExists(metrics_file()));
+ OnDidCreateMetricsLog();
+ RunTasks();
+ EXPECT_FALSE(base::PathExists(metrics_file()));
+}
+
+TEST_P(FileMetricsProviderTest, AccessEmbeddedFallbackMetricsWithProfile) {
+ ASSERT_FALSE(PathExists(metrics_file()));
+ CreateMetricsFileWithHistograms(
+ 2, [](base::PersistentHistogramAllocator* allocator) {
+ SystemProfileProto profile_proto;
+ SystemProfileProto::FieldTrial* trial = profile_proto.add_field_trial();
+ trial->set_name_id(123);
+ trial->set_group_id(456);
+
+ PersistentSystemProfile persistent_profile;
+ persistent_profile.RegisterPersistentAllocator(
+ allocator->memory_allocator());
+ persistent_profile.SetSystemProfile(profile_proto);
+ });
+
+ // Register the file and allow the "checker" task to run.
+ ASSERT_TRUE(PathExists(metrics_file()));
+ provider()->RegisterSource(
+ metrics_file(), FileMetricsProvider::SOURCE_HISTOGRAMS_ATOMIC_FILE,
+ FileMetricsProvider::ASSOCIATE_INTERNAL_PROFILE_OR_PREVIOUS_RUN,
+ kMetricsName);
+
+ // Record embedded snapshots via snapshot-manager.
+ EXPECT_FALSE(HasInitialStabilityMetrics());
+ RunTasks();
+ {
+ HistogramFlattenerDeltaRecorder flattener;
+ base::HistogramSnapshotManager snapshot_manager(&flattener);
+ RecordInitialHistogramSnapshots(&snapshot_manager);
+ EXPECT_EQ(0U, flattener.GetRecordedDeltaHistogramNames().size());
+
+ // A read of metrics with internal profiles should return one result.
+ SystemProfileProto profile;
+ EXPECT_TRUE(ProvideIndependentMetrics(&profile, &snapshot_manager));
+ EXPECT_FALSE(ProvideIndependentMetrics(&profile, &snapshot_manager));
+ }
+ EXPECT_TRUE(base::PathExists(metrics_file()));
+ OnDidCreateMetricsLog();
+ RunTasks();
+ EXPECT_FALSE(base::PathExists(metrics_file()));
+}
+
} // namespace metrics
diff --git a/components/metrics/metrics_log.cc b/components/metrics/metrics_log.cc
index 0523372..4d3123c 100644
--- a/components/metrics/metrics_log.cc
+++ b/components/metrics/metrics_log.cc
@@ -11,8 +11,11 @@
#include "base/build_time.h"
#include "base/cpu.h"
+#include "base/metrics/histogram_base.h"
+#include "base/metrics/histogram_flattener.h"
#include "base/metrics/histogram_macros.h"
#include "base/metrics/histogram_samples.h"
+#include "base/metrics/histogram_snapshot_manager.h"
#include "base/metrics/metrics_hashes.h"
#include "base/sys_info.h"
#include "base/time/time.h"
@@ -51,6 +54,28 @@
namespace {
+// A simple class to write histogram data to a log.
+class IndependentFlattener : public base::HistogramFlattener {
+ public:
+ explicit IndependentFlattener(MetricsLog* log) : log_(log) {}
+
+ // base::HistogramFlattener:
+ void RecordDelta(const base::HistogramBase& histogram,
+ const base::HistogramSamples& snapshot) override {
+ log_->RecordHistogramDelta(histogram.histogram_name(), snapshot);
+ }
+ void InconsistencyDetected(
+ base::HistogramBase::Inconsistency problem) override {}
+ void UniqueInconsistencyDetected(
+ base::HistogramBase::Inconsistency problem) override {}
+ void InconsistencyDetectedInLoggedCount(int amount) override {}
+
+ private:
+ MetricsLog* const log_;
+
+ DISALLOW_COPY_AND_ASSIGN(IndependentFlattener);
+};
+
// Any id less than 16 bytes is considered to be a testing id.
bool IsTestingID(const std::string& id) {
return id.size() < 16;
@@ -319,6 +344,15 @@
return serialized_proto;
}
+bool MetricsLog::LoadIndependentMetrics(MetricsProvider* metrics_provider) {
+ SystemProfileProto* system_profile = uma_proto()->mutable_system_profile();
+ IndependentFlattener flattener(this);
+ base::HistogramSnapshotManager snapshot_manager(&flattener);
+
+ return metrics_provider->ProvideIndependentMetrics(system_profile,
+ &snapshot_manager);
+}
+
bool MetricsLog::LoadSavedEnvironmentFromPrefs(std::string* app_version) {
DCHECK(app_version);
app_version->clear();
diff --git a/components/metrics/metrics_log.h b/components/metrics/metrics_log.h
index 441cfbc..688a624 100644
--- a/components/metrics/metrics_log.h
+++ b/components/metrics/metrics_log.h
@@ -44,6 +44,7 @@
enum LogType {
INITIAL_STABILITY_LOG, // The initial log containing stability stats.
ONGOING_LOG, // Subsequent logs in a session.
+ INDEPENDENT_LOG, // An independent log from a previous session.
};
// Creates a new metrics log of the specified type.
@@ -105,6 +106,11 @@
int64_t install_date,
int64_t metrics_reporting_enabled_date);
+ // Loads a saved system profile and the associated metrics into the log.
+ // Returns true on success. Keep calling it with fresh logs until it returns
+ // false.
+ bool LoadIndependentMetrics(MetricsProvider* metrics_provider);
+
// Loads the environment proto that was saved by the last RecordEnvironment()
// call from prefs. On success, returns true and |app_version| contains the
// recovered version. Otherwise (if there was no saved environment in prefs
diff --git a/components/metrics/metrics_log_store.cc b/components/metrics/metrics_log_store.cc
index 8b1ec2a7..30cbe6c 100644
--- a/components/metrics/metrics_log_store.cc
+++ b/components/metrics/metrics_log_store.cc
@@ -73,6 +73,7 @@
initial_log_queue_.StoreLog(log_data);
break;
case MetricsLog::ONGOING_LOG:
+ case MetricsLog::INDEPENDENT_LOG:
ongoing_log_queue_.StoreLog(log_data);
break;
}
diff --git a/components/metrics/metrics_provider.cc b/components/metrics/metrics_provider.cc
index fd864e41..c5ca585 100644
--- a/components/metrics/metrics_provider.cc
+++ b/components/metrics/metrics_provider.cc
@@ -27,6 +27,12 @@
void MetricsProvider::OnAppEnterBackground() {
}
+bool MetricsProvider::ProvideIndependentMetrics(
+ SystemProfileProto* system_profile_proto,
+ base::HistogramSnapshotManager* snapshot_manager) {
+ return false;
+}
+
void MetricsProvider::ProvideSystemProfileMetrics(
SystemProfileProto* system_profile_proto) {
}
diff --git a/components/metrics/metrics_provider.h b/components/metrics/metrics_provider.h
index 54c0a89..a867ef4 100644
--- a/components/metrics/metrics_provider.h
+++ b/components/metrics/metrics_provider.h
@@ -42,6 +42,14 @@
// further notification after this callback.
virtual void OnAppEnterBackground();
+ // Provides a complete and independent system profile + metrics for uploading.
+ // Any histograms added to the |snapshot_manager| will also be included. A
+ // return of false indicates there are none. Will be called repeatedly until
+ // there is nothing else.
+ virtual bool ProvideIndependentMetrics(
+ SystemProfileProto* system_profile_proto,
+ base::HistogramSnapshotManager* snapshot_manager);
+
// Provides additional metrics into the system profile.
virtual void ProvideSystemProfileMetrics(
SystemProfileProto* system_profile_proto);
diff --git a/components/metrics/metrics_service.cc b/components/metrics/metrics_service.cc
index f489d16..6a44e35 100644
--- a/components/metrics/metrics_service.cc
+++ b/components/metrics/metrics_service.cc
@@ -625,6 +625,12 @@
FROM_HERE, base::Bind(&MetricsService::StartInitTask,
self_ptr_factory_.GetWeakPtr()),
base::TimeDelta::FromSeconds(kInitializationDelaySeconds));
+
+ base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+ FROM_HERE,
+ base::Bind(&MetricsService::PrepareProviderMetricsTask,
+ self_ptr_factory_.GetWeakPtr()),
+ base::TimeDelta::FromSeconds(2 * kInitializationDelaySeconds));
}
}
@@ -971,6 +977,37 @@
provider->RecordInitialHistogramSnapshots(&histogram_snapshot_manager_);
}
+bool MetricsService::PrepareProviderMetricsLog() {
+ DCHECK(thread_checker_.CalledOnValidThread());
+
+ // Create a new log. This will have some defaut values injected in it but
+ // those will be overwritten when an embedded profile is extracted.
+ std::unique_ptr<MetricsLog> log = CreateLog(MetricsLog::INDEPENDENT_LOG);
+
+ for (auto& provider : metrics_providers_) {
+ if (log->LoadIndependentMetrics(provider.get())) {
+ log_manager_.PauseCurrentLog();
+ log_manager_.BeginLoggingWithLog(std::move(log));
+ log_manager_.FinishCurrentLog(log_store());
+ log_manager_.ResumePausedLog();
+ return true;
+ }
+ }
+ return false;
+}
+
+void MetricsService::PrepareProviderMetricsTask() {
+ DCHECK(thread_checker_.CalledOnValidThread());
+ bool found = PrepareProviderMetricsLog();
+ base::TimeDelta next_check = found ? base::TimeDelta::FromSeconds(5)
+ : base::TimeDelta::FromMinutes(15);
+ base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+ FROM_HERE,
+ base::Bind(&MetricsService::PrepareProviderMetricsTask,
+ self_ptr_factory_.GetWeakPtr()),
+ next_check);
+}
+
void MetricsService::LogCleanShutdown(bool end_completed) {
DCHECK(thread_checker_.CalledOnValidThread());
// Redundant setting to assure that we always reset this value at shutdown
diff --git a/components/metrics/metrics_service.h b/components/metrics/metrics_service.h
index f95fd6d..102ab72 100644
--- a/components/metrics/metrics_service.h
+++ b/components/metrics/metrics_service.h
@@ -353,6 +353,15 @@
// i.e., histograms with the |kUmaStabilityHistogramFlag| flag set.
void RecordCurrentStabilityHistograms();
+ // Record a single independent profile and assocatied histogram from
+ // metrics providers. If this returns true, one was found and there may
+ // be more.
+ bool PrepareProviderMetricsLog();
+
+ // Records one independent histogram log and then reschedules itself to
+ // check for others. The interval is so as to not adversely impact the UI.
+ void PrepareProviderMetricsTask();
+
// Sub-service for uploading logs.
MetricsReportingService reporting_service_;
diff --git a/components/metrics/metrics_service_unittest.cc b/components/metrics/metrics_service_unittest.cc
index 747e38b..e426789 100644
--- a/components/metrics/metrics_service_unittest.cc
+++ b/components/metrics/metrics_service_unittest.cc
@@ -491,45 +491,46 @@
service.Start();
// Rotation loop should create a log and mark state as idle.
// Upload loop should start upload or be restarted.
+ // The independent-metrics upload job will be started and always be a task.
task_runner_->RunPendingTasks();
// Rotation loop should terminated due to being idle.
// Upload loop should start uploading if it isn't already.
task_runner_->RunPendingTasks();
EXPECT_TRUE(client.uploader()->is_uploading());
- EXPECT_EQ(0U, task_runner_->NumPendingTasks());
+ EXPECT_EQ(1U, task_runner_->NumPendingTasks());
service.OnApplicationNotIdle();
EXPECT_TRUE(client.uploader()->is_uploading());
- EXPECT_EQ(1U, task_runner_->NumPendingTasks());
+ EXPECT_EQ(2U, task_runner_->NumPendingTasks());
// Log generation should be suppressed due to unsent log.
// Idle state should not be reset.
task_runner_->RunPendingTasks();
EXPECT_TRUE(client.uploader()->is_uploading());
- EXPECT_EQ(1U, task_runner_->NumPendingTasks());
+ EXPECT_EQ(2U, task_runner_->NumPendingTasks());
// Make sure idle state was not reset.
task_runner_->RunPendingTasks();
EXPECT_TRUE(client.uploader()->is_uploading());
- EXPECT_EQ(1U, task_runner_->NumPendingTasks());
+ EXPECT_EQ(2U, task_runner_->NumPendingTasks());
// Upload should not be rescheduled, since there are no other logs.
client.uploader()->CompleteUpload(200);
EXPECT_FALSE(client.uploader()->is_uploading());
- EXPECT_EQ(1U, task_runner_->NumPendingTasks());
+ EXPECT_EQ(2U, task_runner_->NumPendingTasks());
// Running should generate a log, restart upload loop, and mark idle.
task_runner_->RunPendingTasks();
EXPECT_FALSE(client.uploader()->is_uploading());
- EXPECT_EQ(2U, task_runner_->NumPendingTasks());
+ EXPECT_EQ(3U, task_runner_->NumPendingTasks());
// Upload should start, and rotation loop should idle out.
task_runner_->RunPendingTasks();
EXPECT_TRUE(client.uploader()->is_uploading());
- EXPECT_EQ(0U, task_runner_->NumPendingTasks());
+ EXPECT_EQ(1U, task_runner_->NumPendingTasks());
// Uploader should reschedule when there is another log available.
service.PushExternalLog("Blah");
client.uploader()->CompleteUpload(200);
EXPECT_FALSE(client.uploader()->is_uploading());
- EXPECT_EQ(1U, task_runner_->NumPendingTasks());
+ EXPECT_EQ(2U, task_runner_->NumPendingTasks());
// Upload should start.
task_runner_->RunPendingTasks();
EXPECT_TRUE(client.uploader()->is_uploading());
- EXPECT_EQ(0U, task_runner_->NumPendingTasks());
+ EXPECT_EQ(1U, task_runner_->NumPendingTasks());
}
TEST_F(MetricsServiceTest, GetSyntheticFieldTrialActiveGroups) {
diff --git a/components/metrics/persistent_system_profile.cc b/components/metrics/persistent_system_profile.cc
index 526b1407..dc4e7eb 100644
--- a/components/metrics/persistent_system_profile.cc
+++ b/components/metrics/persistent_system_profile.cc
@@ -101,6 +101,22 @@
return true;
}
+bool PersistentSystemProfile::RecordAllocator::HasMoreData() const {
+ if (alloc_reference_ == 0 && !NextSegment())
+ return false;
+
+ char* block =
+ allocator_->GetAsArray<char>(alloc_reference_, kTypeIdSystemProfile,
+ base::PersistentMemoryAllocator::kSizeAny);
+ if (!block)
+ return false;
+
+ RecordHeader header;
+ header.as_atomic = base::subtle::Acquire_Load(
+ reinterpret_cast<base::subtle::Atomic32*>(block + end_offset_));
+ return header.as_parts.type != kUnusedSpace;
+}
+
bool PersistentSystemProfile::RecordAllocator::Read(RecordType* type,
std::string* record) const {
*type = kUnusedSpace;
@@ -276,6 +292,21 @@
}
}
+void PersistentSystemProfile::SetSystemProfile(
+ const SystemProfileProto& profile) {
+ std::string serialized_profile;
+ if (!profile.SerializeToString(&serialized_profile))
+ return;
+ SetSystemProfile(serialized_profile);
+}
+
+// static
+bool PersistentSystemProfile::HasSystemProfile(
+ const base::PersistentMemoryAllocator& memory_allocator) {
+ const RecordAllocator records(&memory_allocator);
+ return records.HasMoreData();
+}
+
// static
bool PersistentSystemProfile::GetSystemProfile(
const base::PersistentMemoryAllocator& memory_allocator,
diff --git a/components/metrics/persistent_system_profile.h b/components/metrics/persistent_system_profile.h
index 30d971f6..1775dde 100644
--- a/components/metrics/persistent_system_profile.h
+++ b/components/metrics/persistent_system_profile.h
@@ -30,8 +30,14 @@
void DeregisterPersistentAllocator(
base::PersistentMemoryAllocator* memory_allocator);
- // Stores a complete system profile.
+ // Stores a complete system profile. Use the version taking the serialized
+ // version if available to avoid multiple serialization actions.
void SetSystemProfile(const std::string& serialized_profile);
+ void SetSystemProfile(const SystemProfileProto& profile);
+
+ // Tests if a persistent memory allocator contains an system profile.
+ static bool HasSystemProfile(
+ const base::PersistentMemoryAllocator& memory_allocator);
// Retrieves the system profile from a persistent memory allocator. Returns
// true if a profile was successfully retrieved.
@@ -65,6 +71,7 @@
// Read a record from the allocator. Do not mix this with "write" calls;
// it's one or the other.
+ bool HasMoreData() const;
bool Read(RecordType* type, std::string* record) const;
base::PersistentMemoryAllocator* allocator() { return allocator_; }
diff --git a/components/metrics/persistent_system_profile_unittest.cc b/components/metrics/persistent_system_profile_unittest.cc
index 93f297a..5ca4e898 100644
--- a/components/metrics/persistent_system_profile_unittest.cc
+++ b/components/metrics/persistent_system_profile_unittest.cc
@@ -91,11 +91,10 @@
trial->set_name_id(123);
trial->set_group_id(456);
- std::string serialized_proto;
- ASSERT_TRUE(proto1.SerializeToString(&serialized_proto));
- persistent_profile()->SetSystemProfile(serialized_proto);
+ persistent_profile()->SetSystemProfile(proto1);
SystemProfileProto proto2;
+ ASSERT_TRUE(PersistentSystemProfile::HasSystemProfile(*memory_allocator()));
ASSERT_TRUE(
PersistentSystemProfile::GetSystemProfile(*memory_allocator(), &proto2));
ASSERT_EQ(1, proto2.field_trial_size());
@@ -108,8 +107,7 @@
trial->set_name_id(78);
trial->set_group_id(90);
- ASSERT_TRUE(proto1.SerializeToString(&serialized_proto));
- persistent_profile()->SetSystemProfile(serialized_proto);
+ persistent_profile()->SetSystemProfile(proto1);
ASSERT_TRUE(
PersistentSystemProfile::GetSystemProfile(*memory_allocator(), &proto2));
diff --git a/tools/metrics/histograms/enums.xml b/tools/metrics/histograms/enums.xml
index 9fd1f81..13a7262 100644
--- a/tools/metrics/histograms/enums.xml
+++ b/tools/metrics/histograms/enums.xml
@@ -16312,6 +16312,15 @@
<int value="7" label="File contents internally deleted."/>
</enum>
+<enum name="FileMetricsProviderEmbeddedProfileResult" type="int">
+ <int value="0" label="File was accessed."/>
+ <int value="1" label="File had embedded profile."/>
+ <int value="2"
+ label="File used fallback because no embedded profile was found."/>
+ <int value="3"
+ label="File was dropped because no embedded profile was found."/>
+</enum>
+
<enum name="FileReaderSyncWorkerType" type="int">
<int value="0" label="Other"/>
<int value="1" label="Dedicated Worker"/>
diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml
index 3064958..f3310ae 100644
--- a/tools/metrics/histograms/histograms.xml
+++ b/tools/metrics/histograms/histograms.xml
@@ -78703,6 +78703,15 @@
</summary>
</histogram>
+<histogram name="UMA.FileMetricsProvider.EmbeddedProfileResult"
+ enum="FileMetricsProviderEmbeddedProfileResult">
+ <owner>asvitkine@chromium.org</owner>
+ <owner>bcwhite@chromium.org</owner>
+ <summary>
+ Records attempts to upload metrics from files with embedded system profiles.
+ </summary>
+</histogram>
+
<histogram name="UMA.FileMetricsProvider.InitialAccessResult"
enum="FileMetricsProviderAccessResult">
<owner>asvitkine@chromium.org</owner>
