Check if App Sync is enabled during perf collection.

We read App Sync settings from all initialized user profiles on device.

If disabled, possible Android app names in perf data should be removed.

ChromeOS split sync feature splits the sync setting to different places.

If split sync feature is disabled, we read from Chrome sync settings.

If it is enabled, we read from ChromeOS sync settings.

Metrics show majority of users on stable channel enable App Sync.

Add a UMA histogram to track the status of recording profile data.

Bug=b:171796981
TEST=tested sync settings on a Chromebook, MetricProvider unit test.

Change-Id: Iad00f2755e3b55c0e1e380efdf9283028b0a145c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2499369
Commit-Queue: Bin Du <dubin@google.com>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Gabriel Marin <gmx@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#832457}

chrome/browser/metrics/perf/BUILD.gn

@@ -11,6 +11,7 @@ test("profile_provider_unittest") {
     "//base",
     "//base/test:test_support",
     "//chrome/browser",
+    "//chrome/test:test_support",
     "//chromeos/dbus",
     "//content/test:test_support",
     "//testing/gtest",

chrome/browser/metrics/perf/metric_provider.cc

@@ -9,6 +9,13 @@
 #include "base/task/post_task.h"
 #include "base/task/task_traits.h"
 #include "base/task/thread_pool.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/sync/profile_sync_service_factory.h"
+#include "chromeos/constants/chromeos_features.h"
+#include "components/sync/base/user_selectable_type.h"
+#include "components/sync/driver/sync_service.h"
+#include "components/sync/driver/sync_user_settings.h"
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
 #include "third_party/metrics_proto/sampled_profile.pb.h"
@@ -21,17 +28,64 @@ namespace {
 // metric provider.
 const char kUploadCountHistogramPrefix[] = "ChromeOS.CWP.Upload";
 
+// Name prefix of the histogram that tracks the various outcomes of saving the
+// collected profile to local cache.
+const char kRecordOutcomeHistogramPrefix[] = "ChromeOS.CWP.Record";
+
 // An upper bound on the count of reports expected to be uploaded by an UMA
 // callback.
 const int kMaxValueUploadReports = 10;
 
+// The MD5 prefix to replace the original comm_md5_prefix of COMM events in perf
+// data proto, if necessary. We used string "<redacted>" to compute this MD5
+// prefix.
+const uint64_t kRedactedCommMd5Prefix = 0xee1f021828a1fcbc;
+
+// This function modifies the comm_md5_prefix of all the COMM events in the
+// given perf data proto by replacing it with the md5 prefix of an artificial
+// string.
+void RedactCommMd5Prefixes(PerfDataProto* proto) {
+  for (PerfDataProto::PerfEvent& event : *proto->mutable_events()) {
+    if (event.has_comm_event()) {
+      event.mutable_comm_event()->set_comm_md5_prefix(kRedactedCommMd5Prefix);
+    }
+  }
+}
+
+// Check if App Sync is enabled for a given user profile.
+bool IsAppSyncEnabledForUserProfile(Profile* profile) {
+  syncer::SyncService* sync_service =
+      ProfileSyncServiceFactory::GetForProfile(profile);
+  if (!sync_service)
+    return false;
+  syncer::SyncUserSettings* sync_settings = sync_service->GetUserSettings();
+
+  // Chrome versions >= M78 have a feature that splits the sync settings between
+  // Chrome and ChromeOS. The App Sync toggle is moved under the ChromeOS
+  // settings. If the split sync setting is enabled, we will directly read from
+  // the OS settings. Otherwise, we read from Chrome settings. We then check if
+  // the sync feature is enabled and the App Sync toggle is on.
+  if (chromeos::features::IsSplitSettingsSyncEnabled()) {
+    return sync_settings->IsOsSyncFeatureEnabled() &&
+           sync_settings->GetSelectedOsTypes().Has(
+               syncer::UserSelectableOsType::kOsApps);
+  }
+  // Read chrome settings if split sync is disabled.
+  return sync_service->IsSyncFeatureEnabled() &&
+         sync_settings->GetSelectedTypes().Has(
+             syncer::UserSelectableType::kApps);
+}
+
 }  // namespace
 
 using MetricCollector = internal::MetricCollector;
 
-MetricProvider::MetricProvider(std::unique_ptr<MetricCollector> collector)
+MetricProvider::MetricProvider(std::unique_ptr<MetricCollector> collector,
+                               ProfileManager* profile_manager)
     : upload_uma_histogram_(std::string(kUploadCountHistogramPrefix) +
                             collector->ToolName()),
+      record_uma_histogram_(std::string(kRecordOutcomeHistogramPrefix) +
+                            collector->ToolName()),
       // Run the collector at a higher priority to enable fast triggering of
       // profile collections. In particular, we want fast triggering when
       // jankiness is detected, but even random based periodic collection
@@ -42,6 +96,7 @@ MetricProvider::MetricProvider(std::unique_ptr<MetricCollector> collector)
       collector_task_runner_(base::ThreadPool::CreateSequencedTaskRunner(
           {base::TaskPriority::USER_VISIBLE})),
       metric_collector_(std::move(collector)),
+      profile_manager_(profile_manager),
       weak_factory_(this) {
   metric_collector_->set_profile_done_callback(base::BindRepeating(
       &MetricProvider::OnProfileDone, weak_factory_.GetWeakPtr()));
@@ -156,11 +211,46 @@ void MetricProvider::DisableRecording() {
   recording_enabled_ = false;
 }
 
+// Check the current state of App Sync in the settings. This is done by getting
+// all currently fully initialized profiles and reading the sync settings from
+// them.
+MetricProvider::RecordAttemptStatus MetricProvider::GetAppSyncState() {
+  if (!profile_manager_)
+    return RecordAttemptStatus::kProfileManagerUnset;
+
+  std::vector<Profile*> profiles = profile_manager_->GetLoadedProfiles();
+  if (profiles.size() == 0)
+    return RecordAttemptStatus::kNoLoadedProfile;
+
+  for (Profile* profile : profiles) {
+    if (!IsAppSyncEnabledForUserProfile(profile))
+      return RecordAttemptStatus::kAppSyncDisabled;
+  }
+
+  return RecordAttemptStatus::kAppSyncEnabled;
+}
+
 void MetricProvider::AddProfileToCache(
     std::unique_ptr<SampledProfile> sampled_profile) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  if (!recording_enabled_)
+  if (!recording_enabled_) {
+    base::UmaHistogramEnumeration(record_uma_histogram_,
+                                  RecordAttemptStatus::kRecordingDisabled);
     return;
+  }
+
+  // For privacy reasons, Chrome can not collect Android app names that may be
+  // present in the perf data, unless the user consent to enabling App Sync.
+  // Therefore, if the user does not enable App Sync, we redact comm_md5_prefix
+  // in all COMM events of perf data proto, so these MD5 prefixes can not be
+  // used to recover Android app names. We perform the check on App Sync here
+  // because the procedure to get the user profile (from which sync settings can
+  // be obtained) must execute on the UI thread.
+  auto app_sync_state = GetAppSyncState();
+  base::UmaHistogramEnumeration(record_uma_histogram_, app_sync_state);
+  if (app_sync_state != RecordAttemptStatus::kAppSyncEnabled)
+    RedactCommMd5Prefixes(sampled_profile->mutable_perf_data());
+
   collector_task_runner_->PostTask(
       FROM_HERE, base::BindOnce(&MetricCollector::AddCachedDataDelta,
                                 base::Unretained(metric_collector_.get()),

chrome/browser/metrics/perf/metric_provider.h

@@ -19,6 +19,8 @@ namespace base {
 class TimeDelta;
 }  // namespace base
 
+class ProfileManager;
+
 namespace metrics {
 
 class SampledProfile;
@@ -30,7 +32,8 @@ class SampledProfile;
 // on its dedicated sequence, via PostTask messages.
 class MetricProvider {
  public:
-  explicit MetricProvider(std::unique_ptr<internal::MetricCollector> collector);
+  MetricProvider(std::unique_ptr<internal::MetricCollector> collector,
+                 ProfileManager* profile_manager);
 
   virtual ~MetricProvider();
 
@@ -63,6 +66,18 @@ class MetricProvider {
   void OnJankStopped();
 
  protected:
+  // Enumeration representing the various outcomes of saving the collected
+  // profile to local cache. These values are persisted to logs. Entries should
+  // not be renumbererd and numeric values should never be reused.
+  enum class RecordAttemptStatus {
+    kRecordingDisabled = 0,
+    kProfileManagerUnset = 1,
+    kNoLoadedProfile = 2,
+    kAppSyncDisabled = 3,
+    kAppSyncEnabled = 4,
+    kMaxValue = kAppSyncEnabled,
+  };
+
   // For testing.
   void set_cache_updated_callback(base::RepeatingClosure callback) {
     cache_updated_callback_ = std::move(callback);
@@ -74,6 +89,9 @@ class MetricProvider {
   static void OnProfileDone(base::WeakPtr<MetricProvider> provider,
                             std::unique_ptr<SampledProfile> sampled_profile);
 
+  // Check the state of App Sync in the current session.
+  RecordAttemptStatus GetAppSyncState();
+
   // Saves a profile to the local cache.
   void AddProfileToCache(std::unique_ptr<SampledProfile> sampled_profile);
 
@@ -86,6 +104,10 @@ class MetricProvider {
   // Name of the histogram that counts the number of uploaded reports.
   const std::string upload_uma_histogram_;
 
+  // Name of the histogram that tracks the various outcomes of saving the
+  // collected profile to local cache.
+  const std::string record_uma_histogram_;
+
   // Use a dedicated sequence for the collector. Thread safe. Initialized at
   // construction time, then immutable.
   const scoped_refptr<base::SequencedTaskRunner> collector_task_runner_;
@@ -95,6 +117,10 @@ class MetricProvider {
   // have executed.
   std::unique_ptr<internal::MetricCollector> metric_collector_;
 
+  // The profile manager that manages user profiles with their sync settings, we
+  // do not own this object and only hold a reference to it.
+  ProfileManager* profile_manager_;
+
   // Called when |cached_profile_data_| is populated.
   base::RepeatingClosure cache_updated_callback_;
 

chrome/browser/metrics/perf/profile_provider_chromeos.cc

@@ -9,6 +9,7 @@
 #include "base/feature_list.h"
 #include "base/metrics/field_trial_params.h"
 #include "base/metrics/histogram_macros.h"
+#include "chrome/browser/browser_process.h"
 #include "chrome/browser/metrics/perf/metric_provider.h"
 #include "chrome/browser/metrics/perf/perf_events_collector.h"
 #include "chrome/browser/metrics/perf/windowed_incognito_observer.h"
@@ -58,8 +59,8 @@ ProfileProvider::ProfileProvider()
   // Initialize the WindowedIncognitoMonitor on the UI thread.
   WindowedIncognitoMonitor::Init();
   // Register a perf events collector.
-  collectors_.push_back(
-      std::make_unique<MetricProvider>(std::make_unique<PerfCollector>()));
+  collectors_.push_back(std::make_unique<MetricProvider>(
+      std::make_unique<PerfCollector>(), g_browser_process->profile_manager()));
 }
 
 ProfileProvider::~ProfileProvider() {

chrome/browser/metrics/perf/profile_provider_chromeos_unittest.cc

@@ -109,9 +109,9 @@ class TestProfileProvider : public ProfileProvider {
 
     collectors_.clear();
     collectors_.push_back(std::make_unique<MetricProvider>(
-        std::make_unique<TestMetricCollector<100>>(test_params)));
+        std::make_unique<TestMetricCollector<100>>(test_params), nullptr));
     collectors_.push_back(std::make_unique<MetricProvider>(
-        std::make_unique<TestMetricCollector<200>>(test_params)));
+        std::make_unique<TestMetricCollector<200>>(test_params), nullptr));
   }
 
   using ProfileProvider::collectors_;

chrome/browser/metrics/perf/profile_provider_unittest_main.cc

@@ -16,6 +16,7 @@
 #include "chrome/browser/metrics/perf/collection_params.h"
 #include "chrome/browser/metrics/perf/metric_provider.h"
 #include "chrome/browser/metrics/perf/perf_events_collector.h"
+#include "chrome/test/base/testing_browser_process.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "content/public/test/browser_task_environment.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -65,7 +66,7 @@ class TestProfileProvider : public ProfileProvider {
 
     collectors_.clear();
     auto metric_provider = std::make_unique<TestMetricProvider>(
-        std::make_unique<TestPerfCollector>(test_params));
+        std::make_unique<TestPerfCollector>(test_params), nullptr);
     metric_provider->set_cache_updated_callback(base::BindRepeating(
         &TestProfileProvider::OnProfileDone, base::Unretained(this)));
 
@@ -129,6 +130,10 @@ class ProfileProviderRealCollectionTest : public testing::Test {
     chromeos::PowerManagerClient::InitializeFake();
     chromeos::LoginState::Initialize();
 
+    // The constructor of ProfileProvider uses g_browser_process thus requiring
+    // it to be not null, so initialize it here.
+    TestingBrowserProcess::CreateInstance();
+
     std::map<std::string, std::string> field_trial_params;
     // Only "cycles" event is supported.
     field_trial_params.insert(std::make_pair(

tools/metrics/histograms/enums.xml

@@ -9911,6 +9911,25 @@ histogram as enum -->
   </int>
 </enum>
 
+<enum name="ChromeOSProfileRecordStatus">
+  <int value="0" label="Metrics recording is disabled">
+    Profile data was not recorded since metric recording is disabled.
+  </int>
+  <int value="1" label="Profile manager is unset">
+    Profile data was redacted since the profile manager used to obtain user
+    profile(s) is unset.
+  </int>
+  <int value="2" label="No loaded user profile is found">
+    Profile data was redacted since there is no user profile initialized.
+  </int>
+  <int value="3" label="App sync is disabled">
+    Profile data was redacted since App Sync is disabled.
+  </int>
+  <int value="4" label="App sync is enabled">
+    Profile data was recorded in full since App Sync is enabled.
+  </int>
+</enum>
+
 <enum name="ChromeOSRecoveryReason">
   <summary>
     Reason for entering Recovery Mode. See

tools/metrics/histograms/histograms_xml/chromeos/histograms.xml

@@ -292,6 +292,18 @@ incoming reviews. Googlers can read more about this at go/gwsq-gerrit.
   </summary>
 </histogram>
 
+<histogram name="ChromeOS.CWP.RecordPerf" enum="ChromeOSProfileRecordStatus"
+    expires_after="2021-11-30">
+  <owner>dubin@google.com</owner>
+  <owner>cwp-team@google.com</owner>
+  <summary>
+    A count of the various outcomes related to recording the profile data
+    collected via &quot;perf events&quot; on Chrome OS. The histogram is
+    recorded when system-wide CPU profiling finishes and the collected data is
+    being saved to memory for later upload.
+  </summary>
+</histogram>
+
 <histogram name="ChromeOS.CWP.UploadPerf" units="reports"
     expires_after="2021-05-16">
   <owner>aalexand@google.com</owner>