bug 1450690 - Ensure extra_keys applies to more than just the first dynamic Telemetry Event r=Dexter,froydnj
authorChris H-C <chutten@mozilla.com>
Tue, 03 Apr 2018 14:58:07 -0400
changeset 411697 c0a3a4b5e11c30462155b8d5df18edda6ad1f0af
parent 411696 8e0de009a63f8e4c05e16926d3dddbb1b63167e0
child 411698 149a759643fbf35d45d4ffbc86706db8ba2d4ffd
push id101729
push usercsabou@mozilla.com
push dateWed, 04 Apr 2018 18:07:35 +0000
treeherdermozilla-inbound@3c240f56a113 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersDexter, froydnj
bugs1450690, 1443587
milestone61.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 1450690 - Ensure extra_keys applies to more than just the first dynamic Telemetry Event r=Dexter,froydnj (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.");