Enables Persistent Histograms and Registers a FileMetricsProvider to uploaded metrics

- Reverts the revert in http://crrev/c/1755004, i.e. instantiates the
  collection of persistent histograms.
- Registers a FileMetricsProvider to upload collected metrics.
- Added ios_chrome_metrics_service_client_unittest.mm, patterned after
  chrome_metrics_service_client_unittest.cc.

Bug: 994743
Change-Id: Id8e7ce79f0825eb1cd4c1fb7051709c1d3880cc5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1752699
Commit-Queue: Peter Lee <pkl@chromium.org>
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Reviewed-by: Brian White <bcwhite@chromium.org>
Cr-Commit-Position: refs/heads/master@{#688154}

components/metrics/metrics_service.h

@@ -38,6 +38,8 @@ class PrefService;
 class PrefRegistrySimple;
 FORWARD_DECLARE_TEST(ChromeMetricsServiceClientTest,
                      TestRegisterMetricsServiceProviders);
+FORWARD_DECLARE_TEST(IOSChromeMetricsServiceClientTest,
+                     TestRegisterMetricsServiceProviders);
 
 namespace base {
 class HistogramSamples;
@@ -390,6 +392,8 @@ class MetricsService : public base::HistogramFlattener {
   FRIEND_TEST_ALL_PREFIXES(MetricsServiceTest, IsPluginProcess);
   FRIEND_TEST_ALL_PREFIXES(::ChromeMetricsServiceClientTest,
                            TestRegisterMetricsServiceProviders);
+  FRIEND_TEST_ALL_PREFIXES(::IOSChromeMetricsServiceClientTest,
+                           TestRegisterMetricsServiceProviders);
   SEQUENCE_CHECKER(sequence_checker_);
 
   // Weak pointers factory used to post task on different threads. All weak

ios/chrome/browser/ios_chrome_field_trials.cc

@@ -4,6 +4,18 @@
 
 #include "ios/chrome/browser/ios_chrome_field_trials.h"
 
+#include "base/path_service.h"
+#include "components/metrics/persistent_histograms.h"
+#include "ios/chrome/browser/chrome_paths.h"
+
+void IOSChromeFieldTrials::SetupFieldTrials() {
+  // Persistent histograms must be enabled as soon as possible.
+  base::FilePath user_data_dir;
+  if (base::PathService::Get(ios::DIR_USER_DATA, &user_data_dir)) {
+    InstantiatePersistentHistograms(user_data_dir);
+  }
+}
+
 void IOSChromeFieldTrials::SetupFeatureControllingFieldTrials(
     bool has_seed,
     base::FeatureList* feature_list) {

ios/chrome/browser/ios_chrome_field_trials.h

@@ -16,7 +16,7 @@ class IOSChromeFieldTrials : public variations::PlatformFieldTrials {
   ~IOSChromeFieldTrials() override {}
 
   // variations::PlatformFieldTrials:
-  void SetupFieldTrials() override {}
+  void SetupFieldTrials() override;
   void SetupFeatureControllingFieldTrials(
       bool has_seed,
       base::FeatureList* feature_list) override;

ios/chrome/browser/metrics/BUILD.gn

@@ -121,6 +121,7 @@ source_set("unit_tests") {
   sources = [
     "chrome_browser_state_client_unittest.mm",
     "ios_chrome_metrics_service_accessor_unittest.cc",
+    "ios_chrome_metrics_service_client_unittest.mm",
     "ios_chrome_stability_metrics_provider_unittest.mm",
     "mobile_session_shutdown_metrics_provider_unittest.mm",
     "previous_session_info_unittest.mm",

ios/chrome/browser/metrics/ios_chrome_metrics_service_client.h

@@ -15,6 +15,7 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/threading/thread_checker.h"
+#include "components/metrics/file_metrics_provider.h"
 #include "components/metrics/metrics_log_uploader.h"
 #include "components/metrics/metrics_service_client.h"
 #include "components/omnibox/browser/omnibox_event_global_tracker.h"
@@ -95,6 +96,10 @@ class IOSChromeMetricsServiceClient : public IncognitoWebStateObserver,
 
   metrics::EnableMetricsDefault GetMetricsReportingDefaultState() override;
 
+  // Determine what to do with a file based on filename. Visible for testing.
+  static metrics::FileMetricsProvider::FilterAction FilterBrowserMetricsFiles(
+      const base::FilePath& path);
+
  private:
   explicit IOSChromeMetricsServiceClient(
       metrics::MetricsStateManager* state_manager);

ios/chrome/browser/metrics/ios_chrome_metrics_service_client.mm

@@ -17,6 +17,7 @@
 #include "base/files/file_util.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/metrics/persistent_histogram_allocator.h"
 #include "base/path_service.h"
 #include "base/process/process_metrics.h"
 #include "base/rand_util.h"
@@ -33,6 +34,7 @@
 #include "components/metrics/metrics_pref_names.h"
 #include "components/metrics/metrics_reporting_default_state.h"
 #include "components/metrics/metrics_service.h"
+#include "components/metrics/metrics_state_manager.h"
 #include "components/metrics/net/cellular_logic_helper.h"
 #include "components/metrics/net/net_metrics_log_uploader.h"
 #include "components/metrics/net/network_metrics_provider.h"
@@ -75,28 +77,49 @@
 
 namespace {
 
+// Maximum amount of local storage for storing persistent histograms.
+const int kMaxHistogramStorageKiB = 50 << 10;  // 50 MiB
+
 void GetNetworkConnectionTrackerAsync(
     base::OnceCallback<void(network::NetworkConnectionTracker*)> callback) {
   std::move(callback).Run(
       GetApplicationContext()->GetNetworkConnectionTracker());
 }
 
-void CleanupBrowserMetricsDataFiles() {
+std::unique_ptr<metrics::FileMetricsProvider> CreateFileMetricsProvider(
+    bool metrics_reporting_enabled) {
+  // Create an object to monitor files of metrics and include them in reports.
+  std::unique_ptr<metrics::FileMetricsProvider> file_metrics_provider(
+      new metrics::FileMetricsProvider(
+          GetApplicationContext()->GetLocalState()));
+
   base::FilePath user_data_dir;
-  if (!base::PathService::Get(ios::DIR_USER_DATA, &user_data_dir))
-    return;
-  base::FilePath browser_metrics_upload_dir =
-      user_data_dir.AppendASCII(kBrowserMetricsName);
-  if (base::IsDirectoryEmpty(browser_metrics_upload_dir))
-    return;
-  // Delete accumulated metrics files due to http://crbug/992946
-  base::PostTask(
-      FROM_HERE,
-      {base::ThreadPool(), base::MayBlock(), base::TaskPriority::BEST_EFFORT,
-       base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN},
-      base::BindOnce(base::IgnoreResult(&base::DeleteFile),
-                     std::move(browser_metrics_upload_dir),
-                     /*recursive=*/true));
+  if (base::PathService::Get(ios::DIR_USER_DATA, &user_data_dir)) {
+    base::FilePath browser_metrics_upload_dir =
+        user_data_dir.AppendASCII(kBrowserMetricsName);
+    if (metrics_reporting_enabled) {
+      metrics::FileMetricsProvider::Params browser_metrics_params(
+          browser_metrics_upload_dir,
+          metrics::FileMetricsProvider::SOURCE_HISTOGRAMS_ATOMIC_DIR,
+          metrics::FileMetricsProvider::ASSOCIATE_INTERNAL_PROFILE,
+          kBrowserMetricsName);
+      browser_metrics_params.max_dir_kib = kMaxHistogramStorageKiB;
+      browser_metrics_params.filter = base::BindRepeating(
+          &IOSChromeMetricsServiceClient::FilterBrowserMetricsFiles);
+      file_metrics_provider->RegisterSource(browser_metrics_params);
+    } else {
+      // When metrics reporting is not enabled, any existing files should be
+      // deleted in order to preserve user privacy.
+      base::PostTask(FROM_HERE,
+                     {base::ThreadPool(), base::MayBlock(),
+                      base::TaskPriority::BEST_EFFORT,
+                      base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN},
+                     base::BindOnce(base::IgnoreResult(&base::DeleteFile),
+                                    std::move(browser_metrics_upload_dir),
+                                    /*recursive=*/true));
+    }
+  }
+  return file_metrics_provider;
 }
 
 }  // namespace
@@ -132,6 +155,7 @@ void IOSChromeMetricsServiceClient::RegisterPrefs(
     PrefRegistrySimple* registry) {
   metrics::MetricsService::RegisterPrefs(registry);
   metrics::StabilityMetricsHelper::RegisterPrefs(registry);
+  metrics::FileMetricsProvider::RegisterPrefs(registry, kBrowserMetricsName);
   metrics::RegisterMetricsReportingStatePrefs(registry);
   ukm::UkmService::RegisterPrefs(registry);
 }
@@ -239,6 +263,12 @@ void IOSChromeMetricsServiceClient::Initialize() {
         std::move(stability_metrics_provider));
   }
 
+  // NOTE: metrics_state_manager_->IsMetricsReportingEnabled() returns false
+  // during local testing. To test locally, modify
+  // MetricsServiceAccessor::IsMetricsReportingEnabled() to return true.
+  metrics_service_->RegisterMetricsProvider(CreateFileMetricsProvider(
+      metrics_state_manager_->IsMetricsReportingEnabled()));
+
   metrics_service_->RegisterMetricsProvider(
       std::make_unique<metrics::ScreenInfoMetricsProvider>());
 
@@ -266,10 +296,6 @@ void IOSChromeMetricsServiceClient::Initialize() {
   metrics_service_->RegisterMetricsProvider(
       std::make_unique<metrics::DemographicMetricsProvider>(
           std::make_unique<metrics::ChromeBrowserStateClient>()));
-
-  // TODO(crbug.com/992946): This is an interim fix to stop logging of
-  // persistent histograms and delete any accumulated metrics files.
-  CleanupBrowserMetricsDataFiles();
 }
 
 void IOSChromeMetricsServiceClient::CollectFinalHistograms() {
@@ -337,6 +363,23 @@ void IOSChromeMetricsServiceClient::OnURLOpenedFromOmnibox(OmniboxLog* log) {
   metrics_service_->OnApplicationNotIdle();
 }
 
+// static
+metrics::FileMetricsProvider::FilterAction
+IOSChromeMetricsServiceClient::FilterBrowserMetricsFiles(
+    const base::FilePath& path) {
+  // Do not process the file if it corresponds to the current process id.
+  base::ProcessId pid;
+  bool parse_success = base::GlobalHistogramAllocator::ParseFilePath(
+      path, nullptr, nullptr, &pid);
+  if (!parse_success)
+    return metrics::FileMetricsProvider::FILTER_PROCESS_FILE;
+  if (pid == base::GetCurrentProcId())
+    return metrics::FileMetricsProvider::FILTER_ACTIVE_THIS_PID;
+  // No need to test whether |pid| is a different active process. This isn't
+  // applicable to iOS because there cannot be two copies of Chrome running.
+  return metrics::FileMetricsProvider::FILTER_PROCESS_FILE;
+}
+
 metrics::EnableMetricsDefault
 IOSChromeMetricsServiceClient::GetMetricsReportingDefaultState() {
   return metrics::GetMetricsReportingDefaultState(

ios/chrome/browser/metrics/ios_chrome_metrics_service_client_unittest.mm

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#import "ios/chrome/browser/metrics/ios_chrome_metrics_service_client.h"
+
+#include "base/metrics/persistent_histogram_allocator.h"
+#include "components/metrics/client_info.h"
+#include "components/metrics/metrics_service.h"
+#include "components/metrics/metrics_state_manager.h"
+#include "components/metrics/test_enabled_state_provider.h"
+#include "components/prefs/testing_pref_service.h"
+#include "ios/chrome/browser/browser_state/test_chrome_browser_state.h"
+#include "ios/chrome/browser/browser_state/test_chrome_browser_state_manager.h"
+#include "ios/chrome/test/ios_chrome_scoped_testing_chrome_browser_state_manager.h"
+#include "ios/web/public/test/test_web_thread_bundle.h"
+#include "testing/platform_test.h"
+
+#if !defined(__has_feature) || !__has_feature(objc_arc)
+#error "This file requires ARC support."
+#endif
+
+class IOSChromeMetricsServiceClientTest : public PlatformTest {
+ public:
+  IOSChromeMetricsServiceClientTest()
+      : scoped_browser_state_manager_(
+            std::make_unique<TestChromeBrowserStateManager>(base::FilePath())),
+        browser_state_(TestChromeBrowserState::Builder().Build()),
+        enabled_state_provider_(/*consent=*/false, /*enabled=*/false) {}
+
+  void SetUp() override {
+    PlatformTest::SetUp();
+    metrics::MetricsService::RegisterPrefs(prefs_.registry());
+    metrics_state_manager_ = metrics::MetricsStateManager::Create(
+        &prefs_, &enabled_state_provider_, base::string16(),
+        base::BindRepeating(
+            &IOSChromeMetricsServiceClientTest::FakeStoreClientInfoBackup,
+            base::Unretained(this)),
+        base::BindRepeating(
+            &IOSChromeMetricsServiceClientTest::LoadFakeClientInfoBackup,
+            base::Unretained(this)));
+  }
+
+ protected:
+  void FakeStoreClientInfoBackup(const metrics::ClientInfo& client_info) {}
+
+  std::unique_ptr<metrics::ClientInfo> LoadFakeClientInfoBackup() {
+    return std::make_unique<metrics::ClientInfo>();
+  }
+
+  web::TestWebThreadBundle thread_bundle_;
+  IOSChromeScopedTestingChromeBrowserStateManager scoped_browser_state_manager_;
+  std::unique_ptr<ios::ChromeBrowserState> browser_state_;
+  metrics::TestEnabledStateProvider enabled_state_provider_;
+  TestingPrefServiceSimple prefs_;
+  std::unique_ptr<metrics::MetricsStateManager> metrics_state_manager_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(IOSChromeMetricsServiceClientTest);
+};
+
+namespace {
+
+TEST_F(IOSChromeMetricsServiceClientTest, FilterFiles) {
+  base::ProcessId my_pid = base::GetCurrentProcId();
+  base::FilePath active_dir(FILE_PATH_LITERAL("foo"));
+  base::FilePath upload_dir(FILE_PATH_LITERAL("bar"));
+  base::FilePath upload_path;
+  base::GlobalHistogramAllocator::ConstructFilePathsForUploadDir(
+      active_dir, upload_dir, "TestMetrics", &upload_path, nullptr, nullptr);
+  EXPECT_EQ(
+      metrics::FileMetricsProvider::FILTER_ACTIVE_THIS_PID,
+      IOSChromeMetricsServiceClient::FilterBrowserMetricsFiles(upload_path));
+  EXPECT_EQ(metrics::FileMetricsProvider::FILTER_PROCESS_FILE,
+            IOSChromeMetricsServiceClient::FilterBrowserMetricsFiles(
+                base::GlobalHistogramAllocator::ConstructFilePathForUploadDir(
+                    upload_dir, "Test", base::Time::Now(), (my_pid + 10))));
+}
+
+}  // namespace
+
+// This is not in anonymous namespace so this test can be a friend class of
+// MetricsService for accessing protected ivars.
+TEST_F(IOSChromeMetricsServiceClientTest, TestRegisterMetricsServiceProviders) {
+  // This is the metrics provider added in MetricsService constructor.
+  // StabilityMetricsProvider, FieldTrialsProvider and
+  // MetricsStateMetricsProvider.
+  size_t expected_providers = 3;
+
+  // This is the number of metrics providers that are registered inside
+  // IOSChromeMetricsServiceClient::Initialize().
+  expected_providers += 12;
+
+  std::unique_ptr<IOSChromeMetricsServiceClient> chrome_metrics_service_client =
+      IOSChromeMetricsServiceClient::Create(metrics_state_manager_.get());
+  EXPECT_EQ(expected_providers,
+            chrome_metrics_service_client->GetMetricsService()
+                ->delegating_provider_.GetProviders()
+                .size());
+}