Bug 1408975 - Allow to register new addon events for an existing category. r=dexter
☠☠ backed out by f4d3c53ff03d ☠ ☠
authorGeorg Fritzsche <georg.fritzsche@googlemail.com>
Tue, 07 Nov 2017 20:33:24 +0100
changeset 443855 82a3eab8a078e2d7d17817c47ae3c417928554e5
parent 443854 1c6eac3c74d516b2e76ebee8b07a663341133fda
child 443856 a94b8078bb5c22ab440acd30cb378328422b5dee
push id1618
push userCallek@gmail.com
push dateThu, 11 Jan 2018 17:45:48 +0000
treeherdermozilla-release@882ca853e05a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdexter
bugs1408975
milestone58.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 1408975 - Allow to register new addon events for an existing category. r=dexter
toolkit/components/telemetry/TelemetryEvent.cpp
toolkit/components/telemetry/docs/collection/events.rst
toolkit/components/telemetry/tests/unit/test_TelemetryEvents.js
--- a/toolkit/components/telemetry/TelemetryEvent.cpp
+++ b/toolkit/components/telemetry/TelemetryEvent.cpp
@@ -163,21 +163,16 @@ enum class RecordEventResult {
   Ok,
   UnknownEvent,
   InvalidExtraKey,
   StorageLimitReached,
   ExpiredEvent,
   WrongProcess,
 };
 
-enum class RegisterEventResult {
-  Ok,
-  AlreadyRegistered,
-};
-
 typedef nsTArray<EventExtraEntry> ExtraArray;
 
 class EventRecord {
 public:
   EventRecord(double timestamp, const EventKey& key, const Maybe<nsCString>& value,
               const ExtraArray& extra)
     : mTimestamp(timestamp)
     , mEventKey(key)
@@ -515,45 +510,48 @@ ShouldRecordChildEvent(const StaticMutex
   const auto processes = gEventInfo[eventKey->id].common_info.record_in_processes;
   if (!CanRecordInProcess(processes, XRE_GetProcessType())) {
     return RecordEventResult::WrongProcess;
   }
 
   return RecordEventResult::Ok;
 }
 
-RegisterEventResult
+void
 RegisterEvents(const StaticMutexAutoLock& lock, const nsACString& category,
                const nsTArray<DynamicEventInfo>& eventInfos,
                const nsTArray<bool>& eventExpired)
 {
   MOZ_ASSERT(eventInfos.Length() == eventExpired.Length(), "Event data array sizes should match.");
 
-  // Check that none of the events are already registered.
-  for (auto& info : eventInfos) {
-    if (gEventNameIDMap.Get(UniqueEventName(info))) {
-      return RegisterEventResult::AlreadyRegistered;
-    }
-  }
-
   // Register the new events.
   if (!gDynamicEventInfo) {
     gDynamicEventInfo = new nsTArray<DynamicEventInfo>();
   }
 
   for (uint32_t i = 0, len = eventInfos.Length(); i < len; ++i) {
+    const nsCString& eventName = UniqueEventName(eventInfos[i]);
+
+    // Re-registering events can happen when add-ons update, so we don't print warnings.
+    // We don't support changing their definition, but the expiry might have changed.
+    EventKey* existing = nullptr;
+    if (gEventNameIDMap.Get(eventName, &existing)) {
+      if (eventExpired[i]) {
+        existing->id = kExpiredEventId;
+      }
+      continue;
+    }
+
     gDynamicEventInfo->AppendElement(eventInfos[i]);
     uint32_t eventId = eventExpired[i] ? kExpiredEventId : gDynamicEventInfo->Length() - 1;
-    gEventNameIDMap.Put(UniqueEventName(eventInfos[i]), new EventKey{eventId, true});
+    gEventNameIDMap.Put(eventName, new EventKey{eventId, true});
   }
 
   // Now after successful registration enable recording for this category.
   gEnabledCategories.PutEntry(category);
-
-  return RegisterEventResult::Ok;
 }
 
 } // anonymous namespace
 
 ////////////////////////////////////////////////////////////////////////
 ////////////////////////////////////////////////////////////////////////
 //
 // PRIVATE: thread-unsafe helpers for event handling.
@@ -1075,28 +1073,19 @@ TelemetryEvent::RegisterEvents(const nsA
         DynamicEventInfo info{nsCString(aCategory), method, object,
                               nsTArray<nsCString>(extra_keys), recordOnRelease};
         newEventInfos.AppendElement(info);
         newEventExpired.AppendElement(expired);
       }
     }
   }
 
-  RegisterEventResult res = RegisterEventResult::Ok;
   {
     StaticMutexAutoLock locker(gTelemetryEventsMutex);
-    res = ::RegisterEvents(locker, aCategory, newEventInfos, newEventExpired);
-  }
-
-  switch (res) {
-    case RegisterEventResult::AlreadyRegistered:
-      JS_ReportErrorASCII(cx, "Attempt to register event that is already registered.");
-      return NS_ERROR_INVALID_ARG;
-    default:
-      break;
+    RegisterEvents(locker, aCategory, newEventInfos, newEventExpired);
   }
 
   return NS_OK;
 }
 
 nsresult
 TelemetryEvent::CreateSnapshots(uint32_t aDataset, bool aClear, JSContext* cx,
                                 uint8_t optional_argc, JS::MutableHandleValue aResult)
--- a/toolkit/components/telemetry/docs/collection/events.rst
+++ b/toolkit/components/telemetry/docs/collection/events.rst
@@ -192,16 +192,18 @@ Register new events from add-ons.
 
 For events recorded from add-ons, registration happens at runtime. Any new events must first be registered through this function before they can be recorded.
 The registered categories will automatically be enabled for recording.
 
 After registration, the events can be recorded through the ``recordEvent()`` function. They will be submitted in the main pings payload under ``processes.dynamic.events``.
 
 New events registered here are subject to the same limitations as the ones registered through ``Events.yaml``, although the naming was in parts updated to recent policy changes.
 
+When add-ons are updated, they may re-register all of their events. In that case, any changes to events that are already registered are ignored. The only exception is expiry; an event that is re-registered with ``expired: true`` will not be recorded anymore.
+
 Example:
 
 .. code-block:: js
 
   Services.telemetry.registerEvents("myAddon.interaction", {
     "click": {
       methods: ["click"],
       objects: ["red_button", "blue_button"],
@@ -222,8 +224,9 @@ These functions are only supposed to be 
 
 Version History
 ===============
 
 - Firefox 52: Initial event support (`bug 1302663 <https://bugzilla.mozilla.org/show_bug.cgi?id=1302663>`_).
 - Firefox 53: Event recording disabled by default (`bug 1329139 <https://bugzilla.mozilla.org/show_bug.cgi?id=1329139>`_).
 - Firefox 54: Added child process events (`bug 1313326 <https://bugzilla.mozilla.org/show_bug.cgi?id=1313326>`_).
 - Firefox 56: Added support for recording new probes from add-ons (`bug 1302681 <bug https://bugzilla.mozilla.org/show_bug.cgi?id=1302681>`_).
+- Firefox 58: Ignore re-registering existing events for a category instead of failing (`bug 1408975 <https://bugzilla.mozilla.org/show_bug.cgi?id=1408975>`_).
--- a/toolkit/components/telemetry/tests/unit/test_TelemetryEvents.js
+++ b/toolkit/components/telemetry/tests/unit/test_TelemetryEvents.js
@@ -514,19 +514,95 @@ add_task(function* test_dynamicEventRegi
     "Should throw when registering too many extra keys.");
   Telemetry.registerEvents("telemetry.test.dynamic10", {
     "test1": {
       methods: ["test1"],
       objects: ["object1"],
       extra_keys: ["a1", "a2", "a3", "a4", "a5", "a6", "a7", "a8", "a9", "a10"],
     },
   });
+});
 
-  // Test registering an event thats already registered through Events.yaml.
-  Assert.throws(() => Telemetry.registerEvents("telemetry.test", {
-      "test1": {
-        methods: ["test1"],
-        objects: ["object1"],
-      },
-    }),
-    /Attempt to register event that is already registered\./,
-    "Should throw when registering event that already was registered.");
+// When add-ons update, they may re-register some of the dynamic events.
+// Test through some possible scenarios.
+add_task(function* test_dynamicEventRegisterAgain() {
+  Telemetry.canRecordExtended = true;
+  Telemetry.clearEvents();
+
+  const category = "telemetry.test.register.again";
+  let events = {
+    "test1": {
+      methods: ["test1"],
+      objects: ["object1"],
+    }
+  };
+
+  // First register the initial event and make sure it can be recorded.
+  Telemetry.registerEvents(category, events);
+  let expected = [
+    [category, "test1", "object1"],
+  ];
+  expected.forEach(e => Telemetry.recordEvent(...e));
+
+  let snapshot = Telemetry.snapshotEvents(OPTIN, true);
+  Assert.equal(snapshot.dynamic.length, expected.length,
+               "Should have right number of events in the snapshot.");
+  Assert.deepEqual(snapshot.dynamic.map(e => e.slice(1)), expected);
+
+  // Register the same event again and make sure it can still be recorded.
+  Telemetry.registerEvents(category, events);
+  Telemetry.recordEvent(category, "test1", "object1");
+
+  snapshot = Telemetry.snapshotEvents(OPTIN, true);
+  Assert.equal(snapshot.dynamic.length, expected.length,
+               "Should have right number of events in the snapshot.");
+  Assert.deepEqual(snapshot.dynamic.map(e => e.slice(1)), expected);
+
+  // Now register another event in the same category and make sure both events can be recorded.
+  events["test2"] = {
+    methods: ["test2"],
+    objects: ["object2"],
+  };
+  Telemetry.registerEvents(category, events);
+
+  expected = [
+    [category, "test1", "object1"],
+    [category, "test2", "object2"],
+  ];
+  expected.forEach(e => Telemetry.recordEvent(...e));
+
+  snapshot = Telemetry.snapshotEvents(OPTIN, true);
+  Assert.equal(snapshot.dynamic.length, expected.length,
+               "Should have right number of events in the snapshot.");
+  Assert.deepEqual(snapshot.dynamic.map(e => e.slice(1)), expected);
+
+  // Check that adding a new object to an event entry works.
+  events["test1"].methods = ["test1a"];
+  events["test2"].objects = ["object2", "object2a"];
+  Telemetry.registerEvents(category, events);
+
+  expected = [
+    [category, "test1", "object1"],
+    [category, "test2", "object2"],
+    [category, "test1a", "object1"],
+    [category, "test2", "object2a"],
+  ];
+  expected.forEach(e => Telemetry.recordEvent(...e));
+
+  snapshot = Telemetry.snapshotEvents(OPTIN, true);
+  Assert.equal(snapshot.dynamic.length, expected.length,
+               "Should have right number of events in the snapshot.");
+  Assert.deepEqual(snapshot.dynamic.map(e => e.slice(1)), expected);
+
+  // Make sure that we can expire events that are already registered.
+  events["test2"].expired = true;
+  Telemetry.registerEvents(category, events);
+
+  expected = [
+    [category, "test1", "object1"],
+  ];
+  expected.forEach(e => Telemetry.recordEvent(...e));
+
+  snapshot = Telemetry.snapshotEvents(OPTIN, true);
+  Assert.equal(snapshot.dynamic.length, expected.length,
+               "Should have right number of events in the snapshot.");
+  Assert.deepEqual(snapshot.dynamic.map(e => e.slice(1)), expected);
 });