Support filters in file metrics provider.

Sometimes it is necessary to delay or skip the processing of certain
files (perhaps because it is still being written). Filtering gives
these options to the call site.

Moving all the "source" parameters into a single structure and passing
that allows for more fields to be added in the future with default
values so they don't have to be specified by callers that don't need
to change them.

Bug: 696721
Change-Id: I3bbfcc3a05ceba9d37e32ae86406599328554b9e
Reviewed-on: https://chromium-review.googlesource.com/678453
Commit-Queue: Brian White <bcwhite@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#505182}

chrome/browser/metrics/chrome_metrics_service_client.cc

@@ -208,10 +208,10 @@ void RegisterOrRemovePreviousRunMetricsFile(
 
   if (metrics_reporting_enabled) {
     // Enable reading any existing saved metrics.
-    file_metrics_provider->RegisterSource(
+    file_metrics_provider->RegisterSource(metrics::FileMetricsProvider::Params(
         metrics_file,
         metrics::FileMetricsProvider::SOURCE_HISTOGRAMS_ATOMIC_FILE,
-        association, metrics_name);
+        association, metrics_name));
   } else {
     // When metrics reporting is not enabled, any existing file should be
     // deleted in order to preserve user privacy.
@@ -246,10 +246,11 @@ std::unique_ptr<metrics::FileMetricsProvider> CreateFileMetricsProvider(
         ChromeMetricsServiceClient::kBrowserMetricsName);
     if (metrics_reporting_enabled) {
       file_metrics_provider->RegisterSource(
-          browser_metrics_upload_dir,
-          metrics::FileMetricsProvider::SOURCE_HISTOGRAMS_ATOMIC_DIR,
-          metrics::FileMetricsProvider::ASSOCIATE_INTERNAL_PROFILE,
-          ChromeMetricsServiceClient::kBrowserMetricsName);
+          metrics::FileMetricsProvider::Params(
+              browser_metrics_upload_dir,
+              metrics::FileMetricsProvider::SOURCE_HISTOGRAMS_ATOMIC_DIR,
+              metrics::FileMetricsProvider::ASSOCIATE_INTERNAL_PROFILE,
+              ChromeMetricsServiceClient::kBrowserMetricsName));
 
       base::FilePath active_path;
       base::GlobalHistogramAllocator::ConstructFilePaths(
@@ -258,10 +259,10 @@ std::unique_ptr<metrics::FileMetricsProvider> CreateFileMetricsProvider(
       // Register data that will be populated for the current run. "Active"
       // files need an empty "prefs_key" because they update the file itself.
       file_metrics_provider->RegisterSource(
-          active_path,
-          metrics::FileMetricsProvider::SOURCE_HISTOGRAMS_ACTIVE_FILE,
-          metrics::FileMetricsProvider::ASSOCIATE_CURRENT_RUN,
-          base::StringPiece());
+          metrics::FileMetricsProvider::Params(
+              active_path,
+              metrics::FileMetricsProvider::SOURCE_HISTOGRAMS_ACTIVE_FILE,
+              metrics::FileMetricsProvider::ASSOCIATE_CURRENT_RUN));
     } else {
       // When metrics reporting is not enabled, any existing files should be
       // deleted in order to preserve user privacy.
@@ -279,11 +280,11 @@ std::unique_ptr<metrics::FileMetricsProvider> CreateFileMetricsProvider(
   // Read metrics file from setup.exe.
   base::FilePath program_dir;
   base::PathService::Get(base::DIR_EXE, &program_dir);
-  file_metrics_provider->RegisterSource(
+  file_metrics_provider->RegisterSource(metrics::FileMetricsProvider::Params(
       program_dir.AppendASCII(installer::kSetupHistogramAllocatorName),
       metrics::FileMetricsProvider::SOURCE_HISTOGRAMS_ATOMIC_DIR,
       metrics::FileMetricsProvider::ASSOCIATE_CURRENT_RUN,
-      installer::kSetupHistogramAllocatorName);
+      installer::kSetupHistogramAllocatorName));
 #endif
 
   return file_metrics_provider;

components/metrics/file_metrics_provider.cc

@@ -5,6 +5,7 @@
 #include "components/metrics/file_metrics_provider.h"
 
 #include "base/command_line.h"
+#include "base/containers/flat_map.h"
 #include "base/files/file.h"
 #include "base/files/file_enumerator.h"
 #include "base/files/file_util.h"
@@ -114,8 +115,23 @@ scoped_refptr<base::TaskRunner> CreateBackgroundTaskRunner() {
 // This structure stores all the information about the sources being monitored
 // and their current reporting state.
 struct FileMetricsProvider::SourceInfo {
-  SourceInfo(SourceType source_type, SourceAssociation source_association)
-      : type(source_type), association(source_association) {}
+  SourceInfo(const Params& params)
+      : type(params.type),
+        association(params.association),
+        prefs_key(params.prefs_key),
+        filter(params.filter) {
+    switch (type) {
+      case SOURCE_HISTOGRAMS_ACTIVE_FILE:
+        DCHECK(prefs_key.empty());
+      // fall through
+      case SOURCE_HISTOGRAMS_ATOMIC_FILE:
+        path = params.path;
+        break;
+      case SOURCE_HISTOGRAMS_ATOMIC_DIR:
+        directory = params.path;
+        break;
+    }
+  }
   ~SourceInfo() {}
 
   // How to access this source (file/dir, atomic/active).
@@ -135,6 +151,9 @@ struct FileMetricsProvider::SourceInfo {
   // Name used inside prefs to persistent metadata.
   std::string prefs_key;
 
+  // The filter callback for determining what to do with found files.
+  FilterCallback filter;
+
   // The last-seen time of this source to detect change.
   base::Time last_seen;
 
@@ -149,6 +168,14 @@ struct FileMetricsProvider::SourceInfo {
   DISALLOW_COPY_AND_ASSIGN(SourceInfo);
 };
 
+FileMetricsProvider::Params::Params(const base::FilePath& path,
+                                    SourceType type,
+                                    SourceAssociation association,
+                                    base::StringPiece prefs_key)
+    : path(path), type(type), association(association), prefs_key(prefs_key) {}
+
+FileMetricsProvider::Params::~Params() {}
+
 FileMetricsProvider::FileMetricsProvider(PrefService* local_state)
     : task_runner_(CreateBackgroundTaskRunner()),
       pref_service_(local_state),
@@ -159,39 +186,23 @@ FileMetricsProvider::FileMetricsProvider(PrefService* local_state)
 
 FileMetricsProvider::~FileMetricsProvider() {}
 
-void FileMetricsProvider::RegisterSource(const base::FilePath& path,
-                                         SourceType type,
-                                         SourceAssociation source_association,
-                                         const base::StringPiece prefs_key) {
+void FileMetricsProvider::RegisterSource(const Params& params) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   // Ensure that kSourceOptions has been filled for this type.
-  DCHECK_GT(arraysize(kSourceOptions), static_cast<size_t>(type));
+  DCHECK_GT(arraysize(kSourceOptions), static_cast<size_t>(params.type));
 
-  std::unique_ptr<SourceInfo> source(new SourceInfo(type, source_association));
-  source->prefs_key = prefs_key.as_string();
-
-  switch (source->type) {
-    case SOURCE_HISTOGRAMS_ACTIVE_FILE:
-      DCHECK(prefs_key.empty());
-    // fall through
-    case SOURCE_HISTOGRAMS_ATOMIC_FILE:
-      source->path = path;
-      break;
-    case SOURCE_HISTOGRAMS_ATOMIC_DIR:
-      source->directory = path;
-      break;
-  }
+  std::unique_ptr<SourceInfo> source(new SourceInfo(params));
 
   // |prefs_key| may be empty if the caller does not wish to persist the
   // state across instances of the program.
-  if (pref_service_ && !prefs_key.empty()) {
+  if (pref_service_ && !params.prefs_key.empty()) {
     source->last_seen = base::Time::FromInternalValue(
         pref_service_->GetInt64(metrics::prefs::kMetricsLastSeenPrefix +
                                 source->prefs_key));
   }
 
-  switch (source_association) {
+  switch (params.association) {
     case ASSOCIATE_CURRENT_RUN:
     case ASSOCIATE_INTERNAL_PROFILE:
       sources_to_check_.push_back(std::move(source));
@@ -214,21 +225,26 @@ void FileMetricsProvider::RegisterPrefs(PrefRegistrySimple* prefs,
 // static
 void FileMetricsProvider::SetTaskRunnerForTesting(
     const scoped_refptr<base::TaskRunner>& task_runner) {
-  DCHECK(!g_task_runner_for_testing);
+  DCHECK(!g_task_runner_for_testing || !task_runner);
   g_task_runner_for_testing = task_runner.get();
 }
 
+// static
+void FileMetricsProvider::RecordAccessResult(AccessResult result) {
+  UMA_HISTOGRAM_ENUMERATION("UMA.FileMetricsProvider.AccessResult", result,
+                            ACCESS_RESULT_MAX);
+}
+
 // static
 bool FileMetricsProvider::LocateNextFileInDirectory(SourceInfo* source) {
   DCHECK_EQ(SOURCE_HISTOGRAMS_ATOMIC_DIR, source->type);
   DCHECK(!source->directory.empty());
 
-  // Open the directory and find all the files, remembering the oldest that
-  // has not been read. They can be removed and/or ignored if they're older
-  // than the last-check time.
-  base::Time oldest_file_time = base::Time::Now();
-  base::FilePath oldest_file_path;
+  // Open the directory and find all the files, remembering the last-modified
+  // time of each.
+  base::flat_map<base::Time, base::FilePath> found_files;
   base::FilePath file_path;
+  base::Time now_time = base::Time::Now();
   int file_count = 0;
   int delete_count = 0;
   base::FileEnumerator file_iter(source->directory, /*recursive=*/false,
@@ -258,11 +274,9 @@ bool FileMetricsProvider::LocateNextFileInDirectory(SourceInfo* source) {
     // Process real files.
     base::Time modified = file_info.GetLastModifiedTime();
     if (modified > source->last_seen) {
-      // This file hasn't been read. Remember it if it is older than others.
-      if (modified < oldest_file_time) {
-        oldest_file_path = std::move(file_path);
-        oldest_file_time = modified;
-      }
+      // This file hasn't been read. Remember it (unless it's from the future).
+      if (modified <= now_time)
+        found_files.emplace(modified, std::move(file_path));
       ++file_count;
     } else {
       // This file has been read. Try to delete it. Ignore any errors because
@@ -280,14 +294,22 @@ bool FileMetricsProvider::LocateNextFileInDirectory(SourceInfo* source) {
   UMA_HISTOGRAM_COUNTS_100("UMA.FileMetricsProvider.DeletedFiles",
                            delete_count);
 
-  // Stop now if there are no files to read.
-  if (oldest_file_path.empty())
-    return false;
+  // Filter files from the front until one is found for processing.
+  while (!found_files.empty()) {
+    base::FilePath& path = found_files.begin()->second;
+    AccessResult result = HandleFilterSource(source, path);
+    if (result == ACCESS_RESULT_SUCCESS) {
+      source->path = std::move(path);
+      return true;
+    }
+
+    // Record the result. Success will be recorded by the caller.
+    RecordAccessResult(result);
+    found_files.erase(found_files.begin());
+  }
 
-  // Set the active file to be the oldest modified file that has not yet
-  // been read.
-  source->path = std::move(oldest_file_path);
-  return true;
+  // No files to read.
+  return false;
 }
 
 // static
@@ -325,8 +347,7 @@ void FileMetricsProvider::CheckAndMergeMetricSourcesOnTaskRunner(
     // Some results are not reported in order to keep the dashboard clean.
     if (result != ACCESS_RESULT_DOESNT_EXIST &&
         result != ACCESS_RESULT_NOT_MODIFIED) {
-      UMA_HISTOGRAM_ENUMERATION(
-          "UMA.FileMetricsProvider.AccessResult", result, ACCESS_RESULT_MAX);
+      RecordAccessResult(result);
     }
 
     // Metrics associated with internal profiles have to be fetched directly
@@ -371,6 +392,13 @@ FileMetricsProvider::AccessResult FileMetricsProvider::CheckAndMapMetricSource(
   if (source->last_seen >= info.last_modified)
     return ACCESS_RESULT_NOT_MODIFIED;
 
+  // Non-directory files still need to be filtered.
+  if (source->directory.empty()) {
+    AccessResult result = HandleFilterSource(source, source->path);
+    if (result != ACCESS_RESULT_SUCCESS)
+      return result;
+  }
+
   // A new file of metrics has been found.
   base::File file(source->path, kSourceOptions[source->type].file_open_flags);
   if (!file.IsValid())
@@ -458,6 +486,48 @@ void FileMetricsProvider::RecordHistogramSnapshotsFromSource(
            << source->path.value();
 }
 
+FileMetricsProvider::AccessResult FileMetricsProvider::HandleFilterSource(
+    SourceInfo* source,
+    const base::FilePath& path) {
+  if (!source->filter)
+    return ACCESS_RESULT_SUCCESS;
+
+  // Alternatively, pass a Params object to the filter like what was originally
+  // used to configure the source.
+  // Params params(path, source->type, source->association, source->prefs_key);
+  switch (source->filter.Run(path)) {
+    case FILTER_PROCESS_FILE:
+      // Process the file.
+      return ACCESS_RESULT_SUCCESS;
+
+    case FILTER_TRY_LATER: {
+      // Touch the file with the current timestamp making it (presumably) the
+      // newest file in the directory.
+      base::Time now = base::Time::Now();
+      base::TouchFile(path, /*accessed=*/now, /*modified=*/now);
+      return ACCESS_RESULT_FILTER_TRY_LATER;
+    }
+
+    case FILTER_SKIP_FILE:
+      switch (source->type) {
+        case SOURCE_HISTOGRAMS_ATOMIC_FILE:
+        case SOURCE_HISTOGRAMS_ATOMIC_DIR:
+          // Only "atomic" files are deleted (best-effort).
+          DeleteFileWhenPossible(path);
+          break;
+        case SOURCE_HISTOGRAMS_ACTIVE_FILE:
+          // File will presumably get modified elsewhere and thus tried again.
+          break;
+      }
+      return ACCESS_RESULT_FILTER_SKIP_FILE;
+  }
+
+  // Code never gets here but some compilers don't realize that and so complain
+  // that "not all control paths return a value".
+  NOTREACHED();
+  return ACCESS_RESULT_SUCCESS;
+}
+
 void FileMetricsProvider::ScheduleSourcesCheck() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (sources_to_check_.empty())

components/metrics/file_metrics_provider.h

@@ -9,7 +9,7 @@
 #include <memory>
 #include <string>
 
-#include "base/callback.h"
+#include "base/callback_forward.h"
 #include "base/files/file_path.h"
 #include "base/gtest_prod_util.h"
 #include "base/memory/weak_ptr.h"
@@ -34,6 +34,8 @@ namespace metrics {
 class FileMetricsProvider : public MetricsProvider,
                             public base::StatisticsRecorder::HistogramProvider {
  public:
+  struct Params;
+
   enum SourceType {
     // "Atomic" files are a collection of histograms that are written
     // completely in a single atomic operation (typically a write followed
@@ -97,6 +99,51 @@ class FileMetricsProvider : public MetricsProvider,
     ASSOCIATE_INTERNAL_PROFILE_OR_PREVIOUS_RUN,
   };
 
+  enum FilterAction {
+    // Process this file normally.
+    FILTER_PROCESS_FILE,
+
+    // Try again. This could happen within milliseconds or minutes but no other
+    // files from the same source will get processed in between. The process
+    // must have permission to "touch" the file and alter its last-modified
+    // time because files are always processed in order of those stamps.
+    FILTER_TRY_LATER,
+
+    // Skip this file. This file will not be processed until it has changed
+    // (i.e. had its last-modifided time updated). If it is "atomic", an
+    // attempt will be made to delete it.
+    FILTER_SKIP_FILE,
+  };
+
+  // A "filter" can be defined to determine what to do on a per-file basis.
+  // This is called only after a file has been found to be the next one to
+  // be processed so it's okay if filter calls are relatively expensive.
+  // Calls are made on a background thread of low-priority and capable of
+  // doing I/O.
+  using FilterCallback =
+      base::RepeatingCallback<FilterAction(const base::FilePath& path)>;
+
+  // Parameters for RegisterSource, defined as a structure to allow new
+  // ones to be added (with default values) that doesn't require changes
+  // to all call sites.
+  struct Params {
+    Params(const base::FilePath& path,
+           SourceType type,
+           SourceAssociation association,
+           base::StringPiece prefs_key = base::StringPiece());
+
+    ~Params();
+
+    // The standard parameters, set during construction.
+    const base::FilePath path;
+    const SourceType type;
+    const SourceAssociation association;
+    const base::StringPiece prefs_key;
+
+    // Other parameters that can be set after construction.
+    FilterCallback filter;
+  };
+
   explicit FileMetricsProvider(PrefService* local_state);
   ~FileMetricsProvider() override;
 
@@ -107,10 +154,7 @@ class FileMetricsProvider : public MetricsProvider,
   // necessary keys in advance. Set |prefs_key| empty (nullptr will work) if
   // no persistence is required. ACTIVE files shouldn't have a pref key as
   // they update internal state about what has been previously sent.
-  void RegisterSource(const base::FilePath& path,
-                      SourceType type,
-                      SourceAssociation source_association,
-                      const base::StringPiece prefs_key);
+  void RegisterSource(const Params& params);
 
   // Registers all necessary preferences for maintaining persistent state
   // about a monitored file across process restarts. The |prefs_key| is
@@ -151,6 +195,12 @@ class FileMetricsProvider : public MetricsProvider,
     // File contents were internally deleted.
     ACCESS_RESULT_MEMORY_DELETED,
 
+    // File is scheduled to be tried again later.
+    ACCESS_RESULT_FILTER_TRY_LATER,
+
+    // The file was skipped according to filtering rules.
+    ACCESS_RESULT_FILTER_SKIP_FILE,
+
     ACCESS_RESULT_MAX
   };
 
@@ -159,6 +209,9 @@ class FileMetricsProvider : public MetricsProvider,
   struct SourceInfo;
   using SourceInfoList = std::list<std::unique_ptr<SourceInfo>>;
 
+  // Records an access result in a histogram.
+  static void RecordAccessResult(AccessResult result);
+
   // Looks for the next file to read within a directory. Returns true if a
   // file was found. This is part of CheckAndMapNewMetricSourcesOnTaskRunner
   // and so runs on an thread capable of I/O. The |source| structure will
@@ -183,6 +236,10 @@ class FileMetricsProvider : public MetricsProvider,
       base::HistogramSnapshotManager* snapshot_manager,
       SourceInfo* source);
 
+  // Calls source filter (if any) and returns the desired action.
+  static AccessResult HandleFilterSource(SourceInfo* source,
+                                         const base::FilePath& path);
+
   // Creates a task to check all monitored sources for updates.
   void ScheduleSourcesCheck();
 

tools/metrics/histograms/enums.xml

@@ -17278,6 +17278,8 @@ uploading your change for review.  These are checked by presubmit scripts.
   <int value="5" label="File has invalid contents."/>
   <int value="6" label="File could not be exclusively opened."/>
   <int value="7" label="File contents internally deleted."/>
+  <int value="8" label="File processing was postponed due to filtering."/>
+  <int value="9" label="File was skipped/deleted due to filtering."/>
 </enum>
 
 <enum name="FileMetricsProviderEmbeddedProfileResult">