Bug 1463694 - Allow dynamic builtin events to overwrite existing events on registration. r?chutten draft
authorJan-Erik Rediger <jrediger@mozilla.com>
Tue, 29 May 2018 16:05:56 +0200
changeset 801617 e407c6734d46cdd4be098fd3398737faf8e624a8
parent 801403 5866d6685849311f057e7e229b9ace63a2641c29
push id111695
push userbmo:jrediger@mozilla.com
push dateWed, 30 May 2018 15:20:22 +0000
reviewerschutten
bugs1463694
milestone62.0a1
Bug 1463694 - Allow dynamic builtin events to overwrite existing events on registration. r?chutten This means that during a normal "mach build faster" build all the events will be overwritten, as the generated EventArtifactsDefinitions.json contains all events from the Events.yaml it was generated from. Real dynamic events follow similar code paths internally. We also ensure they trigger the right code path and don't overwrite. MozReview-Commit-ID: JvFZfwCbjQN
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
@@ -541,20 +541,24 @@ RegisterEvents(const StaticMutexAutoLock
   // 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.
+    // Re-registering events can happen for two reasons and we don't print warnings:
+    //
+    // * When add-ons update.
+    //   We don't support changing their definition, but the expiry might have changed.
+    // * When dynamic builtins ("build faster") events are registered.
+    //   The dynamic definition takes precedence then.
     EventKey* existing = nullptr;
-    if (gEventNameIDMap.Get(eventName, &existing)) {
+    if (!aBuiltin && gEventNameIDMap.Get(eventName, &existing)) {
       if (eventExpired[i]) {
         existing->id = kExpiredEventId;
       }
       continue;
     }
 
     gDynamicEventInfo->AppendElement(eventInfos[i]);
     uint32_t eventId = eventExpired[i] ? kExpiredEventId : gDynamicEventInfo->Length() - 1;
--- a/toolkit/components/telemetry/tests/unit/test_TelemetryEvents_buildFaster.js
+++ b/toolkit/components/telemetry/tests/unit/test_TelemetryEvents_buildFaster.js
@@ -232,8 +232,110 @@ 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_dynamicBuiltinEventsOverridingStatic() {
+  Telemetry.clearEvents();
+  Telemetry.canRecordExtended = true;
+
+  const TEST_EVENT_NAME = "telemetry.test";
+
+  // Register dynamic builtin test events, overwriting existing one.
+  Telemetry.registerBuiltinEvents(TEST_EVENT_NAME, {
+    // Event with only required fields.
+    "test1": {
+      methods: ["test1"],
+      objects: ["object1", "object2"],
+    },
+    // Event with extra_keys.
+    "test2": {
+      methods: ["test2"],
+      objects: ["object1", "object2", "object3"],
+      extra_keys: ["key1", "key2", "newdynamickey"],
+    },
+  });
+
+  // Record some events that should be available in the static event already .
+  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"});
+  // Record events with newly added objects and keys.
+  Telemetry.recordEvent(TEST_EVENT_NAME, "test2", "object2", null,
+                        {"newdynamickey": "foo"});
+  Telemetry.recordEvent(TEST_EVENT_NAME, "test2", "object3", null,
+                        {"key1": "foo"});
+  // 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 have parent events in the snapshot.");
+
+  let expected = [
+    [TEST_EVENT_NAME, "test1", "object1"],
+    [TEST_EVENT_NAME, "test2", "object1", null, {key1: "foo", key2: "bar"}],
+    [TEST_EVENT_NAME, "test2", "object2", null, {newdynamickey: "foo"}],
+    [TEST_EVENT_NAME, "test2", "object3", null, {key1: "foo"}],
+  ];
+  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_realDynamicDontOverwrite() {
+  // Real dynamic events follow similar code paths internally.
+  // Let's ensure they trigger the right code path and don't overwrite.
+
+  Telemetry.clearEvents();
+  Telemetry.canRecordExtended = true;
+
+  const TEST_EVENT_NAME = "telemetry.test";
+
+  // Register dynamic test events, this should not overwrite existing ones.
+  Telemetry.registerEvents(TEST_EVENT_NAME, {
+    // Event with only required fields.
+    "test1": {
+      methods: ["test1"],
+      objects: ["object1", "object2"],
+    },
+    // Event with extra_keys.
+    "test2": {
+      methods: ["test2"],
+      objects: ["object1", "object2", "object3"],
+      extra_keys: ["key1", "key2", "realdynamic"],
+    },
+  });
+
+  // Record some events that should be available in the static event already .
+  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"});
+  // Record events with newly added objects and keys.
+  Telemetry.recordEvent(TEST_EVENT_NAME, "test2", "object2", null,
+                        {"realdynamic": "foo"});
+  Telemetry.recordEvent(TEST_EVENT_NAME, "test2", "object3", null,
+                        {"key1": "foo"});
+  // 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 have parent events in the snapshot.");
+
+  let expected = [
+    [TEST_EVENT_NAME, "test1", "object1"],
+    [TEST_EVENT_NAME, "test2", "object1", null, {key1: "foo", key2: "bar"}],
+    [TEST_EVENT_NAME, "test2", "object3", null, {key1: "foo"}],
+  ];
+  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.");
+  }
+});