Reland "Deprecated shutdown metrics V1"
This is a reland of commit 6a5b8a2bf336fa05dd1fdba5086f6f011ff44e86
OBSOLETE_HISTOGRAMS="Shutdown Metrics V1 are replaced by Shutdown Metrics V2"
Original change's description:
> Deprecated shutdown metrics V1
>
> The V1 set of metrics were using the local state prefs to keep
> the shutdown metrics until the next startup where they will be
> reported.
>
> A recent change in Chrome fixed the persistent memory for metrics.
> This means that UMA metrics macros are now working during shutdown.
>
> The V2 metrics are landed for months and are stable. We can deprecate
> the V1 set of metrics.
>
> Deprecate UMA metrics:
> * Shutdown.NotValid.Time
> * Shutdown.SilentExit.Time
> * Shutdown.WindowClose.Time
> * Shutdown.BrowserExit.Time
> * Shutdown.EndSession.Time
> * Shutdown.OtherExit.Time
>
> Deprecate the shutdown time file:
> * chrome_shutdown_ms.txt
>
> Deprecate local state prefs:
> * shutdown.num_processes
> * shutdown.num_processes_slow"
>
> OBSOLETE_HISTOGRAMS="Shutdown Metrics V1 are replaced by Shutdown Metrics V2"
>
> Bug: 1456205, 1468093
> Change-Id: I1a48b5b3e0b5791ef53152448c8cd096471d6308
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4734233
> Reviewed-by: Scott Violet <[email protected]>
> Reviewed-by: Luc Nguyen <[email protected]>
> Commit-Queue: Etienne Bergeron <[email protected]>
> Cr-Commit-Position: refs/heads/main@{#1178085}
Bug: 1456205, 1468093
Change-Id: I9cf3ced4d3620c6719e269c193750e2393aae1f9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4742700
Reviewed-by: Scott Violet <[email protected]>
Commit-Queue: Etienne Bergeron <[email protected]>
Reviewed-by: Luc Nguyen <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1178528}
diff --git a/chrome/browser/lifetime/browser_shutdown.cc b/chrome/browser/lifetime/browser_shutdown.cc
index 4682b3c9..6ceb274 100644
--- a/chrome/browser/lifetime/browser_shutdown.cc
+++ b/chrome/browser/lifetime/browser_shutdown.cc
@@ -12,12 +12,8 @@
#include "base/clang_profiling_buildflags.h"
#include "base/command_line.h"
-#include "base/files/file_path.h"
-#include "base/files/file_util.h"
#include "base/functional/bind.h"
#include "base/metrics/histogram_functions.h"
-#include "base/path_service.h"
-#include "base/strings/string_number_conversions.h"
#include "base/task/thread_pool.h"
#include "base/threading/scoped_blocking_call.h"
#include "base/threading/thread.h"
@@ -84,14 +80,6 @@
int g_shutdown_num_processes;
int g_shutdown_num_processes_slow;
-constexpr char kShutdownMsFile[] = "chrome_shutdown_ms.txt";
-
-base::FilePath GetShutdownMsPath() {
- base::FilePath shutdown_ms_file;
- base::PathService::Get(chrome::DIR_USER_DATA, &shutdown_ms_file);
- return shutdown_ms_file.AppendASCII(kShutdownMsFile);
-}
-
const char* ToShutdownTypeString(ShutdownType type) {
switch (type) {
case ShutdownType::kNotValid:
@@ -125,8 +113,6 @@
void RegisterPrefs(PrefRegistrySimple* registry) {
registry->RegisterIntegerPref(prefs::kShutdownType,
static_cast<int>(ShutdownType::kNotValid));
- registry->RegisterIntegerPref(prefs::kShutdownNumProcesses, 0);
- registry->RegisterIntegerPref(prefs::kShutdownNumProcessesSlow, 0);
registry->RegisterBooleanPref(prefs::kRestartLastSessionOnShutdown, false);
}
@@ -267,9 +253,6 @@
// Record the shutdown info so that we can put it into a histogram at next
// startup.
prefs->SetInteger(prefs::kShutdownType, static_cast<int>(g_shutdown_type));
- prefs->SetInteger(prefs::kShutdownNumProcesses, g_shutdown_num_processes);
- prefs->SetInteger(prefs::kShutdownNumProcessesSlow,
- g_shutdown_num_processes_slow);
}
// Check local state for the restart flag so we can restart the session later.
@@ -347,100 +330,21 @@
#endif // BUILDFLAG(IS_CHROMEOS_ASH)
}
- if (g_shutdown_type != ShutdownType::kNotValid &&
- g_shutdown_num_processes > 0) {
- // Measure total shutdown time as late in the process as possible
- // and then write it to a file to be read at startup.
- // We can't use prefs since all services are shutdown at this point.
- base::TimeDelta shutdown_delta = base::Time::Now() - *g_shutdown_started;
- std::string shutdown_ms =
- base::NumberToString(shutdown_delta.InMilliseconds());
- base::FilePath shutdown_ms_file = GetShutdownMsPath();
- // Note: ReadLastShutdownFile() is done as a BLOCK_SHUTDOWN task so there's
- // an implicit sequencing between it and this write which happens after
- // threads have been stopped (and thus ThreadPoolInstance::Shutdown() is
- // complete).
- base::WriteFile(shutdown_ms_file, shutdown_ms);
- }
-
#if BUILDFLAG(IS_CHROMEOS_ASH)
chrome::StopSession();
#endif
}
#endif // !BUILDFLAG(IS_ANDROID)
-void ReadLastShutdownFile(ShutdownType type,
- int num_procs,
- int num_procs_slow) {
- base::ScopedBlockingCall scoped_blocking_call(FROM_HERE,
- base::BlockingType::MAY_BLOCK);
-
- base::FilePath shutdown_ms_file = GetShutdownMsPath();
- std::string shutdown_ms_str;
- int64_t shutdown_ms = 0;
- if (base::ReadFileToString(shutdown_ms_file, &shutdown_ms_str))
- base::StringToInt64(shutdown_ms_str, &shutdown_ms);
- base::DeleteFile(shutdown_ms_file);
-
- if (shutdown_ms == 0 || num_procs == 0)
- return;
-
- // TODO(1456205): Remove these metrics as follow-up to
- // https://crrev.com/c/4624774. They are replaced by Shutdown.*.time2.
-
- const char* time_metric_name = nullptr;
- switch (type) {
- case ShutdownType::kNotValid:
- time_metric_name = "Shutdown.NotValid.Time";
- break;
-
- case ShutdownType::kSilentExit:
- time_metric_name = "Shutdown.SilentExit.Time";
- break;
-
- case ShutdownType::kWindowClose:
- time_metric_name = "Shutdown.WindowClose.Time";
- break;
-
- case ShutdownType::kBrowserExit:
- time_metric_name = "Shutdown.BrowserExit.Time";
- break;
-
- case ShutdownType::kEndSession:
- time_metric_name = "Shutdown.EndSession.Time";
- break;
-
- case ShutdownType::kOtherExit:
- time_metric_name = "Shutdown.OtherExit.Time";
- break;
- }
- DCHECK(time_metric_name);
-
- base::UmaHistogramMediumTimes(time_metric_name,
- base::Milliseconds(shutdown_ms));
- base::UmaHistogramCounts100("Shutdown.Renderers.Total", num_procs);
- base::UmaHistogramCounts100("Shutdown.Renderers.Slow", num_procs_slow);
-}
-
void ReadLastShutdownInfo() {
PrefService* prefs = g_browser_process->local_state();
ShutdownType type =
static_cast<ShutdownType>(prefs->GetInteger(prefs::kShutdownType));
- int num_procs = prefs->GetInteger(prefs::kShutdownNumProcesses);
- int num_procs_slow = prefs->GetInteger(prefs::kShutdownNumProcessesSlow);
// clear the prefs immediately so we don't pick them up on a future run
prefs->SetInteger(prefs::kShutdownType,
static_cast<int>(ShutdownType::kNotValid));
- prefs->SetInteger(prefs::kShutdownNumProcesses, 0);
- prefs->SetInteger(prefs::kShutdownNumProcessesSlow, 0);
base::UmaHistogramEnumeration("Shutdown.ShutdownType", type);
-
- base::ThreadPool::PostTask(
- FROM_HERE,
- {base::MayBlock(), base::TaskPriority::BEST_EFFORT,
- base::TaskShutdownBehavior::BLOCK_SHUTDOWN},
- base::BindOnce(&ReadLastShutdownFile, type, num_procs, num_procs_slow));
}
void SetTryingToQuit(bool quitting) {
diff --git a/chrome/browser/lifetime/browser_shutdown_browsertest.cc b/chrome/browser/lifetime/browser_shutdown_browsertest.cc
index b6216f5..817b5c0 100644
--- a/chrome/browser/lifetime/browser_shutdown_browsertest.cc
+++ b/chrome/browser/lifetime/browser_shutdown_browsertest.cc
@@ -55,6 +55,32 @@
// ChromeOS has the different shutdown flow on user initiated exit process.
// See the comment for chrome::AttemptUserExit() function declaration.
#if !BUILDFLAG(IS_CHROMEOS_ASH)
+// Mac browser shutdown is flaky: https://crbug.com/1259913
+#if BUILDFLAG(IS_MAC)
+#define MAYBE_ClosingShutdownHistograms DISABLED_ClosingShutdownHistograms
+#else
+#define MAYBE_ClosingShutdownHistograms ClosingShutdownHistograms
+#endif
+IN_PROC_BROWSER_TEST_F(BrowserShutdownBrowserTest,
+ MAYBE_ClosingShutdownHistograms) {
+ ASSERT_TRUE(
+ ui_test_utils::NavigateToURL(browser(), GURL("browser://version")));
+ CloseBrowserSynchronously(browser());
+ // RecordShutdownMetrics() is called in the ChromeMainDelegate destructor.
+ browser_shutdown::RecordShutdownMetrics();
+
+ EXPECT_TRUE(browser_shutdown::IsTryingToQuit());
+ EXPECT_TRUE(BrowserList::GetInstance()->empty());
+ EXPECT_EQ(browser_shutdown::GetShutdownType(),
+ browser_shutdown::ShutdownType::kWindowClose);
+
+ histogram_tester_.ExpectUniqueSample(
+ "Shutdown.ShutdownType2",
+ static_cast<int>(browser_shutdown::ShutdownType::kWindowClose), 1);
+ histogram_tester_.ExpectTotalCount("Shutdown.WindowClose.Time2", 1);
+ histogram_tester_.ExpectTotalCount("Shutdown.Renderers.Total2", 1);
+}
+
IN_PROC_BROWSER_TEST_F(BrowserShutdownBrowserTest,
PRE_TwoBrowsersClosingShutdownHistograms) {
ASSERT_TRUE(
@@ -77,6 +103,15 @@
EXPECT_EQ(browser_shutdown::GetShutdownType(),
browser_shutdown::ShutdownType::kWindowClose);
BrowserList::RemoveObserver(&closing_observer);
+
+ // RecordShutdownMetrics() is called in the ChromeMainDelegate destructor.
+ browser_shutdown::RecordShutdownMetrics();
+
+ histogram_tester_.ExpectUniqueSample(
+ "Shutdown.ShutdownType2",
+ static_cast<int>(browser_shutdown::ShutdownType::kWindowClose), 1);
+ histogram_tester_.ExpectTotalCount("Shutdown.WindowClose.Time2", 1);
+ histogram_tester_.ExpectTotalCount("Shutdown.Renderers.Total2", 1);
}
// Flakes on Mac12.0: https://crbug.com/1259913
@@ -92,8 +127,6 @@
histogram_tester_.ExpectUniqueSample(
"Shutdown.ShutdownType",
static_cast<int>(browser_shutdown::ShutdownType::kWindowClose), 1);
- histogram_tester_.ExpectTotalCount("Shutdown.Renderers.Total", 1);
- histogram_tester_.ExpectTotalCount("Shutdown.WindowClose.Time", 1);
}
#else
// On Chrome OS, the shutdown accelerator is handled by Ash and requires
diff --git a/chrome/browser/prefs/browser_prefs.cc b/chrome/browser/prefs/browser_prefs.cc
index 56a625fe..54f774c 100644
--- a/chrome/browser/prefs/browser_prefs.cc
+++ b/chrome/browser/prefs/browser_prefs.cc
@@ -952,6 +952,10 @@
const char kClearUserDataDir1Pref[] = "lacros.clear_user_data_dir_1";
#endif
+// Deprecated 07/2023.
+const char kShutdownNumProcesses[] = "shutdown.num_processes";
+const char kShutdownNumProcessesSlow[] = "shutdown.num_processes_slow";
+
// Register local state used only for migration (clearing or moving to a new
// key).
void RegisterLocalStatePrefsForMigration(PrefRegistrySimple* registry) {
@@ -1082,6 +1086,10 @@
#if !BUILDFLAG(IS_ANDROID)
registry->RegisterBooleanPref(kLegacyHoverCardImagesEnabled, false);
#endif // !BUILDFLAG(IS_ANDROID)
+
+ // Deprecated 07/2023.
+ registry->RegisterIntegerPref(kShutdownNumProcesses, 0);
+ registry->RegisterIntegerPref(kShutdownNumProcessesSlow, 0);
}
// Register prefs used only for migration (clearing or moving to a new key).
@@ -2192,6 +2200,10 @@
local_state->ClearPref(kLegacyHoverCardImagesEnabled);
#endif
+ // Added 07/2023.
+ local_state->ClearPref(kShutdownNumProcesses);
+ local_state->ClearPref(kShutdownNumProcessesSlow);
+
// Please don't delete the following line. It is used by PRESUBMIT.py.
// END_MIGRATE_OBSOLETE_LOCAL_STATE_PREFS
diff --git a/chrome/common/pref_names.h b/chrome/common/pref_names.h
index 0d861f0..121b766 100644
--- a/chrome/common/pref_names.h
+++ b/chrome/common/pref_names.h
@@ -2126,11 +2126,6 @@
// An enum value of how the browser was shut down (see browser_shutdown.h).
inline constexpr char kShutdownType[] = "shutdown.type";
-// Number of processes that were open when the user shut down.
-inline constexpr char kShutdownNumProcesses[] = "shutdown.num_processes";
-// Number of processes that were shut down using the slow path.
-inline constexpr char kShutdownNumProcessesSlow[] =
- "shutdown.num_processes_slow";
// Whether to restart the current Chrome session automatically as the last thing
// before shutting everything down.
diff --git a/tools/metrics/histograms/metadata/others/histograms.xml b/tools/metrics/histograms/metadata/others/histograms.xml
index f48d1a69..0a67d03 100644
--- a/tools/metrics/histograms/metadata/others/histograms.xml
+++ b/tools/metrics/histograms/metadata/others/histograms.xml
@@ -11390,15 +11390,6 @@
</summary>
</histogram>
-<histogram name="Shutdown.BrowserExit.Time" units="ms"
- expires_after="2024-01-14">
- <owner>[email protected]</owner>
- <owner>[email protected]</owner>
- <summary>
- Time for shutdown initiated by the browser exit menu command.
- </summary>
-</histogram>
-
<histogram name="Shutdown.BrowserExit.Time2" units="ms"
expires_after="2024-01-14">
<owner>[email protected]</owner>
@@ -11410,16 +11401,6 @@
</summary>
</histogram>
-<histogram name="Shutdown.EndSession.Time" units="ms"
- expires_after="2024-01-14">
- <owner>[email protected]</owner>
- <owner>[email protected]</owner>
- <summary>
- Time for shutdown initiated by an end session (user logs off, shuts down or
- reboots without explicitly exiting).
- </summary>
-</histogram>
-
<histogram name="Shutdown.EndSession.Time2" units="ms"
expires_after="2024-01-14">
<owner>[email protected]</owner>
@@ -11432,16 +11413,6 @@
</summary>
</histogram>
-<histogram name="Shutdown.NotValid.Time" units="ms" expires_after="2023-09-03">
- <owner>[email protected]</owner>
- <owner>[email protected]</owner>
- <summary>
- Time duration for shutdown for a not valid exit. This situation happen when
- a chrome process is already running and the process is terminated after the
- process singleton rendez-vous.
- </summary>
-</histogram>
-
<histogram name="Shutdown.NotValid.Time2" units="ms" expires_after="2023-09-03">
<owner>[email protected]</owner>
<owner>[email protected]</owner>
@@ -11453,16 +11424,6 @@
</summary>
</histogram>
-<histogram name="Shutdown.OtherExit.Time" units="ms" expires_after="2023-09-03">
- <owner>[email protected]</owner>
- <owner>[email protected]</owner>
- <summary>
- Time duration for clean shutdown that are not initiated by the users. The
- duration covers from the shutdown trigger to the deletion of the
- BrowserImpl. Note: Android does not have shutdown metrics.
- </summary>
-</histogram>
-
<histogram name="Shutdown.OtherExit.Time2" units="ms"
expires_after="2024-01-07">
<owner>[email protected]</owner>
@@ -11474,25 +11435,6 @@
</summary>
</histogram>
-<histogram name="Shutdown.Renderers.Slow" units="renderers"
- expires_after="2023-09-03">
- <owner>[email protected]</owner>
- <owner>[email protected]</owner>
- <summary>
- The number of renderer processes that couldn't be shutdown quickly due to
- onbeforeunload or onunload listeners.
- </summary>
-</histogram>
-
-<histogram name="Shutdown.Renderers.Total" units="renderers"
- expires_after="2023-11-19">
- <owner>[email protected]</owner>
- <owner>[email protected]</owner>
- <summary>
- The number of renderer processes running when shutdown was called.
- </summary>
-</histogram>
-
<histogram name="Shutdown.ShutdownType" enum="ShutdownType"
expires_after="2024-01-14">
<owner>[email protected]</owner>
@@ -11519,13 +11461,6 @@
</summary>
</histogram>
-<histogram name="Shutdown.SilentExit.Time" units="ms"
- expires_after="2023-12-04">
- <owner>[email protected]</owner>
- <owner>[email protected]</owner>
- <summary>Time for shutdown for a silent exit.</summary>
-</histogram>
-
<histogram name="Shutdown.SilentExit.Time2" units="ms"
expires_after="2024-01-07">
<owner>[email protected]</owner>
@@ -11537,15 +11472,6 @@
</summary>
</histogram>
-<histogram name="Shutdown.WindowClose.Time" units="ms"
- expires_after="2024-01-14">
- <owner>[email protected]</owner>
- <owner>[email protected]</owner>
- <summary>
- Time for shutdown initiated by the last browser window being closed.
- </summary>
-</histogram>
-
<histogram name="Shutdown.WindowClose.Time2" units="ms"
expires_after="2024-01-14">
<owner>[email protected]</owner>
