chromeos: Add Power.LidClosedSuspendCountDaily histogram.

Add a new histogram metric tracking the number of daily
suspends triggered by Chromebooks' lids being closed.

Bug: 782953
Change-Id: I86540cec8b677823b00d79d8699dc005c238ab1d
Reviewed-on: https://chromium-review.googlesource.com/780427
Commit-Queue: Dan Erat <derat@chromium.org>
Reviewed-by: Michael Giuffrida <michaelpg@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#518447}

chrome/browser/chromeos/power/power_metrics_reporter.cc

@@ -4,7 +4,7 @@
 
 #include "chrome/browser/chromeos/power/power_metrics_reporter.h"
 
-#include "base/metrics/histogram_macros.h"
+#include "base/metrics/histogram_functions.h"
 #include "chrome/common/pref_names.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/power_manager/suspend.pb.h"
@@ -18,6 +18,33 @@ namespace {
 constexpr base::TimeDelta kCheckDailyEventInternal =
     base::TimeDelta::FromSeconds(60);
 
+// Information about a daily count that should be tracked and reported.
+struct DailyCountInfo {
+  // Name of the local store pref backing the count across Chrome restarts.
+  const char* pref_name;
+  // Histogram metric name used to report the count.
+  const char* metric_name;
+};
+
+// Registry of all daily counts.
+const DailyCountInfo kDailyCounts[] = {
+    {
+        prefs::kPowerMetricsIdleScreenDimCount,
+        PowerMetricsReporter::kIdleScreenDimCountName,
+    },
+    {
+        prefs::kPowerMetricsIdleScreenOffCount,
+        PowerMetricsReporter::kIdleScreenOffCountName,
+    },
+    {
+        prefs::kPowerMetricsIdleSuspendCount,
+        PowerMetricsReporter::kIdleSuspendCountName,
+    },
+    {
+        prefs::kPowerMetricsLidClosedSuspendCount,
+        PowerMetricsReporter::kLidClosedSuspendCountName,
+    }};
+
 }  // namespace
 
 // This shim class is needed since metrics::DailyEvent requires taking ownership
@@ -48,14 +75,15 @@ const char PowerMetricsReporter::kIdleScreenOffCountName[] =
     "Power.IdleScreenOffCountDaily";
 const char PowerMetricsReporter::kIdleSuspendCountName[] =
     "Power.IdleSuspendCountDaily";
+const char PowerMetricsReporter::kLidClosedSuspendCountName[] =
+    "Power.LidClosedSuspendCountDaily";
 
 // static
 void PowerMetricsReporter::RegisterLocalStatePrefs(
     PrefRegistrySimple* registry) {
   metrics::DailyEvent::RegisterPref(registry, prefs::kPowerMetricsDailySample);
-  registry->RegisterIntegerPref(prefs::kPowerMetricsIdleScreenDimCount, 0);
-  registry->RegisterIntegerPref(prefs::kPowerMetricsIdleScreenOffCount, 0);
-  registry->RegisterIntegerPref(prefs::kPowerMetricsIdleSuspendCount, 0);
+  for (size_t i = 0; i < arraysize(kDailyCounts); ++i)
+    registry->RegisterIntegerPref(kDailyCounts[i].pref_name, 0);
 }
 
 PowerMetricsReporter::PowerMetricsReporter(
@@ -66,13 +94,11 @@ PowerMetricsReporter::PowerMetricsReporter(
       daily_event_(
           std::make_unique<metrics::DailyEvent>(pref_service_,
                                                 prefs::kPowerMetricsDailySample,
-                                                kDailyEventIntervalName)),
-      idle_screen_dim_count_(
-          pref_service_->GetInteger(prefs::kPowerMetricsIdleScreenDimCount)),
-      idle_screen_off_count_(
-          pref_service_->GetInteger(prefs::kPowerMetricsIdleScreenOffCount)),
-      idle_suspend_count_(
-          pref_service_->GetInteger(prefs::kPowerMetricsIdleSuspendCount)) {
+                                                kDailyEventIntervalName)) {
+  for (size_t i = 0; i < arraysize(kDailyCounts); ++i) {
+    daily_counts_[kDailyCounts[i].pref_name] =
+        pref_service_->GetInteger(kDailyCounts[i].pref_name);
+  }
   power_manager_client_->AddObserver(this);
 
   daily_event_->AddObserver(std::make_unique<DailyEventObserver>(this));
@@ -88,17 +114,25 @@ PowerMetricsReporter::~PowerMetricsReporter() {
 void PowerMetricsReporter::ScreenIdleStateChanged(
     const power_manager::ScreenIdleState& state) {
   if (state.dimmed() && !old_screen_idle_state_.dimmed())
-    SetIdleScreenDimCount(idle_screen_dim_count_ + 1);
+    AddToCount(prefs::kPowerMetricsIdleScreenDimCount, 1);
   if (state.off() && !old_screen_idle_state_.off())
-    SetIdleScreenOffCount(idle_screen_off_count_ + 1);
+    AddToCount(prefs::kPowerMetricsIdleScreenOffCount, 1);
 
   old_screen_idle_state_ = state;
 }
 
 void PowerMetricsReporter::SuspendImminent(
     power_manager::SuspendImminent::Reason reason) {
-  if (reason == power_manager::SuspendImminent_Reason_IDLE)
-    SetIdleSuspendCount(idle_suspend_count_ + 1);
+  switch (reason) {
+    case power_manager::SuspendImminent_Reason_IDLE:
+      AddToCount(prefs::kPowerMetricsIdleSuspendCount, 1);
+      break;
+    case power_manager::SuspendImminent_Reason_LID_CLOSED:
+      AddToCount(prefs::kPowerMetricsLidClosedSuspendCount, 1);
+      break;
+    case power_manager::SuspendImminent_Reason_OTHER:
+      break;
+  }
 }
 
 void PowerMetricsReporter::SuspendDone(const base::TimeDelta& duration) {
@@ -109,29 +143,23 @@ void PowerMetricsReporter::ReportDailyMetrics(
     metrics::DailyEvent::IntervalType type) {
   // Don't send metrics on first run or if the clock is changed.
   if (type == metrics::DailyEvent::IntervalType::DAY_ELAPSED) {
-    UMA_HISTOGRAM_COUNTS_100(kIdleScreenDimCountName, idle_screen_dim_count_);
-    UMA_HISTOGRAM_COUNTS_100(kIdleScreenOffCountName, idle_screen_off_count_);
-    UMA_HISTOGRAM_COUNTS_100(kIdleSuspendCountName, idle_suspend_count_);
+    for (size_t i = 0; i < arraysize(kDailyCounts); ++i) {
+      base::UmaHistogramCounts100(kDailyCounts[i].metric_name,
+                                  daily_counts_[kDailyCounts[i].pref_name]);
+    }
   }
 
-  SetIdleScreenDimCount(0);
-  SetIdleScreenOffCount(0);
-  SetIdleSuspendCount(0);
-}
-
-void PowerMetricsReporter::SetIdleScreenDimCount(int count) {
-  idle_screen_dim_count_ = count;
-  pref_service_->SetInteger(prefs::kPowerMetricsIdleScreenDimCount, count);
-}
-
-void PowerMetricsReporter::SetIdleScreenOffCount(int count) {
-  idle_screen_off_count_ = count;
-  pref_service_->SetInteger(prefs::kPowerMetricsIdleScreenOffCount, count);
+  // Reset all counts now that they've been reported.
+  for (size_t i = 0; i < arraysize(kDailyCounts); ++i) {
+    const char* pref_name = kDailyCounts[i].pref_name;
+    AddToCount(pref_name, -1 * daily_counts_[pref_name]);
+  }
 }
 
-void PowerMetricsReporter::SetIdleSuspendCount(int count) {
-  idle_suspend_count_ = count;
-  pref_service_->SetInteger(prefs::kPowerMetricsIdleSuspendCount, count);
+void PowerMetricsReporter::AddToCount(const std::string& pref_name, int num) {
+  DCHECK(daily_counts_.count(pref_name));
+  daily_counts_[pref_name] += num;
+  pref_service_->SetInteger(pref_name, daily_counts_[pref_name]);
 }
 
 }  // namespace chromeos

chrome/browser/chromeos/power/power_metrics_reporter.h

@@ -5,7 +5,9 @@
 #ifndef CHROME_BROWSER_CHROMEOS_POWER_POWER_METRICS_REPORTER_H_
 #define CHROME_BROWSER_CHROMEOS_POWER_POWER_METRICS_REPORTER_H_
 
+#include <map>
 #include <memory>
+#include <string>
 
 #include "base/macros.h"
 #include "base/timer/timer.h"
@@ -27,6 +29,7 @@ class PowerMetricsReporter : public PowerManagerClient::Observer {
   static const char kIdleScreenDimCountName[];
   static const char kIdleScreenOffCountName[];
   static const char kIdleSuspendCountName[];
+  static const char kLidClosedSuspendCountName[];
 
   // Registers prefs used by PowerMetricsReporter in |registry|.
   static void RegisterLocalStatePrefs(PrefRegistrySimple* registry);
@@ -51,10 +54,9 @@ class PowerMetricsReporter : public PowerManagerClient::Observer {
   // |daily_event_|.
   void ReportDailyMetrics(metrics::DailyEvent::IntervalType type);
 
-  // Updates |*_count_| members and the corresponding prefs.
-  void SetIdleScreenDimCount(int count);
-  void SetIdleScreenOffCount(int count);
-  void SetIdleSuspendCount(int count);
+  // Adds |num| to |pref_name|'s count in |daily_counts_| and updates the
+  // corresponding pref.
+  void AddToCount(const std::string& pref_name, int num);
 
   PowerManagerClient* power_manager_client_;  // Not owned.
   PrefService* pref_service_;                 // Not owned.
@@ -67,10 +69,8 @@ class PowerMetricsReporter : public PowerManagerClient::Observer {
   // Last-received screen-idle state from powerd.
   power_manager::ScreenIdleState old_screen_idle_state_;
 
-  // Current counts of various inactivity-triggered power management events.
-  int idle_screen_dim_count_ = 0;
-  int idle_screen_off_count_ = 0;
-  int idle_suspend_count_ = 0;
+  // Map from local store pref name backing a daily count to the count itself.
+  std::map<std::string, int> daily_counts_;
 
   DISALLOW_COPY_AND_ASSIGN(PowerMetricsReporter);
 };

chrome/browser/chromeos/power/power_metrics_reporter_unittest.cc

@@ -59,19 +59,22 @@ class PowerMetricsReporterTest : public testing::Test {
   }
 
   // Instructs |reporter_| to report daily metrics due to the passage of a day
-  // and verifies that it reports one sample for each of |dim_count|,
-  // |off_count|, and |suspend_count|.
-  void TriggerDailyEventAndVerifyHistograms(int dim_count,
-                                            int off_count,
-                                            int suspend_count) {
+  // and verifies that it reports one sample with each of the passed values.
+  void TriggerDailyEventAndVerifyHistograms(int idle_dim_count,
+                                            int idle_off_count,
+                                            int idle_suspend_count,
+                                            int lid_closed_suspend_count) {
     base::HistogramTester histogram_tester;
     TriggerDailyEvent(metrics::DailyEvent::IntervalType::DAY_ELAPSED);
     histogram_tester.ExpectUniqueSample(
-        PowerMetricsReporter::kIdleScreenDimCountName, dim_count, 1);
+        PowerMetricsReporter::kIdleScreenDimCountName, idle_dim_count, 1);
     histogram_tester.ExpectUniqueSample(
-        PowerMetricsReporter::kIdleScreenOffCountName, off_count, 1);
+        PowerMetricsReporter::kIdleScreenOffCountName, idle_off_count, 1);
     histogram_tester.ExpectUniqueSample(
-        PowerMetricsReporter::kIdleSuspendCountName, suspend_count, 1);
+        PowerMetricsReporter::kIdleSuspendCountName, idle_suspend_count, 1);
+    histogram_tester.ExpectUniqueSample(
+        PowerMetricsReporter::kLidClosedSuspendCountName,
+        lid_closed_suspend_count, 1);
   }
 
   base::test::ScopedTaskEnvironment scoped_task_environment_;
@@ -91,7 +94,7 @@ TEST_F(PowerMetricsReporterTest, CountAndReportEvents) {
   SendNormalScreenIdleState();
   SendDimmedScreenIdleState();
   SendNormalScreenIdleState();
-  TriggerDailyEventAndVerifyHistograms(2, 1, 0);
+  TriggerDailyEventAndVerifyHistograms(2, 1, 0, 0);
 
   // The next day, report three dims, two screen-offs, and one idle suspend.
   SendDimmedScreenIdleState();
@@ -105,10 +108,16 @@ TEST_F(PowerMetricsReporterTest, CountAndReportEvents) {
   SendNormalScreenIdleState();
   SendDimmedScreenIdleState();
   SendNormalScreenIdleState();
-  TriggerDailyEventAndVerifyHistograms(3, 2, 1);
+  TriggerDailyEventAndVerifyHistograms(3, 2, 1, 0);
+
+  // The next day, report a single lid-closed suspend.
+  power_manager_client_.SendSuspendImminent(
+      power_manager::SuspendImminent_Reason_LID_CLOSED);
+  power_manager_client_.SendSuspendDone();
+  TriggerDailyEventAndVerifyHistograms(0, 0, 0, 1);
 
   // We should report zeros if a day passes without any events.
-  TriggerDailyEventAndVerifyHistograms(0, 0, 0);
+  TriggerDailyEventAndVerifyHistograms(0, 0, 0, 0);
 }
 
 TEST_F(PowerMetricsReporterTest, LoadInitialCountsFromPrefs) {
@@ -118,12 +127,12 @@ TEST_F(PowerMetricsReporterTest, LoadInitialCountsFromPrefs) {
   pref_service_.SetInteger(prefs::kPowerMetricsIdleScreenOffCount, 3);
   pref_service_.SetInteger(prefs::kPowerMetricsIdleSuspendCount, 2);
   ResetReporter();
-  TriggerDailyEventAndVerifyHistograms(5, 3, 2);
+  TriggerDailyEventAndVerifyHistograms(5, 3, 2, 0);
 
   // The previous report should've cleared the prefs, so a new reporter should
   // start out at zero.
   ResetReporter();
-  TriggerDailyEventAndVerifyHistograms(0, 0, 0);
+  TriggerDailyEventAndVerifyHistograms(0, 0, 0, 0);
 }
 
 TEST_F(PowerMetricsReporterTest, IgnoreUnchangedScreenIdleState) {
@@ -132,20 +141,15 @@ TEST_F(PowerMetricsReporterTest, IgnoreUnchangedScreenIdleState) {
   SendDimmedAndOffScreenIdleState();
   SendDimmedScreenIdleState();
   SendDimmedScreenIdleState();
-  TriggerDailyEventAndVerifyHistograms(1, 1, 0);
+  TriggerDailyEventAndVerifyHistograms(1, 1, 0, 0);
 }
 
 TEST_F(PowerMetricsReporterTest, IgnoreOtherSuspendReasons) {
-  // Non-idle-triggered suspends shouldn't be reported.
-  power_manager_client_.SendSuspendImminent(
-      power_manager::SuspendImminent_Reason_LID_CLOSED);
-  power_manager_client_.SendSuspendDone();
-  TriggerDailyEventAndVerifyHistograms(0, 0, 0);
-
+  // Suspends triggered for other reasons shouldn't be reported.
   power_manager_client_.SendSuspendImminent(
       power_manager::SuspendImminent_Reason_OTHER);
   power_manager_client_.SendSuspendDone();
-  TriggerDailyEventAndVerifyHistograms(0, 0, 0);
+  TriggerDailyEventAndVerifyHistograms(0, 0, 0, 0);
 }
 
 TEST_F(PowerMetricsReporterTest, IgnoreDailyEventFirstRun) {
@@ -156,6 +160,7 @@ TEST_F(PowerMetricsReporterTest, IgnoreDailyEventFirstRun) {
   tester.ExpectTotalCount(PowerMetricsReporter::kIdleScreenDimCountName, 0);
   tester.ExpectTotalCount(PowerMetricsReporter::kIdleScreenOffCountName, 0);
   tester.ExpectTotalCount(PowerMetricsReporter::kIdleSuspendCountName, 0);
+  tester.ExpectTotalCount(PowerMetricsReporter::kLidClosedSuspendCountName, 0);
 }
 
 TEST_F(PowerMetricsReporterTest, IgnoreDailyEventClockChanged) {
@@ -169,10 +174,11 @@ TEST_F(PowerMetricsReporterTest, IgnoreDailyEventClockChanged) {
   tester.ExpectTotalCount(PowerMetricsReporter::kIdleScreenDimCountName, 0);
   tester.ExpectTotalCount(PowerMetricsReporter::kIdleScreenOffCountName, 0);
   tester.ExpectTotalCount(PowerMetricsReporter::kIdleSuspendCountName, 0);
+  tester.ExpectTotalCount(PowerMetricsReporter::kLidClosedSuspendCountName, 0);
 
   // The existing stats should be cleared when the clock change notification is
   // received, so the next report should only contain zeros.
-  TriggerDailyEventAndVerifyHistograms(0, 0, 0);
+  TriggerDailyEventAndVerifyHistograms(0, 0, 0, 0);
 }
 
 }  // namespace chromeos

chrome/common/pref_names.cc

@@ -1896,6 +1896,8 @@ const char kPowerMetricsIdleScreenDimCount[] =
 const char kPowerMetricsIdleScreenOffCount[] =
     "power.metrics.idle_screen_off_count";
 const char kPowerMetricsIdleSuspendCount[] = "power.metrics.idle_suspend_count";
+const char kPowerMetricsLidClosedSuspendCount[] =
+    "power.metrics.lid_closed_suspend_count";
 
 #endif  // defined(OS_CHROMEOS)
 

chrome/common/pref_names.h

@@ -660,6 +660,7 @@ extern const char kPowerMetricsDailySample[];
 extern const char kPowerMetricsIdleScreenDimCount[];
 extern const char kPowerMetricsIdleScreenOffCount[];
 extern const char kPowerMetricsIdleSuspendCount[];
+extern const char kPowerMetricsLidClosedSuspendCount[];
 #endif  // defined(OS_CHROMEOS)
 
 extern const char kClearPluginLSODataEnabled[];

tools/metrics/histograms/histograms.xml

@@ -63949,6 +63949,19 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   </summary>
 </histogram>
 
+<histogram name="Power.LidClosedSuspendCountDaily" units="count">
+  <owner>derat@chromium.org</owner>
+  <summary>
+    Number of times that that the system has suspended in response to its lid
+    being closed, reported daily. The count is accumulated through the day,
+    spanning reboots, and sent once the system clock indicates that a full day
+    or more has passed since the last report. If the system is suspended or off
+    for more than a day, the current count will be reported immediately the next
+    time the system boots, but the skipped days will not be reported. Chrome OS
+    only.
+  </summary>
+</histogram>
+
 <histogram name="Power.MetricsDailyEventInterval" enum="DailyEventIntervalType">
   <owner>derat@chromium.org</owner>
   <summary>