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 238706)
+++ chrome/browser/metrics/metrics_service.h	(working copy)
@@ -114,6 +114,13 @@
   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_will_be_enabled| parameter which specifies
+  // whether UMA is enabled.
+  void InitializeMetricsRecordingState(bool reporting_will_be_enabled);

[Ilya Sherman, 2013/12/06 06:21:26]

nit: Please define an enum to use in place of the boolean, since the meaning of
"true" or "false" is currently not obvious at call sites.

[Ilya Sherman, 2013/12/06 06:21:26]

Why is this method public?  It doesn't look like you added any clients of this
code as part of this CL.

[Alexei Svitkine (slow), 2013/12/06 17:59:22]

Yes, that was my mistake - I forgot to include the corresponding changes to
chrome_browser_main.cc as part of the original CL - fixed now. The reason this
is public is so that it can be called after field trials are created (so that
its behavior can be based on field trial state).

[Alexei Svitkine (slow), 2013/12/06 17:59:22]

Done.

+
   // 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.
@@ -253,12 +260,13 @@
   // 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 task to finish.

[Ilya Sherman, 2013/12/06 06:21:26]

nit: "tasks" seems more accurate, since this can be waiting on any of a number
of steps.

[Alexei Svitkine (slow), 2013/12/06 17:59:22]

Unfortunately, it goes above the 80 col limit (if we want to keep the //'s
aligned). We could change the comments to be before each enum, WDYT?

[Ilya Sherman, 2013/12/07 06:51:53]

You can wrap the line like so:

    INIT_TASK_SCHEDULED,            // Waiting for deferred init tasks to
                                    // complete.

[Alexei Svitkine (slow), 2013/12/09 21:17:02]

Done.

+    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.

[Ilya Sherman, 2013/12/06 06:21:26]

Jim made a good point in a previous discussion that there might not be much
reason anymore to distinguish between the initial log and ongoing logs, other
than stability logs.  Is the plan to get rid of or otherwise rename this state
once the field trial is at 100%?

[Alexei Svitkine (slow), 2013/12/06 17:59:22]

The code already treats it as an ongoing log (semantically) when the field trial
is on. However, it is special still - for example it has profiler data and
doesn't have counts, so having a special name "initial metrics log" still makes
sense as the current code stands. However, long term I agree we should try to
make it similar to regular ongoing logs - but we'd probably want to figure out
how we want to log profiler data in the future before that happens, for example.
So I'd leave it as is.

[Ilya Sherman, 2013/12/07 06:51:53]

Ok.

+    SENDING_OLD_LOGS,               // Sending unsent logs from last session.
+    SENDING_CURRENT_LOGS,           // Sending ongoing logs as they accrue.
   };
 
   enum ShutdownCleanliness {
@@ -349,7 +357,7 @@
   void HandleIdleSinceLastTransmission(bool in_idle);
 
   // Set up client ID, session ID, etc.
-  void InitializeMetricsState();
+  void InitializeMetricsState(bool reporting_will_be_enabled);
 
   // Generates a new client ID to use to identify self to metrics server.
   static std::string GenerateClientID();
@@ -393,9 +401,14 @@
   // (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, based on the saved system profile from
+  // a previous session and metrics stability stats and stages it for upload.

[Ilya Sherman, 2013/12/06 06:21:26]

nit: "and metrics stability stats" -> rephrase somehow?  Maybe try to clarify
that this *only* uploads data coming from a previous session.

[Alexei Svitkine (slow), 2013/12/06 17:59:22]

Done.

+  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();
 
@@ -487,8 +500,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_;