Commit 6502eb0f authored by Alexei Svitkine's avatar Alexei Svitkine Committed by Commit Bot

Revert "Add full system profiles in .pma file as part of metrics init."

This reverts commit 87dac882.

Reason for revert: Breaks UMA recording.

Original change's description:
> Add full system profiles in .pma file as part of metrics init.
> 
> Prior to this change, only an incomplete system profile, notably
> missing field trials and uma_default_state fields, was stored
> at this point. So unclean shut downs after this point in time,
> but before the initial metrics log was saved would not include
> that info. This change fixes that by running the code that
> populates all the system profile fields.
> 
> Since we're now calling ProvideSystemProfileMetrics() twice, the
> logic in FieldTrialsProvider was no longer correct in tracking
> the log creation time. Instead, this CL updates the API to have
> that be passed in.
> 
> Additionally, Chrome OS an Screen Info providers needed updates since
> their dependencies are not yet available this early, so they are not
> able to provide all the information this early.
> 
> Finally, webview unit tests needed updating to init mojo as they are
> now exercising the provider code which has a Mojo dependency.
> 
> Bug: 992538
> TBR: sky@chromium.org
> Change-Id: Iae54398065470e390b70507d5b3ac09d5a983c9f
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1746909
> Commit-Queue: Alexei Svitkine <asvitkine@chromium.org>
> Reviewed-by: Richard Coles <torne@chromium.org>
> Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
> Reviewed-by: Brian White <bcwhite@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#687000}

TBR=sky@chromium.org,rkaplow@chromium.org,asvitkine@chromium.org,torne@chromium.org,bcwhite@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 992538
Change-Id: I4a25324788960d2e61325c2a41812c5da18087f8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1760869Reviewed-by: default avatarAlexei Svitkine <asvitkine@chromium.org>
Commit-Queue: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#688146}
parent 1ff8cf38
......@@ -9,7 +9,6 @@ include_rules = [
"+gin/public",
"+gin/v8_initializer.h",
"+media/base/media_switches.h", # For media command line switches.
"+mojo/core/embedder/embedder.h",
"+ui/events/gesture_detection",
"+ui/gl",
]
......@@ -6,7 +6,6 @@
#include "base/command_line.h"
#include "base/test/test_suite.h"
#include "content/public/common/content_switches.h"
#include "mojo/core/embedder/embedder.h"
#include "ui/gl/gl_surface.h"
#include "ui/gl/test/gl_surface_test_support.h"
......@@ -15,7 +14,5 @@ int main(int argc, char** argv) {
switches::kSingleProcess);
gl::GLSurfaceTestSupport::InitializeNoExtensionsOneOff();
android_webview::GpuServiceWebView::GetInstance();
base::TestSuite test_suite(argc, argv);
mojo::core::Init();
return test_suite.Run();
return base::TestSuite(argc, argv).Run();
}
......@@ -353,7 +353,6 @@ test("android_webview_unittests") {
"//components/prefs:test_support",
"//content:content",
"//content/test:test_support",
"//mojo/core/embedder",
"//net:net",
"//net:test_support",
"//ui/base:ui_base_jni_headers",
......
......@@ -309,11 +309,6 @@ void ChromeOSMetricsProvider::ProvideCurrentSessionData(
void ChromeOSMetricsProvider::WriteBluetoothProto(
metrics::SystemProfileProto* system_profile_proto) {
// This may be called before the async init task to set |adapter_| is set,
// such as when the persistent system profile gets filled in initially.
if (!adapter_)
return;
metrics::SystemProfileProto::Hardware* hardware =
system_profile_proto->mutable_hardware();
......
......@@ -78,8 +78,7 @@ void StartupData::RecordCoreSystemProfile() {
// |field_trial_provider|.
variations::FieldTrialsProvider field_trial_provider(nullptr,
base::StringPiece());
field_trial_provider.ProvideSystemProfileMetricsWithLogCreationTime(
base::TimeTicks(), &system_profile);
field_trial_provider.ProvideSystemProfileMetrics(&system_profile);
// TODO(crbug.com/965482): Records information from other providers.
metrics::GlobalPersistentSystemProfile::GetInstance()->SetSystemProfile(
......
......@@ -63,17 +63,8 @@ bool DelegatingProvider::HasIndependentMetrics() {
void DelegatingProvider::ProvideSystemProfileMetrics(
SystemProfileProto* system_profile_proto) {
// ProvideSystemProfileMetricsWithLogCreationTime() should be called instead.
NOTREACHED();
}
void DelegatingProvider::ProvideSystemProfileMetricsWithLogCreationTime(
base::TimeTicks log_creation_time,
SystemProfileProto* system_profile_proto) {
for (auto& provider : metrics_providers_) {
provider->ProvideSystemProfileMetricsWithLogCreationTime(
log_creation_time, system_profile_proto);
}
for (auto& provider : metrics_providers_)
provider->ProvideSystemProfileMetrics(system_profile_proto);
}
bool DelegatingProvider::HasPreviousSessionData() {
......
......@@ -36,9 +36,6 @@ class DelegatingProvider final : public MetricsProvider {
bool HasIndependentMetrics() override;
void ProvideSystemProfileMetrics(
SystemProfileProto* system_profile_proto) override;
void ProvideSystemProfileMetricsWithLogCreationTime(
base::TimeTicks log_creation_time,
SystemProfileProto* system_profile_proto) override;
bool HasPreviousSessionData() override;
void ProvidePreviousSessionData(
ChromeUserMetricsExtension* uma_proto) override;
......
......@@ -36,14 +36,13 @@ void FieldTrialsProvider::GetFieldTrialIds(
variations::GetFieldTrialActiveGroupIds(suffix_, field_trial_ids);
}
void FieldTrialsProvider::ProvideSystemProfileMetrics(
metrics::SystemProfileProto* system_profile_proto) {
// ProvideSystemProfileMetricsWithLogCreationTime() should be called instead.
NOTREACHED();
void FieldTrialsProvider::OnDidCreateMetricsLog() {
if (registry_) {
creation_times_.push_back(base::TimeTicks::Now());
}
}
void FieldTrialsProvider::ProvideSystemProfileMetricsWithLogCreationTime(
base::TimeTicks log_creation_time,
void FieldTrialsProvider::ProvideSystemProfileMetrics(
metrics::SystemProfileProto* system_profile_proto) {
std::vector<ActiveGroupId> field_trial_ids;
const std::string& version = variations::GetSeedVersion();
......@@ -53,8 +52,14 @@ void FieldTrialsProvider::ProvideSystemProfileMetricsWithLogCreationTime(
WriteFieldTrials(field_trial_ids, system_profile_proto);
if (registry_) {
base::TimeTicks creation_time;
// Should always be true, but don't crash even if there is a bug.
if (!creation_times_.empty()) {
creation_time = creation_times_.back();
creation_times_.pop_back();
}
std::vector<ActiveGroupId> synthetic_trials;
registry_->GetSyntheticFieldTrialsOlderThan(log_creation_time,
registry_->GetSyntheticFieldTrialsOlderThan(creation_time,
&synthetic_trials);
WriteFieldTrials(synthetic_trials, system_profile_proto);
}
......
......@@ -5,6 +5,8 @@
#ifndef COMPONENTS_METRICS_FIELD_TRIALS_PROVIDER_H_
#define COMPONENTS_METRICS_FIELD_TRIALS_PROVIDER_H_
#include <vector>
#include "base/strings/string_piece.h"
#include "base/time/time.h"
#include "components/metrics/metrics_provider.h"
......@@ -25,11 +27,9 @@ class FieldTrialsProvider : public metrics::MetricsProvider {
~FieldTrialsProvider() override;
// metrics::MetricsProvider:
void OnDidCreateMetricsLog() override;
void ProvideSystemProfileMetrics(
metrics::SystemProfileProto* system_profile_proto) override;
void ProvideSystemProfileMetricsWithLogCreationTime(
base::TimeTicks log_creation_time,
metrics::SystemProfileProto* system_profile_proto) override;
private:
// Overrideable for testing.
......@@ -41,7 +41,13 @@ class FieldTrialsProvider : public metrics::MetricsProvider {
// Suffix used for the field trial names before they are hashed for uploads.
std::string suffix_;
DISALLOW_COPY_AND_ASSIGN(FieldTrialsProvider);
// A stack of log creation times.
// While the initial metrics log exists, there will be two logs open.
// Use a stack so that we use the right creation time for the first ongoing
// log.
// TODO(crbug/746098): Simplify InitialMetricsLog logic so this is not
// necessary.
std::vector<base::TimeTicks> creation_times_;
};
} // namespace variations
......
......@@ -96,17 +96,14 @@ TEST_F(FieldTrialsProviderTest, ProvideSyntheticTrials) {
// Make sure these trials are older than the log.
WaitUntilTimeChanges(base::TimeTicks::Now());
// Get the current time and wait for it to change.
base::TimeTicks log_creation_time = base::TimeTicks::Now();
provider.OnDidCreateMetricsLog();
// Make sure that the log is older than the trials that should be excluded.
WaitUntilTimeChanges(log_creation_time);
WaitUntilTimeChanges(base::TimeTicks::Now());
RegisterExtraSyntheticTrial();
metrics::SystemProfileProto proto;
provider.ProvideSystemProfileMetricsWithLogCreationTime(log_creation_time,
&proto);
provider.ProvideSystemProfileMetrics(&proto);
ASSERT_EQ(base::size(kFieldTrialIds) + base::size(kSyntheticTrials),
static_cast<size_t>(proto.field_trial_size()));
......@@ -118,8 +115,7 @@ TEST_F(FieldTrialsProviderTest, NoSyntheticTrials) {
TestProvider provider(nullptr, base::StringPiece());
metrics::SystemProfileProto proto;
provider.ProvideSystemProfileMetricsWithLogCreationTime(base::TimeTicks(),
&proto);
provider.ProvideSystemProfileMetrics(&proto);
ASSERT_EQ(base::size(kFieldTrialIds),
static_cast<size_t>(proto.field_trial_size()));
......
......@@ -297,10 +297,7 @@ void MetricsLog::WriteRealtimeStabilityAttributes(
const SystemProfileProto& MetricsLog::RecordEnvironment(
DelegatingProvider* delegating_provider) {
// Clear any previous system profile. This will be present in the case when
// the initial system profile got recorded on start up. Clearing it ensures
// there is no duplication of repeated fields.
uma_proto()->clear_system_profile();
DCHECK(!has_environment_);
has_environment_ = true;
SystemProfileProto* system_profile = uma_proto()->mutable_system_profile();
......@@ -312,8 +309,7 @@ const SystemProfileProto& MetricsLog::RecordEnvironment(
if (client_->GetBrand(&brand_code))
system_profile->set_brand_code(brand_code);
delegating_provider->ProvideSystemProfileMetricsWithLogCreationTime(
creation_time_, system_profile);
delegating_provider->ProvideSystemProfileMetrics(system_profile);
return *system_profile;
}
......
......@@ -48,12 +48,7 @@ void MetricsProvider::ProvideIndependentMetrics(
}
void MetricsProvider::ProvideSystemProfileMetrics(
SystemProfileProto* system_profile_proto) {}
void MetricsProvider::ProvideSystemProfileMetricsWithLogCreationTime(
base::TimeTicks log_creation_time,
SystemProfileProto* system_profile_proto) {
ProvideSystemProfileMetrics(system_profile_proto);
}
bool MetricsProvider::HasPreviousSessionData() {
......
......@@ -7,7 +7,6 @@
#include "base/callback.h"
#include "base/macros.h"
#include "base/time/time.h"
namespace base {
class HistogramSnapshotManager;
......@@ -65,19 +64,10 @@ class MetricsProvider {
ChromeUserMetricsExtension* uma_proto,
base::HistogramSnapshotManager* snapshot_manager);
// Provides additional metrics into the system profile. This is a convenience
// method over ProvideSystemProfileMetricsWithLogCreationTime() without the
// |log_creation_time| param. Should not be called directly by services.
// Provides additional metrics into the system profile.
virtual void ProvideSystemProfileMetrics(
SystemProfileProto* system_profile_proto);
// Provides additional metrics into the system profile. The log creation
// time param provides a timestamp of when the log was opened, which is needed
// for some metrics providers.
virtual void ProvideSystemProfileMetricsWithLogCreationTime(
base::TimeTicks log_creation_time,
SystemProfileProto* system_profile_proto);
// Called once at startup to see whether this provider has critical data
// to provide about the previous session.
// Returning true will trigger ProvidePreviousSessionData on all other
......
......@@ -316,17 +316,16 @@ void MetricsService::EnableRecording() {
state_manager_->ForceClientIdCreation();
client_->SetMetricsClientId(state_manager_->client_id());
SystemProfileProto system_profile;
MetricsLog::RecordCoreSystemProfile(client_, &system_profile);
GlobalPersistentSystemProfile::GetInstance()->SetSystemProfile(
system_profile, /*complete=*/false);
if (!log_manager_.current_log())
OpenNewLog();
delegating_provider_.OnRecordingEnabled();
// Fill in the system profile in the log and persist it (to prefs, .pma and
// crashpad). This includes running the providers so that information like
// field trials and hardware info is provided. If Chrome crashes before this
// log is completed, the .pma file will have this system profile.
RecordCurrentEnvironment(log_manager_.current_log(), /*complete=*/false);
base::RemoveActionCallback(action_callback_);
action_callback_ = base::Bind(&MetricsService::OnUserAction,
base::Unretained(this));
......@@ -621,7 +620,7 @@ void MetricsService::CloseCurrentLog() {
// MetricsLog class.
MetricsLog* current_log = log_manager_.current_log();
DCHECK(current_log);
RecordCurrentEnvironment(current_log, /*complete=*/true);
RecordCurrentEnvironment(current_log);
base::TimeDelta incremental_uptime;
base::TimeDelta uptime;
GetUptimes(local_state_, &incremental_uptime, &uptime);
......@@ -753,7 +752,7 @@ bool MetricsService::PrepareInitialStabilityLog(
void MetricsService::PrepareInitialMetricsLog() {
DCHECK_EQ(INIT_TASK_DONE, state_);
RecordCurrentEnvironment(initial_metrics_log_.get(), /*complete=*/true);
RecordCurrentEnvironment(initial_metrics_log_.get());
base::TimeDelta incremental_uptime;
base::TimeDelta uptime;
GetUptimes(local_state_, &incremental_uptime, &uptime);
......@@ -807,14 +806,6 @@ std::unique_ptr<MetricsLog> MetricsService::CreateLog(
log_type, client_);
}
void MetricsService::SetPersistentSystemProfile(
const std::string& serialized_proto,
bool complete) {
GlobalPersistentSystemProfile::GetInstance()->SetSystemProfile(
serialized_proto, complete);
}
// static
std::string MetricsService::RecordCurrentEnvironmentHelper(
MetricsLog* log,
PrefService* local_state,
......@@ -825,12 +816,12 @@ std::string MetricsService::RecordCurrentEnvironmentHelper(
return recorder.SerializeAndRecordEnvironmentToPrefs(system_profile);
}
void MetricsService::RecordCurrentEnvironment(MetricsLog* log, bool complete) {
void MetricsService::RecordCurrentEnvironment(MetricsLog* log) {
DCHECK(client_);
std::string serialized_proto =
RecordCurrentEnvironmentHelper(log, local_state_, &delegating_provider_);
SetPersistentSystemProfile(serialized_proto, complete);
GlobalPersistentSystemProfile::GetInstance()->SetSystemProfile(
serialized_proto, /*complete=*/true);
client_->OnEnvironmentUpdate(&serialized_proto);
}
......
......@@ -180,10 +180,6 @@ class MetricsService : public base::HistogramFlattener {
return reporting_service_.metrics_log_store();
}
// Sets the persistent system profile. Virtual for tests.
virtual void SetPersistentSystemProfile(const std::string& serialized_proto,
bool complete);
// Records the current environment (system profile) in |log|, and persists
// the results in prefs.
// Exposed for testing.
......@@ -295,7 +291,8 @@ class MetricsService : public base::HistogramFlattener {
// Records the current environment (system profile) in |log|, and persists
// the results in prefs and GlobalPersistentSystemProfile.
void RecordCurrentEnvironment(MetricsLog* log, bool complete);
// Exposed for testing.
void RecordCurrentEnvironment(MetricsLog* log);
// Record complete list of histograms into the current log.
// Called when we close a log.
......
......@@ -62,24 +62,7 @@ class TestMetricsService : public MetricsService {
using MetricsService::log_store;
using MetricsService::RecordCurrentEnvironmentHelper;
// MetricsService:
void SetPersistentSystemProfile(const std::string& serialized_proto,
bool complete) override {
persistent_system_profile_provided_ = true;
persistent_system_profile_complete_ = complete;
}
bool persistent_system_profile_provided() const {
return persistent_system_profile_provided_;
}
bool persistent_system_profile_complete() const {
return persistent_system_profile_complete_;
}
private:
bool persistent_system_profile_provided_ = false;
bool persistent_system_profile_complete_ = false;
DISALLOW_COPY_AND_ASSIGN(TestMetricsService);
};
......@@ -362,30 +345,6 @@ TEST_F(MetricsServiceTest, MetricsProvidersInitialized) {
EXPECT_TRUE(test_provider->init_called());
}
TEST_F(MetricsServiceTest, SystemProfileDataProvidedOnEnableRecording) {
EnableMetricsReporting();
TestMetricsServiceClient client;
TestMetricsService service(GetMetricsStateManager(), &client,
GetLocalState());
TestMetricsProvider* test_provider = new TestMetricsProvider();
service.RegisterMetricsProvider(
std::unique_ptr<MetricsProvider>(test_provider));
service.InitializeMetricsRecordingState();
// ProvideSystemProfileMetrics() shouldn't be called initially.
EXPECT_FALSE(test_provider->provide_system_profile_metrics_called());
EXPECT_FALSE(service.persistent_system_profile_provided());
service.Start();
// Start should call ProvideSystemProfileMetrics().
EXPECT_TRUE(test_provider->provide_system_profile_metrics_called());
EXPECT_TRUE(service.persistent_system_profile_provided());
EXPECT_FALSE(service.persistent_system_profile_complete());
}
TEST_F(MetricsServiceTest, SplitRotation) {
EnableMetricsReporting();
TestMetricsServiceClient client;
......
......@@ -66,17 +66,12 @@ ScreenInfoMetricsProvider::~ScreenInfoMetricsProvider() {
void ScreenInfoMetricsProvider::ProvideSystemProfileMetrics(
SystemProfileProto* system_profile_proto) {
// This may be called before the screen info has been initialized, such as
// when the persistent system profile gets filled in initially.
const base::Optional<gfx::Size> display_size = GetScreenSize();
if (!display_size.has_value())
return;
SystemProfileProto::Hardware* hardware =
system_profile_proto->mutable_hardware();
hardware->set_primary_screen_width(display_size->width());
hardware->set_primary_screen_height(display_size->height());
const gfx::Size display_size = GetScreenSize();
hardware->set_primary_screen_width(display_size.width());
hardware->set_primary_screen_height(display_size.height());
hardware->set_primary_screen_scale_factor(GetScreenDeviceScaleFactor());
hardware->set_screen_count(GetScreenCount());
......@@ -85,11 +80,8 @@ void ScreenInfoMetricsProvider::ProvideSystemProfileMetrics(
#endif
}
base::Optional<gfx::Size> ScreenInfoMetricsProvider::GetScreenSize() const {
auto* screen = display::Screen::GetScreen();
if (!screen)
return base::nullopt;
return base::make_optional(screen->GetPrimaryDisplay().GetSizeInPixel());
gfx::Size ScreenInfoMetricsProvider::GetScreenSize() const {
return display::Screen::GetScreen()->GetPrimaryDisplay().GetSizeInPixel();
}
float ScreenInfoMetricsProvider::GetScreenDeviceScaleFactor() const {
......
......@@ -6,7 +6,6 @@
#define COMPONENTS_METRICS_UI_SCREEN_INFO_METRICS_PROVIDER_H_
#include "base/macros.h"
#include "base/optional.h"
#include "components/metrics/metrics_provider.h"
#include "ui/gfx/geometry/size.h"
......@@ -25,8 +24,8 @@ class ScreenInfoMetricsProvider : public MetricsProvider {
protected:
// Exposed for the sake of mocking in test code.
// Returns the screen size for the primary monitor if available.
virtual base::Optional<gfx::Size> GetScreenSize() const;
// Returns the screen size for the primary monitor.
virtual gfx::Size GetScreenSize() const;
// Returns the device scale factor for the primary monitor.
virtual float GetScreenDeviceScaleFactor() const;
......
......@@ -24,8 +24,8 @@ class TestScreenInfoMetricsProvider : public ScreenInfoMetricsProvider {
~TestScreenInfoMetricsProvider() override {}
private:
base::Optional<gfx::Size> GetScreenSize() const override {
return base::make_optional(gfx::Size(kScreenWidth, kScreenHeight));
gfx::Size GetScreenSize() const override {
return gfx::Size(kScreenWidth, kScreenHeight);
}
float GetScreenDeviceScaleFactor() const override {
......
......@@ -78,8 +78,13 @@ UkmService::UkmService(PrefService* pref_service,
bool restrict_to_whitelist_entries)
: pref_service_(pref_service),
restrict_to_whitelist_entries_(restrict_to_whitelist_entries),
client_id_(0),
session_id_(0),
report_count_(0),
client_(client),
reporting_service_(client, pref_service) {
reporting_service_(client, pref_service),
initialize_started_(false),
initialize_complete_(false) {
DCHECK(pref_service_);
DCHECK(client_);
DVLOG(1) << "UkmService::Constructor";
......@@ -126,7 +131,6 @@ void UkmService::EnableReporting() {
if (reporting_service_.reporting_active())
return;
log_creation_time_ = base::TimeTicks::Now();
metrics_providers_.OnRecordingEnabled();
if (!initialize_started_)
......@@ -256,8 +260,8 @@ void UkmService::BuildAndStoreLog() {
metrics::MetricsLog::RecordCoreSystemProfile(client_,
report.mutable_system_profile());
metrics_providers_.ProvideSystemProfileMetricsWithLogCreationTime(
log_creation_time_, report.mutable_system_profile());
metrics_providers_.ProvideSystemProfileMetrics(
report.mutable_system_profile());
std::string serialized_log;
report.SerializeToString(&serialized_log);
......
......@@ -128,13 +128,13 @@ class UkmService : public UkmRecorderImpl {
bool restrict_to_whitelist_entries_;
// The UKM client id stored in prefs.
uint64_t client_id_ = 0;
uint64_t client_id_;
// The UKM session id stored in prefs.
int32_t session_id_ = 0;
int32_t session_id_;
// The number of reports generated this session.
int32_t report_count_ = 0;
int32_t report_count_;
// Used to interact with the embedder. Weak pointer; must outlive |this|
// instance.
......@@ -149,13 +149,11 @@ class UkmService : public UkmRecorderImpl {
// The scheduler for determining when uploads should happen.
std::unique_ptr<metrics::MetricsRotationScheduler> scheduler_;
base::TimeTicks log_creation_time_;
bool initialize_started_ = false;
bool initialize_complete_ = false;
SEQUENCE_CHECKER(sequence_checker_);
bool initialize_started_;
bool initialize_complete_;
// Weak pointers factory used to post task on different threads. All weak
// pointers managed by this factory have the same lifetime as UkmService.
base::WeakPtrFactory<UkmService> self_ptr_factory_{this};
......
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