Backed out changeset cb8d52e34d80 (bug 1689438) for xpcshell failures on test_client_id.js. CLOSED TREE
authorNarcis Beleuzu <nbeleuzu@mozilla.com>
Mon, 01 Feb 2021 17:53:33 +0200
changeset 565406 f6e71a0eec550054607f5ff7796ae7461dd0189d
parent 565405 9608ef6278b5a82fbad23364fe7deaf21982bd76
child 565407 a16a70db69d7b4425a9187abdd6de5aa6731715c
push id38160
push userapavel@mozilla.com
push dateMon, 01 Feb 2021 21:36:20 +0000
treeherdermozilla-central@f6e71a0eec55 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1689438
milestone87.0a1
backs outcb8d52e34d8019494c348a2e64df6e9d0a89fb4d
first release with
nightly linux32
f6e71a0eec55 / 87.0a1 / 20210201213620 / files
nightly linux64
f6e71a0eec55 / 87.0a1 / 20210201213620 / files
nightly mac
f6e71a0eec55 / 87.0a1 / 20210201213620 / files
nightly win32
f6e71a0eec55 / 87.0a1 / 20210201213620 / files
nightly win64
f6e71a0eec55 / 87.0a1 / 20210201213620 / files
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
releases
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Backed out changeset cb8d52e34d80 (bug 1689438) for xpcshell failures on test_client_id.js. CLOSED TREE
toolkit/components/telemetry/Scalars.yaml
toolkit/components/telemetry/app/ClientID.jsm
toolkit/components/telemetry/tests/unit/test_client_id.js
--- a/toolkit/components/telemetry/Scalars.yaml
+++ b/toolkit/components/telemetry/Scalars.yaml
@@ -3295,62 +3295,16 @@ telemetry:
     notification_emails:
       - sync-team@mozilla.com
     products:
       - 'firefox'
     release_channel_collection: opt-out
     record_in_processes:
       - main
 
-  state_file_save_errors:
-    bug_numbers:
-      - 1689438
-    description: >
-      How many times we errored out trying to save state.json
-    expires: "90"
-    kind: uint
-    notification_emails:
-      - chutten@mozilla.com
-    products:
-      - 'firefox'
-    release_channel_collection: opt-out
-    record_in_processes:
-      - main
-
-  generated_new_client_id:
-    bug_numbers:
-      - 1689438
-    description: >
-      Whether we generated a new client id this subsession.
-    expires: "90"
-    kind: boolean
-    notification_emails:
-      - chutten@mozilla.com
-    products:
-      - 'firefox'
-    release_channel_collection: opt-out
-    record_in_processes:
-      - main
-
-  loaded_client_id_doesnt_match_pref:
-    bug_numbers:
-      - 1689438
-    description: >
-      How often we loaded a client_id from disk that doesn't match
-      what's cached in the pref.
-    expires: "90"
-    kind: uint
-    notification_emails:
-      - chutten@mozilla.com
-    products:
-      - 'firefox'
-    release_channel_collection: opt-out
-    record_in_processes:
-      - main
-
 telemetry.discarded:
   accumulations:
     bug_numbers:
       - 1369041
     description: >
       Number of discarded accumulations to histograms in child processes
     expires: "never"
     kind: uint
--- a/toolkit/components/telemetry/app/ClientID.jsm
+++ b/toolkit/components/telemetry/app/ClientID.jsm
@@ -211,32 +211,16 @@ var ClientIDImpl = {
     let hasCurrentClientID = false;
     let hasCurrentEcosystemClientID = false;
     try {
       let state = await CommonUtils.readJSON(gStateFilePath);
       if (AppConstants.platform == "android" && state && "wasCanary" in state) {
         this._wasCanary = state.wasCanary;
       }
       if (state) {
-        try {
-          if (Services.prefs.prefHasUserValue(PREF_CACHED_CLIENTID)) {
-            let cachedID = Services.prefs.getStringPref(
-              PREF_CACHED_CLIENTID,
-              null
-            );
-            if (cachedID && cachedID != state.clientID) {
-              Services.telemetry.scalarAdd(
-                "telemetry.loaded_client_id_doesnt_match_pref",
-                1
-              );
-            }
-          }
-        } catch (e) {
-          // This data collection's not that important.
-        }
         hasCurrentClientID = this.updateClientID(state.clientID);
         hasCurrentEcosystemClientID = this.updateEcosystemClientID(
           state.ecosystemClientID
         );
         if (hasCurrentClientID && hasCurrentEcosystemClientID) {
           this._log.trace(`_doLoadClientIDs: Client IDs loaded from state.`);
           return {
             clientID: this._clientID,
@@ -245,17 +229,16 @@ var ClientIDImpl = {
         }
       }
     } catch (e) {
       // fall through to next option
     }
 
     // We're missing one or both IDs from the DRS state file, generate new ones.
     if (!hasCurrentClientID) {
-      Services.telemetry.scalarSet("telemetry.generated_new_client_id", true);
       this.updateClientID(CommonUtils.generateUUID());
     }
     if (!hasCurrentEcosystemClientID) {
       this.updateEcosystemClientID(CommonUtils.generateUUID());
     }
     this._saveClientIdsTask = this._saveClientIDs();
 
     // Wait on persisting the id. Otherwise failure to save the ID would result in
@@ -272,39 +255,34 @@ var ClientIDImpl = {
   },
 
   /**
    * Save the client ID to the client ID file.
    *
    * @return {Promise} A promise resolved when the client ID is saved to disk.
    */
   async _saveClientIDs() {
+    this._log.trace(`_saveClientIDs`);
+    let obj = {
+      clientID: this._clientID,
+      ecosystemClientID: this._ecosystemClientID,
+    };
+    // We detected a canary client ID when resetting, storing this as a flag
+    if (AppConstants.platform == "android" && this._wasCanary) {
+      obj.wasCanary = true;
+    }
     try {
-      this._log.trace(`_saveClientIDs`);
-      let obj = {
-        clientID: this._clientID,
-        ecosystemClientID: this._ecosystemClientID,
-      };
-      // We detected a canary client ID when resetting, storing this as a flag
-      if (AppConstants.platform == "android" && this._wasCanary) {
-        obj.wasCanary = true;
+      await IOUtils.makeDirectory(gDatareportingPath);
+    } catch (ex) {
+      if (ex.name != "NotAllowedError") {
+        throw ex;
       }
-      try {
-        await IOUtils.makeDirectory(gDatareportingPath);
-      } catch (ex) {
-        if (ex.name != "NotAllowedError") {
-          throw ex;
-        }
-      }
-      await CommonUtils.writeJSON(obj, gStateFilePath);
-      this._saveClientIdsTask = null;
-    } catch (ex) {
-      Services.telemetry.scalarAdd("telemetry.state_file_save_errors", 1);
-      throw ex;
     }
+    await CommonUtils.writeJSON(obj, gStateFilePath);
+    this._saveClientIdsTask = null;
   },
 
   /**
    * This returns a promise resolving to the the stable client ID we use for
    * data reporting (FHR & Telemetry).
    *
    * @return {Promise<string>} The stable client ID.
    */
--- a/toolkit/components/telemetry/tests/unit/test_client_id.js
+++ b/toolkit/components/telemetry/tests/unit/test_client_id.js
@@ -70,37 +70,29 @@ add_task(async function test_client_id()
     [-1, "setIntPref"],
     [0.5, "setIntPref"],
     ["INVALID-UUID", "setStringPref"],
     [true, "setBoolPref"],
     ["", "setStringPref"],
     ["3d1e1560-682a-4043-8cf2-aaaaaaaaaaaZ", "setStringPref"],
   ];
 
-  // Clear the scalar snapshot from previous tests.
-  Services.telemetry.getSnapshotForScalars("main", true);
-
   // If there is no DRS file, we should get a new client ID.
   await ClientID._reset();
-  await OS.File.remove(drsPath, { ignoreAbsent: true });
   let clientID = await ClientID.getClientID();
   Assert.equal(typeof clientID, "string");
   Assert.ok(uuidRegex.test(clientID));
   if (AppConstants.MOZ_GLEAN) {
     Assert.equal(
       Glean.fogValidation.legacyTelemetryClientId.testGetValue(
         "fog-validation"
       ),
       clientID
     );
   }
-  let snapshot = Services.telemetry.getSnapshotForScalars("main", true).parent;
-  Assert.equal(snapshot["telemetry.generated_new_client_id"], true);
-  // No file to read means no value to mismatch with pref.
-  Assert.ok(!("telemetry.loaded_client_id_doesnt_match_pref" in snapshot));
 
   // We should be guarded against invalid DRS json.
   await ClientID._reset();
   await OS.File.writeAtomic(drsPath, "abcd", {
     encoding: "utf-8",
     tmpPath: drsPath + ".tmp",
   });
   clientID = await ClientID.getClientID();
@@ -109,77 +101,34 @@ add_task(async function test_client_id()
   if (AppConstants.MOZ_GLEAN) {
     Assert.equal(
       Glean.fogValidation.legacyTelemetryClientId.testGetValue(
         "fog-validation"
       ),
       clientID
     );
   }
-  snapshot = Services.telemetry.getSnapshotForScalars("main", true).parent;
-  Assert.equal(snapshot["telemetry.generated_new_client_id"], true);
-  // Invalid file means no value to mismatch with pref.
-  Assert.ok(!("telemetry.loaded_client_id_doesnt_match_pref" in snapshot));
 
   // If the DRS data is broken, we should end up with a new client ID.
   for (let [invalidID] of invalidIDs) {
     await ClientID._reset();
     await CommonUtils.writeJSON({ clientID: invalidID }, drsPath);
     clientID = await ClientID.getClientID();
     Assert.equal(typeof clientID, "string");
     Assert.ok(uuidRegex.test(clientID));
     if (AppConstants.MOZ_GLEAN) {
       Assert.equal(
         Glean.fogValidation.legacyTelemetryClientId.testGetValue(
           "fog-validation"
         ),
         clientID
       );
     }
-    snapshot = Services.telemetry.getSnapshotForScalars("main", true).parent;
-    Assert.equal(snapshot["telemetry.generated_new_client_id"], true);
-    Assert.equal(snapshot["telemetry.loaded_client_id_doesnt_match_pref"], 1);
   }
 
-  // Test that valid DRS actually works.
-  const validClientID = "5afebd62-a33c-416c-b519-5c60fb988e8e";
-  await ClientID._reset();
-  await CommonUtils.writeJSON({ clientID: validClientID }, drsPath);
-  clientID = await ClientID.getClientID();
-  Assert.equal(clientID, validClientID);
-  if (AppConstants.MOZ_GLEAN) {
-    Assert.equal(
-      Glean.fogValidation.legacyTelemetryClientId.testGetValue(
-        "fog-validation"
-      ),
-      clientID
-    );
-  }
-  snapshot = Services.telemetry.getSnapshotForScalars("main", true).parent;
-  Assert.ok(!("telemetry.generated_new_client_id" in snapshot));
-  Assert.equal(snapshot["telemetry.loaded_client_id_doesnt_match_pref"], 1);
-
-  // Test that reloading a valid DRS works.
-  await ClientID._reset();
-  clientID = await ClientID.getClientID();
-  Assert.equal(clientID, validClientID);
-  if (AppConstants.MOZ_GLEAN) {
-    Assert.equal(
-      Glean.fogValidation.legacyTelemetryClientId.testGetValue(
-        "fog-validation"
-      ),
-      clientID
-    );
-  }
-  // snapshot may be empty if no other scalars are recorded.
-  snapshot =
-    Services.telemetry.getSnapshotForScalars("main", true).parent || {};
-  Assert.ok(!("telemetry.generated_new_client_id" in snapshot));
-  Assert.ok(!("telemetry.loaded_client_id_doesnt_match_pref" in snapshot));
-
   // Assure that cached IDs are being checked for validity.
   for (let [invalidID, prefFunc] of invalidIDs) {
     await ClientID._reset();
     Services.prefs[prefFunc](PREF_CACHED_CLIENTID, invalidID);
     let cachedID = ClientID.getCachedClientID();
     Assert.strictEqual(
       cachedID,
       null,