Bug 1409323 - Allow to register new addon scalars for an existing category. r?chutten draft
authorAlessio Placitelli <alessio.placitelli@gmail.com>
Tue, 17 Oct 2017 16:00:12 +0200
changeset 694185 9f07186edd098ce289edc432710e0bbc3d9d667c
parent 693376 179dae92e4d794e7f45ad080ff01908c80691f31
child 739284 9cde7e95b8601f2dcf77e8b9e270b522d13175fd
push id88074
push useralessio.placitelli@gmail.com
push dateTue, 07 Nov 2017 14:24:40 +0000
reviewerschutten
bugs1409323
milestone58.0a1
Bug 1409323 - Allow to register new addon scalars for an existing category. r?chutten If we attempt to register again an old dynamic scalar, check if the new definition makes it expired. If so, update the expiration state and propagate the new "expired" scalar to the content proecesses. MozReview-Commit-ID: A2LFN1irw4i
toolkit/components/telemetry/TelemetryScalar.cpp
toolkit/components/telemetry/docs/collection/scalars.rst
toolkit/components/telemetry/tests/browser/browser_DynamicScalars.js
toolkit/components/telemetry/tests/unit/test_TelemetryScalars.js
--- a/toolkit/components/telemetry/TelemetryScalar.cpp
+++ b/toolkit/components/telemetry/TelemetryScalar.cpp
@@ -115,18 +115,16 @@ enum class ScalarResult : uint8_t {
   KeyIsEmpty,
   KeyTooLong,
   TooManyKeys,
   // String Scalar Errors
   StringTooLong,
   // Unsigned Scalar Errors
   UnsignedNegativeValue,
   UnsignedTruncatedValue,
-  // Dynamic Scalar Errors
-  AlreadyRegistered,
 };
 
 // A common identifier for both built-in and dynamic scalars.
 struct ScalarKey {
   uint32_t id;
   bool dynamic;
 };
 
@@ -1117,20 +1115,32 @@ internal_GetScalarByEnum(const StaticMut
   // available.
   if (!gScalarStorageMap.Get(storageId, &scalarStorage)) {
     scalarStorage = new ScalarStorageMapType();
     gScalarStorageMap.Put(storageId, scalarStorage);
   }
 
   // Check if the scalar is already allocated in the parent or in the child storage.
   if (scalarStorage->Get(aId.id, &scalar)) {
+    // Dynamic scalars can expire at any time during the session (e.g. an
+    // add-on was updated). Check if it expired.
+    if (aId.dynamic) {
+      const DynamicScalarInfo& dynInfo = static_cast<const DynamicScalarInfo&>(info);
+      if (dynInfo.mDynamicExpiration) {
+        // The Dynamic scalar is expired.
+        return NS_ERROR_NOT_AVAILABLE;
+      }
+    }
+    // This was not a dynamic scalar or was not expired.
     *aRet = scalar;
     return NS_OK;
   }
 
+  // The scalar storage wasn't already allocated. Check if the scalar is expired and
+  // then allocate the storage, if needed.
   if (IsExpiredVersion(info.expiration())) {
     return NS_ERROR_NOT_AVAILABLE;
   }
 
   scalar = internal_ScalarAllocate(info.kind);
   if (!scalar) {
     return NS_ERROR_INVALID_ARG;
   }
@@ -1396,40 +1406,42 @@ internal_BroadcastDefinitions(const Stat
   internal_DynamicScalarToIPC(lock, scalarInfos, ipcDefinitions);
 
   // Broadcast the definitions to the other content processes.
   for (auto parent : parents) {
       mozilla::Unused << parent->SendAddDynamicScalars(ipcDefinitions);
   }
 }
 
-ScalarResult
+void
 internal_RegisterScalars(const StaticMutexAutoLock& lock,
                          const nsTArray<DynamicScalarInfo>& scalarInfos)
 {
-  // Check that none of the events are already registered.
-  for (auto& info : scalarInfos) {
-    if (gScalarNameIDMap.GetEntry(info.name())) {
-      return ScalarResult::AlreadyRegistered;
-    }
-  }
-
   // Register the new scalars.
   if (!gDynamicScalarInfo) {
     gDynamicScalarInfo = new nsTArray<DynamicScalarInfo>();
   }
 
   for (auto scalarInfo : scalarInfos) {
+    // Allow expiring scalars that were already registered.
+    CharPtrEntryType *existingKey = gScalarNameIDMap.GetEntry(scalarInfo.name());
+    if (existingKey) {
+      // Change the scalar to expired if needed.
+      if (scalarInfo.mDynamicExpiration) {
+        DynamicScalarInfo& scalarData = (*gDynamicScalarInfo)[existingKey->mData.id];
+        scalarData.mDynamicExpiration = true;
+      }
+      continue;
+    }
+
     gDynamicScalarInfo->AppendElement(scalarInfo);
     uint32_t scalarId = gDynamicScalarInfo->Length() - 1;
     CharPtrEntryType *entry = gScalarNameIDMap.PutEntry(scalarInfo.name());
     entry->mData = ScalarKey{scalarId, true};
   }
-
-  return ScalarResult::Ok;
 }
 
 } // namespace
 
 ////////////////////////////////////////////////////////////////////////
 ////////////////////////////////////////////////////////////////////////
 //
 // EXTERNALLY VISIBLE FUNCTIONS in namespace TelemetryScalars::
@@ -2419,26 +2431,19 @@ TelemetryScalar::RegisterScalars(const n
     // We defer the actual registration here in case any other event description is invalid.
     // In that case we don't need to roll back any partial registration.
     newScalarInfos.AppendElement(DynamicScalarInfo{
       kind, recordOnRelease, expired, fullName, keyed
     });
   }
 
   // Register the dynamic definition on the parent process.
-  ScalarResult res = ScalarResult::Ok;
   {
     StaticMutexAutoLock locker(gTelemetryScalarsMutex);
-    res = ::internal_RegisterScalars(locker, newScalarInfos);
-
-
-    if (res == ScalarResult::AlreadyRegistered) {
-      JS_ReportErrorASCII(cx, "Attempt to register a scalar that is already registered.");
-      return NS_ERROR_INVALID_ARG;
-    }
+    ::internal_RegisterScalars(locker, newScalarInfos);
 
     // Propagate the registration to all the content-processes. Please note that
     // this does not require to hold the mutex.
     ::internal_BroadcastDefinitions(locker, newScalarInfos);
   }
   return NS_OK;
 }
 
@@ -2735,17 +2740,20 @@ TelemetryScalar::GetDynamicScalarDefinit
   }
 
   StaticMutexAutoLock locker(gTelemetryScalarsMutex);
   internal_DynamicScalarToIPC(locker, *gDynamicScalarInfo, aDefArray);
 }
 
 /**
  * This adds the dynamic scalar definitions coming from
- * the parent process to this child process.
+ * the parent process to this child process. If a dynamic
+ * scalar definition is already defined, check if the new definition
+ * makes the scalar expired and eventually update the expiration
+ * state.
  */
 void
 TelemetryScalar::AddDynamicScalarDefinitions(
   const nsTArray<DynamicScalarDefinition>& aDefs)
 {
   MOZ_ASSERT(!XRE_IsParentProcess());
 
   nsTArray<DynamicScalarInfo> dynamicStubs;
--- a/toolkit/components/telemetry/docs/collection/scalars.rst
+++ b/toolkit/components/telemetry/docs/collection/scalars.rst
@@ -56,16 +56,18 @@ For scalars recorded from add-ons, regis
 After registration, the scalars can be recorded through the usual scalar JS API. If the accumulation happens in a content process right after the registration and the definition still has to reach this process, it will be discarded: one way to work around the problem is to send an IPC message to the content process and start accumulating data once this message has been received. The accumulated data will be submitted in the main pings payload under ``processes.dynamic.scalars``.
 
 .. note::
 
     Accumulating in dynamic scalars only works in content child processes and in the parent process. All the accumulations (parent and content chldren) are aggregated together .
 
 New scalars registered here are subject to the same :ref:`limitations <scalar-limitations>` as the ones registered through ``Scalars.yaml``, e.g. the length of the category name or the allowed characters.
 
+When add-ons are updated, they may re-register all of their scalars. In that case, any changes to scalars that are already registered are ignored. The only exception is expiry; a scalar that is re-registered with ``expired: true`` will not be recorded anymore.
+
 Example:
 
 .. code-block:: js
 
   Services.telemetry.registerScalars("myAddon.category", {
     "counter_scalar": {
       kind: Ci.nsITelemetry.SCALAR_TYPE_COUNT,
       keyed: false,
@@ -279,9 +281,12 @@ The ``ScalarID`` enum is automatically g
 Other examples can be found in the `test coverage <https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/tests/gtest/TestScalars.cpp>`_ for the scalars C++ API.
 
 Version History
 ===============
 
 - Firefox 50: Initial scalar support (`bug 1276195 <https://bugzilla.mozilla.org/show_bug.cgi?id=1276195>`_).
 - Firefox 51: Added keyed scalars (`bug 1277806 <https://bugzilla.mozilla.org/show_bug.cgi?id=1277806>`_).
 - Firefox 53: Added child process scalars (`bug 1278556 <https://bugzilla.mozilla.org/show_bug.cgi?id=1278556>`_).
-- Firefox 58: Added support for recording new scalars from add-ons (`bug 1393801 <bug https://bugzilla.mozilla.org/show_bug.cgi?id=1393801>`_).
+- Firefox 58
+
+  - Added support for recording new scalars from add-ons (`bug 1393801 <bug https://bugzilla.mozilla.org/show_bug.cgi?id=1393801>`_).
+  - Ignore re-registering existing scalars for a category instead of failing (`bug 1409323 <https://bugzilla.mozilla.org/show_bug.cgi?id=1409323>`_).
--- a/toolkit/components/telemetry/tests/browser/browser_DynamicScalars.js
+++ b/toolkit/components/telemetry/tests/browser/browser_DynamicScalars.js
@@ -34,63 +34,84 @@ add_task(async function test_recording()
   // Register test scalars before spawning the content process: the scalar
   // definitions will propagate to it.
   Services.telemetry.registerScalars("telemetry.test.dynamic", {
     "pre_content_spawn": {
       kind: Ci.nsITelemetry.SCALAR_TYPE_COUNT,
       keyed: false,
       record_on_release: true
     },
+    "pre_content_spawn_expiration": {
+      kind: Ci.nsITelemetry.SCALAR_TYPE_COUNT,
+      keyed: false,
+      release_channel_collection: true
+    },
   });
 
+  Services.telemetry.scalarSet("telemetry.test.dynamic.pre_content_spawn_expiration", 3);
+
   let processCreated = TestUtils.topicObserved(CONTENT_CREATED);
   await BrowserTestUtils.withNewTab({ gBrowser, url: "about:blank", forceNewProcess: true },
                                     async function(browser) {
       // Make sure our new browser is in its own process. The processCreated
       // promise should have already resolved by this point.
       await processCreated;
       let newPid = browser.frameLoader.tabParent.osPid;
       ok(currentPid != newPid, "The new tab must spawn its own process");
 
-      // Register a test scalar after spawning the content process: the scalar
+      // Register test scalars after spawning the content process: the scalar
       // definitions will propagate to it.
+      // Also attempt to register again "pre_content_spawn_expiration" and set
+      // it to expired.
       Services.telemetry.registerScalars("telemetry.test.dynamic", {
         "post_content_spawn": {
           kind: Ci.nsITelemetry.SCALAR_TYPE_BOOLEAN,
           keyed: false,
           release_channel_collection: false
         },
         "post_content_spawn_keyed": {
           kind: Ci.nsITelemetry.SCALAR_TYPE_COUNT,
           keyed: true,
           release_channel_collection: true
         },
+        "pre_content_spawn_expiration": {
+          kind: Ci.nsITelemetry.SCALAR_TYPE_COUNT,
+          keyed: false,
+          release_channel_collection: true,
+          expired: true
+        },
       });
 
       // Accumulate from the content process into both dynamic scalars.
       await ContentTask.spawn(browser, {}, async function() {
+        Services.telemetry.scalarAdd("telemetry.test.dynamic.pre_content_spawn_expiration", 1);
         Services.telemetry.scalarSet("telemetry.test.dynamic.pre_content_spawn", 3);
         Services.telemetry.scalarSet("telemetry.test.dynamic.post_content_spawn", true);
         Services.telemetry.keyedScalarSet("telemetry.test.dynamic.post_content_spawn_keyed",
                                           "testKey", 3);
       });
   });
 
   // Wait for the dynamic scalars to appear non-keyed snapshots.
-  await waitForProcessesScalars(["dynamic"], false);
+  await waitForProcessesScalars(["dynamic"], false, scalars => {
+    // Wait for the scalars set in the content process to be available.
+    return "telemetry.test.dynamic.pre_content_spawn" in scalars.dynamic;
+  });
 
   // Verify the content of the snapshots.
   const scalars =
       Services.telemetry.snapshotScalars(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN);
   ok("dynamic" in scalars,
      "The scalars must contain the 'dynamic' process section");
   ok("telemetry.test.dynamic.pre_content_spawn" in scalars.dynamic,
      "Dynamic scalars registered before a process spawns must be present.");
   is(scalars.dynamic["telemetry.test.dynamic.pre_content_spawn"], 3,
     "The dynamic scalar must contain the expected value.");
+  is(scalars.dynamic["telemetry.test.dynamic.pre_content_spawn_expiration"], 3,
+    "The dynamic scalar must not be updated after being expired.");
   ok("telemetry.test.dynamic.post_content_spawn" in scalars.dynamic,
      "Dynamic scalars registered after a process spawns must be present.");
   is(scalars.dynamic["telemetry.test.dynamic.post_content_spawn"], true,
      "The dynamic scalar must contain the expected value.");
 
   // Wait for the dynamic scalars to appear in the keyed snapshots.
   await waitForProcessesScalars(["dynamic"], true);
 
--- a/toolkit/components/telemetry/tests/unit/test_TelemetryScalars.js
+++ b/toolkit/components/telemetry/tests/unit/test_TelemetryScalars.js
@@ -593,36 +593,82 @@ add_task(function* test_dynamicScalars_r
         },
         "invalid_scalar": {
           expired: false,
         },
       },
       "evaluation": /Invalid or missing 'kind'/,
       "description": "No scalar must be registered if the batch contains an invalid one"
     },
-    {
-      "category": "telemetry.test",
-      "data": {
-        "unsigned_int_kind": {
-          kind: Ci.nsITelemetry.SCALAR_TYPE_COUNT,
-          keyed: false,
-          record_on_release: true
-        },
-      },
-      "evaluation": /already registered/,
-      "description": "Registration must fail if a scalar with the same name exists already"
-    },
   ];
 
   for (let testCase of TEST_CASES) {
     Assert.throws(() => Telemetry.registerScalars(testCase.category, testCase.data),
                   testCase.evaluation, testCase.description);
   }
 });
 
+add_task(function* test_dynamicScalars_doubleRegistration() {
+  Telemetry.clearScalars();
+
+  // Register a test scalar.
+  Telemetry.registerScalars("telemetry.test.dynamic", {
+    "double_registration_1": {
+      kind: Ci.nsITelemetry.SCALAR_TYPE_COUNT,
+      record_on_release: true
+    },
+  });
+
+  // Verify that we can record the scalar.
+  Telemetry.scalarSet("telemetry.test.dynamic.double_registration_1", 1);
+
+  // Register the same scalar again, along with a second scalar.
+  // This must not throw.
+  Telemetry.registerScalars("telemetry.test.dynamic", {
+    "double_registration_1": {
+      kind: Ci.nsITelemetry.SCALAR_TYPE_COUNT,
+      record_on_release: true
+    },
+    "double_registration_2": {
+      kind: Ci.nsITelemetry.SCALAR_TYPE_COUNT,
+      record_on_release: true
+    },
+  });
+
+  // Set the dynamic scalars to some test values.
+  Telemetry.scalarAdd("telemetry.test.dynamic.double_registration_1", 1);
+  Telemetry.scalarSet("telemetry.test.dynamic.double_registration_2", 3);
+
+  // Get a snapshot of the scalars and check that the dynamic ones were correctly set.
+  let scalars =
+    getProcessScalars(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, "dynamic", false, false);
+
+  Assert.equal(scalars["telemetry.test.dynamic.double_registration_1"], 2,
+               "The recorded scalar must contain the right value.");
+  Assert.equal(scalars["telemetry.test.dynamic.double_registration_2"], 3,
+               "The recorded scalar must contain the right value.");
+
+  // Register an existing scalar again, only change the definition
+  // to make it expire.
+  Telemetry.registerScalars("telemetry.test.dynamic", {
+    "double_registration_2": {
+      kind: Ci.nsITelemetry.SCALAR_TYPE_COUNT,
+      record_on_release: true,
+      expired: true
+    },
+  });
+
+  // Attempt to record and make sure that no recording happens.
+  Telemetry.scalarAdd("telemetry.test.dynamic.double_registration_2", 1);
+  scalars =
+    getProcessScalars(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, "dynamic", false, false);
+  Assert.equal(scalars["telemetry.test.dynamic.double_registration_2"], 3,
+               "The recorded scalar must contain the right value.");
+});
+
 add_task(function* test_dynamicScalars_recording() {
   Telemetry.clearScalars();
 
   // Disable extended recording so that we will just record opt-out.
   Telemetry.canRecordExtended = false;
 
   // Register some test scalars.
   Telemetry.registerScalars("telemetry.test.dynamic", {