Add ProvideInitialStabilityMetrics to metrics providers

ProvideStabilityMetrics has been separated in
ProvideInitialStabilityMetrics and ProvideStabilityMetrics.
The first one is only called for an initial stability logs.
The second one is called for all ongoing logs.

BUG=482716
R=asvitkine

Review URL: https://codereview.chromium.org/1117933002

Cr-Commit-Position: refs/heads/master@{#327742}

11 files changed, 154 insertions, 53 deletions

diff --git a/components/metrics.gypi b/components/metrics.gypi
index 45a36d1..d22f9f5 100644
--- a/components/metrics.gypi
+++ b/components/metrics.gypi
@@ -194,6 +194,8 @@
         'component_metrics_proto',
       ],
       'sources': [
+        'metrics/test_metrics_provider.cc',
+        'metrics/test_metrics_provider.h',
         'metrics/test_metrics_service_client.cc',
         'metrics/test_metrics_service_client.h',
       ],
diff --git a/components/metrics/BUILD.gn b/components/metrics/BUILD.gn
index 47dc5be..90e1c6a 100644
--- a/components/metrics/BUILD.gn
+++ b/components/metrics/BUILD.gn
@@ -138,6 +138,8 @@ source_set("profiler") {
 # GYP version: components/metrics.gypi:metrics_test_support
 source_set("test_support") {
   sources = [
+    "test_metrics_provider.cc",
+    "test_metrics_provider.h",
     "test_metrics_service_client.cc",
     "test_metrics_service_client.h",
   ]
diff --git a/components/metrics/metrics_log.cc b/components/metrics/metrics_log.cc
index 42dca83..f7b1287 100644
--- a/components/metrics/metrics_log.cc
+++ b/components/metrics/metrics_log.cc
@@ -198,8 +198,11 @@ void MetricsLog::RecordStabilityMetrics(
   WriteRealtimeStabilityAttributes(pref, incremental_uptime, uptime);
 
   SystemProfileProto* system_profile = uma_proto()->mutable_system_profile();
-  for (size_t i = 0; i < metrics_providers.size(); ++i)
+  for (size_t i = 0; i < metrics_providers.size(); ++i) {
+    if (log_type() == INITIAL_STABILITY_LOG)
+      metrics_providers[i]->ProvideInitialStabilityMetrics(system_profile);
     metrics_providers[i]->ProvideStabilityMetrics(system_profile);
+  }
 
   // Omit some stats unless this is the initial stability log.
   if (log_type() != INITIAL_STABILITY_LOG)
diff --git a/components/metrics/metrics_log_unittest.cc b/components/metrics/metrics_log_unittest.cc
index ee299d1..10663b3 100644
--- a/components/metrics/metrics_log_unittest.cc
+++ b/components/metrics/metrics_log_unittest.cc
@@ -8,6 +8,7 @@
 
 #include "base/base64.h"
 #include "base/basictypes.h"
+#include "base/memory/scoped_vector.h"
 #include "base/metrics/bucket_ranges.h"
 #include "base/metrics/sample_vector.h"
 #include "base/prefs/pref_service.h"
@@ -17,6 +18,7 @@
 #include "components/metrics/metrics_pref_names.h"
 #include "components/metrics/metrics_state_manager.h"
 #include "components/metrics/proto/chrome_user_metrics_extension.pb.h"
+#include "components/metrics/test_metrics_provider.h"
 #include "components/metrics/test_metrics_service_client.h"
 #include "components/variations/active_field_trials.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -332,11 +334,13 @@ TEST_F(MetricsLogTest, InitialLogStabilityMetrics) {
                      MetricsLog::INITIAL_STABILITY_LOG,
                      &client,
                      &prefs_);
-  std::vector<MetricsProvider*> metrics_providers;
-  log.RecordEnvironment(metrics_providers,
-                        std::vector<variations::ActiveGroupId>(),
-                        kInstallDate, kEnabledDate);
-  log.RecordStabilityMetrics(metrics_providers, base::TimeDelta(),
+  TestMetricsProvider* test_provider = new TestMetricsProvider();
+  ScopedVector<MetricsProvider> metrics_providers;
+  metrics_providers.push_back(test_provider);
+  log.RecordEnvironment(metrics_providers.get(),
+                        std::vector<variations::ActiveGroupId>(), kInstallDate,
+                        kEnabledDate);
+  log.RecordStabilityMetrics(metrics_providers.get(), base::TimeDelta(),
                              base::TimeDelta());
   const SystemProfileProto_Stability& stability =
       log.system_profile().stability();
@@ -349,17 +353,24 @@ TEST_F(MetricsLogTest, InitialLogStabilityMetrics) {
   EXPECT_TRUE(stability.has_breakpad_registration_failure_count());
   EXPECT_TRUE(stability.has_debugger_present_count());
   EXPECT_TRUE(stability.has_debugger_not_present_count());
+
+  // The test provider should have been called upon to provide initial
+  // stability and regular stability metrics.
+  EXPECT_TRUE(test_provider->provide_initial_stability_metrics_called());
+  EXPECT_TRUE(test_provider->provide_stability_metrics_called());
 }
 
 TEST_F(MetricsLogTest, OngoingLogStabilityMetrics) {
   TestMetricsServiceClient client;
   TestMetricsLog log(
       kClientId, kSessionId, MetricsLog::ONGOING_LOG, &client, &prefs_);
-  std::vector<MetricsProvider*> metrics_providers;
-  log.RecordEnvironment(metrics_providers,
-                        std::vector<variations::ActiveGroupId>(),
-                        kInstallDate, kEnabledDate);
-  log.RecordStabilityMetrics(metrics_providers, base::TimeDelta(),
+  TestMetricsProvider* test_provider = new TestMetricsProvider();
+  ScopedVector<MetricsProvider> metrics_providers;
+  metrics_providers.push_back(test_provider);
+  log.RecordEnvironment(metrics_providers.get(),
+                        std::vector<variations::ActiveGroupId>(), kInstallDate,
+                        kEnabledDate);
+  log.RecordStabilityMetrics(metrics_providers.get(), base::TimeDelta(),
                              base::TimeDelta());
   const SystemProfileProto_Stability& stability =
       log.system_profile().stability();
@@ -372,6 +383,11 @@ TEST_F(MetricsLogTest, OngoingLogStabilityMetrics) {
   EXPECT_FALSE(stability.has_breakpad_registration_failure_count());
   EXPECT_FALSE(stability.has_debugger_present_count());
   EXPECT_FALSE(stability.has_debugger_not_present_count());
+
+  // The test provider should have been called upon to provide regular but not
+  // initial stability metrics.
+  EXPECT_FALSE(test_provider->provide_initial_stability_metrics_called());
+  EXPECT_TRUE(test_provider->provide_stability_metrics_called());
 }
 
 TEST_F(MetricsLogTest, ChromeChannelWrittenToProtobuf) {
diff --git a/components/metrics/metrics_provider.cc b/components/metrics/metrics_provider.cc
index a4ac7eb..fb9f931 100644
--- a/components/metrics/metrics_provider.cc
+++ b/components/metrics/metrics_provider.cc
@@ -25,10 +25,14 @@ void MetricsProvider::ProvideSystemProfileMetrics(
     SystemProfileProto* system_profile_proto) {
 }
 
-bool MetricsProvider::HasStabilityMetrics() {
+bool MetricsProvider::HasInitialStabilityMetrics() {
   return false;
 }
 
+void MetricsProvider::ProvideInitialStabilityMetrics(
+    SystemProfileProto* system_profile_proto) {
+}
+
 void MetricsProvider::ProvideStabilityMetrics(
     SystemProfileProto* system_profile_proto) {
 }
diff --git a/components/metrics/metrics_provider.h b/components/metrics/metrics_provider.h
index 22afe14..226d1cd 100644
--- a/components/metrics/metrics_provider.h
+++ b/components/metrics/metrics_provider.h
@@ -33,9 +33,19 @@ class MetricsProvider {
   virtual void ProvideSystemProfileMetrics(
       SystemProfileProto* system_profile_proto);
 
-  // Called once at startup to see whether this provider has stability events
-  // to share. Default implementation always returns false.
-  virtual bool HasStabilityMetrics();
+  // Called once at startup to see whether this provider has critical stability
+  // events to share in an initial stability log.
+  // Returning true can trigger ProvideInitialStabilityMetrics and
+  // ProvideStabilityMetrics on all other registered metrics providers.
+  // Default implementation always returns false.
+  virtual bool HasInitialStabilityMetrics();
+
+  // Called at most once at startup when an initial stability log is created.
+  // It provides critical statiblity metrics that need to be reported in an
+  // initial stability log.
+  // Default implementation is a no-op.
+  virtual void ProvideInitialStabilityMetrics(
+      SystemProfileProto* system_profile_proto);
 
   // Provides additional stability metrics. Stability metrics can be provided
   // directly into |stability_proto| fields or by logging stability histograms
diff --git a/components/metrics/metrics_service.cc b/components/metrics/metrics_service.cc
index 9d04827..fe53489 100644
--- a/components/metrics/metrics_service.cc
+++ b/components/metrics/metrics_service.cc
@@ -561,7 +561,8 @@ void MetricsService::InitializeMetricsState() {
   }
 
   bool has_initial_stability_log = false;
-  if (!clean_exit_beacon_.exited_cleanly() || ProvidersHaveStabilityMetrics()) {
+  if (!clean_exit_beacon_.exited_cleanly() ||
+      ProvidersHaveInitialStabilityMetrics()) {
     // TODO(rtenneti): On windows, consider saving/getting execution_phase from
     // the registry.
     int execution_phase =
@@ -847,10 +848,10 @@ void MetricsService::SendNextLog() {
   SendStagedLog();
 }
 
-bool MetricsService::ProvidersHaveStabilityMetrics() {
-  // Check whether any metrics provider has stability metrics.
+bool MetricsService::ProvidersHaveInitialStabilityMetrics() {
+  // Check whether any metrics provider has initial stability metrics.
   for (size_t i = 0; i < metrics_providers_.size(); ++i) {
-    if (metrics_providers_[i]->HasStabilityMetrics())
+    if (metrics_providers_[i]->HasInitialStabilityMetrics())
       return true;
   }
 
diff --git a/components/metrics/metrics_service.h b/components/metrics/metrics_service.h
index 4f18599..ffbe831 100644
--- a/components/metrics/metrics_service.h
+++ b/components/metrics/metrics_service.h
@@ -344,9 +344,9 @@ class MetricsService : public base::HistogramFlattener {
   // the log manager, staging it if necessary.
   void SendNextLog();
 
-  // Returns true if any of the registered metrics providers have stability
-  // metrics to report.
-  bool ProvidersHaveStabilityMetrics();
+  // Returns true if any of the registered metrics providers have critical
+  // stability metrics to report in an initial stability log.
+  bool ProvidersHaveInitialStabilityMetrics();
 
   // Prepares the initial stability log, which is only logged when the previous
   // run of Chrome crashed.  This log contains any stability metrics left over
diff --git a/components/metrics/metrics_service_unittest.cc b/components/metrics/metrics_service_unittest.cc
index 81e8341..550b0ae 100644
--- a/components/metrics/metrics_service_unittest.cc
+++ b/components/metrics/metrics_service_unittest.cc
@@ -18,6 +18,7 @@
 #include "components/metrics/metrics_log.h"
 #include "components/metrics/metrics_pref_names.h"
 #include "components/metrics/metrics_state_manager.h"
+#include "components/metrics/test_metrics_provider.h"
 #include "components/metrics/test_metrics_service_client.h"
 #include "components/variations/metrics_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -33,31 +34,6 @@ scoped_ptr<ClientInfo> ReturnNoBackup() {
   return scoped_ptr<ClientInfo>();
 }
 
-class TestMetricsProvider : public MetricsProvider {
- public:
-  explicit TestMetricsProvider(bool has_stability_metrics) :
-      has_stability_metrics_(has_stability_metrics),
-      provide_stability_metrics_called_(false) {
-  }
-
-  bool HasStabilityMetrics() override { return has_stability_metrics_; }
-  void ProvideStabilityMetrics(
-      SystemProfileProto* system_profile_proto) override {
-    UMA_STABILITY_HISTOGRAM_ENUMERATION("TestMetricsProvider.Metric", 1, 2);
-    provide_stability_metrics_called_ = true;
-  }
-
-  bool provide_stability_metrics_called() const {
-    return provide_stability_metrics_called_;
-  }
-
- private:
-  bool has_stability_metrics_;
-  bool provide_stability_metrics_called_;
-
-  DISALLOW_COPY_AND_ASSIGN(TestMetricsProvider);
-};
-
 class TestMetricsService : public MetricsService {
  public:
   TestMetricsService(MetricsStateManager* state_manager,
@@ -193,7 +169,7 @@ TEST_F(MetricsServiceTest, InitialStabilityLogAfterCleanShutDown) {
   TestMetricsService service(
       GetMetricsStateManager(), &client, GetLocalState());
 
-  TestMetricsProvider* test_provider = new TestMetricsProvider(false);
+  TestMetricsProvider* test_provider = new TestMetricsProvider();
   service.RegisterMetricsProvider(scoped_ptr<MetricsProvider>(test_provider));
 
   service.InitializeMetricsRecordingState();
@@ -201,8 +177,9 @@ TEST_F(MetricsServiceTest, InitialStabilityLogAfterCleanShutDown) {
   EXPECT_FALSE(service.log_manager()->has_unsent_logs());
   EXPECT_FALSE(service.log_manager()->has_staged_log());
 
-  // The test provider should not have been called upon to provide stability
-  // metrics.
+  // The test provider should not have been called upon to provide initial
+  // stability nor regular stability metrics.
+  EXPECT_FALSE(test_provider->provide_initial_stability_metrics_called());
   EXPECT_FALSE(test_provider->provide_stability_metrics_called());
 }
 
@@ -230,7 +207,8 @@ TEST_F(MetricsServiceTest, InitialStabilityLogAtProviderRequest) {
   TestMetricsService service(
       GetMetricsStateManager(), &client, GetLocalState());
   // Add a metrics provider that requests a stability log.
-  TestMetricsProvider* test_provider = new TestMetricsProvider(true);
+  TestMetricsProvider* test_provider = new TestMetricsProvider();
+  test_provider->set_has_initial_stability_metrics(true);
   service.RegisterMetricsProvider(
       scoped_ptr<MetricsProvider>(test_provider));
 
@@ -241,8 +219,9 @@ TEST_F(MetricsServiceTest, InitialStabilityLogAtProviderRequest) {
   EXPECT_TRUE(log_manager->has_unsent_logs());
   EXPECT_FALSE(log_manager->has_staged_log());
 
-  // The test provider should have been called upon to provide stability
-  // metrics.
+  // The test provider should have been called upon to provide initial
+  // stability and regular stability metrics.
+  EXPECT_TRUE(test_provider->provide_initial_stability_metrics_called());
   EXPECT_TRUE(test_provider->provide_stability_metrics_called());
 
   // Stage the log and retrieve it.
@@ -292,6 +271,9 @@ TEST_F(MetricsServiceTest, InitialStabilityLogAfterCrash) {
 
   TestMetricsService service(
       GetMetricsStateManager(), &client, GetLocalState());
+  // Add a provider.
+  TestMetricsProvider* test_provider = new TestMetricsProvider();
+  service.RegisterMetricsProvider(scoped_ptr<MetricsProvider>(test_provider));
   service.InitializeMetricsRecordingState();
 
   // The initial stability log should be generated and persisted in unsent logs.
@@ -299,6 +281,11 @@ TEST_F(MetricsServiceTest, InitialStabilityLogAfterCrash) {
   EXPECT_TRUE(log_manager->has_unsent_logs());
   EXPECT_FALSE(log_manager->has_staged_log());
 
+  // The test provider should have been called upon to provide initial
+  // stability and regular stability metrics.
+  EXPECT_TRUE(test_provider->provide_initial_stability_metrics_called());
+  EXPECT_TRUE(test_provider->provide_stability_metrics_called());
+
   // Stage the log and retrieve it.
   log_manager->StageNextLogForUpload();
   EXPECT_TRUE(log_manager->has_staged_log());
diff --git a/components/metrics/test_metrics_provider.cc b/components/metrics/test_metrics_provider.cc
new file mode 100644
index 0000000..f91f71c
--- /dev/null
+++ b/components/metrics/test_metrics_provider.cc
@@ -0,0 +1,27 @@
+// Copyright 2015 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 "components/metrics/test_metrics_provider.h"
+
+#include "base/metrics/histogram_macros.h"
+
+namespace metrics {
+
+bool TestMetricsProvider::HasInitialStabilityMetrics() {
+  return has_initial_stability_metrics_;
+}
+
+void TestMetricsProvider::ProvideInitialStabilityMetrics(
+    SystemProfileProto* system_profile_proto) {
+  UMA_STABILITY_HISTOGRAM_ENUMERATION("TestMetricsProvider.Initial", 1, 2);
+  provide_initial_stability_metrics_called_ = true;
+}
+
+void TestMetricsProvider::ProvideStabilityMetrics(
+    SystemProfileProto* system_profile_proto) {
+  UMA_STABILITY_HISTOGRAM_ENUMERATION("TestMetricsProvider.Regular", 1, 2);
+  provide_stability_metrics_called_ = true;
+}
+
+}  // namespace metrics
diff --git a/components/metrics/test_metrics_provider.h b/components/metrics/test_metrics_provider.h
new file mode 100644
index 0000000..31cbce1
--- /dev/null
+++ b/components/metrics/test_metrics_provider.h
@@ -0,0 +1,49 @@
+// Copyright 2015 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 COMPONENTS_METRICS_TEST_METRICS_PROVIDER_H_
+#define COMPONENTS_METRICS_TEST_METRICS_PROVIDER_H_
+
+#include "base/macros.h"
+#include "components/metrics/metrics_provider.h"
+
+namespace metrics {
+
+// A simple implementation of MetricsProvider that checks that its providing
+// functions are called, for use in tests.
+class TestMetricsProvider : public MetricsProvider {
+ public:
+  TestMetricsProvider()
+      : has_initial_stability_metrics_(false),
+        provide_initial_stability_metrics_called_(false),
+        provide_stability_metrics_called_(false) {}
+
+  // MetricsProvider:
+  bool HasInitialStabilityMetrics() override;
+  void ProvideInitialStabilityMetrics(
+      SystemProfileProto* system_profile_proto) override;
+  void ProvideStabilityMetrics(
+      SystemProfileProto* system_profile_proto) override;
+
+  void set_has_initial_stability_metrics(bool has_initial_stability_metrics) {
+    has_initial_stability_metrics_ = has_initial_stability_metrics;
+  }
+  bool provide_initial_stability_metrics_called() const {
+    return provide_initial_stability_metrics_called_;
+  }
+  bool provide_stability_metrics_called() const {
+    return provide_stability_metrics_called_;
+  }
+
+ private:
+  bool has_initial_stability_metrics_;
+  bool provide_initial_stability_metrics_called_;
+  bool provide_stability_metrics_called_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestMetricsProvider);
+};
+
+}  // namespace metrics
+
+#endif  // COMPONENTS_METRICS_TEST_METRICS_PROVIDER_H_