Added asynchronous notification of readiness to the StatisticsProvider, and

chrome/browser/metrics/metrics_service.cc

Index: chrome/browser/metrics/metrics_service.cc
diff --git a/chrome/browser/metrics/metrics_service.cc b/chrome/browser/metrics/metrics_service.cc
index 25d0af77d158de0edcca51d289e3e5d91efe5b60..b8c4f532aa8af7f404ef51e2a305a6828228d9aa 100644
--- a/chrome/browser/metrics/metrics_service.cc
+++ b/chrome/browser/metrics/metrics_service.cc
@@ -89,11 +89,10 @@
 // initial log.
 //
 //    INIT_TASK_SCHEDULED,    // Waiting for deferred init tasks to complete.
-// Typically about 30 seconds after startup, a task is sent to a second thread
-// (the file thread) to perform deferred (lower priority and slower)
-// initialization steps such as getting the list of plugins.  That task will
-// (when complete) make an async callback (via a Task) to indicate the
-// completion.
+// Typically about 30 seconds after startup, a task is posted to perform
+// deferred (lower priority and slower) initialization steps such as getting the
+// list of plugins.  That task will (when complete) make an async callback (via
+// a Task) to indicate the completion.
 //
 //    INIT_TASK_DONE,         // Waiting for timer to send initial log.
 // The callback has arrived, and it is now possible for an initial log to be
@@ -247,6 +246,11 @@ const int kSaveStateIntervalMinutes = 5;
 // e.g., the server is down.
 const int kNoResponseCode = content::URLFetcher::RESPONSE_CODE_INVALID - 1;
 
+// Used to indicate that a report was generated before the hardware_class
+// property was available from the StatisticsProvider. This is used to identify
+// faulty reports from Chrome OS clients.
+const char kHardwareClassNotReady[] = "(not ready)";
+
 }
 
 // static
@@ -393,6 +397,9 @@ MetricsService::MetricsService()
     : recording_active_(false),
       reporting_active_(false),
       state_(INITIALIZED),
+#if defined(OS_CHROMEOS)
+      hardware_class_(kHardwareClassNotReady),
+#endif
       idle_since_last_transmission_(false),
       next_window_id_(0),
       ALLOW_THIS_IN_INITIALIZER_LIST(self_ptr_factory_(this)),
@@ -754,32 +761,22 @@ void MetricsService::InitializeMetricsState() {
   ScheduleNextStateSave();
 }
 
-// static
-void MetricsService::InitTaskGetHardwareClass(
-    base::WeakPtr<MetricsService> self,
-    base::MessageLoopProxy* target_loop) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-
-  std::string hardware_class;
+void MetricsService::OnStatisticsProviderReady() {
 #if defined(OS_CHROMEOS)
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   chromeos::system::StatisticsProvider::GetInstance()->GetMachineStatistic(
-      "hardware_class", &hardware_class);
-#endif  // OS_CHROMEOS
-
-  target_loop->PostTask(FROM_HERE,
-      base::Bind(&MetricsService::OnInitTaskGotHardwareClass,
-          self, hardware_class));
+      "hardware_class", &hardware_class_);
+#endif
 }
 
-void MetricsService::OnInitTaskGotHardwareClass(
-    const std::string& hardware_class) {
+void MetricsService::InitTaskGetPluginInfo() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK_EQ(state_, INIT_TASK_SCHEDULED);
-  hardware_class_ = hardware_class;
 
   // Start the next part of the init task: loading plugin information.
   PluginService::GetInstance()->GetPlugins(
       base::Bind(&MetricsService::OnInitTaskGotPluginInfo,
-          self_ptr_factory_.GetWeakPtr()));
+                 self_ptr_factory_.GetWeakPtr()));
 }
 
 void MetricsService::OnInitTaskGotPluginInfo(
@@ -834,16 +831,21 @@ void MetricsService::StartRecording() {
     // We only need to schedule that run once.
     state_ = INIT_TASK_SCHEDULED;
 
-    // Schedules a task on the file thread for execution of slower
-    // initialization steps (such as plugin list generation) necessary
-    // for sending the initial log.  This avoids blocking the main UI
-    // thread.
-    BrowserThread::PostDelayedTask(
-        BrowserThread::FILE,
+#if defined(OS_CHROMEOS)
+    // The hardware class can only be retrieved once the StatisticsProvider is
+    // ready. This usually happens early enough, but can take longer on some
+    // faulty hardware.
+    chromeos::system::StatisticsProvider::GetInstance()->WhenReady(

[petkov, 2012/04/23 10:32:55]

So, now we don't wait for the hardware class to become available, right? How
does this solve the potential problem with missing HWIDs?

If initialization is not complete, I think we still collect metrics (I may be
wrong), we just don't send them, no? So maybe we can wait for the HWID with some
increased timeout -- a couple of minutes, or whatever.

[Joao da Silva, 2012/04/23 11:15:56]

@isherman: if histograms are still collected even when the MetricsService is not
initialized, then making the MetricsService wait for the StatisticsProvider to
become ready should not be a problem. Is that the case? WDYT?

[Ilya Sherman, 2012/04/23 22:44:13]

Data is collected independently of uploading, but the initial upload includes
critical heartbeat metrics like crash & stability counts and other basic sanity
checks.  Jim can correct me on this, but I don't think the hardware class is
critical enough for us to want to potentially block reporting of these heartbeat
metrics on fetching that data.

Note that with your change, the hardware class will be included with the
recurring logs as soon as it is available.  In contrast, previously, if fetching
the hardware class failed for the initial upload, it would never be properly
set.

Since UMA users also have a stable client_id on each device, a single client_id
should map to exactly one hardware class.  Hence, if you need to, you should be
able to scan over the uploaded logs and fill in any missing hardware class info
in earlier uploads based on the data in later uploads.  Does that address your
concern?

[petkov, 2012/04/24 07:42:18]

I do queries on UMA data extremely rarely and I don't know how easy it is to
match a HWIDs with client_id's. Maybe you can loop in interested PMs/TPMs. This
may also require changes to quite a few server-side query tools and dashboards.

We don't want to make it difficult to match critical data included in the
initial report -- stability info, boot times, etc. -- with the hardware platform
it is coming from.

What's the downside with delaying the initial report? It will either be sent a
few seconds or minutes later, or it will be sent on the next Chrome start,
right?

On 2012/04/23 22:44:13, Ilya Sherman wrote:

[petkov, 2012/04/24 07:49:36]

Of course, this needs to be checked. Maybe the initial report will be sent 20
minutes later or something if initialization hasn't finished. But that can be
fixed too, I guess.

+        base::Bind(&MetricsService::OnStatisticsProviderReady,
+                   self_ptr_factory_.GetWeakPtr()));
+#endif
+
+    // Schedules a delayed task for execution of slower initialization steps
+    // (such as plugin list generation) necessary for sending the initial log.
+    MessageLoop::current()->PostDelayedTask(
         FROM_HERE,
-        base::Bind(&MetricsService::InitTaskGetHardwareClass,
-            self_ptr_factory_.GetWeakPtr(),
-            MessageLoop::current()->message_loop_proxy()),
+        base::Bind(&MetricsService::InitTaskGetPluginInfo,
+                   self_ptr_factory_.GetWeakPtr()),
         base::TimeDelta::FromSeconds(kInitializationDelaySeconds));
   }
 }