bug 1498172 - Support Scalar Snapshotting from multiple stores r=janerik
authorChris H-C <chutten@mozilla.com>
Thu, 22 Nov 2018 14:55:36 +0000
changeset 506903 f643580be3cb0f73a5f175fa366730ab16f6a42b
parent 506902 9d7f274f9f908875662c7a3e42a143b9b1d7da00
child 506904 9a4db52c22c20c1d872c1eaac1bafd4b6b1cca66
push id1905
push userffxbld-merge
push dateMon, 21 Jan 2019 12:33:13 +0000
treeherdermozilla-release@c2fca1944d8c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjanerik
bugs1498172
milestone65.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
bug 1498172 - Support Scalar Snapshotting from multiple stores r=janerik And test it. Paritcular annoying bits are that we now have to iterate over Scalar objects that are -present- even if they don't have data for the store being snapshot. To maintain our previous semantics about not having process entries in the snapshot object when the process is null (and, similarly for keyed scalars), we need to retcon out some things during the snapshotting process. Differential Revision: https://phabricator.services.mozilla.com/D12545
toolkit/components/telemetry/Scalars.yaml
toolkit/components/telemetry/core/TelemetryScalar.cpp
toolkit/components/telemetry/tests/unit/test_TelemetryScalars_multistore.js
toolkit/components/telemetry/tests/unit/xpcshell.ini
--- a/toolkit/components/telemetry/Scalars.yaml
+++ b/toolkit/components/telemetry/Scalars.yaml
@@ -2510,11 +2510,57 @@ telemetry.test:
     notification_emails:
       - telemetry-client-dev@mozilla.com
     record_in_processes:
       - 'main'
     record_into_store:
       - 'main'
       - 'sync'
 
+  multiple_stores_string:
+    bug_numbers:
+      - 1498164
+    description: >
+      This is a test string type with multiple stores.
+    expires: never
+    kind: string
+    notification_emails:
+      - telemetry-client-dev@mozilla.com
+    record_in_processes:
+      - 'main'
+    record_into_store:
+      - 'main'
+      - 'sync'
+
+  multiple_stores_bool:
+    bug_numbers:
+      - 1498164
+    description: >
+      This is a test bool type with multiple stores.
+    expires: never
+    kind: boolean
+    notification_emails:
+      - telemetry-client-dev@mozilla.com
+    record_in_processes:
+      - 'main'
+    record_into_store:
+      - 'main'
+      - 'sync'
+
+  multiple_stores_keyed:
+    bug_numbers:
+      - 1498164
+    description: >
+      This is a test keyed uint type with multiple stores.
+    expires: never
+    kind: uint
+    keyed: true
+    notification_emails:
+      - telemetry-client-dev@mozilla.com
+    record_in_processes:
+      - 'main'
+    record_into_store:
+      - 'main'
+      - 'sync'
+
 # NOTE: Please don't add new definitions below this point. Consider adding
 # them earlier in the file and leave the telemetry.test category as the last
 # one for readability.
--- a/toolkit/components/telemetry/core/TelemetryScalar.cpp
+++ b/toolkit/components/telemetry/core/TelemetryScalar.cpp
@@ -411,28 +411,28 @@ nsresult
 ScalarBase::StoreIndex(const nsACString& aStoreName, size_t* aStoreIndex) const
 {
   if (mStoreCount == 1 && mStoreOffset == UINT16_MAX) {
     // This Scalar is only in the "main" store.
     if (aStoreName.EqualsLiteral("main")) {
       *aStoreIndex = 0;
       return NS_OK;
     }
-    return NS_ERROR_FAILURE;
+    return NS_ERROR_NO_CONTENT;
   }
 
   // Multiple stores. Linear scan to find one that matches aStoreName.
   for (uint32_t i = 0; i < mStoreCount; ++i) {
     uint32_t stringIndex = gScalarStoresTable[mStoreOffset + i];
     if (aStoreName.EqualsASCII(&gScalarsStringTable[stringIndex])) {
       *aStoreIndex = i;
       return NS_OK;
     }
   }
-  return NS_ERROR_FAILURE;
+  return NS_ERROR_NO_CONTENT;
 }
 
 size_t
 ScalarBase::SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) const
 {
   return mStoreHasValue.ShallowSizeOfExcludingThis(aMallocSizeOf);
 }
 
@@ -1991,16 +1991,19 @@ internal_ScalarSnapshotter(const StaticM
         }
         if (NS_FAILED(rv)) {
           return rv;
         }
         // Append it to our list.
         processScalars.AppendElement(mozilla::MakeTuple(info.name(), scalarValue, info.kind));
       }
     }
+    if (processScalars.Length() == 0) {
+      aScalarsToReflect.Remove(iter.Key());
+    }
   }
   return NS_OK;
 }
 
 /**
  * Creates a snapshot of the desired keyed scalar storage.
  * @param {aLock} The proof of lock to access scalar data.
  * @param {aScalarsToReflect} The table that will contain the snapshot.
@@ -2039,21 +2042,28 @@ internal_KeyedScalarSnapshotter(const St
       // Serialize the scalar if it's in the desired dataset.
       if (IsInDataset(info.dataset, aDataset)) {
         // Get the keys for this scalar.
         nsTArray<KeyedScalar::KeyValuePair> scalarKeyedData;
         nsresult rv = scalar->GetValue(aStoreName, aClearScalars, scalarKeyedData);
         if (NS_FAILED(rv)) {
           return rv;
         }
+        if (scalarKeyedData.Length() == 0) {
+          // Don't bother with empty keyed scalars.
+          continue;
+        }
         // Append it to our list.
         processScalars.AppendElement(
           mozilla::MakeTuple(info.name(), scalarKeyedData, info.kind));
       }
     }
+    if (processScalars.Length() == 0) {
+      aScalarsToReflect.Remove(iter.Key());
+    }
   }
   return NS_OK;
 }
 
 /**
  * Helper function to get a snapshot of the scalars.
  *
  * @param {aLock} The proof of lock to access scalar data.
@@ -2088,22 +2098,16 @@ internal_GetScalarSnapshot(const StaticM
                                   gDynamicBuiltinScalarStorageMap,
                                   true, /*aIsBuiltinDynamic*/
                                   aClearScalars,
                                   aStoreName);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
-  if (aClearScalars) {
-    // The map already takes care of freeing the allocated memory.
-    gScalarStorageMap.Clear();
-    gDynamicBuiltinScalarStorageMap.Clear();
-  }
-
   return NS_OK;
 }
 
 /**
  * Helper function to get a snapshot of the keyed scalars.
  *
  * @param {aLock} The proof of lock to access scalar data.
  * @param {aScalarsToReflect} The table that will contain the snapshot.
@@ -2137,22 +2141,16 @@ internal_GetKeyedScalarSnapshot(const St
                                        gDynamicBuiltinKeyedScalarStorageMap,
                                        true, /*aIsBuiltinDynamic*/
                                        aClearScalars,
                                        aStoreName);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
-  if (aClearScalars) {
-    // The map already takes care of freeing the allocated memory.
-    gKeyedScalarStorageMap.Clear();
-    gDynamicBuiltinKeyedScalarStorageMap.Clear();
-  }
-
   return NS_OK;
 }
 
 } // namespace
 
 // helpers for recording/applying scalar operations
 namespace {
 
new file mode 100644
--- /dev/null
+++ b/toolkit/components/telemetry/tests/unit/test_TelemetryScalars_multistore.js
@@ -0,0 +1,223 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/
+*/
+
+const CATEGORY = "telemetry.test";
+const MAIN_ONLY = `${CATEGORY}.main_only`;
+const SYNC_ONLY = `${CATEGORY}.sync_only`;
+const MULTIPLE_STORES = `${CATEGORY}.multiple_stores`;
+const MULTIPLE_STORES_STRING = `${CATEGORY}.multiple_stores_string`;
+const MULTIPLE_STORES_BOOL = `${CATEGORY}.multiple_stores_bool`;
+const MULTIPLE_STORES_KEYED = `${CATEGORY}.multiple_stores_keyed`;
+
+function getParentSnapshot(store, keyed = false, clear = false) {
+  return keyed
+         ? Telemetry.getSnapshotForKeyedScalars(store, clear).parent
+         : Telemetry.getSnapshotForScalars(store, clear).parent;
+}
+
+add_task(async function test_multistore_basics() {
+  Telemetry.clearScalars();
+
+  const expectedUint = 3785;
+  const expectedBool = true;
+  const expectedString = "some value";
+  const expectedKey = "some key";
+  Telemetry.scalarSet(MAIN_ONLY, expectedUint);
+  Telemetry.scalarSet(SYNC_ONLY, expectedUint);
+  Telemetry.scalarSet(MULTIPLE_STORES, expectedUint);
+  Telemetry.scalarSet(MULTIPLE_STORES_STRING, expectedString);
+  Telemetry.scalarSet(MULTIPLE_STORES_BOOL, expectedBool);
+  Telemetry.keyedScalarSet(MULTIPLE_STORES_KEYED, expectedKey, expectedUint);
+
+  const mainScalars = getParentSnapshot("main");
+  const syncScalars = getParentSnapshot("sync");
+  const mainKeyedScalars = getParentSnapshot("main", true /* keyed */);
+  const syncKeyedScalars = getParentSnapshot("sync", true /* keyed */);
+
+  Assert.ok(MAIN_ONLY in mainScalars,
+            `Main-store scalar ${MAIN_ONLY} must be in main snapshot.`);
+  Assert.ok(!(MAIN_ONLY in syncScalars),
+            `Main-store scalar ${MAIN_ONLY} must not be in sync snapshot.`);
+  Assert.equal(mainScalars[MAIN_ONLY], expectedUint,
+               `Main-store scalar ${MAIN_ONLY} must have correct value.`);
+
+  Assert.ok(SYNC_ONLY in syncScalars,
+            `Sync-store scalar ${SYNC_ONLY} must be in sync snapshot.`);
+  Assert.ok(!(SYNC_ONLY in mainScalars),
+            `Sync-store scalar ${SYNC_ONLY} must not be in main snapshot.`);
+  Assert.equal(syncScalars[SYNC_ONLY], expectedUint,
+               `Sync-store scalar ${SYNC_ONLY} must have correct value.`);
+
+  Assert.ok((MULTIPLE_STORES in mainScalars) && (MULTIPLE_STORES in syncScalars),
+            `Multi-store scalar ${MULTIPLE_STORES} must be in both main and sync snapshots.`);
+  Assert.equal(mainScalars[MULTIPLE_STORES], expectedUint,
+               `Multi-store scalar ${MULTIPLE_STORES} must have correct value in main store.`);
+  Assert.equal(syncScalars[MULTIPLE_STORES], expectedUint,
+               `Multi-store scalar ${MULTIPLE_STORES} must have correct value in sync store.`);
+
+  Assert.ok((MULTIPLE_STORES_STRING in mainScalars) && (MULTIPLE_STORES_STRING in syncScalars),
+            `Multi-store scalar ${MULTIPLE_STORES_STRING} must be in both main and sync snapshots.`);
+  Assert.equal(mainScalars[MULTIPLE_STORES_STRING], expectedString,
+               `Multi-store scalar ${MULTIPLE_STORES_STRING} must have correct value in main store.`);
+  Assert.equal(syncScalars[MULTIPLE_STORES_STRING], expectedString,
+               `Multi-store scalar ${MULTIPLE_STORES_STRING} must have correct value in sync store.`);
+
+  Assert.ok((MULTIPLE_STORES_BOOL in mainScalars) && (MULTIPLE_STORES_BOOL in syncScalars),
+            `Multi-store scalar ${MULTIPLE_STORES_BOOL} must be in both main and sync snapshots.`);
+  Assert.equal(mainScalars[MULTIPLE_STORES_BOOL], expectedBool,
+               `Multi-store scalar ${MULTIPLE_STORES_BOOL} must have correct value in main store.`);
+  Assert.equal(syncScalars[MULTIPLE_STORES_BOOL], expectedBool,
+               `Multi-store scalar ${MULTIPLE_STORES_BOOL} must have correct value in sync store.`);
+
+  Assert.ok((MULTIPLE_STORES_KEYED in mainKeyedScalars) && (MULTIPLE_STORES_KEYED in syncKeyedScalars),
+            `Multi-store scalar ${MULTIPLE_STORES_KEYED} must be in both main and sync snapshots.`);
+  Assert.ok((expectedKey in mainKeyedScalars[MULTIPLE_STORES_KEYED]) && (expectedKey in syncKeyedScalars[MULTIPLE_STORES_KEYED]),
+            `Multi-store scalar ${MULTIPLE_STORES_KEYED} must have key ${expectedKey} in both snapshots.`);
+  Assert.equal(mainKeyedScalars[MULTIPLE_STORES_KEYED][expectedKey], expectedUint,
+               `Multi-store scalar ${MULTIPLE_STORES_KEYED} must have correct value in main store.`);
+  Assert.equal(syncKeyedScalars[MULTIPLE_STORES_KEYED][expectedKey], expectedUint,
+               `Multi-store scalar ${MULTIPLE_STORES_KEYED} must have correct value in sync store.`);
+});
+
+add_task(async function test_multistore_uint() {
+  Telemetry.clearScalars();
+
+  // Uint scalars are the only kind with an implicit default value of 0.
+  // They shouldn't report any value until set, but if you Add or SetMaximum
+  // they pretend that they have been set to 0 for the purposes of that operation.
+
+  function assertNotIn() {
+    let mainScalars = getParentSnapshot("main");
+    let syncScalars = getParentSnapshot("sync");
+
+    if (!mainScalars && !syncScalars) {
+      Assert.ok(true, "No scalars at all");
+    } else {
+      Assert.ok(!(MULTIPLE_STORES in mainScalars) && !(MULTIPLE_STORES in syncScalars),
+                `Multi-store scalar ${MULTIPLE_STORES} must not have an initial value in either store.`);
+    }
+  }
+  assertNotIn();
+
+  // Test that Add operates on implicit 0.
+  Telemetry.scalarAdd(MULTIPLE_STORES, 1);
+
+  function assertBothEqual(val, clear = false) {
+    let mainScalars = getParentSnapshot("main", false, clear);
+    let syncScalars = getParentSnapshot("sync", false, clear);
+
+    Assert.ok((MULTIPLE_STORES in mainScalars) && (MULTIPLE_STORES in syncScalars),
+              `Multi-store scalar ${MULTIPLE_STORES} must be in both main and sync snapshots.`);
+    Assert.equal(mainScalars[MULTIPLE_STORES], val,
+                 `Multi-store scalar ${MULTIPLE_STORES} must have the correct value in main store.`);
+    Assert.equal(syncScalars[MULTIPLE_STORES], val,
+                 `Multi-store scalar ${MULTIPLE_STORES} must have the correct value in sync store.`);
+  }
+
+  assertBothEqual(1, true /* clear */);
+
+  assertNotIn();
+
+  // Test that SetMaximum operates on implicit 0.
+  Telemetry.scalarSetMaximum(MULTIPLE_STORES, 1337);
+  assertBothEqual(1337);
+
+  // Test that Add works, since we're in the neighbourhood.
+  Telemetry.scalarAdd(MULTIPLE_STORES, 1);
+  assertBothEqual(1338, true /* clear */);
+
+  assertNotIn();
+
+  // Test that clearing individual stores works
+  // and that afterwards the values are managed independently.
+  Telemetry.scalarAdd(MULTIPLE_STORES, 1234);
+  assertBothEqual(1234);
+  let syncScalars = getParentSnapshot("sync", false /* keyed */, true /* clear */);
+  Assert.equal(syncScalars[MULTIPLE_STORES], 1234,
+               `Multi-store scalar ${MULTIPLE_STORES} must be present in a second snapshot.`);
+  syncScalars = getParentSnapshot("sync");
+  Assert.equal(syncScalars, undefined,
+               `Multi-store scalar ${MULTIPLE_STORES} must not be present after clearing.`);
+  let mainScalars = getParentSnapshot("main");
+  Assert.equal(mainScalars[MULTIPLE_STORES], 1234,
+               `Multi-store scalar ${MULTIPLE_STORES} must maintain value in main store after sync store is cleared.`);
+
+  Telemetry.scalarSetMaximum(MULTIPLE_STORES, 1);
+  syncScalars = getParentSnapshot("sync");
+  Assert.equal(syncScalars[MULTIPLE_STORES], 1,
+               `Multi-store scalar ${MULTIPLE_STORES} must return to using implicit 0 for setMax operation.`);
+  mainScalars = getParentSnapshot("main");
+  Assert.equal(mainScalars[MULTIPLE_STORES], 1234,
+               `Multi-store scalar ${MULTIPLE_STORES} must retain old value.`);
+
+  Telemetry.scalarAdd(MULTIPLE_STORES, 1);
+  syncScalars = getParentSnapshot("sync");
+  Assert.equal(syncScalars[MULTIPLE_STORES], 2,
+               `Multi-store scalar ${MULTIPLE_STORES} must manage independently for add operations.`);
+  mainScalars = getParentSnapshot("main");
+  Assert.equal(mainScalars[MULTIPLE_STORES], 1235,
+               `Multi-store scalar ${MULTIPLE_STORES} must add properly.`);
+
+  Telemetry.scalarSet(MULTIPLE_STORES, 9876);
+  assertBothEqual(9876);
+
+});
+
+add_task(async function test_empty_absence() {
+  // Current semantics are we don't snapshot empty things.
+  // So no {parent: {}, ...}. Instead {...}.
+
+  Telemetry.clearScalars();
+
+  Telemetry.scalarSet(MULTIPLE_STORES, 1);
+  let snapshot = getParentSnapshot("main", false /* keyed */, true /* clear */);
+
+  Assert.ok(MULTIPLE_STORES in snapshot,
+            `${MULTIPLE_STORES} must be in the snapshot.`);
+  Assert.equal(snapshot[MULTIPLE_STORES], 1,
+               `${MULTIPLE_STORES} must have the correct value.`);
+
+  snapshot = getParentSnapshot("main", false /* keyed */, true /* clear */);
+  Assert.equal(snapshot, undefined,
+               `Parent snapshot must be empty if no data.`);
+
+
+  snapshot = getParentSnapshot("sync", false /* keyed */, true /* clear */);
+  Assert.ok(MULTIPLE_STORES in snapshot,
+            `${MULTIPLE_STORES} must be in the sync snapshot.`);
+  Assert.equal(snapshot[MULTIPLE_STORES], 1,
+               `${MULTIPLE_STORES} must have the correct value in the sync snapshot.`);
+});
+
+add_task(async function test_empty_absence_keyed() {
+  // Current semantics are we don't snapshot empty things.
+  // So no {parent: {}, ...}. Instead {...}.
+  // And for Keyed Scalars, no {parent: { keyed_scalar: {} }, ...}. Just {...}.
+
+  Telemetry.clearScalars();
+
+  const key = "just a key, y'know";
+  Telemetry.keyedScalarSet(MULTIPLE_STORES_KEYED, key, 1);
+  let snapshot = getParentSnapshot("main", true /* keyed */, true /* clear */);
+
+  Assert.ok(MULTIPLE_STORES_KEYED in snapshot,
+            `${MULTIPLE_STORES_KEYED} must be in the snapshot.`);
+  Assert.ok(key in snapshot[MULTIPLE_STORES_KEYED],
+            `${MULTIPLE_STORES_KEYED} must have the stored key.`);
+  Assert.equal(snapshot[MULTIPLE_STORES_KEYED][key], 1,
+               `${MULTIPLE_STORES_KEYED}[${key}] should have the correct value.`);
+
+  snapshot = getParentSnapshot("main", true /* keyed */);
+  Assert.equal(snapshot, undefined,
+               `Parent snapshot should be empty if no data.`);
+  snapshot = getParentSnapshot("sync", true /* keyed */);
+
+  Assert.ok(MULTIPLE_STORES_KEYED in snapshot,
+            `${MULTIPLE_STORES_KEYED} must be in the sync snapshot.`);
+  Assert.ok(key in snapshot[MULTIPLE_STORES_KEYED],
+            `${MULTIPLE_STORES_KEYED} must have the stored key.`);
+  Assert.equal(snapshot[MULTIPLE_STORES_KEYED][key], 1,
+               `${MULTIPLE_STORES_KEYED}[${key}] should have the correct value.`);
+
+});
--- a/toolkit/components/telemetry/tests/unit/xpcshell.ini
+++ b/toolkit/components/telemetry/tests/unit/xpcshell.ini
@@ -67,16 +67,17 @@ skip-if = os == "android" # Disabled due
 tags = addons
 [test_ChildScalars.js]
 skip-if = os == "android" # Disabled due to crashes (see bug 1331366)
 [test_TelemetryReportingPolicy.js]
 skip-if = os == "android" # Disabled due to crashes (see bug 1367762)
 tags = addons
 [test_TelemetryScalars.js]
 [test_TelemetryScalars_buildFaster.js]
+[test_TelemetryScalars_multistore.js]
 [test_TelemetryTimestamps.js]
 skip-if = toolkit == 'android'
 [test_TelemetryCaptureStack.js]
 [test_TelemetryChildEvents_buildFaster.js]
 skip-if = os == "android" # Disabled due to crashes (see bug 1331366)
 [test_TelemetryEvents.js]
 [test_TelemetryEvents_buildFaster.js]
 [test_ChildEvents.js]