Make UKM unaffected by the state of the MetricsReporting Feature.

Bug: 767192
Change-Id: Ibea39c5a94c36d8c7c67672ee7fc380f50fa7acb
Reviewed-on: https://chromium-review.googlesource.com/676193Reviewed-by: Olivier Robin <olivierrobin@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Commit-Queue: Robert Kaplow <rkaplow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#505253}

chrome/browser/metrics/chrome_metrics_services_manager_client.cc

@@ -46,17 +46,23 @@
 #include "chromeos/settings/cros_settings_names.h"
 #endif  // defined(OS_CHROMEOS)
 
-namespace {
-
-// Name of the variations param that defines the sampling rate.
-const char kRateParamName[] = "sampling_rate_per_mille";
+namespace metrics {
 
+namespace internal {
 // Metrics reporting feature. This feature, along with user consent, controls if
 // recording and reporting are enabled. If the feature is enabled, but no
 // consent is given, then there will be no recording or reporting.
 const base::Feature kMetricsReportingFeature{"MetricsReporting",
                                              base::FEATURE_ENABLED_BY_DEFAULT};
 
+}  // namespace internal
+}  // namespace metrics
+
+namespace {
+
+// Name of the variations param that defines the sampling rate.
+const char kRateParamName[] = "sampling_rate_per_mille";
+
 // Posts |GoogleUpdateSettings::StoreMetricsClientInfo| on blocking pool thread
 // because it needs access to IO and cannot work from UI thread.
 void PostStoreMetricsClientInfo(const metrics::ClientInfo& client_info) {
@@ -180,7 +186,7 @@ void ChromeMetricsServicesManagerClient::CreateFallbackSamplingTrial(
   // Setup the feature.
   const std::string& group_name = trial->GetGroupNameWithoutActivation();
   feature_list->RegisterFieldTrialOverride(
-      kMetricsReportingFeature.name,
+      metrics::internal::kMetricsReportingFeature.name,
       group_name == kSampledOutGroup
           ? base::FeatureList::OVERRIDE_DISABLE_FEATURE
           : base::FeatureList::OVERRIDE_ENABLE_FEATURE,
@@ -196,7 +202,8 @@ bool ChromeMetricsServicesManagerClient::IsClientInSample() {
   if (!IsClientEligibleForSampling())
     return true;
 
-  return base::FeatureList::IsEnabled(kMetricsReportingFeature);
+  return base::FeatureList::IsEnabled(
+      metrics::internal::kMetricsReportingFeature);
 }
 
 // static
@@ -207,7 +214,7 @@ bool ChromeMetricsServicesManagerClient::GetSamplingRatePerMille(int* rate) {
     return false;
 
   std::string rate_str = variations::GetVariationParamValueByFeature(
-      kMetricsReportingFeature, kRateParamName);
+      metrics::internal::kMetricsReportingFeature, kRateParamName);
   if (rate_str.empty())
     return false;
 
@@ -253,6 +260,10 @@ bool ChromeMetricsServicesManagerClient::IsMetricsReportingEnabled() {
   return enabled_state_provider_->IsReportingEnabled();
 }
 
+bool ChromeMetricsServicesManagerClient::IsMetricsConsentGiven() {
+  return enabled_state_provider_->IsConsentGiven();
+}
+
 #if defined(OS_WIN)
 void ChromeMetricsServicesManagerClient::UpdateRunningServices(
     bool may_record,

chrome/browser/metrics/chrome_metrics_services_manager_client.h

@@ -22,6 +22,11 @@ class PrefService;
 namespace metrics {
 class EnabledStateProvider;
 class MetricsStateManager;
+
+// Used only for testing.
+namespace internal {
+extern const base::Feature kMetricsReportingFeature;
+}
 }
 
 namespace version_info {
@@ -72,6 +77,7 @@ class ChromeMetricsServicesManagerClient
   CreateEntropyProvider() override;
   net::URLRequestContextGetter* GetURLRequestContext() override;
   bool IsMetricsReportingEnabled() override;
+  bool IsMetricsConsentGiven() override;
 
 #if defined(OS_WIN)
   // On Windows, the client controls whether Crashpad can upload crash reports.

chrome/browser/metrics/ukm_browsertest.cc

@@ -5,6 +5,7 @@
 #include "base/run_loop.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/metrics/chrome_metrics_service_accessor.h"
+#include "chrome/browser/metrics/chrome_metrics_services_manager_client.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/browser/sync/test/integration/profile_sync_service_harness.h"
@@ -49,10 +50,12 @@ class UkmBrowserTest : public SyncTest {
  public:
   UkmBrowserTest() : SyncTest(SINGLE_CLIENT) {}
 
-  void SetUpCommandLine(base::CommandLine* command_line) override {
-    SyncTest::SetUpCommandLine(command_line);
-    command_line->AppendSwitchASCII(switches::kEnableFeatures,
-                                    ukm::kUkmFeature.name);
+  void SetUp() override {
+    // Explicitly enable UKM and disable the MetricsReporting (which should
+    // not affect UKM).
+    scoped_feature_list_.InitWithFeatures({ukm::kUkmFeature},
+                                          {internal::kMetricsReportingFeature});
+    SyncTest::SetUp();
   }
 
  protected:
@@ -97,6 +100,7 @@ class UkmBrowserTest : public SyncTest {
   ukm::UkmService* ukm_service() {
     return static_cast<ukm::UkmService*>(ukm::UkmRecorder::Get());
   }
+  base::test::ScopedFeatureList scoped_feature_list_;
 };
 
 // Make sure that UKM is disabled while an incognito window is open.
@@ -223,6 +227,36 @@ IN_PROC_BROWSER_TEST_F(UkmBrowserTest, DisableSyncCheck) {
   ChromeMetricsServiceAccessor::SetMetricsAndCrashReportingForTesting(nullptr);
 }
 
+// Make sure that UKM is not affected by MetricsReporting Feature (sampling).
+IN_PROC_BROWSER_TEST_F(UkmBrowserTest, MetricsReportingCheck) {
+  // Need to set the Metrics Default to OPT_OUT to trigger MetricsReporting.
+  DCHECK(g_browser_process);
+  PrefService* local_state = g_browser_process->local_state();
+  metrics::ForceRecordMetricsReportingDefaultState(
+      local_state, metrics::EnableMetricsDefault::OPT_OUT);
+  // Verify that kMetricsReportingFeature is disabled (i.e. other metrics
+  // services will be sampled out).
+  EXPECT_FALSE(
+      base::FeatureList::IsEnabled(internal::kMetricsReportingFeature));
+
+  // Enable metrics recording and update MetricsServicesManager.
+  bool metrics_enabled = true;
+  ChromeMetricsServiceAccessor::SetMetricsAndCrashReportingForTesting(
+      &metrics_enabled);
+  g_browser_process->GetMetricsServicesManager()->UpdateUploadPermissions(true);
+
+  Profile* profile = ProfileManager::GetActiveUserProfile();
+  std::unique_ptr<ProfileSyncServiceHarness> harness =
+      EnableSyncForProfile(profile);
+
+  Browser* sync_browser = CreateBrowser(profile);
+  EXPECT_TRUE(ukm_enabled());
+
+  harness->service()->RequestStop(browser_sync::ProfileSyncService::CLEAR_DATA);
+  CloseBrowserSynchronously(sync_browser);
+  ChromeMetricsServiceAccessor::SetMetricsAndCrashReportingForTesting(nullptr);
+}
+
 // TODO(crbug/745939): Add a tests for disable w/ multiprofiles.
 
 // TODO(crbug/745939): Add a tests for guest profile.

components/metrics/metrics_reporting_default_state.cc

@@ -22,6 +22,12 @@ void RecordMetricsReportingDefaultState(PrefService* local_state,
   local_state->SetInteger(prefs::kMetricsDefaultOptIn, default_state);
 }
 
+void ForceRecordMetricsReportingDefaultState(
+    PrefService* local_state,
+    EnableMetricsDefault default_state) {
+  local_state->SetInteger(prefs::kMetricsDefaultOptIn, default_state);
+}
+
 EnableMetricsDefault GetMetricsReportingDefaultState(PrefService* local_state) {
   return static_cast<EnableMetricsDefault>(
       local_state->GetInteger(prefs::kMetricsDefaultOptIn));

components/metrics/metrics_reporting_default_state.h

@@ -34,6 +34,11 @@ void RegisterMetricsReportingStatePrefs(PrefRegistrySimple* registry);
 void RecordMetricsReportingDefaultState(PrefService* local_state,
                                         EnableMetricsDefault default_state);
 
+// Same as above, but does not verify the current state is UNKNOWN.
+void ForceRecordMetricsReportingDefaultState(
+    PrefService* local_state,
+    EnableMetricsDefault default_state);
+
 // Gets information about the default value for the enable metrics reporting
 // checkbox shown during first-run.
 EnableMetricsDefault GetMetricsReportingDefaultState(PrefService* local_state);

components/metrics_services_manager/metrics_services_manager.cc

@@ -21,7 +21,10 @@ namespace metrics_services_manager {
 
 MetricsServicesManager::MetricsServicesManager(
     std::unique_ptr<MetricsServicesManagerClient> client)
-    : client_(std::move(client)), may_upload_(false), may_record_(false) {
+    : client_(std::move(client)),
+      may_upload_(false),
+      may_record_(false),
+      consent_given_(false) {
   DCHECK(client_);
 }
 
@@ -81,10 +84,12 @@ MetricsServicesManager::GetMetricsServiceClient() {
 }
 
 void MetricsServicesManager::UpdatePermissions(bool current_may_record,
+                                               bool current_consent_given,
                                                bool current_may_upload) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  // If the user has opted out of metrics, delete local UKM state.
-  if (may_record_ && !current_may_record) {
+  // If the user has opted out of metrics, delete local UKM state. We Only check
+  // consent for UKM.
+  if (consent_given_ && !current_consent_given) {
     ukm::UkmService* ukm = GetUkmService();
     if (ukm) {
       ukm->Purge();
@@ -93,10 +98,9 @@ void MetricsServicesManager::UpdatePermissions(bool current_may_record,
   }
 
   // Stash the current permissions so that we can update the RapporServiceImpl
-  // correctly when the Rappor preference changes.  The metrics recording
-  // preference partially determines the initial rappor setting, and also
-  // controls whether FINE metrics are sent.
+  // correctly when the Rappor preference changes.
   may_record_ = current_may_record;
+  consent_given_ = current_consent_given;
   may_upload_ = current_may_upload;
   UpdateRunningServices();
 }
@@ -138,7 +142,8 @@ void MetricsServicesManager::UpdateUkmService() {
       client_->IsMetricsReportingForceEnabled() ||
       metrics_service_client_->IsHistorySyncEnabledOnAllProfiles();
   bool is_incognito = client_->IsIncognitoSessionActive();
-  if (may_record_ && sync_enabled & !is_incognito) {
+
+  if (consent_given_ && sync_enabled & !is_incognito) {
     ukm->EnableRecording();
     if (may_upload_)
       ukm->EnableReporting();
@@ -151,9 +156,13 @@ void MetricsServicesManager::UpdateUkmService() {
 }
 
 void MetricsServicesManager::UpdateUploadPermissions(bool may_upload) {
-  UpdatePermissions((client_->IsMetricsReportingForceEnabled() ||
-                     client_->IsMetricsReportingEnabled()),
-                    client_->IsMetricsReportingForceEnabled() || may_upload);
+  if (client_->IsMetricsReportingForceEnabled()) {
+    UpdatePermissions(true, true, true);
+    return;
+  }
+
+  UpdatePermissions(client_->IsMetricsReportingEnabled(),
+                    client_->IsMetricsConsentGiven(), may_upload);
 }
 
 }  // namespace metrics_services_manager

components/metrics_services_manager/metrics_services_manager.h

@@ -77,10 +77,6 @@ class MetricsServicesManager {
   // renderer process exits unexpectedly.
   void OnRendererProcessCrash();
 
-  // Update the managed services when permissions for recording/uploading
-  // metrics change.
-  void UpdatePermissions(bool current_may_record, bool current_may_upload);
-
   // Update the managed services when permissions for uploading metrics change.
   void UpdateUploadPermissions(bool may_upload);
 
@@ -101,6 +97,12 @@ class MetricsServicesManager {
   // Update the state of UkmService to match current permissions.
   void UpdateUkmService();
 
+  // Update the managed services when permissions for recording/uploading
+  // metrics change.
+  void UpdatePermissions(bool current_may_record,
+                         bool current_consent_given,
+                         bool current_may_upload);
+
   // The client passed in from the embedder.
   std::unique_ptr<MetricsServicesManagerClient> client_;
 
@@ -113,6 +115,9 @@ class MetricsServicesManager {
   // The current metrics recording setting.
   bool may_record_;
 
+  // The current metrics setting reflecting if consent was given.
+  bool consent_given_;
+
   // The MetricsServiceClient. Owns the MetricsService.
   std::unique_ptr<metrics::MetricsServiceClient> metrics_service_client_;
 

components/metrics_services_manager/metrics_services_manager_client.h

@@ -51,6 +51,9 @@ class MetricsServicesManagerClient {
   // Returns whether metrics reporting is enabled.
   virtual bool IsMetricsReportingEnabled() = 0;
 
+  // Returns whether metrics consent is given.
+  virtual bool IsMetricsConsentGiven() = 0;
+
   // Returns whether there are any Incognito browsers/tabs open.
   virtual bool IsIncognitoSessionActive() = 0;
 

ios/chrome/browser/metrics/ios_chrome_metrics_services_manager_client.h

@@ -44,6 +44,8 @@ class IOSChromeMetricsServicesManagerClient
 
   net::URLRequestContextGetter* GetURLRequestContext() override;
   bool IsMetricsReportingEnabled() override;
+  bool IsMetricsConsentGiven() override;
+
   bool IsIncognitoSessionActive() override;
 
   // Gets the MetricsStateManager, creating it if it has not already been

ios/chrome/browser/metrics/ios_chrome_metrics_services_manager_client.mm

@@ -99,6 +99,10 @@ bool IOSChromeMetricsServicesManagerClient::IsMetricsReportingEnabled() {
   return enabled_state_provider_->IsReportingEnabled();
 }
 
+bool IOSChromeMetricsServicesManagerClient::IsMetricsConsentGiven() {
+  return enabled_state_provider_->IsConsentGiven();
+}
+
 metrics::MetricsStateManager*
 IOSChromeMetricsServicesManagerClient::GetMetricsStateManager() {
   DCHECK(thread_checker_.CalledOnValidThread());