From d19d2962b3f85f354982c4650a72ff9f03339ff8 Mon Sep 17 00:00:00 2001 From: Eliot Courtney Date: Fri, 8 Apr 2022 07:32:54 +0000 Subject: [PATCH] Revert "arc: Add ANR stats per period." This reverts commit 5f0165a8599948db862c49ce3856c82c583d9838. Reason for revert: See http://b/228557820. This is blocking LKGM - causes DCHECK for nonexistent pref. Original change's description: > arc: Add ANR stats per period. > > This adds ANR rate for starting period and ANR rate on regular basis > taken each 4 hours. Starting period is 10 minutes after ARC start. > Shorter sessions are ignored. Regular period is calculated based on ARC > session activity and updated each 4-hours interval. It also supports > ARC session restarts. That means if session is restarted in the middle > of the period, it would be continued when user logs in the next time. > > BUG=b:225214120 > TEST=unit test > > Change-Id: I5fe522e4fcb051f06470f76969884aa4ff580be9 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3537585 > Reviewed-by: Muhammad Hasan Khan > Reviewed-by: Yusuke Sato > Commit-Queue: Yury Khmel > Cr-Commit-Position: refs/heads/main@{#988608} Bug: b:225214120 Change-Id: Ib89397b8681686cc8e1e4cb4de25c890e2743116 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3578196 Reviewed-by: Hidehiko Abe Commit-Queue: Eliot Courtney Owners-Override: Eliot Courtney Cr-Commit-Position: refs/heads/main@{#990293} --- ash/components/arc/BUILD.gn | 3 - ash/components/arc/arc_prefs.cc | 13 - ash/components/arc/arc_prefs.h | 3 - ash/components/arc/metrics/arc_metrics_anr.cc | 147 ---------- ash/components/arc/metrics/arc_metrics_anr.h | 49 ---- .../arc/metrics/arc_metrics_anr_unittest.cc | 255 ------------------ .../arc/metrics/arc_metrics_service.cc | 57 ++-- .../arc/metrics/arc_metrics_service.h | 13 - .../metrics/arc_metrics_service_unittest.cc | 116 +++++++- .../arc/test/test_browser_context.h | 3 +- .../arc/metrics/arc_metrics_service_proxy.cc | 8 - .../arc/metrics/arc_metrics_service_proxy.h | 1 - .../histograms/metadata/arc/histograms.xml | 19 +- 13 files changed, 148 insertions(+), 539 deletions(-) delete mode 100644 ash/components/arc/metrics/arc_metrics_anr.cc delete mode 100644 ash/components/arc/metrics/arc_metrics_anr.h delete mode 100644 ash/components/arc/metrics/arc_metrics_anr_unittest.cc diff --git a/ash/components/arc/BUILD.gn b/ash/components/arc/BUILD.gn index 5e829b2e2253c6..fa67c471361c75 100644 --- a/ash/components/arc/BUILD.gn +++ b/ash/components/arc/BUILD.gn @@ -66,8 +66,6 @@ static_library("arc") { "memory/arc_memory_bridge.h", "memory_pressure/arc_memory_pressure_bridge.cc", "memory_pressure/arc_memory_pressure_bridge.h", - "metrics/arc_metrics_anr.cc", - "metrics/arc_metrics_anr.h", "metrics/arc_metrics_service.cc", "metrics/arc_metrics_service.h", "metrics/stability_metrics_manager.cc", @@ -400,7 +398,6 @@ source_set("unit_tests") { "media_session/arc_media_session_bridge_unittest.cc", "memory/arc_memory_bridge_unittest.cc", "memory_pressure/arc_memory_pressure_bridge_unittest.cc", - "metrics/arc_metrics_anr_unittest.cc", "metrics/arc_metrics_service_unittest.cc", "metrics/stability_metrics_manager_unittest.cc", "midis/arc_midis_bridge_unittest.cc", diff --git a/ash/components/arc/arc_prefs.cc b/ash/components/arc/arc_prefs.cc index 68cabd7887b21f..32ac663ef42f92 100644 --- a/ash/components/arc/arc_prefs.cc +++ b/ash/components/arc/arc_prefs.cc @@ -120,15 +120,6 @@ const char kArcPlayStoreLaunchMetricCanBeRecorded[] = "arc.playstore_launched_by_user"; // ======== LOCAL STATE PREFS ======== -// ANR count which is currently pending counted for ARC start, not flashed to -// UMA. -const char kAnrOnStartPendingCount[] = "arc.anr_pending_count_on_start"; - -// ANR count which is currently pending, not flashed to UMA. -const char kAnrPendingCount[] = "arc.anr_pending_count"; - -// Keeps the duration of the current ANR rate period. -const char kAnrPendingDuration[] = "arc.anr_pending_duration"; // A boolean preference that indicates whether this device has run with the // native bridge 64 bit support experiment enabled. Persisting value in local @@ -161,10 +152,6 @@ void RegisterLocalStatePrefs(PrefRegistrySimple* registry) { registry->RegisterBooleanPref(kNativeBridge64BitSupportExperimentEnabled, false); registry->RegisterDictionaryPref(kStabilityMetrics); - - registry->RegisterIntegerPref(kAnrPendingCount, 0); - registry->RegisterIntegerPref(kAnrOnStartPendingCount, 0); - registry->RegisterTimeDeltaPref(kAnrPendingDuration, base::TimeDelta()); } void RegisterProfilePrefs(PrefRegistrySimple* registry) { diff --git a/ash/components/arc/arc_prefs.h b/ash/components/arc/arc_prefs.h index 0c20ba04e0b291..136048b26745aa 100644 --- a/ash/components/arc/arc_prefs.h +++ b/ash/components/arc/arc_prefs.h @@ -47,9 +47,6 @@ ARC_EXPORT extern const char kEngagementPrefsPrefix[]; ARC_EXPORT extern const char kArcPlayStoreLaunchMetricCanBeRecorded[]; // Local state prefs in lexicographical order. -ARC_EXPORT extern const char kAnrOnStartPendingCount[]; -ARC_EXPORT extern const char kAnrPendingCount[]; -ARC_EXPORT extern const char kAnrPendingDuration[]; ARC_EXPORT extern const char kArcSerialNumberSalt[]; ARC_EXPORT extern const char kArcSnapshotHours[]; ARC_EXPORT extern const char kArcSnapshotInfo[]; diff --git a/ash/components/arc/metrics/arc_metrics_anr.cc b/ash/components/arc/metrics/arc_metrics_anr.cc deleted file mode 100644 index e7c2925cc5a142..00000000000000 --- a/ash/components/arc/metrics/arc_metrics_anr.cc +++ /dev/null @@ -1,147 +0,0 @@ -// Copyright 2022 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "ash/components/arc/metrics/arc_metrics_anr.h" - -#include - -#include "ash/components/arc/arc_prefs.h" -#include "base/bind.h" -#include "base/metrics/histogram_functions.h" -#include "base/metrics/histogram_macros.h" -#include "components/prefs/pref_service.h" - -namespace arc { - -namespace { -// Intervals to collect ANR rate after login. Collected only once per session. -// In case the session is shorter than |kMinStartPeriodDuration| interval, it -// is discarded. Shorter sessions distort stats due to ANR is long process that -// may take 10, 20 and more seconds. This also done to avoid false reporting -// when session gets restarted due to applying configuration, policy, and so on. -constexpr base::TimeDelta kMaxStartPeriodDuration = base::Minutes(10); -constexpr base::TimeDelta kMinStartPeriodDuration = base::Minutes(2); - -// Interval to report ANR rate. ANR rate is reported each interval of ARC -// active state. -constexpr base::TimeDelta kRateInterval = base::Hours(4); - -// Interval to update ANR interval. Once accumulated interval is -// greater than |kArcAnrRateInterval| UMA is updated. -constexpr base::TimeDelta kUpdateInterval = base::Minutes(5); - -// It is very unlikely to do have more ANR events than -// |kForPeriodMaxCount| times in |kMaxStartPeriodDuration| or -// |kRateInterval|. This acts as an upper bound of UMA buckets. -constexpr int kForPeriodMaxCount = 64; - -constexpr char kStartPeriodHistogram[] = "Arc.Anr.First10MinutesAfterStart"; -constexpr char kRegularPeriodHistogram[] = "Arc.Anr.Per4Hours"; - -// App types to report. -constexpr char kAppTypeArcAppLauncher[] = "ArcAppLauncher"; -constexpr char kAppTypeArcOther[] = "ArcOther"; -constexpr char kAppTypeFirstParty[] = "FirstParty"; -constexpr char kAppTypeGmsCore[] = "GmsCore"; -constexpr char kAppTypePlayStore[] = "PlayStore"; -constexpr char kAppTypeSystemServer[] = "SystemServer"; -constexpr char kAppTypeSystem[] = "SystemApp"; -constexpr char kAppTypeOther[] = "Other"; - -std::string SourceToTableName(mojom::AnrSource value) { - switch (value) { - case mojom::AnrSource::OTHER: - return kAppTypeOther; - case mojom::AnrSource::SYSTEM_SERVER: - return kAppTypeSystemServer; - case mojom::AnrSource::SYSTEM_APP: - return kAppTypeSystem; - case mojom::AnrSource::GMS_CORE: - return kAppTypeGmsCore; - case mojom::AnrSource::PLAY_STORE: - return kAppTypePlayStore; - case mojom::AnrSource::FIRST_PARTY: - return kAppTypeFirstParty; - case mojom::AnrSource::ARC_OTHER: - return kAppTypeArcOther; - case mojom::AnrSource::ARC_APP_LAUNCHER: - return kAppTypeArcAppLauncher; - default: - LOG(ERROR) << "Unrecognized source ANR " << value; - return kAppTypeOther; - } -} - -} // namespace - -ArcMetricsAnr::ArcMetricsAnr(PrefService* prefs) : prefs_(prefs) { - pending_start_timer_.Start( - FROM_HERE, kMinStartPeriodDuration, - base::BindOnce(&ArcMetricsAnr::SetLogOnStartPending, - base::Unretained(this))); - start_timer_.Start( - FROM_HERE, kMaxStartPeriodDuration, - base::BindOnce(&ArcMetricsAnr::LogOnStart, base::Unretained(this))); - period_updater_.Start( - FROM_HERE, kUpdateInterval, - base::BindRepeating(&ArcMetricsAnr::UpdateRate, base::Unretained(this))); - - if (prefs_->HasPrefPath(prefs::kAnrOnStartPendingCount)) { - base::UmaHistogramExactLinear( - kStartPeriodHistogram, - prefs_->GetInteger(prefs::kAnrOnStartPendingCount), kForPeriodMaxCount); - prefs_->ClearPref(prefs::kAnrOnStartPendingCount); - } -} - -ArcMetricsAnr::~ArcMetricsAnr() { - if (log_on_start_pending_) { - // Session is shorter than |kMaxStartPeriodDuration| but longer than - // |kMinStartPeriodDuration|. Save current counter and report on next - // login. - prefs_->SetInteger(prefs::kAnrOnStartPendingCount, - count_10min_after_start_); - } -} - -void ArcMetricsAnr::Report(mojom::AnrPtr anr) { - DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); - // TODO (b/227337741): Retire these in favor of ANR rate. - base::UmaHistogramEnumeration("Arc.Anr.Overall", anr->type); - base::UmaHistogramEnumeration("Arc.Anr." + SourceToTableName(anr->source), - anr->type); - ++count_10min_after_start_; - prefs_->SetInteger(prefs::kAnrPendingCount, - prefs_->GetInteger(prefs::kAnrPendingCount) + 1); -} - -void ArcMetricsAnr::LogOnStart() { - DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); - base::UmaHistogramExactLinear(kStartPeriodHistogram, count_10min_after_start_, - kForPeriodMaxCount); - // We already reported ANR count on start for this session. - log_on_start_pending_ = false; -} - -void ArcMetricsAnr::UpdateRate() { - DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); - - base::TimeDelta duration = - prefs_->GetTimeDelta(prefs::kAnrPendingDuration) + kUpdateInterval; - if (duration >= kRateInterval) { - duration = base::TimeDelta(); - base::UmaHistogramExactLinear(kRegularPeriodHistogram, - prefs_->GetInteger(prefs::kAnrPendingCount), - kForPeriodMaxCount); - prefs_->SetInteger(prefs::kAnrPendingCount, 0); - } - - prefs_->SetTimeDelta(prefs::kAnrPendingDuration, duration); -} - -void ArcMetricsAnr::SetLogOnStartPending() { - log_on_start_pending_ = true; -} - -} // namespace arc diff --git a/ash/components/arc/metrics/arc_metrics_anr.h b/ash/components/arc/metrics/arc_metrics_anr.h deleted file mode 100644 index efdd9d4ef338f0..00000000000000 --- a/ash/components/arc/metrics/arc_metrics_anr.h +++ /dev/null @@ -1,49 +0,0 @@ -// Copyright 2022 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#ifndef ASH_COMPONENTS_ARC_METRICS_ARC_METRICS_ANR_H_ -#define ASH_COMPONENTS_ARC_METRICS_ARC_METRICS_ANR_H_ - -#include "ash/components/arc/mojom/anr.mojom.h" -#include "base/threading/thread_checker.h" -#include "base/timer/timer.h" - -class PrefService; - -namespace arc { - -// Handles ANR events and supports UMA metrics. -class ArcMetricsAnr { - public: - explicit ArcMetricsAnr(PrefService* prefs); - - ArcMetricsAnr(const ArcMetricsAnr&) = delete; - ArcMetricsAnr& operator=(const ArcMetricsAnr&) = delete; - - ~ArcMetricsAnr(); - - void Report(mojom::AnrPtr anr); - - private: - void LogOnStart(); - void UpdateRate(); - void SetLogOnStartPending(); - - // ANR count for first 10 minitues after start. - int count_10min_after_start_ = 0; - // Set to true in case |count_10min_after_start_| is pending and has to be - // persisted on restart. Once ANR count on start is reported this is set to - // false. - bool log_on_start_pending_ = false; - base::OneShotTimer start_timer_; - base::OneShotTimer pending_start_timer_; - base::RepeatingTimer period_updater_; - PrefService* const prefs_ = nullptr; - - THREAD_CHECKER(thread_checker_); -}; - -} // namespace arc - -#endif // ASH_COMPONENTS_ARC_METRICS_ARC_METRICS_ANR_H_ diff --git a/ash/components/arc/metrics/arc_metrics_anr_unittest.cc b/ash/components/arc/metrics/arc_metrics_anr_unittest.cc deleted file mode 100644 index 4c1aa44dc32144..00000000000000 --- a/ash/components/arc/metrics/arc_metrics_anr_unittest.cc +++ /dev/null @@ -1,255 +0,0 @@ -// Copyright 2022 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "ash/components/arc/metrics/arc_metrics_anr.h" - -#include -#include -#include -#include -#include - -#include "ash/components/arc/arc_prefs.h" -#include "ash/components/arc/test/test_browser_context.h" -#include "base/metrics/histogram_samples.h" -#include "base/test/metrics/histogram_tester.h" -#include "components/prefs/testing_pref_service.h" -#include "content/public/test/browser_task_environment.h" -#include "testing/gtest/include/gtest/gtest.h" - -namespace arc { -namespace { - -constexpr char kAppTypeArcAppLauncher[] = "ArcAppLauncher"; -constexpr char kAppTypeArcOther[] = "ArcOther"; -constexpr char kAppTypeFirstParty[] = "FirstParty"; -constexpr char kAppTypeGmsCore[] = "GmsCore"; -constexpr char kAppTypePlayStore[] = "PlayStore"; -constexpr char kAppTypeSystemServer[] = "SystemServer"; -constexpr char kAppTypeSystem[] = "SystemApp"; -constexpr char kAppTypeOther[] = "Other"; -constexpr char kAppOverall[] = "Overall"; - -constexpr std::array kAppTypes{ - kAppTypeArcAppLauncher, kAppTypeArcOther, kAppTypeFirstParty, - kAppTypeGmsCore, kAppTypePlayStore, kAppTypeSystemServer, - kAppTypeSystem, kAppTypeOther, kAppOverall, -}; - -std::string CreateAnrKey(const std::string& app_type, mojom::AnrType type) { - std::stringstream output; - output << app_type << "/" << type; - return output.str(); -} - -mojom::AnrPtr GetAnr(mojom::AnrSource source, mojom::AnrType type) { - return mojom::Anr::New(type, source); -} - -void VerifyAnr(const base::HistogramTester& tester, - const std::map& expectation) { - std::map current; - for (const char* app_type : kAppTypes) { - const std::vector buckets = - tester.GetAllSamples("Arc.Anr." + std::string(app_type)); - for (const auto& bucket : buckets) { - current[CreateAnrKey(app_type, static_cast(bucket.min))] = - bucket.count; - } - } - EXPECT_EQ(expectation, current); -} - -class ArcMetricsAnrTest : public testing::Test { - public: - ArcMetricsAnrTest(const ArcMetricsAnrTest&) = delete; - ArcMetricsAnrTest& operator=(const ArcMetricsAnrTest&) = delete; - - protected: - ArcMetricsAnrTest() { - prefs::RegisterLocalStatePrefs(local_state_.registry()); - context_ = std::make_unique(); - prefs::RegisterLocalStatePrefs(context_->pref_registry()); - prefs::RegisterProfilePrefs(context_->pref_registry()); - } - - ~ArcMetricsAnrTest() override = default; - - void FastForwardBy(base::TimeDelta delta) { - task_environment_.FastForwardBy(delta); - } - - PrefService* prefs() { return context_->prefs(); } - - private: - content::BrowserTaskEnvironment task_environment_{ - base::test::TaskEnvironment::TimeSource::MOCK_TIME}; - TestingPrefServiceSimple local_state_; - std::unique_ptr context_; -}; - -TEST_F(ArcMetricsAnrTest, Basic) { - base::HistogramTester tester; - std::map expectation; - - ArcMetricsAnr anrs(prefs()); - - anrs.Report(GetAnr(mojom::AnrSource::OTHER, mojom::AnrType::UNKNOWN)); - expectation[CreateAnrKey(kAppOverall, mojom::AnrType::UNKNOWN)] = 1; - expectation[CreateAnrKey(kAppTypeOther, mojom::AnrType::UNKNOWN)] = 1; - VerifyAnr(tester, expectation); - - anrs.Report(GetAnr(mojom::AnrSource::SYSTEM_SERVER, mojom::AnrType::INPUT)); - expectation[CreateAnrKey(kAppOverall, mojom::AnrType::INPUT)] = 1; - expectation[CreateAnrKey(kAppTypeSystemServer, mojom::AnrType::INPUT)] = 1; - VerifyAnr(tester, expectation); - - anrs.Report(GetAnr(mojom::AnrSource::SYSTEM_SERVER, - mojom::AnrType::FOREGROUND_SERVICE)); - expectation[CreateAnrKey(kAppOverall, mojom::AnrType::FOREGROUND_SERVICE)] = - 1; - expectation[CreateAnrKey(kAppTypeSystemServer, - mojom::AnrType::FOREGROUND_SERVICE)] = 1; - VerifyAnr(tester, expectation); - - anrs.Report(GetAnr(mojom::AnrSource::SYSTEM_SERVER, - mojom::AnrType::BACKGROUND_SERVICE)); - expectation[CreateAnrKey(kAppOverall, mojom::AnrType::BACKGROUND_SERVICE)] = - 1; - expectation[CreateAnrKey(kAppTypeSystemServer, - mojom::AnrType::BACKGROUND_SERVICE)] = 1; - VerifyAnr(tester, expectation); - - anrs.Report(GetAnr(mojom::AnrSource::GMS_CORE, mojom::AnrType::BROADCAST)); - expectation[CreateAnrKey(kAppOverall, mojom::AnrType::BROADCAST)] = 1; - expectation[CreateAnrKey(kAppTypeGmsCore, mojom::AnrType::BROADCAST)] = 1; - VerifyAnr(tester, expectation); - - anrs.Report( - GetAnr(mojom::AnrSource::PLAY_STORE, mojom::AnrType::CONTENT_PROVIDER)); - expectation[CreateAnrKey(kAppOverall, mojom::AnrType::CONTENT_PROVIDER)] = 1; - expectation[CreateAnrKey(kAppTypePlayStore, - mojom::AnrType::CONTENT_PROVIDER)] = 1; - VerifyAnr(tester, expectation); - - anrs.Report( - GetAnr(mojom::AnrSource::FIRST_PARTY, mojom::AnrType::APP_REQUESTED)); - expectation[CreateAnrKey(kAppOverall, mojom::AnrType::APP_REQUESTED)] = 1; - expectation[CreateAnrKey(kAppTypeFirstParty, mojom::AnrType::APP_REQUESTED)] = - 1; - VerifyAnr(tester, expectation); - - anrs.Report(GetAnr(mojom::AnrSource::ARC_OTHER, mojom::AnrType::INPUT)); - expectation[CreateAnrKey(kAppOverall, mojom::AnrType::INPUT)] = 2; - expectation[CreateAnrKey(kAppTypeArcOther, mojom::AnrType::INPUT)] = 1; - VerifyAnr(tester, expectation); - - anrs.Report(GetAnr(mojom::AnrSource::ARC_APP_LAUNCHER, - mojom::AnrType::FOREGROUND_SERVICE)); - expectation[CreateAnrKey(kAppOverall, mojom::AnrType::FOREGROUND_SERVICE)] = - 2; - expectation[CreateAnrKey(kAppTypeArcAppLauncher, - mojom::AnrType::FOREGROUND_SERVICE)] = 1; - VerifyAnr(tester, expectation); -} - -TEST_F(ArcMetricsAnrTest, ShortSessionOnStart) { - base::HistogramTester tester; - - std::unique_ptr anrs = - std::make_unique(prefs()); - - anrs->Report(GetAnr(mojom::AnrSource::OTHER, mojom::AnrType::UNKNOWN)); - // Session shorter than 2 min is discarded and ANR should not be reported - // on restart. - FastForwardBy(base::Seconds(119)); - - // Use explicit reset to preserve right sequence of DTOR/CTOR. - anrs.reset(); - anrs = std::make_unique(prefs()); - tester.ExpectTotalCount("Arc.Anr.First10MinutesAfterStart", 0); - EXPECT_EQ(0, tester.GetTotalSum("Arc.Anr.First10MinutesAfterStart")); - - anrs->Report(GetAnr(mojom::AnrSource::OTHER, mojom::AnrType::UNKNOWN)); - // Shorter than 10 minutes but longer than 2 minutes. ANR count should - // be reported on restart. - FastForwardBy(base::Minutes(9)); - tester.ExpectTotalCount("Arc.Anr.First10MinutesAfterStart", 0); - EXPECT_EQ(0, tester.GetTotalSum("Arc.Anr.First10MinutesAfterStart")); - - anrs.reset(); - anrs = std::make_unique(prefs()); - tester.ExpectTotalCount("Arc.Anr.First10MinutesAfterStart", 1); - EXPECT_EQ(1, tester.GetTotalSum("Arc.Anr.First10MinutesAfterStart")); - - // Confirm it is not reported twice. - FastForwardBy(base::Minutes(10)); - // Note, after 10 min one more ANR is reported with 0 ANR. - tester.ExpectTotalCount("Arc.Anr.First10MinutesAfterStart", 2); - EXPECT_EQ(1, tester.GetTotalSum("Arc.Anr.First10MinutesAfterStart")); - - anrs.reset(); - anrs = std::make_unique(prefs()); - tester.ExpectTotalCount("Arc.Anr.First10MinutesAfterStart", 2); - EXPECT_EQ(1, tester.GetTotalSum("Arc.Anr.First10MinutesAfterStart")); -} - -TEST_F(ArcMetricsAnrTest, ForPeriod) { - base::HistogramTester tester; - - std::unique_ptr anrs = - std::make_unique(prefs()); - - anrs->Report(GetAnr(mojom::AnrSource::OTHER, mojom::AnrType::UNKNOWN)); - anrs->Report(GetAnr(mojom::AnrSource::ARC_APP_LAUNCHER, - mojom::AnrType::FOREGROUND_SERVICE)); - FastForwardBy(base::Minutes(5)); - tester.ExpectTotalCount("Arc.Anr.First10MinutesAfterStart", 0); - FastForwardBy(base::Minutes(5)); - // 10 min start period elapsed, UMA should be ready for the start period. - tester.ExpectTotalCount("Arc.Anr.First10MinutesAfterStart", 1); - EXPECT_EQ(2, tester.GetTotalSum("Arc.Anr.First10MinutesAfterStart")); - tester.ExpectTotalCount("Arc.Anr.Per4Hours", 0); - - anrs->Report(GetAnr(mojom::AnrSource::ARC_OTHER, mojom::AnrType::INPUT)); - const int forward_count_per_update = 4 * 60 / 5; - // Forward to 5 min before 4 hours interval. Note 10 minutes already elapsed. - for (int i = 0; i < forward_count_per_update - 2 - 1; ++i) { - FastForwardBy(base::Minutes(5)); - } - tester.ExpectTotalCount("Arc.Anr.Per4Hours", 0); - FastForwardBy(base::Minutes(5)); - // One hour elapsed after start period. UMA should be ready for the regular - // period. - tester.ExpectTotalCount("Arc.Anr.Per4Hours", 1); - EXPECT_EQ(3, tester.GetTotalSum("Arc.Anr.Per4Hours")); - - // One more 4-hours period elapsed. 0 ANR rate should be added for the - // this period. - for (int i = 0; i < forward_count_per_update; ++i) { - FastForwardBy(base::Minutes(5)); - } - tester.ExpectTotalCount("Arc.Anr.Per4Hours", 2); - EXPECT_EQ(3, tester.GetTotalSum("Arc.Anr.Per4Hours")); - - anrs->Report( - GetAnr(mojom::AnrSource::PLAY_STORE, mojom::AnrType::CONTENT_PROVIDER)); - tester.ExpectTotalCount("Arc.Anr.Per4Hours", 2); - - for (int i = 0; i < forward_count_per_update; ++i) { - FastForwardBy(base::Minutes(5)); - } - tester.ExpectTotalCount("Arc.Anr.Per4Hours", 3); - EXPECT_EQ(4, tester.GetTotalSum("Arc.Anr.Per4Hours")); - - // Stop ARC and wait. No more updates are expected. - anrs.reset(); - for (int i = 0; i < forward_count_per_update; ++i) { - FastForwardBy(base::Minutes(5)); - } - tester.ExpectTotalCount("Arc.Anr.Per4Hours", 3); -} - -} // namespace -} // namespace arc diff --git a/ash/components/arc/metrics/arc_metrics_service.cc b/ash/components/arc/metrics/arc_metrics_service.cc index 0419dd3989756f..5050f492710fe2 100644 --- a/ash/components/arc/metrics/arc_metrics_service.cc +++ b/ash/components/arc/metrics/arc_metrics_service.cc @@ -10,7 +10,6 @@ #include "ash/components/arc/arc_features.h" #include "ash/components/arc/arc_prefs.h" #include "ash/components/arc/arc_util.h" -#include "ash/components/arc/metrics/arc_metrics_anr.h" #include "ash/components/arc/metrics/stability_metrics_manager.h" #include "ash/components/arc/session/arc_bridge_service.h" #include "ash/public/cpp/app_types_util.h" @@ -26,6 +25,7 @@ #include "chromeos/dbus/session_manager/session_manager_client.h" #include "components/exo/wm_helper.h" #include "components/metrics/psi_memory_parser.h" +#include "components/prefs/pref_service.h" #include "components/user_prefs/user_prefs.h" #include "content/public/browser/browser_context.h" #include "ui/events/ozone/gamepad/gamepad_provider_ozone.h" @@ -54,6 +54,16 @@ constexpr char kBootProgressArcUpgraded[] = "boot_progress_arc_upgraded"; // Interval for collecting UMA "Arc.App.LowMemoryKills.*Count10Minutes" metrics. constexpr base::TimeDelta kRequestKillCountPeriod = base::Minutes(10); +// App types to report. +constexpr char kAppTypeArcAppLauncher[] = "ArcAppLauncher"; +constexpr char kAppTypeArcOther[] = "ArcOther"; +constexpr char kAppTypeFirstParty[] = "FirstParty"; +constexpr char kAppTypeGmsCore[] = "GmsCore"; +constexpr char kAppTypePlayStore[] = "PlayStore"; +constexpr char kAppTypeSystemServer[] = "SystemServer"; +constexpr char kAppTypeSystem[] = "SystemApp"; +constexpr char kAppTypeOther[] = "Other"; + // Memory pressure histograms. const char kPSIMemoryPressureSomeARC[] = "ChromeOS.CWP.PSIMemPressure.ArcSome"; const char kPSIMemoryPressureFullARC[] = "ChromeOS.CWP.PSIMemPressure.ArcFull"; @@ -69,6 +79,30 @@ void LogStabilityUmaEnum(const std::string& name, T sample) { VLOG(1) << name << ": " << static_cast>(sample); } +std::string AnrSourceToTableName(mojom::AnrSource value) { + switch (value) { + case mojom::AnrSource::OTHER: + return kAppTypeOther; + case mojom::AnrSource::SYSTEM_SERVER: + return kAppTypeSystemServer; + case mojom::AnrSource::SYSTEM_APP: + return kAppTypeSystem; + case mojom::AnrSource::GMS_CORE: + return kAppTypeGmsCore; + case mojom::AnrSource::PLAY_STORE: + return kAppTypePlayStore; + case mojom::AnrSource::FIRST_PARTY: + return kAppTypeFirstParty; + case mojom::AnrSource::ARC_OTHER: + return kAppTypeArcOther; + case mojom::AnrSource::ARC_APP_LAUNCHER: + return kAppTypeArcAppLauncher; + default: + LOG(ERROR) << "Unrecognized source ANR " << value; + return kAppTypeOther; + } +} + std::string BootTypeToString(mojom::BootType boot_type) { switch (boot_type) { case mojom::BootType::UNKNOWN: @@ -180,7 +214,6 @@ ArcMetricsService::~ArcMetricsService() { } void ArcMetricsService::Shutdown() { - metrics_anr_.reset(); for (auto& obs : app_kill_observers_) obs.OnArcMetricsServiceDestroyed(); app_kill_observers_.Clear(); @@ -530,13 +563,9 @@ void ArcMetricsService::ReportClipboardDragDropEvent( void ArcMetricsService::ReportAnr(mojom::AnrPtr anr) { DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); - - if (!metrics_anr_) { - LOG(ERROR) << "ANR is reported when no ANR metric service is connected"; - return; - } - - metrics_anr_->Report(std::move(anr)); + base::UmaHistogramEnumeration("Arc.Anr.Overall", anr->type); + LogStabilityUmaEnum("Arc.Anr." + AnrSourceToTableName(anr->source), + anr->type); } void ArcMetricsService::ReportLowLatencyStylusLibApiUsage( @@ -688,16 +717,6 @@ void ArcMetricsService::OnTaskDestroyed(int32_t task_id) { guest_os_engagement_metrics_.SetBackgroundActive(!task_ids_.empty()); } -void ArcMetricsService::OnArcStarted() { - DCHECK(prefs_); - - metrics_anr_ = std::make_unique(prefs_); -} - -void ArcMetricsService::OnArcSessionStopped() { - metrics_anr_.reset(); -} - void ArcMetricsService::AddAppKillObserver(AppKillObserver* obs) { DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); app_kill_observers_.AddObserver(obs); diff --git a/ash/components/arc/metrics/arc_metrics_service.h b/ash/components/arc/metrics/arc_metrics_service.h index 98597d140c29ab..132d530399173f 100644 --- a/ash/components/arc/metrics/arc_metrics_service.h +++ b/ash/components/arc/metrics/arc_metrics_service.h @@ -31,7 +31,6 @@ #include "ui/wm/public/activation_change_observer.h" class BrowserContextKeyedServiceFactory; -class PrefService; namespace metrics { class PSIMemoryParser; @@ -47,8 +46,6 @@ class BrowserContext; namespace arc { -class ArcMetricsAnr; - namespace mojom { class AppInstance; class IntentHelperInstance; @@ -165,11 +162,6 @@ class ArcMetricsService : public KeyedService, const std::string& intent); void OnTaskDestroyed(int32_t task_id); - // ArcSessionManagerObserver callbacks which are called through - // ArcMetricsServiceProxy. - void OnArcStarted(); - void OnArcSessionStopped(); - void AddAppKillObserver(AppKillObserver* obs); void RemoveAppKillObserver(AppKillObserver* obs); @@ -191,8 +183,6 @@ class ArcMetricsService : public KeyedService, // counts can be logged to UMA. Public for testing. void RequestLowMemoryKillCountsForTesting(); - void set_prefs(PrefService* prefs) { prefs_ = prefs; } - // Record the starting time of ARC provisioning, for later use. void ReportProvisioningStartTime(const base::TimeTicks& start_time, const std::string& account_type_suffix); @@ -323,9 +313,6 @@ class ArcMetricsService : public KeyedService, base::ObserverList app_kill_observers_; base::ObserverList user_interaction_observers_; - PrefService* prefs_ = nullptr; - std::unique_ptr metrics_anr_; - // For reporting Arc.Provisioning.PreSignInTimeDelta. absl::optional arc_provisioning_start_time_; absl::optional arc_provisioning_account_type_suffix_; diff --git a/ash/components/arc/metrics/arc_metrics_service_unittest.cc b/ash/components/arc/metrics/arc_metrics_service_unittest.cc index 829c65592cc850..71057b988faae0 100644 --- a/ash/components/arc/metrics/arc_metrics_service_unittest.cc +++ b/ash/components/arc/metrics/arc_metrics_service_unittest.cc @@ -50,6 +50,46 @@ constexpr std::array kBootEvents{ constexpr const char kBootProgressArcUpgraded[] = "boot_progress_arc_upgraded"; +constexpr char kAppTypeArcAppLauncher[] = "ArcAppLauncher"; +constexpr char kAppTypeArcOther[] = "ArcOther"; +constexpr char kAppTypeFirstParty[] = "FirstParty"; +constexpr char kAppTypeGmsCore[] = "GmsCore"; +constexpr char kAppTypePlayStore[] = "PlayStore"; +constexpr char kAppTypeSystemServer[] = "SystemServer"; +constexpr char kAppTypeSystem[] = "SystemApp"; +constexpr char kAppTypeOther[] = "Other"; +constexpr char kAppOverall[] = "Overall"; + +constexpr std::array kAppTypes{ + kAppTypeArcAppLauncher, kAppTypeArcOther, kAppTypeFirstParty, + kAppTypeGmsCore, kAppTypePlayStore, kAppTypeSystemServer, + kAppTypeSystem, kAppTypeOther, kAppOverall, +}; + +std::string CreateAnrKey(const std::string& app_type, mojom::AnrType type) { + std::stringstream output; + output << app_type << "/" << type; + return output.str(); +} + +mojom::AnrPtr GetAnr(mojom::AnrSource source, mojom::AnrType type) { + return mojom::Anr::New(type, source); +} + +void VerifyAnr(const base::HistogramTester& tester, + const std::map& expectation) { + std::map current; + for (const char* app_type : kAppTypes) { + const std::vector buckets = + tester.GetAllSamples("Arc.Anr." + std::string(app_type)); + for (const auto& bucket : buckets) { + current[CreateAnrKey(app_type, static_cast(bucket.min))] = + bucket.count; + } + } + EXPECT_EQ(expectation, current); +} + class ArcMetricsServiceTest : public testing::Test { public: ArcMetricsServiceTest(const ArcMetricsServiceTest&) = delete; @@ -65,11 +105,9 @@ class ArcMetricsServiceTest : public testing::Test { arc_service_manager_ = std::make_unique(); context_ = std::make_unique(); - prefs::RegisterLocalStatePrefs(context_->pref_registry()); prefs::RegisterProfilePrefs(context_->pref_registry()); service_ = ArcMetricsService::GetForBrowserContextForTesting(context_.get()); - service_->set_prefs(context_->prefs()); CreateFakeWindows(); @@ -109,10 +147,6 @@ class ArcMetricsServiceTest : public testing::Test { return events; } - void FastForwardBy(base::TimeDelta delta) { - task_environment_.FastForwardBy(delta); - } - aura::Window* fake_arc_window() { return fake_arc_window_.get(); } aura::Window* fake_non_arc_window() { return fake_non_arc_window_.get(); } @@ -128,8 +162,7 @@ class ArcMetricsServiceTest : public testing::Test { /*id=*/1, nullptr)); } - content::BrowserTaskEnvironment task_environment_{ - base::test::TaskEnvironment::TimeSource::MOCK_TIME}; + content::BrowserTaskEnvironment task_environment_; TestingPrefServiceSimple local_state_; session_manager::SessionManager session_manager_; @@ -362,6 +395,73 @@ TEST_F(ArcMetricsServiceTest, UserInteractionObserver) { service()->RemoveUserInteractionObserver(&observer); } +TEST_F(ArcMetricsServiceTest, ArcAnr) { + base::HistogramTester tester; + std::map expectation; + + service()->ReportAnr( + GetAnr(mojom::AnrSource::OTHER, mojom::AnrType::UNKNOWN)); + expectation[CreateAnrKey(kAppOverall, mojom::AnrType::UNKNOWN)] = 1; + expectation[CreateAnrKey(kAppTypeOther, mojom::AnrType::UNKNOWN)] = 1; + VerifyAnr(tester, expectation); + + service()->ReportAnr( + GetAnr(mojom::AnrSource::SYSTEM_SERVER, mojom::AnrType::INPUT)); + expectation[CreateAnrKey(kAppOverall, mojom::AnrType::INPUT)] = 1; + expectation[CreateAnrKey(kAppTypeSystemServer, mojom::AnrType::INPUT)] = 1; + VerifyAnr(tester, expectation); + + service()->ReportAnr(GetAnr(mojom::AnrSource::SYSTEM_SERVER, + mojom::AnrType::FOREGROUND_SERVICE)); + expectation[CreateAnrKey(kAppOverall, mojom::AnrType::FOREGROUND_SERVICE)] = + 1; + expectation[CreateAnrKey(kAppTypeSystemServer, + mojom::AnrType::FOREGROUND_SERVICE)] = 1; + VerifyAnr(tester, expectation); + + service()->ReportAnr(GetAnr(mojom::AnrSource::SYSTEM_SERVER, + mojom::AnrType::BACKGROUND_SERVICE)); + expectation[CreateAnrKey(kAppOverall, mojom::AnrType::BACKGROUND_SERVICE)] = + 1; + expectation[CreateAnrKey(kAppTypeSystemServer, + mojom::AnrType::BACKGROUND_SERVICE)] = 1; + VerifyAnr(tester, expectation); + + service()->ReportAnr( + GetAnr(mojom::AnrSource::GMS_CORE, mojom::AnrType::BROADCAST)); + expectation[CreateAnrKey(kAppOverall, mojom::AnrType::BROADCAST)] = 1; + expectation[CreateAnrKey(kAppTypeGmsCore, mojom::AnrType::BROADCAST)] = 1; + VerifyAnr(tester, expectation); + + service()->ReportAnr( + GetAnr(mojom::AnrSource::PLAY_STORE, mojom::AnrType::CONTENT_PROVIDER)); + expectation[CreateAnrKey(kAppOverall, mojom::AnrType::CONTENT_PROVIDER)] = 1; + expectation[CreateAnrKey(kAppTypePlayStore, + mojom::AnrType::CONTENT_PROVIDER)] = 1; + VerifyAnr(tester, expectation); + + service()->ReportAnr( + GetAnr(mojom::AnrSource::FIRST_PARTY, mojom::AnrType::APP_REQUESTED)); + expectation[CreateAnrKey(kAppOverall, mojom::AnrType::APP_REQUESTED)] = 1; + expectation[CreateAnrKey(kAppTypeFirstParty, mojom::AnrType::APP_REQUESTED)] = + 1; + VerifyAnr(tester, expectation); + + service()->ReportAnr( + GetAnr(mojom::AnrSource::ARC_OTHER, mojom::AnrType::INPUT)); + expectation[CreateAnrKey(kAppOverall, mojom::AnrType::INPUT)] = 2; + expectation[CreateAnrKey(kAppTypeArcOther, mojom::AnrType::INPUT)] = 1; + VerifyAnr(tester, expectation); + + service()->ReportAnr(GetAnr(mojom::AnrSource::ARC_APP_LAUNCHER, + mojom::AnrType::FOREGROUND_SERVICE)); + expectation[CreateAnrKey(kAppOverall, mojom::AnrType::FOREGROUND_SERVICE)] = + 2; + expectation[CreateAnrKey(kAppTypeArcAppLauncher, + mojom::AnrType::FOREGROUND_SERVICE)] = 1; + VerifyAnr(tester, expectation); +} + TEST_F(ArcMetricsServiceTest, AppLowMemoryKills) { base::HistogramTester tester; service()->RequestLowMemoryKillCountsForTesting(); diff --git a/ash/components/arc/test/test_browser_context.h b/ash/components/arc/test/test_browser_context.h index 1b78d73eadd89c..fce99e8b7fc578 100644 --- a/ash/components/arc/test/test_browser_context.h +++ b/ash/components/arc/test/test_browser_context.h @@ -23,8 +23,7 @@ class TestBrowserContext : public content::TestBrowserContext { ~TestBrowserContext() override; - PrefService* prefs() { return &prefs_; } - PrefRegistrySimple* pref_registry() { return prefs_.registry(); } + inline PrefRegistrySimple* pref_registry() { return prefs_.registry(); } private: BrowserContextDependencyManager* const browser_context_dependency_manager_; diff --git a/chrome/browser/ash/arc/metrics/arc_metrics_service_proxy.cc b/chrome/browser/ash/arc/metrics/arc_metrics_service_proxy.cc index 26ce474b9b7154..6099feb8215321 100644 --- a/chrome/browser/ash/arc/metrics/arc_metrics_service_proxy.cc +++ b/chrome/browser/ash/arc/metrics/arc_metrics_service_proxy.cc @@ -10,7 +10,6 @@ #include "chrome/browser/ash/arc/arc_util.h" #include "chrome/browser/ash/arc/session/arc_session_manager.h" #include "chrome/browser/memory/memory_kills_monitor.h" -#include "chrome/browser/profiles/profile.h" #include "chrome/browser/profiles/profile_manager.h" #include "chrome/browser/ui/app_list/arc/arc_app_list_prefs_factory.h" @@ -56,8 +55,6 @@ ArcMetricsServiceProxy::ArcMetricsServiceProxy( arc_app_list_prefs_->AddObserver(this); arc::ArcSessionManager::Get()->AddObserver(this); arc_metrics_service_->AddAppKillObserver(this); - arc_metrics_service_->set_prefs( - Profile::FromBrowserContext(context)->GetPrefs()); } void ArcMetricsServiceProxy::Shutdown() { @@ -78,10 +75,6 @@ void ArcMetricsServiceProxy::OnTaskDestroyed(int32_t task_id) { arc_metrics_service_->OnTaskDestroyed(task_id); } -void ArcMetricsServiceProxy::OnArcStarted() { - arc_metrics_service_->OnArcStarted(); -} - void ArcMetricsServiceProxy::OnArcSessionStopped(ArcStopReason stop_reason) { const auto* profile = ProfileManager::GetPrimaryUserProfile(); if (arc::IsArcAllowedForProfile(profile)) { @@ -92,7 +85,6 @@ void ArcMetricsServiceProxy::OnArcSessionStopped(ArcStopReason stop_reason) { VLOG(1) << metric_name << ": " << static_cast>(stop_reason); } - arc_metrics_service_->OnArcSessionStopped(); } void ArcMetricsServiceProxy::OnArcLowMemoryKill() { diff --git a/chrome/browser/ash/arc/metrics/arc_metrics_service_proxy.h b/chrome/browser/ash/arc/metrics/arc_metrics_service_proxy.h index 1e235108fcc53a..40d363627d40f8 100644 --- a/chrome/browser/ash/arc/metrics/arc_metrics_service_proxy.h +++ b/chrome/browser/ash/arc/metrics/arc_metrics_service_proxy.h @@ -49,7 +49,6 @@ class ArcMetricsServiceProxy : public KeyedService, void OnTaskDestroyed(int32_t task_id) override; // ArcSessionManagerObserver overrides. - void OnArcStarted() override; void OnArcSessionStopped(ArcStopReason stop_reason) override; // ArcMetricsService::AppKillObserver overrides. diff --git a/tools/metrics/histograms/metadata/arc/histograms.xml b/tools/metrics/histograms/metadata/arc/histograms.xml index d62ad33c296974..a878405b6fc798 100644 --- a/tools/metrics/histograms/metadata/arc/histograms.xml +++ b/tools/metrics/histograms/metadata/arc/histograms.xml @@ -179,26 +179,9 @@ chromium-metrics-reviews@google.com. The time elapsed for booting up the ARC instance. - + khmel@google.com alanding@google.com - arc-performance@google.com - - ANR rate separated by {AnrPeriod} as ANR count happened in this period. - - - - - - - - - khmel@google.com - alanding@google.com - arc-performance@google.com Counts ANR events in the system separated by ANR type and {AnrSource}.