bug 1450690 - Ensure extra_keys applies to more than just the first dynamic Telemetry Event r?froydnj,Dexter draft
authorChris H-C <chutten@mozilla.com>
Tue, 03 Apr 2018 14:58:07 -0400
changeset 776799 58a2d5d6971c077106aedf99065eb05751a47f09
parent 773144 b906009d875d1f5d29b0d1252cdb43a9b1a5889c
push id104998
push userbmo:chutten@mozilla.com
push dateTue, 03 Apr 2018 19:05:10 +0000
reviewersfroydnj, Dexter
bugs1450690, 1443587
milestone61.0a1
bug 1450690 - Ensure extra_keys applies to more than just the first dynamic Telemetry Event r?froydnj,Dexter (This is a partial revert of bug 1443587 part 2 where the `Move`s were added.) When registering multiple methods and objects within the same `eventData` item, we must copy the list of acceptable event keys into each new DynamicEventInfo. Also includes a test. MozReview-Commit-ID: 5PSH15nSWEB
toolkit/components/telemetry/TelemetryEvent.cpp
toolkit/components/telemetry/tests/unit/test_TelemetryEvents.js
--- a/toolkit/components/telemetry/TelemetryEvent.cpp
+++ b/toolkit/components/telemetry/TelemetryEvent.cpp
@@ -121,22 +121,22 @@ typedef nsClassHashtable<nsCStringHashKe
 
 struct EventKey {
   uint32_t id;
   bool dynamic;
 };
 
 struct DynamicEventInfo {
   DynamicEventInfo(const nsACString& category, const nsACString& method,
-                   const nsACString& object, nsTArray<nsCString>&& extra_keys,
+                   const nsACString& object, nsTArray<nsCString>& extra_keys,
                    bool recordOnRelease)
     : category(category)
     , method(method)
     , object(object)
-    , extra_keys(Move(extra_keys))
+    , extra_keys(extra_keys)
     , recordOnRelease(recordOnRelease)
   {}
 
   DynamicEventInfo(const DynamicEventInfo&) = default;
   DynamicEventInfo& operator=(const DynamicEventInfo&) = delete;
 
   const nsCString category;
   const nsCString method;
@@ -1061,17 +1061,17 @@ TelemetryEvent::RegisterEvents(const nsA
     }
 
     // Append event infos to be registered.
     for (auto& method : methods) {
       for (auto& object : objects) {
         // 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.
         DynamicEventInfo info{aCategory, method, object,
-                              Move(extra_keys), recordOnRelease};
+                              extra_keys, recordOnRelease};
         newEventInfos.AppendElement(info);
         newEventExpired.AppendElement(expired);
       }
     }
   }
 
   {
     StaticMutexAutoLock locker(gTelemetryEventsMutex);
--- a/toolkit/components/telemetry/tests/unit/test_TelemetryEvents.js
+++ b/toolkit/components/telemetry/tests/unit/test_TelemetryEvents.js
@@ -292,17 +292,17 @@ add_task(async function test_unicodeValu
   let snapshot = Telemetry.snapshotEvents(OPTIN, true);
   Assert.ok(("parent" in snapshot), "Should have entry for main process.");
   let events = snapshot.parent;
   Assert.equal(events.length, 2, "Should have recorded 2 events.");
   Assert.equal(events[0][4], value, "Should have recorded the right value.");
   Assert.equal(events[1][5].key1, value, "Should have recorded the right extra value.");
 });
 
-add_task(function* test_dynamicEvents() {
+add_task(async function test_dynamicEvents() {
   Telemetry.clearEvents();
   Telemetry.canRecordExtended = true;
 
   // Register some test events.
   Telemetry.registerEvents("telemetry.test.dynamic", {
     // Event with only required fields.
     "test1": {
       methods: ["test1"],
@@ -327,31 +327,34 @@ add_task(function* test_dynamicEvents() 
       record_on_release: true,
     },
   });
 
   // Record some valid events.
   Telemetry.recordEvent("telemetry.test.dynamic", "test1", "object1");
   Telemetry.recordEvent("telemetry.test.dynamic", "test2", "object1", null,
                         {"key1": "foo", "key2": "bar"});
+  Telemetry.recordEvent("telemetry.test.dynamic", "test2b", "object1", null,
+                        {"key1": "foo", "key2": "bar"});
   Telemetry.recordEvent("telemetry.test.dynamic", "test3", "object1", "some value");
   Telemetry.recordEvent("telemetry.test.dynamic", "test4", "object1", null);
 
   // Test recording an unknown event.
   Assert.throws(() => Telemetry.recordEvent("telemetry.test.dynamic", "unknown", "unknown"),
                 /Error: Unknown event: \["telemetry\.test\.dynamic", "unknown", "unknown"\]/,
                 "Should throw when recording an unknown dynamic event.");
 
   // Now check that the snapshot contains the expected data.
   let snapshot = Telemetry.snapshotEvents(OPTIN, false);
   Assert.ok(("dynamic" in snapshot), "Should have dynamic events in the snapshot.");
 
   let expected = [
     ["telemetry.test.dynamic", "test1", "object1"],
     ["telemetry.test.dynamic", "test2", "object1", null, {key1: "foo", key2: "bar"}],
+    ["telemetry.test.dynamic", "test2b", "object1", null, {key1: "foo", key2: "bar"}],
     // "test3" is epxired, so it should not be recorded.
     ["telemetry.test.dynamic", "test4", "object1"],
   ];
   let events = snapshot.dynamic;
   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.");