Send UMA stability stats in a separate UMA log on startup.

chrome/browser/metrics/metrics_service.h

Index: chrome/browser/metrics/metrics_service.h
===================================================================
--- chrome/browser/metrics/metrics_service.h	(revision 240327)
+++ chrome/browser/metrics/metrics_service.h	(working copy)
@@ -111,9 +111,21 @@
     SHUTDOWN_COMPLETE = 700,
   };
 
+  enum ReportingState {

[jar (doing other things), 2013/12/12 17:34:34]

nit: curious question:  Why did you decide to use an enum, when a bool
seem(s)(ed) like a perfect match?  I usually turn to an enum if/when I find a
function has a bunch of bools as arguments, and is getting unwieldy.  If not,
bool is often more readable etc.

[Alexei Svitkine (slow), 2013/12/12 18:34:54]

I went with a bool initially, but Ilya preferred an enum (so that callsites are
more descriptive).

+    REPORTING_ENABLED,
+    REPORTING_DISABLED,
+  };
+
   MetricsService();
   virtual ~MetricsService();
 
+  // Initializes metrics recording state. Updates various bookkeeping values in
+  // prefs and sets up the scheduler. This is a separate function rather than
+  // being done by the constructor so that field trials could be created before
+  // this is run. Takes |reporting_state| parameter which specifies whether UMA
+  // is enabled.
+  void InitializeMetricsRecordingState(ReportingState reporting_state);
+
   // Starts the metrics system, turning on recording and uploading of metrics.
   // Should be called when starting up with metrics enabled, or when metrics
   // are turned on.
@@ -153,7 +165,7 @@
   // because this method may need to be called before the MetricsService needs
   // to be started.
   scoped_ptr<const base::FieldTrial::EntropyProvider> CreateEntropyProvider(
-      bool reporting_will_be_enabled);
+      ReportingState reporting_state);
 
   // Force the client ID to be generated. This is useful in case it's needed
   // before recording.
@@ -217,10 +229,6 @@
   void CountBrowserCrashDumpAttempts();
 #endif  // OS_WIN
 
-  // Save any unsent logs into a persistent store in a pref.  We always do this
-  // at shutdown, but we can do it as we reduce the list as well.
-  void StoreUnsentLogs();
-
 #if defined(OS_CHROMEOS)
   // Start the external metrics service, which collects metrics from Chrome OS
   // and passes them to UMA.
@@ -253,12 +261,14 @@
   // The MetricsService has a lifecycle that is stored as a state.
   // See metrics_service.cc for description of this lifecycle.
   enum State {
-    INITIALIZED,            // Constructor was called.
-    INIT_TASK_SCHEDULED,    // Waiting for deferred init tasks to complete.
-    INIT_TASK_DONE,         // Waiting for timer to send initial log.
-    INITIAL_LOG_READY,      // Initial log generated, and waiting for reply.
-    SENDING_OLD_LOGS,       // Sending unsent logs from previous session.
-    SENDING_CURRENT_LOGS,   // Sending standard current logs as they acrue.
+    INITIALIZED,                    // Constructor was called.
+    INIT_TASK_SCHEDULED,            // Waiting for deferred init tasks to
+                                    // complete.
+    INIT_TASK_DONE,                 // Waiting for timer to send initial log.
+    SENDING_INITIAL_STABILITY_LOG,  // Initial stability log being sent.
+    SENDING_INITIAL_METRICS_LOG,    // Initial metrics log being sent.
+    SENDING_OLD_LOGS,               // Sending unsent logs from last session.
+    SENDING_CURRENT_LOGS,           // Sending ongoing logs as they accrue.
   };
 
   enum ShutdownCleanliness {
@@ -349,7 +359,7 @@
   void HandleIdleSinceLastTransmission(bool in_idle);
 
   // Set up client ID, session ID, etc.
-  void InitializeMetricsState();
+  void InitializeMetricsState(ReportingState reporting_state);
 
   // Generates a new client ID to use to identify self to metrics server.
   static std::string GenerateClientID();
@@ -393,9 +403,16 @@
   // (depending on |state_|), and stages it for upload.
   void StageNewLog();
 
-  // Record stats, client ID, Session ID, etc. in a special "first" log.
-  void PrepareInitialLog();
+  // Prepares the initial stability log, which is only logged when the previous
+  // run of Chrome crashed.  This log contains any stability metrics left over

[jar (doing other things), 2013/12/12 17:34:34]

nit: consider:
"...run of chrome crashed." --> ...run of Chrome has some crash(es) to report."

I'm assuming that you send a stability log if you have browser or renderer
crashes, and not merely browser crashes.

...but reading the code, it seems slightly different from either choice.  Can
you clear up the confusion (for me)?

[Alexei Svitkine (slow), 2013/12/12 18:34:54]

I addressed this more in another comment (specifically "have any crashes" vs.
"crashed last session"), but let me discuss specifically the case of renderer
(and other process crashes) here.

On a clean shutdown, MetricsService will produce a final UMA log that will be
persisted with the stats since the last UMA log. So any renderer crashes (and
other process crashes) in such a case will already have been recorded to that
log (and cleared from prefs).

So the only there can be renderer crashes on the next start up would be in the
case of an unclean shutdown, which is consistent with what this code is doing
and what this comment is describing. (I've also done some queries of the logs
before deciding to go this way to ensure that the stats are consistent with the
above and did find they are - there were very very few renderer crashes reported
in the initial UMA log - though still some since in the current code the initial
log happens after 60 seconds, so it is still possible for a renderer to crash
before then.)

+  // from that previous run, and only these stability metrics.  It uses the
+  // system profile from the previous session.
+  void PrepareInitialStabilityLog();
 
+  // Prepares the initial metrics log, which includes startup histograms and
+  // profiler data, as well as incremental stability-related metrics.
+  void PrepareInitialMetricsLog(MetricsLog::LogType log_type);
+
   // Uploads the currently staged log (which must be non-null).
   void SendStagedLog();
 
@@ -471,10 +488,13 @@
   // be cut, and logs are neither persisted nor uploaded.
   bool test_mode_active_;
 
-  // The progession of states made by the browser are recorded in the following
+  // The progression of states made by the browser are recorded in the following
   // state.
   State state_;
 
+  // Whether the initial stability log has been recorded during startup.
+  bool has_initial_stability_log_;
+
   // Chrome OS hardware class (e.g., hardware qualification ID). This
   // class identifies the configured system components such as CPU,
   // WiFi adapter, etc.  For non Chrome OS hosts, this will be an
@@ -487,8 +507,10 @@
   // Google Update statistics, which were retrieved on a blocking pool thread.
   GoogleUpdateMetrics google_update_metrics_;
 
-  // The initial log, used to record startup metrics.
-  scoped_ptr<MetricsLog> initial_log_;
+  // The initial metrics log, used to record startup metrics (histograms and
+  // profiler data). Note that if a crash occurred in the previous session, an
+  // initial stability log may be sent before this.
+  scoped_ptr<MetricsLog> initial_metrics_log_;
 
   // The outstanding transmission appears as a URL Fetch operation.
   scoped_ptr<net::URLFetcher> current_fetch_;