Commit 42f2e27a authored by Timothy Loh's avatar Timothy Loh Committed by Commit Bot

Move UMA logging logic out of Plugin VM installer UI

This CL moves the logging of setup result histograms from the Plugin VM
installer frontend to the installer backend.

Also tidy up some unneeded plugin_vm:: namespace prefixes.

Bug: 1063748
Change-Id: I39cff19fdca9387ac4ca300c59dea6e9a28cb6d9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2304609Reviewed-by: default avatarJoel Hockey <joelhockey@chromium.org>
Commit-Queue: Timothy Loh <timloh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#789839}
parent a49e3165
......@@ -37,6 +37,8 @@
#include "content/public/browser/network_service_instance.h"
#include "net/traffic_annotation/network_traffic_annotation.h"
namespace plugin_vm {
namespace {
constexpr int64_t kBytesPerGigabyte = 1024 * 1024 * 1024;
......@@ -44,6 +46,7 @@ constexpr int64_t kBytesPerGigabyte = 1024 * 1024 * 1024;
constexpr int64_t kDownloadSizeFallbackEstimate = 15LL * kBytesPerGigabyte;
constexpr char kFailureReasonHistogram[] = "PluginVm.SetupFailureReason";
constexpr char kSetupTimeHistogram[] = "PluginVm.SetupTime";
constexpr char kHomeDirectory[] = "/home";
......@@ -77,9 +80,28 @@ bool DeleteFileWrapper(const base::FilePath& to_delete) {
return base::DeleteFile(to_delete);
}
} // namespace
PluginVmSetupResult BucketForCancelledInstall(
PluginVmInstaller::InstallingState installing_state) {
switch (installing_state) {
case PluginVmInstaller::InstallingState::kInactive:
NOTREACHED();
FALLTHROUGH;
case PluginVmInstaller::InstallingState::kCheckingLicense:
return PluginVmSetupResult::kUserCancelledValidatingLicense;
case PluginVmInstaller::InstallingState::kCheckingDiskSpace:
return PluginVmSetupResult::kUserCancelledCheckingDiskSpace;
case PluginVmInstaller::InstallingState::kDownloadingDlc:
return PluginVmSetupResult::kUserCancelledDownloadingPluginVmDlc;
case PluginVmInstaller::InstallingState::kCheckingForExistingVm:
return PluginVmSetupResult::kUserCancelledCheckingForExistingVm;
case PluginVmInstaller::InstallingState::kDownloadingImage:
return PluginVmSetupResult::kUserCancelledDownloadingPluginVmImage;
case PluginVmInstaller::InstallingState::kImporting:
return PluginVmSetupResult::kUserCancelledImportingPluginVmImage;
}
}
namespace plugin_vm {
} // namespace
PluginVmInstaller::~PluginVmInstaller() = default;
......@@ -112,16 +134,21 @@ void PluginVmInstaller::Start() {
return;
}
setup_start_tick_ = base::TimeTicks::Now();
progress_ = 0;
CheckLicense();
}
void PluginVmInstaller::Cancel() {
if (state_ != State::kInstalling) {
LOG(ERROR) << "Tried to cancel installation from unexpected state "
<< GetStateName(state_);
RecordPluginVmSetupResultHistogram(
PluginVmSetupResult::kUserCancelledWithoutStarting);
return;
}
RecordPluginVmSetupResultHistogram(
BucketForCancelledInstall(installing_state_));
state_ = State::kCancelling;
switch (installing_state_) {
case InstallingState::kCheckingLicense:
......@@ -148,7 +175,7 @@ void PluginVmInstaller::CheckLicense() {
// If the server has provided a license key, responsibility of validating is
// passed to the Plugin VM application.
if (!plugin_vm::GetPluginVmLicenseKey().empty()) {
if (!GetPluginVmLicenseKey().empty()) {
OnLicenseChecked(true);
return;
}
......@@ -224,10 +251,10 @@ void PluginVmInstaller::OnUpdateVmState(bool default_vm_exists) {
}
if (default_vm_exists) {
RecordPluginVmSetupResultHistogram(PluginVmSetupResult::kVmAlreadyExists);
if (observer_)
observer_->OnVmExists();
profile_->GetPrefs()->SetBoolean(plugin_vm::prefs::kPluginVmImageExists,
true);
profile_->GetPrefs()->SetBoolean(prefs::kPluginVmImageExists, true);
InstallFinished();
return;
}
......@@ -672,8 +699,8 @@ void PluginVmInstaller::OnImported(
return;
}
profile_->GetPrefs()->SetBoolean(plugin_vm::prefs::kPluginVmImageExists,
true);
profile_->GetPrefs()->SetBoolean(prefs::kPluginVmImageExists, true);
RecordPluginVmSetupResultHistogram(PluginVmSetupResult::kSuccess);
if (observer_) {
if (creating_new_vm_)
observer_->OnCreated();
......@@ -753,10 +780,9 @@ PluginVmInstaller::PluginVmInstaller(Profile* profile)
DownloadServiceFactory::GetForKey(profile->GetProfileKey())) {}
GURL PluginVmInstaller::GetPluginVmImageDownloadUrl() {
const base::Value* url_ptr =
profile_->GetPrefs()
->GetDictionary(plugin_vm::prefs::kPluginVmImage)
->FindKey("url");
const base::Value* url_ptr = profile_->GetPrefs()
->GetDictionary(prefs::kPluginVmImage)
->FindKey("url");
if (!url_ptr) {
LOG(ERROR) << "Url to PluginVm image is not specified";
return GURL();
......@@ -845,7 +871,7 @@ bool PluginVmInstaller::VerifyDownload(
}
const base::Value* plugin_vm_image_hash_ptr =
profile_->GetPrefs()
->GetDictionary(plugin_vm::prefs::kPluginVmImage)
->GetDictionary(prefs::kPluginVmImage)
->FindKey("hash");
if (!plugin_vm_image_hash_ptr) {
LOG(ERROR) << "Hash of PluginVm image is not specified";
......@@ -903,11 +929,14 @@ void PluginVmInstaller::InstallFailed(FailureReason reason) {
state_ = State::kIdle;
installing_state_ = InstallingState::kInactive;
base::UmaHistogramEnumeration(kFailureReasonHistogram, reason);
RecordPluginVmSetupResultHistogram(PluginVmSetupResult::kError);
if (observer_)
observer_->OnError(reason);
}
void PluginVmInstaller::InstallFinished() {
base::UmaHistogramLongTimes(kSetupTimeHistogram,
base::TimeTicks::Now() - setup_start_tick_);
state_ = State::kIdle;
installing_state_ = InstallingState::kInactive;
}
......
......@@ -193,6 +193,7 @@ class PluginVmInstaller : public KeyedService,
download::DownloadService* download_service_ = nullptr;
State state_ = State::kIdle;
InstallingState installing_state_ = InstallingState::kInactive;
base::TimeTicks setup_start_tick_;
std::string current_download_guid_;
base::FilePath downloaded_image_;
// Used to identify our running import with concierge:
......
......@@ -193,17 +193,15 @@ class PluginVmInstallerTestBase : public testing::Test {
}
void SetPluginVmImagePref(std::string url, std::string hash) {
DictionaryPrefUpdate update(profile_->GetPrefs(),
plugin_vm::prefs::kPluginVmImage);
DictionaryPrefUpdate update(profile_->GetPrefs(), prefs::kPluginVmImage);
base::DictionaryValue* plugin_vm_image = update.Get();
plugin_vm_image->SetKey("url", base::Value(url));
plugin_vm_image->SetKey("hash", base::Value(hash));
}
void SetRequiredFreeDiskSpaceGBPref(int required_free_disk_space) {
profile_->GetPrefs()->SetInteger(
plugin_vm::prefs::kPluginVmRequiredFreeDiskSpaceGB,
required_free_disk_space);
profile_->GetPrefs()->SetInteger(prefs::kPluginVmRequiredFreeDiskSpaceGB,
required_free_disk_space);
}
base::FilePath CreateZipFile() {
......@@ -416,6 +414,8 @@ TEST_F(PluginVmInstallerDownloadServiceTest, InsufficientDisk) {
StartAndRunToCompletion();
histogram_tester_->ExpectUniqueSample(
kFailureReasonHistogram, FailureReason::INSUFFICIENT_DISK_SPACE, 1);
histogram_tester_->ExpectUniqueSample(kPluginVmSetupResultHistogram,
PluginVmSetupResult::kError, 1);
}
TEST_F(PluginVmInstallerDownloadServiceTest, InsufficientDiskWhenSetInPolicy) {
......@@ -441,6 +441,9 @@ TEST_F(PluginVmInstallerDownloadServiceTest, VmExists) {
ExpectObserverEventsUntil(InstallingState::kCheckingForExistingVm);
EXPECT_CALL(*observer_, OnVmExists());
StartAndRunToCompletion();
histogram_tester_->ExpectUniqueSample(
kPluginVmSetupResultHistogram, PluginVmSetupResult::kVmAlreadyExists, 1);
}
TEST_F(PluginVmInstallerDownloadServiceTest, CancelOnVmExistsCheck) {
......@@ -454,6 +457,10 @@ TEST_F(PluginVmInstallerDownloadServiceTest, CancelOnVmExistsCheck) {
run_loop.Run();
installer_->Cancel();
task_environment_.RunUntilIdle();
histogram_tester_->ExpectUniqueSample(
kPluginVmSetupResultHistogram,
PluginVmSetupResult::kUserCancelledCheckingForExistingVm, 1);
}
TEST_F(PluginVmInstallerDownloadServiceTest, DownloadPluginVmImageParamsTest) {
......@@ -517,6 +524,8 @@ TEST_F(PluginVmInstallerDownloadServiceTest,
histogram_tester_->ExpectUniqueSample(kPluginVmImageDownloadedSizeHistogram,
kDownloadedPluginVmImageSizeInMb, 2);
histogram_tester_->ExpectUniqueSample(kPluginVmSetupResultHistogram,
PluginVmSetupResult::kSuccess, 2);
}
TEST_F(PluginVmInstallerDownloadServiceTest,
......@@ -541,6 +550,10 @@ TEST_F(PluginVmInstallerDownloadServiceTest,
histogram_tester_->ExpectUniqueSample(kPluginVmImageDownloadedSizeHistogram,
kDownloadedPluginVmImageSizeInMb, 1);
histogram_tester_->ExpectBucketCount(kPluginVmSetupResultHistogram,
PluginVmSetupResult::kError, 1);
histogram_tester_->ExpectBucketCount(kPluginVmSetupResultHistogram,
PluginVmSetupResult::kSuccess, 1);
}
TEST_F(PluginVmInstallerDownloadServiceTest, CancelledDownloadTest) {
......@@ -555,6 +568,9 @@ TEST_F(PluginVmInstallerDownloadServiceTest, CancelledDownloadTest) {
histogram_tester_->ExpectTotalCount(kPluginVmImageDownloadedSizeHistogram, 0);
histogram_tester_->ExpectTotalCount(kFailureReasonHistogram, 0);
histogram_tester_->ExpectUniqueSample(
kPluginVmSetupResultHistogram,
PluginVmSetupResult::kUserCancelledDownloadingPluginVmImage, 1);
}
TEST_F(PluginVmInstallerDownloadServiceTest, ImportNonExistingImageTest) {
......@@ -581,6 +597,10 @@ TEST_F(PluginVmInstallerDownloadServiceTest, CancelledImportTest) {
EXPECT_CALL(*observer_, OnCancelFinished());
installer_->Cancel();
task_environment_.RunUntilIdle();
histogram_tester_->ExpectUniqueSample(
kPluginVmSetupResultHistogram,
PluginVmSetupResult::kUserCancelledImportingPluginVmImage, 1);
}
TEST_F(PluginVmInstallerDownloadServiceTest, EmptyPluginVmImageUrlTest) {
......
......@@ -11,7 +11,6 @@ const char kPluginVmImageDownloadedSizeHistogram[] =
const char kPluginVmLaunchResultHistogram[] = "PluginVm.LaunchResult";
const char kPluginVmSetupResultHistogram[] = "PluginVm.SetupResult";
const char kPluginVmDlcUseResultHistogram[] = "PluginVm.DlcUseResult";
const char kPluginVmSetupTimeHistogram[] = "PluginVm.SetupTime";
void RecordPluginVmImageDownloadedSizeHistogram(uint64_t bytes_downloaded) {
uint64_t megabytes_downloaded = bytes_downloaded / (1024 * 1024);
......@@ -31,8 +30,4 @@ void RecordPluginVmDlcUseResultHistogram(PluginVmDlcUseResult dlc_use_result) {
base::UmaHistogramEnumeration(kPluginVmDlcUseResultHistogram, dlc_use_result);
}
void RecordPluginVmSetupTimeHistogram(base::TimeDelta setup_time) {
base::UmaHistogramLongTimes(kPluginVmSetupTimeHistogram, setup_time);
}
} // namespace plugin_vm
......@@ -13,10 +13,6 @@ extern const char kPluginVmImageDownloadedSizeHistogram[];
extern const char kPluginVmLaunchResultHistogram[];
extern const char kPluginVmSetupResultHistogram[];
extern const char kPluginVmDlcUseResultHistogram[];
// Histogram for recording successful setup time.
// When error occurs and user hits retry button in setup dialog - time between
// pressing retry button and setup being finished is recorded.
extern const char kPluginVmSetupTimeHistogram[];
// These values are persisted to logs. Entries should not be renumbered and
// numeric values should never be reused.
......
......@@ -15,7 +15,6 @@
#include "chrome/browser/chromeos/plugin_vm/plugin_vm_installer_factory.h"
#include "chrome/browser/chromeos/plugin_vm/plugin_vm_manager.h"
#include "chrome/browser/chromeos/plugin_vm/plugin_vm_manager_factory.h"
#include "chrome/browser/chromeos/plugin_vm/plugin_vm_metrics_util.h"
#include "chrome/browser/chromeos/plugin_vm/plugin_vm_util.h"
#include "chrome/browser/ui/browser_navigator.h"
#include "chrome/browser/ui/browser_navigator_params.h"
......@@ -48,31 +47,6 @@ constexpr gfx::Insets kButtonRowInsets(0, 64, 32, 64);
constexpr int kWindowWidth = 768;
constexpr int kWindowHeight = 636;
plugin_vm::PluginVmSetupResult BucketForCancelledInstall(
plugin_vm::PluginVmInstaller::InstallingState installing_state) {
switch (installing_state) {
case plugin_vm::PluginVmInstaller::InstallingState::kInactive:
NOTREACHED();
FALLTHROUGH;
case plugin_vm::PluginVmInstaller::InstallingState::kCheckingLicense:
return plugin_vm::PluginVmSetupResult::kUserCancelledValidatingLicense;
case plugin_vm::PluginVmInstaller::InstallingState::kCheckingDiskSpace:
return plugin_vm::PluginVmSetupResult::kUserCancelledCheckingDiskSpace;
case plugin_vm::PluginVmInstaller::InstallingState::kDownloadingDlc:
return plugin_vm::PluginVmSetupResult::
kUserCancelledDownloadingPluginVmDlc;
case plugin_vm::PluginVmInstaller::InstallingState::kCheckingForExistingVm:
return plugin_vm::PluginVmSetupResult::
kUserCancelledCheckingForExistingVm;
case plugin_vm::PluginVmInstaller::InstallingState::kDownloadingImage:
return plugin_vm::PluginVmSetupResult::
kUserCancelledDownloadingPluginVmImage;
case plugin_vm::PluginVmInstaller::InstallingState::kImporting:
return plugin_vm::PluginVmSetupResult::
kUserCancelledImportingPluginVmImage;
}
}
} // namespace
void plugin_vm::ShowPluginVmInstallerView(Profile* profile) {
......@@ -225,22 +199,9 @@ bool PluginVmInstallerView::Accept() {
}
bool PluginVmInstallerView::Cancel() {
switch (state_) {
case State::kConfirmInstall:
plugin_vm::RecordPluginVmSetupResultHistogram(
plugin_vm::PluginVmSetupResult::kUserCancelledWithoutStarting);
break;
case State::kInstalling:
plugin_vm::RecordPluginVmSetupResultHistogram(
BucketForCancelledInstall(installing_state_));
plugin_vm_installer_->Cancel();
break;
case State::kCreated:
case State::kImported:
case State::kError:
// Setup result has already been logged in these cases.
break;
}
// We call |Cancel()| if the user hasn't started installation to log to UMA.
if (state_ == State::kConfirmInstall || state_ == State::kInstalling)
plugin_vm_installer_->Cancel();
return true;
}
......@@ -290,11 +251,6 @@ void PluginVmInstallerView::OnVmExists() {
state_ = State::kImported;
installing_state_ = InstallingState::kInactive;
OnStateUpdated();
plugin_vm::RecordPluginVmSetupResultHistogram(
plugin_vm::PluginVmSetupResult::kVmAlreadyExists);
plugin_vm::RecordPluginVmSetupTimeHistogram(base::TimeTicks::Now() -
setup_start_tick_);
}
void PluginVmInstallerView::OnCreated() {
......@@ -304,11 +260,6 @@ void PluginVmInstallerView::OnCreated() {
state_ = State::kCreated;
installing_state_ = InstallingState::kInactive;
OnStateUpdated();
plugin_vm::RecordPluginVmSetupResultHistogram(
plugin_vm::PluginVmSetupResult::kSuccess);
plugin_vm::RecordPluginVmSetupTimeHistogram(base::TimeTicks::Now() -
setup_start_tick_);
}
void PluginVmInstallerView::OnImported() {
......@@ -318,11 +269,6 @@ void PluginVmInstallerView::OnImported() {
state_ = State::kImported;
installing_state_ = InstallingState::kInactive;
OnStateUpdated();
plugin_vm::RecordPluginVmSetupResultHistogram(
plugin_vm::PluginVmSetupResult::kSuccess);
plugin_vm::RecordPluginVmSetupTimeHistogram(base::TimeTicks::Now() -
setup_start_tick_);
}
void PluginVmInstallerView::OnError(
......@@ -333,9 +279,6 @@ void PluginVmInstallerView::OnError(
installing_state_ = InstallingState::kInactive;
reason_ = reason;
OnStateUpdated();
plugin_vm::RecordPluginVmSetupResultHistogram(
plugin_vm::PluginVmSetupResult::kError);
}
// TODO(timloh): Cancelling the installation immediately closes the dialog, but
......@@ -633,9 +576,6 @@ void PluginVmInstallerView::SetBigImage() {
}
void PluginVmInstallerView::StartInstallation() {
// Setup always starts from this function, including retries.
setup_start_tick_ = base::TimeTicks::Now();
state_ = State::kInstalling;
installing_state_ = InstallingState::kCheckingLicense;
progress_bar_->SetValue(0);
......
......@@ -93,7 +93,6 @@ class PluginVmInstallerView : public views::BubbleDialogDelegateView,
views::BoxLayout* lower_container_layout_ = nullptr;
views::ImageView* big_image_ = nullptr;
views::Link* learn_more_link_ = nullptr;
base::TimeTicks setup_start_tick_;
State state_ = State::kConfirmInstall;
InstallingState installing_state_ = InstallingState::kInactive;
......
......@@ -6,11 +6,9 @@
#include "base/bind.h"
#include "base/files/file_util.h"
#include "base/test/metrics/histogram_tester.h"
#include "base/test/scoped_feature_list.h"
#include "base/threading/thread_restrictions.h"
#include "chrome/browser/chromeos/login/users/fake_chrome_user_manager.h"
#include "chrome/browser/chromeos/plugin_vm/plugin_vm_metrics_util.h"
#include "chrome/browser/chromeos/plugin_vm/plugin_vm_pref_names.h"
#include "chrome/browser/chromeos/plugin_vm/plugin_vm_test_helper.h"
#include "chrome/browser/chromeos/plugin_vm/plugin_vm_util.h"
......@@ -67,8 +65,6 @@ class PluginVmInstallerViewBrowserTest : public DialogBrowserTest {
chromeos::DBusThreadManager::Get()->GetConciergeClient());
fake_concierge_client_->set_disk_image_progress_signal_connected(true);
histogram_tester_ = std::make_unique<base::HistogramTester>();
network_connection_tracker_ =
network::TestNetworkConnectionTracker::CreateInstance();
content::SetNetworkConnectionTrackerForTesting(nullptr);
......@@ -146,7 +142,6 @@ class PluginVmInstallerViewBrowserTest : public DialogBrowserTest {
network_connection_tracker_;
std::unique_ptr<user_manager::ScopedUserManager> scoped_user_manager_;
PluginVmInstallerView* view_;
std::unique_ptr<base::HistogramTester> histogram_tester_;
chromeos::FakeConciergeClient* fake_concierge_client_;
private:
......@@ -214,10 +209,6 @@ IN_PROC_BROWSER_TEST_F(PluginVmInstallerViewBrowserTestWithFeatureEnabled,
WaitForSetupToFinish();
CheckSetupIsFinishedSuccessfully();
histogram_tester_->ExpectUniqueSample(
plugin_vm::kPluginVmSetupResultHistogram,
plugin_vm::PluginVmSetupResult::kSuccess, 1);
}
IN_PROC_BROWSER_TEST_F(PluginVmInstallerViewBrowserTestWithFeatureEnabled,
......@@ -234,10 +225,6 @@ IN_PROC_BROWSER_TEST_F(PluginVmInstallerViewBrowserTestWithFeatureEnabled,
WaitForSetupToFinish();
CheckSetupFailed();
histogram_tester_->ExpectUniqueSample(
plugin_vm::kPluginVmSetupResultHistogram,
plugin_vm::PluginVmSetupResult::kError, 1);
}
IN_PROC_BROWSER_TEST_F(PluginVmInstallerViewBrowserTestWithFeatureEnabled,
......@@ -253,10 +240,6 @@ IN_PROC_BROWSER_TEST_F(PluginVmInstallerViewBrowserTestWithFeatureEnabled,
WaitForSetupToFinish();
CheckSetupFailed();
histogram_tester_->ExpectUniqueSample(
plugin_vm::kPluginVmSetupResultHistogram,
plugin_vm::PluginVmSetupResult::kError, 1);
}
IN_PROC_BROWSER_TEST_F(PluginVmInstallerViewBrowserTestWithFeatureEnabled,
......@@ -284,13 +267,6 @@ IN_PROC_BROWSER_TEST_F(PluginVmInstallerViewBrowserTestWithFeatureEnabled,
WaitForSetupToFinish();
CheckSetupIsFinishedSuccessfully();
histogram_tester_->ExpectBucketCount(plugin_vm::kPluginVmSetupResultHistogram,
plugin_vm::PluginVmSetupResult::kError,
1);
histogram_tester_->ExpectBucketCount(plugin_vm::kPluginVmSetupResultHistogram,
plugin_vm::PluginVmSetupResult::kSuccess,
1);
}
IN_PROC_BROWSER_TEST_F(
......@@ -315,8 +291,4 @@ IN_PROC_BROWSER_TEST_F(
static_cast<std::underlying_type_t<
plugin_vm::PluginVmInstaller::FailureReason>>(
plugin_vm::PluginVmInstaller::FailureReason::NOT_ALLOWED))));
histogram_tester_->ExpectUniqueSample(
plugin_vm::kPluginVmSetupResultHistogram,
plugin_vm::PluginVmSetupResult::kError, 1);
}
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