Report Chrome OS system profile metrics to UKM.

Specifically, this includes metrics reported by ChromeOSMetricsProvider
in the system profile for UKM. The following fields will now be included:
- hardware.hardware_class
- hardware.internal_display_supports_touch
- hardware.bluetooth (and all the sub-fields)
- multi_profile_user_count

A note on the implementation: This reports the new short hardware class
field that is now reported to UMA (as of M67), as enabled by the kUmaShortHWClass
feature. This CL also improves this logic (both for UMA and UKM) by doing it
synchronously which is possible with the new implementation. The bluetooth data
is still retrieve asynchronously however - which is new for UKM service.

In addition, this CL fixes MetricsProvider initialization order in UkmService,
where previously the Init() call on providers was happening before providers were
added. This was not a problem before because the existing providers did not do
anything in Init().

This change also improe

Bug: 807663, 810872
Change-Id: I4b0dcb848e40a467a26feaf757f43aaf442be050
Reviewed-on: https://chromium-review.googlesource.com/996175
Commit-Queue: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549271}

chrome/browser/metrics/chrome_metrics_service_client.cc

@@ -731,6 +731,11 @@ void ChromeMetricsServiceClient::RegisterUKMProviders() {
           std::make_unique<metrics::NetworkQualityEstimatorProviderImpl>(
               g_browser_process->io_thread())));
 
+#if defined(OS_CHROMEOS)
+  ukm_service_->RegisterMetricsProvider(
+      std::make_unique<ChromeOSMetricsProvider>());
+#endif  // !defined(OS_CHROMEOS)
+
   // TODO(rkaplow): Support synthetic trials for UKM.
   ukm_service_->RegisterMetricsProvider(
       std::make_unique<variations::FieldTrialsProvider>(nullptr,

chrome/browser/metrics/chromeos_metrics_provider.cc

@@ -5,6 +5,8 @@
 #include "chrome/browser/metrics/chromeos_metrics_provider.h"
 
 #include <stddef.h>
+#include <string>
+#include <vector>
 
 #include "base/barrier_closure.h"
 #include "base/feature_list.h"
@@ -88,10 +90,11 @@ const base::Feature kUmaShortHWClass{"UmaShortHWClass",
 
 // Called on a background thread to load hardware class information.
 std::string GetHardwareClassOnBackgroundThread() {
-  // TODO(asvitkine): If we switch to the new API permanently, we should also
-  // move this work off the background thread.
-  if (base::FeatureList::IsEnabled(kUmaShortHWClass))
-    return variations::VariationsFieldTrialCreator::GetShortHardwareClass();
+  // If short hardware class feature is enabled, the hardware class should be
+  // getting set synchronously, so this shouldn't be getting called.
+  // TODO(asvitkine): Get rid of the background thread logic after M67 goes to
+  // stable when the new logic has fully rolled out.
+  DCHECK(!base::FeatureList::IsEnabled(kUmaShortHWClass));
 
   std::string hardware_class;
   chromeos::system::StatisticsProvider::GetInstance()->GetMachineStatistic(
@@ -144,12 +147,20 @@ ChromeOSMetricsProvider::GetEnrollmentStatus() {
 }
 
 void ChromeOSMetricsProvider::Init() {
+  if (base::FeatureList::IsEnabled(kUmaShortHWClass)) {
+    hardware_class_ =
+        variations::VariationsFieldTrialCreator::GetShortHardwareClass();
+  }
   perf_provider_.Init();
 }
 
 void ChromeOSMetricsProvider::AsyncInit(const base::Closure& done_callback) {
-  base::Closure barrier = base::BarrierClosure(2, done_callback);
-  InitTaskGetHardwareClass(barrier);
+  bool need_hardware_class = !base::FeatureList::IsEnabled(kUmaShortHWClass);
+
+  base::RepeatingClosure barrier =
+      base::BarrierClosure(need_hardware_class ? 2 : 1, done_callback);
+  if (need_hardware_class)
+    InitTaskGetHardwareClass(barrier);
   InitTaskGetBluetoothAdapter(barrier);
 }
 

chrome/browser/metrics/ukm_browsertest.cc

@@ -3,6 +3,9 @@
 // found in the LICENSE file.
 
 #include "base/run_loop.h"
+#include "base/strings/string_util.h"
+#include "base/sys_info.h"
+#include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h"
 #include "chrome/browser/metrics/chrome_metrics_service_accessor.h"
@@ -20,11 +23,15 @@
 #include "components/sync/test/fake_server/fake_server_network_resources.h"
 #include "components/ukm/ukm_service.h"
 #include "components/ukm/ukm_source.h"
+#include "components/variations/service/variations_field_trial_creator.h"
+#include "components/version_info/version_info.h"
 #include "content/public/browser/browsing_data_remover.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/test/browsing_data_remover_test_util.h"
 #include "content/public/test/test_utils.h"
 #include "services/metrics/public/cpp/ukm_recorder.h"
+#include "third_party/metrics_proto/ukm/report.pb.h"
+#include "third_party/zlib/google/compression_utils.h"
 
 #if defined(OS_CHROMEOS)
 #include "chrome/browser/signin/signin_manager_factory.h"
@@ -156,6 +163,24 @@ class UkmBrowserTest : public SyncTest {
     return service->reporting_service_.ukm_log_store()->has_unsent_logs();
   }
 
+  ukm::Report GetUkmReport() {
+    EXPECT_TRUE(HasUnsentUkmLogs());
+
+    metrics::PersistedLogs* log_store =
+        ukm_service()->reporting_service_.ukm_log_store();
+    EXPECT_FALSE(log_store->has_staged_log());
+    log_store->StageNextLog();
+    EXPECT_TRUE(log_store->has_staged_log());
+
+    std::string uncompressed_log_data;
+    EXPECT_TRUE(compression::GzipUncompress(log_store->staged_log(),
+                                            &uncompressed_log_data));
+
+    ukm::Report report;
+    EXPECT_TRUE(report.ParseFromString(uncompressed_log_data));
+    return report;
+  }
+
  protected:
   std::unique_ptr<ProfileSyncServiceHarness> EnableSyncForProfile(
       Profile* profile) {
@@ -408,6 +433,44 @@ IN_PROC_BROWSER_TEST_F(UkmBrowserTest, MetricsConsentCheck) {
   CloseBrowserSynchronously(sync_browser);
 }
 
+IN_PROC_BROWSER_TEST_F(UkmBrowserTest, LogProtoData) {
+  MetricsConsentOverride metrics_consent(true);
+
+  Profile* profile = ProfileManager::GetActiveUserProfile();
+  std::unique_ptr<ProfileSyncServiceHarness> harness =
+      EnableSyncForProfile(profile);
+
+  Browser* sync_browser = CreateBrowser(profile);
+  EXPECT_TRUE(ukm_enabled());
+  uint64_t original_client_id = client_id();
+  EXPECT_NE(0U, original_client_id);
+
+  // Make sure there is a persistent log.
+  BuildAndStoreUkmLog();
+  EXPECT_TRUE(HasUnsentUkmLogs());
+
+  // Check log contents.
+  ukm::Report report = GetUkmReport();
+  EXPECT_EQ(original_client_id, report.client_id());
+  // Note: The version number reported in the proto may have a suffix, such as
+  // "-64-devel", so use use StartsWith() rather than checking for equality.
+  EXPECT_TRUE(base::StartsWith(report.system_profile().app_version(),
+                               version_info::GetVersionNumber(),
+                               base::CompareCase::SENSITIVE));
+
+// Chrome OS hardware class comes from a different API than on other platforms.
+#if defined(OS_CHROMEOS)
+  EXPECT_EQ(variations::VariationsFieldTrialCreator::GetShortHardwareClass(),
+            report.system_profile().hardware().hardware_class());
+#else   // !defined(OS_CHROMEOS)
+  EXPECT_EQ(base::SysInfo::HardwareModelName(),
+            report.system_profile().hardware().hardware_class());
+#endif  // defined(OS_CHROMEOS)
+
+  harness->service()->RequestStop(browser_sync::ProfileSyncService::CLEAR_DATA);
+  CloseBrowserSynchronously(sync_browser);
+}
+
 // Make sure that providing consent doesn't enable UKM when sync is disabled.
 // Keep in sync with UkmTest.consentAddedButNoSyncCheck in
 // chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/

components/ukm/ukm_service.cc

@@ -88,8 +88,6 @@ UkmService::UkmService(PrefService* pref_service,
   scheduler_.reset(new ukm::UkmRotationScheduler(rotate_callback,
                                                  get_upload_interval_callback));
 
-  metrics_providers_.Init();
-
   StoreWhitelistedEntries();
 
   DelegatingUkmRecorder::Get()->AddDelegate(self_ptr_factory_.GetWeakPtr());
@@ -106,6 +104,11 @@ void UkmService::Initialize() {
   DVLOG(1) << "UkmService::Initialize";
   initialize_started_ = true;
 
+  DCHECK_EQ(0, report_count_);
+  client_id_ = LoadOrGenerateClientId(pref_service_);
+  session_id_ = LoadSessionId(pref_service_);
+  metrics_providers_.Init();
+
   StartInitTask();
 }
 
@@ -202,10 +205,6 @@ void UkmService::RegisterPrefs(PrefRegistrySimple* registry) {
 void UkmService::StartInitTask() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DVLOG(1) << "UkmService::StartInitTask";
-  client_id_ = LoadOrGenerateClientId(pref_service_);
-  session_id_ = LoadSessionId(pref_service_);
-  report_count_ = 0;
-
   metrics_providers_.AsyncInit(base::Bind(&UkmService::FinishedInitTask,
                                           self_ptr_factory_.GetWeakPtr()));
 }