Quantify initial stability report edge cases.

BUG=415982

Review-Url: https://codereview.chromium.org/2296543002
Cr-Commit-Position: refs/heads/master@{#417433}

base/metrics/histogram_macros.h

@@ -140,18 +140,25 @@
     LOCAL_HISTOGRAM_CUSTOM_COUNTS(name, sample, 1, 10000, 50)
 
 #define LOCAL_HISTOGRAM_CUSTOM_COUNTS(name, sample, min, max, bucket_count) \
-    STATIC_HISTOGRAM_POINTER_BLOCK(name, Add(sample), \
-        base::Histogram::FactoryGet(name, min, max, bucket_count, \
-                                    base::HistogramBase::kNoFlags))
+    INTERNAL_HISTOGRAM_CUSTOM_COUNTS_WITH_FLAG(                             \
+        name, sample, min, max, bucket_count, base::HistogramBase::kNoFlags)
+
+// This is a helper macro used by other macros and shouldn't be used directly.
+#define INTERNAL_HISTOGRAM_CUSTOM_COUNTS_WITH_FLAG(name, sample, min, max, \
+                                                   bucket_count, flag)     \
+    STATIC_HISTOGRAM_POINTER_BLOCK(                                        \
+        name, Add(sample),                                                 \
+        base::Histogram::FactoryGet(name, min, max, bucket_count, flag))
 
 // This is a helper macro used by other macros and shouldn't be used directly.
 // One additional bucket is created in the LinearHistogram for the illegal
 // values >= boundary_value so that mistakes in calling the UMA enumeration
 // macros can be detected.
-#define HISTOGRAM_ENUMERATION_WITH_FLAG(name, sample, boundary, flag) \
-    STATIC_HISTOGRAM_POINTER_BLOCK(name, Add(sample), \
-        base::LinearHistogram::FactoryGet(name, 1, boundary, boundary + 1, \
-            flag))
+#define INTERNAL_HISTOGRAM_ENUMERATION_WITH_FLAG(name, sample, boundary, flag) \
+    STATIC_HISTOGRAM_POINTER_BLOCK(                                            \
+        name, Add(sample),                                                     \
+        base::LinearHistogram::FactoryGet(                                     \
+            name, 1, boundary, boundary + 1, flag))
 
 #define LOCAL_HISTOGRAM_PERCENTAGE(name, under_one_hundred) \
     LOCAL_HISTOGRAM_ENUMERATION(name, under_one_hundred, 101)
@@ -229,9 +236,18 @@
     name, sample, 1, 10000, 50)
 
 #define UMA_HISTOGRAM_CUSTOM_COUNTS(name, sample, min, max, bucket_count) \
-    STATIC_HISTOGRAM_POINTER_BLOCK(name, Add(sample), \
-        base::Histogram::FactoryGet(name, min, max, bucket_count, \
-            base::HistogramBase::kUmaTargetedHistogramFlag))
+    INTERNAL_HISTOGRAM_CUSTOM_COUNTS_WITH_FLAG(                           \
+        name, sample, min, max, bucket_count,                             \
+        base::HistogramBase::kUmaTargetedHistogramFlag)
+
+#define UMA_STABILITY_HISTOGRAM_COUNTS_100(name, sample) \
+    UMA_STABILITY_HISTOGRAM_CUSTOM_COUNTS(name, sample, 1, 100, 50)
+
+#define UMA_STABILITY_HISTOGRAM_CUSTOM_COUNTS(name, sample, min, max, \
+                                              bucket_count)           \
+    INTERNAL_HISTOGRAM_CUSTOM_COUNTS_WITH_FLAG(                       \
+        name, sample, min, max, bucket_count,                         \
+        base::HistogramBase::kUmaStabilityHistogramFlag)
 
 #define UMA_HISTOGRAM_MEMORY_KB(name, sample) UMA_HISTOGRAM_CUSTOM_COUNTS( \
     name, sample, 1000, 500000, 50)
@@ -253,14 +269,16 @@
 // The samples should always be strictly less than |boundary_value|.  For more
 // details, see the comment for the |LOCAL_HISTOGRAM_ENUMERATION| macro, above.
 #define UMA_HISTOGRAM_ENUMERATION(name, sample, boundary_value) \
-    HISTOGRAM_ENUMERATION_WITH_FLAG(name, sample, boundary_value, \
+    INTERNAL_HISTOGRAM_ENUMERATION_WITH_FLAG(                   \
+        name, sample, boundary_value,                           \
         base::HistogramBase::kUmaTargetedHistogramFlag)
 
 // Similar to UMA_HISTOGRAM_ENUMERATION, but used for recording stability
 // histograms.  Use this if recording a histogram that should be part of the
 // initial stability log.
 #define UMA_STABILITY_HISTOGRAM_ENUMERATION(name, sample, boundary_value) \
-    HISTOGRAM_ENUMERATION_WITH_FLAG(name, sample, boundary_value, \
+    INTERNAL_HISTOGRAM_ENUMERATION_WITH_FLAG(                             \
+        name, sample, boundary_value,                                     \
         base::HistogramBase::kUmaStabilityHistogramFlag)
 
 #define UMA_HISTOGRAM_CUSTOM_ENUMERATION(name, sample, custom_ranges) \

components/metrics/metrics_log.cc

@@ -117,9 +117,9 @@ MetricsLog::~MetricsLog() {
 
 // static
 void MetricsLog::RegisterPrefs(PrefRegistrySimple* registry) {
-  registry->RegisterIntegerPref(prefs::kStabilityLaunchCount, 0);
   registry->RegisterIntegerPref(prefs::kStabilityCrashCount, 0);
   registry->RegisterIntegerPref(prefs::kStabilityIncompleteSessionEndCount, 0);
+  registry->RegisterIntegerPref(prefs::kStabilityLaunchCount, 0);
   registry->RegisterIntegerPref(prefs::kStabilityBreakpadRegistrationFail, 0);
   registry->RegisterIntegerPref(
       prefs::kStabilityBreakpadRegistrationSuccess, 0);
@@ -129,6 +129,9 @@ void MetricsLog::RegisterPrefs(PrefRegistrySimple* registry) {
                                std::string());
   registry->RegisterStringPref(prefs::kStabilitySavedSystemProfileHash,
                                std::string());
+  registry->RegisterIntegerPref(prefs::kStabilityDeferredCount, 0);
+  registry->RegisterIntegerPref(prefs::kStabilityDiscardCount, 0);
+  registry->RegisterIntegerPref(prefs::kStabilityVersionMismatchCount, 0);
 }
 
 // static
@@ -242,6 +245,31 @@ void MetricsLog::RecordStabilityMetrics(
     pref->SetInteger(prefs::kStabilityDebuggerNotPresent, 0);
     stability->set_debugger_not_present_count(debugger_not_present_count);
   }
+
+  // Note: only logging the following histograms for non-zero values.
+
+  int deferred_count = pref->GetInteger(prefs::kStabilityDeferredCount);
+  if (deferred_count) {
+    local_state_->SetInteger(prefs::kStabilityDeferredCount, 0);
+    UMA_STABILITY_HISTOGRAM_COUNTS_100(
+        "Stability.Internals.InitialStabilityLogDeferredCount", deferred_count);
+  }
+
+  int discard_count = local_state_->GetInteger(prefs::kStabilityDiscardCount);
+  if (discard_count) {
+    local_state_->SetInteger(prefs::kStabilityDiscardCount, 0);
+    UMA_STABILITY_HISTOGRAM_COUNTS_100("Stability.Internals.DataDiscardCount",
+                                       discard_count);
+  }
+
+  int version_mismatch_count =
+      local_state_->GetInteger(prefs::kStabilityVersionMismatchCount);
+  if (version_mismatch_count) {
+    local_state_->SetInteger(prefs::kStabilityVersionMismatchCount, 0);
+    UMA_STABILITY_HISTOGRAM_COUNTS_100(
+        "Stability.Internals.VersionMismatchCount",
+        version_mismatch_count);
+  }
 }
 
 void MetricsLog::RecordGeneralMetrics(
@@ -401,7 +429,10 @@ void MetricsLog::RecordEnvironment(
   }
 }
 
-bool MetricsLog::LoadSavedEnvironmentFromPrefs() {
+bool MetricsLog::LoadSavedEnvironmentFromPrefs(std::string* app_version) {
+  DCHECK(app_version);
+  app_version->clear();
+
   PrefService* local_state = local_state_;
   const std::string base64_system_profile =
       local_state->GetString(prefs::kStabilitySavedSystemProfile);
@@ -415,10 +446,14 @@ bool MetricsLog::LoadSavedEnvironmentFromPrefs() {
 
   SystemProfileProto* system_profile = uma_proto()->mutable_system_profile();
   std::string serialized_system_profile;
-  return base::Base64Decode(base64_system_profile,
-                            &serialized_system_profile) &&
-         ComputeSHA1(serialized_system_profile) == system_profile_hash &&
-         system_profile->ParseFromString(serialized_system_profile);
+
+  bool success =
+      base::Base64Decode(base64_system_profile, &serialized_system_profile) &&
+      ComputeSHA1(serialized_system_profile) == system_profile_hash &&
+      system_profile->ParseFromString(serialized_system_profile);
+  if (success)
+    *app_version = system_profile->app_version();
+  return success;
 }
 
 void MetricsLog::CloseLog() {

components/metrics/metrics_log.h

@@ -100,9 +100,11 @@ class MetricsLog {
       int64_t metrics_reporting_enabled_date);
 
   // Loads the environment proto that was saved by the last RecordEnvironment()
-  // call from prefs and clears the pref value. Returns true on success or false
-  // if there was no saved environment in prefs or it could not be decoded.
-  bool LoadSavedEnvironmentFromPrefs();
+  // call from prefs and clears the pref value. On success, returns true and
+  // |app_version| contains the recovered version. Otherwise (if there was no
+  // saved environment in prefs or it could not be decoded), returns false and
+  // |app_version| is empty.
+  bool LoadSavedEnvironmentFromPrefs(std::string* app_version);
 
   // Writes application stability metrics, including stability metrics provided
   // by the specified set of |metrics_providers|. The system profile portion of

components/metrics/metrics_log_unittest.cc

@@ -276,12 +276,15 @@ TEST_F(MetricsLogTest, LoadSavedEnvironmentFromPrefs) {
       prefs::kStabilitySavedSystemProfileHash;
 
   TestMetricsServiceClient client;
+  client.set_version_string("bogus version");
 
   // The pref value is empty, so loading it from prefs should fail.
   {
     TestMetricsLog log(
         kClientId, kSessionId, MetricsLog::ONGOING_LOG, &client, &prefs_);
-    EXPECT_FALSE(log.LoadSavedEnvironmentFromPrefs());
+    std::string app_version;
+    EXPECT_FALSE(log.LoadSavedEnvironmentFromPrefs(&app_version));
+    EXPECT_TRUE(app_version.empty());
   }
 
   // Do a RecordEnvironment() call and check whether the pref is recorded.
@@ -298,7 +301,9 @@ TEST_F(MetricsLogTest, LoadSavedEnvironmentFromPrefs) {
   {
     TestMetricsLog log(
         kClientId, kSessionId, MetricsLog::ONGOING_LOG, &client, &prefs_);
-    EXPECT_TRUE(log.LoadSavedEnvironmentFromPrefs());
+    std::string app_version;
+    EXPECT_TRUE(log.LoadSavedEnvironmentFromPrefs(&app_version));
+    EXPECT_EQ("bogus version", app_version);
     // Check some values in the system profile.
     EXPECT_EQ(kInstallDateExpected, log.system_profile().install_date());
     EXPECT_EQ(kEnabledDateExpected, log.system_profile().uma_enabled_date());
@@ -322,7 +327,9 @@ TEST_F(MetricsLogTest, LoadSavedEnvironmentFromPrefs) {
     prefs_.SetString(kSystemProfileHashPref, "deadbeef");
     TestMetricsLog log(
         kClientId, kSessionId, MetricsLog::ONGOING_LOG, &client, &prefs_);
-    EXPECT_FALSE(log.LoadSavedEnvironmentFromPrefs());
+    std::string app_version;
+    EXPECT_FALSE(log.LoadSavedEnvironmentFromPrefs(&app_version));
+    EXPECT_TRUE(app_version.empty());
     // Ensure that the prefs are cleared, even if the call failed.
     EXPECT_TRUE(prefs_.GetString(kSystemProfilePref).empty());
     EXPECT_TRUE(prefs_.GetString(kSystemProfileHashPref).empty());

components/metrics/metrics_pref_names.cc

@@ -86,6 +86,16 @@ const char kStabilityChildProcessCrashCount[] =
 const char kStabilityCrashCount[] =
     "user_experience_metrics.stability.crash_count";
 
+// Number of times the initial stability log upload was deferred to the next
+// startup.
+const char kStabilityDeferredCount[] =
+    "user_experience_metrics.stability.deferred_count";
+
+// Number of times stability data was discarded. This is accumulated since the
+// last report, even across versions.
+const char kStabilityDiscardCount[] =
+    "user_experience_metrics.stability.discard_count";
+
 // Number of times the browser has been run under a debugger.
 const char kStabilityDebuggerPresent[] =
     "user_experience_metrics.stability.debugger_present";
@@ -178,6 +188,11 @@ const char kStabilityStatsBuildTime[] =
 const char kStabilityStatsVersion[] =
     "user_experience_metrics.stability.stats_version";
 
+// Number of times the version number stored in prefs did not match the
+// serialized system profile version number.
+const char kStabilityVersionMismatchCount[] =
+    "user_experience_metrics.stability.version_mismatch_count";
+
 // The keys below are strictly increasing counters over the lifetime of
 // a chrome installation. They are (optionally) sent up to the uninstall
 // survey in the event of uninstallation.

components/metrics/metrics_pref_names.h

@@ -34,6 +34,8 @@ extern const char kStabilityChildProcessCrashCount[];
 extern const char kStabilityCrashCount[];
 extern const char kStabilityDebuggerPresent[];
 extern const char kStabilityDebuggerNotPresent[];
+extern const char kStabilityDeferredCount[];
+extern const char kStabilityDiscardCount[];
 extern const char kStabilityExecutionPhase[];
 extern const char kStabilityExtensionRendererCrashCount[];
 extern const char kStabilityExtensionRendererFailedLaunchCount[];
@@ -53,6 +55,7 @@ extern const char kStabilitySavedSystemProfileHash[];
 extern const char kStabilitySessionEndCompleted[];
 extern const char kStabilityStatsBuildTime[];
 extern const char kStabilityStatsVersion[];
+extern const char kStabilityVersionMismatchCount[];
 extern const char kUninstallLaunchCount[];
 extern const char kUninstallMetricsPageLoadCount[];
 extern const char kUninstallMetricsUptimeSec[];

components/metrics/metrics_service.cc

@@ -533,6 +533,10 @@ void MetricsService::ClearSavedStabilityMetrics() {
   local_state_->SetInteger(prefs::kStabilityIncompleteSessionEndCount, 0);
   local_state_->SetInteger(prefs::kStabilityLaunchCount, 0);
   local_state_->SetBoolean(prefs::kStabilitySessionEndCompleted, true);
+  local_state_->SetInteger(prefs::kStabilityDeferredCount, 0);
+  // Note: kStabilityDiscardCount is not cleared as its intent is to measure
+  // the number of times data is discarded, even across versions.
+  local_state_->SetInteger(prefs::kStabilityVersionMismatchCount, 0);
 }
 
 void MetricsService::PushExternalLog(const std::string& log) {
@@ -566,8 +570,11 @@ void MetricsService::InitializeMetricsState() {
   const int64_t buildtime = MetricsLog::GetBuildTime();
   const std::string version = client_->GetVersionString();
   bool version_changed = false;
-  if (local_state_->GetInt64(prefs::kStabilityStatsBuildTime) != buildtime ||
-      local_state_->GetString(prefs::kStabilityStatsVersion) != version) {
+  int64_t previous_buildtime =
+      local_state_->GetInt64(prefs::kStabilityStatsBuildTime);
+  std::string previous_version =
+      local_state_->GetString(prefs::kStabilityStatsVersion);
+  if (previous_buildtime != buildtime || previous_version != version) {
     local_state_->SetString(prefs::kStabilityStatsVersion, version);
     local_state_->SetInt64(prefs::kStabilityStatsBuildTime, buildtime);
     version_changed = true;
@@ -599,8 +606,11 @@ void MetricsService::InitializeMetricsState() {
     // If the previous session didn't exit cleanly, or if any provider
     // explicitly requests it, prepare an initial stability log -
     // provided UMA is enabled.
-    if (state_manager_->IsMetricsReportingEnabled())
-      has_initial_stability_log = PrepareInitialStabilityLog();
+    if (state_manager_->IsMetricsReportingEnabled()) {
+      has_initial_stability_log = PrepareInitialStabilityLog(previous_version);
+      if (!has_initial_stability_log)
+        IncrementPrefValue(prefs::kStabilityDeferredCount);
+    }
   }
 
   // If no initial stability log was generated and there was a version upgrade,
@@ -609,8 +619,10 @@ void MetricsService::InitializeMetricsState() {
   // number of different edge cases, such as if the last version crashed before
   // it could save off a system profile or if UMA reporting is disabled (which
   // normally results in stats being accumulated).
-  if (!has_initial_stability_log && version_changed)
+  if (!has_initial_stability_log && version_changed) {
     ClearSavedStabilityMetrics();
+    IncrementPrefValue(prefs::kStabilityDiscardCount);
+  }
 
   // Update session ID.
   ++session_id_;
@@ -906,7 +918,8 @@ bool MetricsService::ProvidersHaveInitialStabilityMetrics() {
   return false;
 }
 
-bool MetricsService::PrepareInitialStabilityLog() {
+bool MetricsService::PrepareInitialStabilityLog(
+    const std::string& prefs_previous_version) {
   DCHECK_EQ(INITIALIZED, state_);
 
   std::unique_ptr<MetricsLog> initial_stability_log(
@@ -914,9 +927,13 @@ bool MetricsService::PrepareInitialStabilityLog() {
 
   // Do not call NotifyOnDidCreateMetricsLog here because the stability
   // log describes stats from the _previous_ session.
-
-  if (!initial_stability_log->LoadSavedEnvironmentFromPrefs())
+  std::string system_profile_app_version;
+  if (!initial_stability_log->LoadSavedEnvironmentFromPrefs(
+          &system_profile_app_version)) {
     return false;
+  }
+  if (system_profile_app_version != prefs_previous_version)
+    IncrementPrefValue(prefs::kStabilityVersionMismatchCount);
 
   log_manager_.PauseCurrentLog();
   log_manager_.BeginLoggingWithLog(std::move(initial_stability_log));

components/metrics/metrics_service.h

@@ -352,9 +352,10 @@ class MetricsService : public base::HistogramFlattener {
   // Prepares the initial stability log, which is only logged when the previous
   // run of Chrome crashed.  This log contains any stability metrics left over
   // from that previous run, and only these stability metrics.  It uses the
-  // system profile from the previous session.  Returns true if a log was
-  // created.
-  bool PrepareInitialStabilityLog();
+  // system profile from the previous session.  |prefs_previous_version| is used
+  // to validate the version number recovered from the system profile.  Returns
+  // true if a log was created.
+  bool PrepareInitialStabilityLog(const std::string& prefs_previous_version);
 
   // Prepares the initial metrics log, which includes startup histograms and
   // profiler data, as well as incremental stability-related metrics.

tools/metrics/histograms/histograms.xml

@@ -57743,6 +57743,34 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   </summary>
 </histogram>
 
+<histogram name="Stability.Internals.DataDiscardCount" units="counts">
+  <owner>manzagop@chromium.org</owner>
+  <summary>
+    Number of times stability data was discarded. This is accumulated since the
+    last report, even across versions. This is logged during stability metric
+    recording for the following log sent.
+  </summary>
+</histogram>
+
+<histogram name="Stability.Internals.InitialStabilityLogDeferredCount"
+    units="counts">
+  <owner>manzagop@chromium.org</owner>
+  <summary>
+    Number of times the initial stability log upload was deferred to the next
+    startup. This is logged during stability metric recording for the following
+    log sent.
+  </summary>
+</histogram>
+
+<histogram name="Stability.Internals.VersionMismatchCount" units="counts">
+  <owner>manzagop@chromium.org</owner>
+  <summary>
+    Number of times the version number stored in prefs did not match the
+    serialized system profile version number. This is logged during stability
+    metric recording.
+  </summary>
+</histogram>
+
 <histogram name="Stability.MobileSessionShutdownType"
     enum="MobileSessionShutdownType">
   <owner>lpromero@chromium.org</owner>