Bug 1460400 - Don't enable dynamic builtin events on registration. r=chutten
authorJan-Erik Rediger <jrediger@mozilla.com>
Mon, 14 May 2018 12:31:30 +0200
changeset 472705 843b90da1fad40d2379d327a26288663acd05539
parent 472704 c085f9360dfa4d0fc3d04d6db40d37e1369616b3
child 472706 4d7c95672b79599a1192419115e2b25245f7fad1
push id9374
push userjlund@mozilla.com
push dateMon, 18 Jun 2018 21:43:20 +0000
treeherdermozilla-beta@160e085dfb0b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerschutten
bugs1460400
milestone62.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 1460400 - Don't enable dynamic builtin events on registration. r=chutten This makes sure dynamic builtin events follow the same semantics as static builtin events. On registration of the event the category is stored, but not enabled. For fully-dynamic events, e.g. those registered by addons, the category is enabled immediately (and can't be disabled). This removes now-unused type definitions and switches from a map to a simple set to store the category names. The value stored in the map previously was not used at all. In theory the map was effectively immutable after initialization, but the check was only forced in debug anyway. Now the set is mutable, but is only mutated in exactly 2 places. MozReview-Commit-ID: 8tLEVXzHuHw
toolkit/components/telemetry/TelemetryEvent.cpp
toolkit/components/telemetry/tests/unit/test_TelemetryEvents_buildFaster.js
--- a/toolkit/components/telemetry/TelemetryEvent.cpp
+++ b/toolkit/components/telemetry/TelemetryEvent.cpp
@@ -114,19 +114,16 @@ const uint32_t kMaxExtraValueByteLength 
 const uint32_t kMaxMethodNameByteLength = 20;
 // Maximum length of dynamic object names, in UTF8 byte sequence length.
 const uint32_t kMaxObjectNameByteLength = 20;
 // Maximum length of extra key names, in UTF8 byte sequence length.
 const uint32_t kMaxExtraKeyNameByteLength = 15;
 // The maximum number of valid extra keys for an event.
 const uint32_t kMaxExtraKeyCount = 10;
 
-typedef nsDataHashtable<nsCStringHashKey, uint32_t> StringUintMap;
-typedef nsClassHashtable<nsCStringHashKey, nsCString> StringMap;
-
 struct EventKey {
   uint32_t id;
   bool dynamic;
 };
 
 struct DynamicEventInfo {
   DynamicEventInfo(const nsACString& category, const nsACString& method,
                    const nsACString& object, nsTArray<nsCString>& extra_keys,
@@ -307,18 +304,18 @@ namespace {
 bool gInitDone = false;
 
 bool gCanRecordBase;
 bool gCanRecordExtended;
 
 // The EventName -> EventKey cache map.
 nsClassHashtable<nsCStringHashKey, EventKey> gEventNameIDMap(kEventCount);
 
-// The CategoryName -> CategoryID cache map.
-StringUintMap gCategoryNameIDMap;
+// The CategoryName set.
+nsTHashtable<nsCStringHashKey> gCategoryNames;
 
 // This tracks the IDs of the categories for which recording is enabled.
 nsTHashtable<nsCStringHashKey> gEnabledCategories;
 
 // The main event storage. Events are inserted here, keyed by process id and
 // in recording order.
 typedef nsUint32HashKey ProcessIDHashKey;
 typedef nsTArray<EventRecord> EventRecordArray;
@@ -541,17 +538,17 @@ ShouldRecordChildEvent(const StaticMutex
   }
 
   return RecordEventResult::Ok;
 }
 
 void
 RegisterEvents(const StaticMutexAutoLock& lock, const nsACString& category,
                const nsTArray<DynamicEventInfo>& eventInfos,
-               const nsTArray<bool>& eventExpired)
+               const nsTArray<bool>& eventExpired, bool aBuiltin)
 {
   MOZ_ASSERT(eventInfos.Length() == eventExpired.Length(), "Event data array sizes should match.");
 
   // Register the new events.
   if (!gDynamicEventInfo) {
     gDynamicEventInfo = new nsTArray<DynamicEventInfo>();
   }
 
@@ -568,18 +565,26 @@ RegisterEvents(const StaticMutexAutoLock
       continue;
     }
 
     gDynamicEventInfo->AppendElement(eventInfos[i]);
     uint32_t eventId = eventExpired[i] ? kExpiredEventId : gDynamicEventInfo->Length() - 1;
     gEventNameIDMap.Put(eventName, new EventKey{eventId, true});
   }
 
-  // Now after successful registration enable recording for this category.
-  gEnabledCategories.PutEntry(category);
+  // If it is a builtin, add the category name in order to enable it later.
+  if (aBuiltin) {
+    gCategoryNames.PutEntry(category);
+  }
+
+  if (!aBuiltin) {
+    // Now after successful registration enable recording for this category
+    // (if not a dynamic builtin).
+    gEnabledCategories.PutEntry(category);
+  }
 }
 
 } // anonymous namespace
 
 ////////////////////////////////////////////////////////////////////////
 ////////////////////////////////////////////////////////////////////////
 //
 // PRIVATE: thread-unsafe helpers for event handling.
@@ -727,39 +732,33 @@ TelemetryEvent::InitializeGlobalState(bo
     // If this event is expired or not recorded in this process, mark it with
     // a special event id.
     // This avoids doing repeated checks at runtime.
     if (IsExpiredVersion(info.common_info.expiration_version().get())) {
       eventId = kExpiredEventId;
     }
 
     gEventNameIDMap.Put(UniqueEventName(info), new EventKey{eventId, false});
-    if (!gCategoryNameIDMap.Contains(info.common_info.category())) {
-      gCategoryNameIDMap.Put(info.common_info.category(),
-                             info.common_info.category_offset);
-    }
+    gCategoryNames.PutEntry(info.common_info.category());
   }
 
-#ifdef DEBUG
-  gCategoryNameIDMap.MarkImmutable();
-#endif
   gInitDone = true;
 }
 
 void
 TelemetryEvent::DeInitializeGlobalState()
 {
   StaticMutexAutoLock locker(gTelemetryEventsMutex);
   MOZ_ASSERT(gInitDone);
 
   gCanRecordBase = false;
   gCanRecordExtended = false;
 
   gEventNameIDMap.Clear();
-  gCategoryNameIDMap.Clear();
+  gCategoryNames.Clear();
   gEnabledCategories.Clear();
   gEventRecords.Clear();
   gBuiltinEventRecords.Clear();
 
   gDynamicEventInfo = nullptr;
 
   gInitDone = false;
 }
@@ -1108,17 +1107,17 @@ TelemetryEvent::RegisterEvents(const nsA
         newEventInfos.AppendElement(info);
         newEventExpired.AppendElement(expired);
       }
     }
   }
 
   {
     StaticMutexAutoLock locker(gTelemetryEventsMutex);
-    RegisterEvents(locker, aCategory, newEventInfos, newEventExpired);
+    RegisterEvents(locker, aCategory, newEventInfos, newEventExpired, aBuiltin);
   }
 
   return NS_OK;
 }
 
 nsresult
 TelemetryEvent::CreateSnapshots(uint32_t aDataset, bool aClear, JSContext* cx,
                                 uint8_t optional_argc, JS::MutableHandleValue aResult)
@@ -1216,18 +1215,17 @@ TelemetryEvent::ClearEvents()
   gBuiltinEventRecords.Clear();
 }
 
 void
 TelemetryEvent::SetEventRecordingEnabled(const nsACString& category, bool enabled)
 {
   StaticMutexAutoLock locker(gTelemetryEventsMutex);
 
-  uint32_t categoryId;
-  if (!gCategoryNameIDMap.Get(category, &categoryId)) {
+  if (!gCategoryNames.Contains(category)) {
     LogToBrowserConsole(nsIScriptError::warningFlag,
                         NS_LITERAL_STRING("Unkown category for SetEventRecordingEnabled."));
     return;
   }
 
   if (enabled) {
     gEnabledCategories.PutEntry(category);
   } else {
@@ -1259,21 +1257,17 @@ TelemetryEvent::SizeOfIncludingThis(mozi
   n += getSizeOfRecords(gEventRecords);
   n += getSizeOfRecords(gBuiltinEventRecords);
 
   n += gEventNameIDMap.ShallowSizeOfExcludingThis(aMallocSizeOf);
   for (auto iter = gEventNameIDMap.ConstIter(); !iter.Done(); iter.Next()) {
     n += iter.Key().SizeOfExcludingThisIfUnshared(aMallocSizeOf);
   }
 
-  n += gCategoryNameIDMap.ShallowSizeOfExcludingThis(aMallocSizeOf);
-  for (auto iter = gCategoryNameIDMap.ConstIter(); !iter.Done(); iter.Next()) {
-    n += iter.Key().SizeOfExcludingThisIfUnshared(aMallocSizeOf);
-  }
-
+  n += gCategoryNames.ShallowSizeOfExcludingThis(aMallocSizeOf);
   n += gEnabledCategories.ShallowSizeOfExcludingThis(aMallocSizeOf);
 
   if (gDynamicEventInfo) {
     n += gDynamicEventInfo->ShallowSizeOfIncludingThis(aMallocSizeOf);
     for (auto& info : *gDynamicEventInfo) {
       n += info.SizeOfExcludingThis(aMallocSizeOf);
     }
   }
--- a/toolkit/components/telemetry/tests/unit/test_TelemetryEvents_buildFaster.js
+++ b/toolkit/components/telemetry/tests/unit/test_TelemetryEvents_buildFaster.js
@@ -73,16 +73,17 @@ add_task({
   await CommonUtils.writeJSON(DYNAMIC_EVENT_SPEC, FILE_PATH);
 
   // Start TelemetryController to trigger loading the specs.
   await TelemetryController.testReset();
   await TelemetryController.testPromiseJsProbeRegistration();
 
   // Record the events
   const TEST_EVENT_NAME = "telemetry.test.builtin";
+  Telemetry.setEventRecordingEnabled(TEST_EVENT_NAME, true);
   Telemetry.recordEvent(TEST_EVENT_NAME, "test1", "object1");
   Telemetry.recordEvent(TEST_EVENT_NAME, "test2", "object1", null,
                         {"key1": "foo", "key2": "bar"});
   Telemetry.recordEvent(TEST_EVENT_NAME, "test2", "object2", null,
                         {"key2": "bar"});
 
   // Check the values we tried to store.
   const snapshot =
@@ -123,16 +124,17 @@ add_task(async function test_dynamicBuil
     "test2": {
       methods: ["test2", "test2b"],
       objects: ["object1", "object2"],
       extra_keys: ["key1", "key2"],
     },
   });
 
   // Record some events.
+  Telemetry.setEventRecordingEnabled(TEST_EVENT_NAME, true);
   Telemetry.recordEvent(TEST_EVENT_NAME, "test1", "object1");
   Telemetry.recordEvent(TEST_EVENT_NAME, "test2", "object1", null,
                         {"key1": "foo", "key2": "bar"});
   Telemetry.recordEvent(TEST_EVENT_NAME, "test2b", "object2", null,
                         {"key2": "bar"});
   // Now check that the snapshot contains the expected data.
   let snapshot =
     Telemetry.snapshotEvents(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, false);
@@ -145,8 +147,51 @@ add_task(async function test_dynamicBuil
   ];
   let events = snapshot.parent;
   Assert.equal(events.length, expected.length, "Should have recorded the right amount of events.");
   for (let i = 0; i < expected.length; ++i) {
     Assert.deepEqual(events[i].slice(1), expected[i],
                      "Should have recorded the expected event data.");
   }
 });
+
+add_task(async function test_dynamicBuiltinEventsDisabledByDefault() {
+  Telemetry.clearEvents();
+  Telemetry.canRecordExtended = true;
+
+  const TEST_EVENT_NAME = "telemetry.test.offbydefault";
+
+  // Register some dynamic builtin test events.
+  Telemetry.registerBuiltinEvents(TEST_EVENT_NAME, {
+    // Event with only required fields.
+    "test1": {
+      methods: ["test1"],
+      objects: ["object1"],
+    },
+  });
+
+  // Record some events.
+  // Explicitely _don't_ enable the category
+  Telemetry.recordEvent(TEST_EVENT_NAME, "test1", "object1");
+
+  // Now check that the snapshot contains the expected data.
+  let snapshot =
+    Telemetry.snapshotEvents(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, false);
+  Assert.ok(!("parent" in snapshot), "Should not have parent events in the snapshot.");
+
+  // Now enable the category and record again
+  Telemetry.setEventRecordingEnabled(TEST_EVENT_NAME, true);
+  Telemetry.recordEvent(TEST_EVENT_NAME, "test1", "object1");
+
+  snapshot =
+    Telemetry.snapshotEvents(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, false);
+  Assert.ok(("parent" in snapshot), "Should have parent events in the snapshot.");
+
+  let expected = [
+    [TEST_EVENT_NAME, "test1", "object1"],
+  ];
+  let events = snapshot.parent;
+  Assert.equal(events.length, expected.length, "Should have recorded the right amount of events.");
+  for (let i = 0; i < expected.length; ++i) {
+    Assert.deepEqual(events[i].slice(1), expected[i],
+                     "Should have recorded the expected event data.");
+  }
+});