Bug 1788986 - Part 5: Add Glean metrics for _removeDirectory r=janv,valentin,necko-reviewers,chutten,nalexander,smaug

Differential Revision: https://phabricator.services.mozilla.com/D156941

netwerk/cache2/CacheFileIOManager.cpp

@@ -4143,7 +4143,7 @@ nsresult CacheFileIOManager::DispatchPurgeTask(
       do_GetService("@mozilla.org/backgroundtasksrunner;1");
 
   return runner->RemoveDirectoryInDetachedProcess(
-      path, aCacheDirName, aSecondsToWait, aPurgeExtension);
+      path, aCacheDirName, aSecondsToWait, aPurgeExtension, "HttpCache"_ns);
 #endif
 }
 

toolkit/components/backgroundtasks/BackgroundTask_removeDirectory.sys.mjs

@@ -7,13 +7,78 @@ ChromeUtils.defineESModuleGetters(lazy, {
   setTimeout: "resource://gre/modules/Timer.sys.mjs",
 });
 
+import { AppConstants } from "resource://gre/modules/AppConstants.sys.mjs";
 import { EXIT_CODE } from "resource://gre/modules/BackgroundTasksManager.sys.mjs";
 
+class Metrics {
+  /**
+   * @param {string} metricsId
+   */
+  constructor(metricsId) {
+    this.metricsId = metricsId;
+    this.startedTime = new Date();
+
+    this.wasFirst = true;
+    this.retryCount = 0;
+    this.removalCountObj = { value: 0 };
+    this.succeeded = true;
+
+    this.suffixRemovalCountObj = { value: 0 };
+    this.suffixEverFailed = false;
+  }
+
+  async report() {
+    if (!this.metricsId) {
+      console.warn(`Skipping Glean as no metrics id is passed`);
+      return;
+    }
+    if (AppConstants.MOZ_APP_NAME !== "firefox") {
+      console.warn(
+        `Skipping Glean as the app is not Firefox: ${AppConstants.MOZ_APP_NAME}`
+      );
+      return;
+    }
+
+    const elapsedMs = new Date().valueOf() - this.startedTime.valueOf();
+
+    // Note(krosylight): This FOG initialization happens within a unique
+    // temporary directory created for each background task, which will
+    // be removed after each run.
+    // That means any failed submission will be lost, but we are fine with
+    // that as we only have a single submission.
+    Services.fog.initializeFOG(undefined, "firefox.desktop.background.tasks");
+
+    const gleanMetrics = Glean[`backgroundTasksRmdir${this.metricsId}`];
+    if (!gleanMetrics) {
+      throw new Error(
+        `The metrics id "${this.metricsId}" is not available in toolkit/components/backgroundtasks/metrics.yaml. ` +
+          `Make sure that the id has no typo and is in PascalCase. ` +
+          `Note that you can omit the id for testing.`
+      );
+    }
+
+    gleanMetrics.elapsedMs.set(elapsedMs);
+    gleanMetrics.wasFirst.set(this.wasFirst);
+    gleanMetrics.retryCount.set(this.retryCount);
+    gleanMetrics.removalCount.set(this.removalCountObj.value);
+    gleanMetrics.succeeded.set(this.succeeded);
+    gleanMetrics.suffixRemovalCount.set(this.suffixRemovalCountObj.value);
+    gleanMetrics.suffixEverFailed.set(this.suffixEverFailed);
+
+    GleanPings.backgroundTasks.submit();
+
+    // XXX: We wait for arbitrary time for Glean to submit telemetry.
+    // Bug 1790702 should add a better way.
+    console.error("Pinged glean, waiting for submission.");
+    await new Promise(resolve => lazy.setTimeout(resolve, 5000));
+  }
+}
+
 // Recursively removes a directory.
 // Returns true if it succeeds, false otherwise.
-function tryRemoveDir(aFile) {
+function tryRemoveDir(aFile, countObj) {
   try {
-    aFile.remove(true);
+    aFile.remove(true, countObj);
   } catch (e) {
     return false;
   }
@@ -26,7 +91,8 @@ const FILE_CHECK_ITERATION_TIMEOUT_MS = 1000;
 async function deleteChildDirectory(
   parentDirPath,
   childDirName,
-  secondsToWait
+  secondsToWait,
+  metrics
 ) {
   if (!childDirName || !childDirName.length) {
     return;
@@ -44,17 +110,16 @@ async function deleteChildDirectory(
     Ci.nsICachePurgeLock
   );
 
-  let wasFirst = false;
   let locked = false;
   try {
     dirLock.lock(childDirName);
     locked = true;
-    wasFirst = !dirLock.isOtherInstanceRunning();
+    metrics.wasFirst = !dirLock.isOtherInstanceRunning();
   } catch (e) {
     console.error("Failed to check dirLock");
   }
 
-  if (!wasFirst) {
+  if (!metrics.wasFirst) {
     if (locked) {
       dirLock.unlock();
       locked = false;
@@ -67,9 +132,11 @@ async function deleteChildDirectory(
   // PR_CreateProcessDetached in CacheFileIOManager::SyncRemoveAllCacheFiles
   // Only if spawning the process is successful is the cache folder renamed,
   // so we need to wait until that is done.
-  let retryCount = 0;
   while (!targetFile.exists()) {
-    if (retryCount * FILE_CHECK_ITERATION_TIMEOUT_MS > secondsToWait * 1000) {
+    if (
+      metrics.retryCount * FILE_CHECK_ITERATION_TIMEOUT_MS >
+      secondsToWait * 1000
+    ) {
       // We don't know for sure if the folder was renamed or if a different
       // task removed it already. The second variant is more likely but to
       // be sure we'd have to consult a log file, which introduces
@@ -84,8 +151,8 @@ async function deleteChildDirectory(
     await new Promise(resolve =>
       lazy.setTimeout(resolve, FILE_CHECK_ITERATION_TIMEOUT_MS)
     );
-    retryCount++;
-    console.error(`Cache folder attempt no ${retryCount}`);
+    metrics.retryCount++;
+    console.error(`Cache folder attempt no ${metrics.retryCount}`);
   }
 
   if (!targetFile.isDirectory()) {
@@ -97,16 +164,29 @@ async function deleteChildDirectory(
   }
 
   console.error(`started removing ${targetFile.path}`);
-  targetFile.remove(true);
-  console.error(`done removing ${targetFile.path}`);
-
-  if (locked) {
-    dirLock.unlock();
-    locked = false;
+  try {
+    targetFile.remove(true, metrics.removalCountObj);
+  } catch (err) {
+    console.error(
+      `failed removing ${targetFile.path}. removed ${metrics.removalCountObj.value} entries.`
+    );
+    throw err;
+  } finally {
+    console.error(
+      `done removing ${targetFile.path}. removed ${metrics.removalCountObj.value} entries.`
+    );
+    if (locked) {
+      dirLock.unlock();
+      locked = false;
+    }
   }
 }
 
-async function cleanupOtherDirectories(parentDirPath, otherFoldersSuffix) {
+async function cleanupOtherDirectories(
+  parentDirPath,
+  otherFoldersSuffix,
+  metrics
+) {
   if (!otherFoldersSuffix || !otherFoldersSuffix.length) {
     return;
   }
@@ -152,11 +232,12 @@ async function cleanupOtherDirectories(parentDirPath, otherFoldersSuffix) {
     }
 
     // Remove directory recursively.
-    let removedDir = tryRemoveDir(entry);
+    let removedDir = tryRemoveDir(entry, metrics.suffixRemovalCountObj);
     if (!removedDir && entry.exists()) {
       // If first deletion of the directory failed, then we try again once more
       // just in case.
-      removedDir = tryRemoveDir(entry);
+      metrics.suffixEverFailed = true;
+      removedDir = tryRemoveDir(entry, metrics.suffixRemovalCountObj);
     }
     console.error(
       `Deletion of folder ${entry.leafName} - success=${removedDir}`
@@ -166,8 +247,10 @@ async function cleanupOtherDirectories(parentDirPath, otherFoldersSuffix) {
 }
 
 // Usage:
-// removeDirectory parentDirPath childDirName secondsToWait [otherFoldersSuffix] [--test-sleep testSleep]
+// removeDirectory parentDirPath childDirName secondsToWait [otherFoldersSuffix]
 //                  arg0           arg1     arg2            arg3
+//                 [--test-sleep testSleep]
+//                 [--metrics-id metricsId]
 // parentDirPath - The path to the parent directory that includes the target directory
 // childDirName - The "leaf name" of the moved cache directory
 //                If empty, the background task will only purge folders that have the "otherFoldersSuffix".
@@ -177,10 +260,14 @@ async function cleanupOtherDirectories(parentDirPath, otherFoldersSuffix) {
 //                      the parent dir that end with this suffix
 // testSleep - [optional] A test-only argument to sleep for a given milliseconds before removal.
 //             This exists to test whether a long-running task can survive.
+// metricsId - [optional] The identifier for Glean metrics, in PascalCase.
+//             It'll be submitted only when the matching identifier exists in
+//             toolkit/components/backgroundtasks/metrics.yaml.
 export async function runBackgroundTask(commandLine) {
   const testSleep = Number.parseInt(
     commandLine.handleFlagWithParam("test-sleep", false)
   );
+  const metricsId = commandLine.handleFlagWithParam("metrics-id", false) || "";
 
   if (commandLine.length < 3) {
     throw new Error("Insufficient arguments");
@@ -206,16 +293,34 @@ export async function runBackgroundTask(commandLine) {
     );
   }
 
-  console.error(parentDirPath, childDirName, secondsToWait, otherFoldersSuffix);
+  console.error(
+    parentDirPath,
+    childDirName,
+    secondsToWait,
+    otherFoldersSuffix,
+    metricsId
+  );
 
   if (!Number.isNaN(testSleep)) {
     await new Promise(resolve => lazy.setTimeout(resolve, testSleep));
   }
 
-  await deleteChildDirectory(parentDirPath, childDirName, secondsToWait);
-  await cleanupOtherDirectories(parentDirPath, otherFoldersSuffix);
+  const metrics = new Metrics(metricsId);
 
-  // TODO: event telemetry with timings, and how often we have left over cache folders from previous runs.
+  try {
+    await deleteChildDirectory(
+      parentDirPath,
+      childDirName,
+      secondsToWait,
+      metrics
+    );
+    await cleanupOtherDirectories(parentDirPath, otherFoldersSuffix, metrics);
+  } catch (err) {
+    metrics.succeeded = false;
+    throw err;
+  } finally {
+    await metrics.report();
+  }
 
   return EXIT_CODE.SUCCESS;
 }

toolkit/components/backgroundtasks/BackgroundTasksRunner.cpp

@@ -66,7 +66,8 @@ NS_IMETHODIMP BackgroundTasksRunner::RunInDetachedProcess(
 
 NS_IMETHODIMP BackgroundTasksRunner::RemoveDirectoryInDetachedProcess(
     const nsACString& aParentDirPath, const nsACString& aChildDirName,
-    const nsACString& aSecondsToWait, const nsACString& aOtherFoldersSuffix) {
+    const nsACString& aSecondsToWait, const nsACString& aOtherFoldersSuffix,
+    const nsACString& aMetricsId) {
   nsTArray<nsCString> argv = {aParentDirPath + ""_ns, aChildDirName + ""_ns,
                               aSecondsToWait + ""_ns,
                               aOtherFoldersSuffix + ""_ns};
@@ -79,6 +80,10 @@ NS_IMETHODIMP BackgroundTasksRunner::RemoveDirectoryInDetachedProcess(
     sleep.AppendInt(testingSleepMs);
     argv.AppendElement(sleep);
   }
+  if (!aMetricsId.IsEmpty()) {
+    argv.AppendElement("--metrics-id");
+    argv.AppendElement(aMetricsId);
+  }
 
   return RunInDetachedProcess("removeDirectory"_ns, argv);
 }

toolkit/components/backgroundtasks/metrics.yaml

@@ -0,0 +1,75 @@
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
+
+# Adding a new metric? We have docs for that!
+# https://firefox-source-docs.mozilla.org/toolkit/components/glean/user/new_definitions_file.html
+
+---
+$schema: moz://mozilla.org/schemas/glean/metrics/2-0-0
+$tags:
+  - 'Toolkit :: Background Tasks'
+
+# Use the `rmdir` YAML reference when you want to get your own metrics id for
+# BackgroundTask_removeDirectory.
+# Do not use this base metrics directly.
+background_tasks.rmdir.base: &rmdir
+  metric_base: &metric_base
+    expires: never
+    send_in_pings:
+      - remove-directory
+    bugs:
+      - https://bugzilla.mozilla.org/show_bug.cgi?id=1788986
+    data_reviews:
+      - https://bugzilla.mozilla.org/show_bug.cgi?id=1788986
+    notification_emails:
+      - krosylight@mozilla.com
+      - vgosu@mozilla.com
+    # Override below. These exist solely to workaround validation issues.
+    type: event
+    description: The base metric template for removeDirectory.
+  elapsed_ms:
+    <<: *metric_base
+    type: quantity
+    unit: milliseconds
+    description: >
+      The amount of time the task took for removing the directories.
+  was_first:
+    <<: *metric_base
+    type: boolean
+    description: >
+      Whether this task is the first one removing the directory.
+  retry_count:
+    <<: *metric_base
+    type: quantity
+    unit: files
+    description: >
+      The number of retries before the task started removing the child
+      directory. This can happen when the target directory doesn't exist.
+  removal_count:
+    <<: *metric_base
+    type: quantity
+    unit: files
+    description: >
+      The number of the removed entries at each call, even if the target
+      directory itself couldn't be removed.
+  succeeded:
+    <<: *metric_base
+    type: boolean
+    description: Whether the target directory removal succeeded.
+  suffix_removal_count:
+    <<: *metric_base
+    type: quantity
+    unit: files
+    description: >
+      The number of the removed entries of the suffixed directories.
+  suffix_ever_failed:
+    <<: *metric_base
+    type: boolean
+    description: Whether removing the suffixed directories ever failed.
+
+# Metrics identifiers for each use
+background_tasks.rmdir.quota:
+  <<: *rmdir
+background_tasks.rmdir.http_cache:
+  <<: *rmdir

toolkit/components/backgroundtasks/nsIBackgroundTasksRunner.idl

@@ -35,5 +35,6 @@ interface nsIBackgroundTasksRunner : nsISupports
   void removeDirectoryInDetachedProcess(in ACString aParentDirPath,
                                         in ACString aChildDirName,
                                         in ACString aSecondsToWait,
-                                        in ACString aOtherFoldersSuffix);
+                                        in ACString aOtherFoldersSuffix,
+                                        [optional] in ACString aMetricsId);
 };

toolkit/components/backgroundtasks/pings.yaml

@@ -0,0 +1,20 @@
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
+
+---
+$schema: moz://mozilla.org/schemas/glean/pings/2-0-0
+
+background-tasks:
+  description: |
+    This ping is generic for background tasks. Each background task can
+    gather its metrics under this ping and submit it when the task finishes.
+    Note that the ping submission must be done manually.
+  include_client_id: false
+  bugs:
+    - https://bugzilla.mozilla.org/show_bug.cgi?id=1788986
+  data_reviews:
+    - https://bugzilla.mozilla.org/show_bug.cgi?id=1788986
+  notification_emails:
+    - krosylight@mozilla.com
+    - vgosu@mozilla.com

toolkit/components/cleardata/ClearDataService.sys.mjs

@@ -760,7 +760,8 @@ const QuotaCleaner = {
       storageDir,
       "to-be-removed",
       "0",
-      ""
+      "",
+      "Quota"
     );
   },
 };

toolkit/components/glean/metrics_index.py

@@ -51,6 +51,12 @@ background_update_metrics = [
     "toolkit/mozapps/update/metrics.yaml",
 ]
 
+# Metrics that are sent by the Firefox Desktop Background Tasks
+# Order is lexicographical, enforced by t/c/glean/tests/pytest/test_yaml_indices.py
+background_tasks_metrics = [
+    "toolkit/components/backgroundtasks/metrics.yaml",
+]
+
 # Test metrics
 # Order is lexicographical, enforced by t/c/glean/tests/pytest/test_yaml_indices.py
 test_metrics = [
@@ -60,7 +66,11 @@ test_metrics = [
 # The list of all Glean metrics.yaml files, relative to the top src dir.
 # ONLY TO BE MODIFIED BY FOG PEERS!
 metrics_yamls = (
-    gecko_metrics + firefox_desktop_metrics + background_update_metrics + test_metrics
+    gecko_metrics
+    + firefox_desktop_metrics
+    + background_update_metrics
+    + background_tasks_metrics
+    + test_metrics
 )
 
 # Pings that are sent by Gecko and everyone using Gecko
@@ -85,6 +95,12 @@ background_update_pings = [
     "toolkit/mozapps/update/pings.yaml",
 ]
 
+# Pings that are sent by the Firefox Desktop Background Tasks
+# Order is lexicographical, enforced by t/c/glean/tests/pytest/test_yaml_indices.py
+background_tasks_pings = [
+    "toolkit/components/backgroundtasks/pings.yaml",
+]
+
 # Test pings
 # Order is lexicographical, enforced by t/c/glean/tests/pytest/test_yaml_indices.py
 test_pings = [
@@ -103,7 +119,13 @@ pings_by_app_id = {
 
 # The list of all Glean pings.yaml files, relative to the top src dir.
 # ONLY TO BE MODIFIED BY FOG PEERS!
-pings_yamls = gecko_pings + firefox_desktop_pings + background_update_pings + test_pings
+pings_yamls = (
+    gecko_pings
+    + firefox_desktop_pings
+    + background_update_pings
+    + background_tasks_pings
+    + test_pings
+)
 
 # The list of tags that are allowed in the above to files, and their
 # descriptions. Currently we restrict to a set scraped from bugzilla