Bug 1316810 - Part 2 - Only serialize the events value & extra fields when needed. r=dexter
authorGeorg Fritzsche <georg.fritzsche@googlemail.com>
Fri, 02 Dec 2016 12:17:12 +0100
changeset 325129 0d1d639a320caf91141184debbd92a5b76fb36dd
parent 325128 ce3b7997c1398c73a03a6bf88d55a3f9368b0259
child 325130 bed0ae598c3f1c1eefac3258bda6e714d798819b
push id24
push usermaklebus@msu.edu
push dateTue, 20 Dec 2016 03:11:33 +0000
reviewersdexter
bugs1316810
milestone53.0a1
Bug 1316810 - Part 2 - Only serialize the events value & extra fields when needed. r=dexter
toolkit/components/telemetry/TelemetryEvent.cpp
toolkit/components/telemetry/tests/unit/test_TelemetryEvents.js
toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
--- a/toolkit/components/telemetry/TelemetryEvent.cpp
+++ b/toolkit/components/telemetry/TelemetryEvent.cpp
@@ -568,82 +568,82 @@ TelemetryEvent::CreateSnapshots(uint32_t
   if (!eventsArray) {
     return NS_ERROR_FAILURE;
   }
 
   for (uint32_t i = 0; i < events.Length(); ++i) {
     const EventRecord& record = events[i];
     const EventInfo& info = gEventInfo[record.EventId()];
 
-    // Each entry is an array of the form:
+    // Each entry is an array of one of the forms:
+    // [timestamp, category, method, object, value]
+    // [timestamp, category, method, object, null, extra]
     // [timestamp, category, method, object, value, extra]
-    JS::RootedObject itemsArray(cx, JS_NewArrayObject(cx, 6));
-    if (!itemsArray) {
-      return NS_ERROR_FAILURE;
-    }
+    JS::AutoValueVector items(cx);
 
     // Add timestamp.
     JS::Rooted<JS::Value> val(cx);
-    uint32_t itemIndex = 0;
-    val.setDouble(floor(record.Timestamp()));
-    if (!JS_DefineElement(cx, itemsArray, itemIndex++, val, JSPROP_ENUMERATE)) {
+    if (!items.append(JS::NumberValue(floor(record.Timestamp())))) {
       return NS_ERROR_FAILURE;
     }
 
     // Add category, method, object.
     const char* strings[] = {
       info.common_info.category(),
       info.method(),
       info.object(),
     };
-    for (uint32_t s = 0; s < ArrayLength(strings); ++s) {
-      const NS_ConvertUTF8toUTF16 wide(strings[s]);
-      val.setString(JS_NewUCStringCopyN(cx, wide.Data(), wide.Length()));
-      if (!JS_DefineElement(cx, itemsArray, itemIndex++, val, JSPROP_ENUMERATE)) {
+    for (const char* s : strings) {
+      const NS_ConvertUTF8toUTF16 wide(s);
+      if (!items.append(JS::StringValue(JS_NewUCStringCopyN(cx, wide.Data(), wide.Length())))) {
         return NS_ERROR_FAILURE;
       }
     }
 
-    // Add the optional string value.
-    if (!record.Value()) {
-      val.setNull();
-    } else {
+    // Add the optional string value only when needed.
+    // When extra is empty and this has no value, we can save a little space.
+    if (record.Value()) {
       const NS_ConvertUTF8toUTF16 wide(record.Value().value());
-      val.setString(JS_NewUCStringCopyN(cx, wide.Data(), wide.Length()));
-    }
-    if (!JS_DefineElement(cx, itemsArray, itemIndex++, val, JSPROP_ENUMERATE)) {
-      return NS_ERROR_FAILURE;
+      if (!items.append(JS::StringValue(JS_NewUCStringCopyN(cx, wide.Data(), wide.Length())))) {
+        return NS_ERROR_FAILURE;
+      }
+    } else if (!record.Extra().IsEmpty()) {
+      if (!items.append(JS::NullValue())) {
+        return NS_ERROR_FAILURE;
+      }
     }
 
     // Add the optional extra dictionary.
-    if (record.Extra().IsEmpty()) {
-      val.setNull();
-    } else {
+    // To save a little space, only add it when it is not empty.
+    if (!record.Extra().IsEmpty()) {
       JS::RootedObject obj(cx, JS_NewPlainObject(cx));
       if (!obj) {
         return NS_ERROR_FAILURE;
       }
 
+      // Add extra key & value entries.
       const ExtraArray& extra = record.Extra();
       for (uint32_t i = 0; i < extra.Length(); ++i) {
         const NS_ConvertUTF8toUTF16 wide(extra[i].value);
         JS::Rooted<JS::Value> value(cx);
         value.setString(JS_NewUCStringCopyN(cx, wide.Data(), wide.Length()));
 
         if (!JS_DefineProperty(cx, obj, extra[i].key.get(), value, JSPROP_ENUMERATE)) {
           return NS_ERROR_FAILURE;
         }
       }
       val.setObject(*obj);
-    }
-    if (!JS_DefineElement(cx, itemsArray, itemIndex++, val, JSPROP_ENUMERATE)) {
-      return NS_ERROR_FAILURE;
+
+      if (!items.append(val)) {
+        return NS_ERROR_FAILURE;
+      }
     }
 
     // Add the record to the events array.
+    JS::RootedObject itemsArray(cx, JS_NewArrayObject(cx, items));
     if (!JS_DefineElement(cx, eventsArray, i, itemsArray, JSPROP_ENUMERATE)) {
       return NS_ERROR_FAILURE;
     }
   }
 
   aResult.setObject(*eventsArray);
   return NS_OK;
 }
--- a/toolkit/components/telemetry/tests/unit/test_TelemetryEvents.js
+++ b/toolkit/components/telemetry/tests/unit/test_TelemetryEvents.js
@@ -4,27 +4,32 @@
 
 const OPTIN = Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN;
 const OPTOUT = Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTOUT;
 
 function checkEventFormat(events) {
   Assert.ok(Array.isArray(events), "Events should be serialized to an array.");
   for (let e of events) {
     Assert.ok(Array.isArray(e), "Event should be an array.");
-    Assert.equal(e.length, 6, "Event should have 6 elements.");
+    Assert.greaterOrEqual(e.length, 4, "Event should have at least 4 elements.");
+    Assert.lessOrEqual(e.length, 6, "Event should have at most 6 elements.");
 
     Assert.equal(typeof(e[0]), "number", "Element 0 should be a number.");
     Assert.equal(typeof(e[1]), "string", "Element 1 should be a string.");
     Assert.equal(typeof(e[2]), "string", "Element 2 should be a string.");
     Assert.equal(typeof(e[3]), "string", "Element 3 should be a string.");
 
-    Assert.ok(e[4] === null || typeof(e[4]) == "string",
-              "Event element 4 should be null or a string.");
-    Assert.ok(e[5] === null || typeof(e[5]) == "object",
-              "Event element 4 should be null or an object.");
+    if (e.length > 4) {
+      Assert.ok(e[4] === null || typeof(e[4]) == "string",
+                "Event element 4 should be null or a string.");
+    }
+    if (e.length > 5) {
+      Assert.ok(e[5] === null || typeof(e[5]) == "object",
+                "Event element 5 should be null or an object.");
+    }
 
     let extra = e[5];
     if (extra) {
       Assert.ok(Object.keys(extra).every(k => typeof(k) == "string"),
                 "All extra keys should be strings.");
       Assert.ok(Object.values(extra).every(v => typeof(v) == "string"),
                 "All extra values should be strings.");
     }
@@ -54,16 +59,24 @@ add_task(function* test_recording() {
     try {
       Telemetry.recordEvent(...entry.event);
     } catch (ex) {
       Assert.ok(false, `Failed to record event ${JSON.stringify(entry.event)}: ${ex}`);
     }
     entry.tsAfter = Math.floor(Telemetry.msSinceProcessStart());
   }
 
+  // Strip off trailing null values to match the serialized events.
+  for (let entry of expected) {
+    let e = entry.event;
+    while ((e.length >= 3) && (e[e.length - 1] === null)) {
+      e.pop();
+    }
+  }
+
   // The following should not result in any recorded events.
   Assert.throws(() => Telemetry.recordEvent("unknown.category", "test1", "object1"),
                 /Error: Unknown event\./,
                 "Should throw on unknown category.");
   Assert.throws(() => Telemetry.recordEvent("telemetry.test", "unknown", "object1"),
                 /Error: Unknown event\./,
                 "Should throw on unknown method.");
   Assert.throws(() => Telemetry.recordEvent("telemetry.test", "test1", "unknown"),
@@ -78,19 +91,16 @@ add_task(function* test_recording() {
     for (let i = 0; i < events.length; ++i) {
       let {tsBefore, tsAfter} = expectedEvents[i];
       let ts = events[i][0];
       Assert.greaterOrEqual(ts, tsBefore, "The recorded timestamp should be greater than the one before recording.");
       Assert.lessOrEqual(ts, tsAfter, "The recorded timestamp should be less than the one after recording.");
 
       let recordedData = events[i].slice(1);
       let expectedData = expectedEvents[i].event.slice();
-      for (let j = expectedData.length; j < 5; ++j) {
-        expectedData.push(null);
-      }
       Assert.deepEqual(recordedData, expectedData, "The recorded event data should match.");
     }
   };
 
   // Check that the expected events were recorded.
   let events = Telemetry.snapshotBuiltinEvents(OPTIN, false);
   checkEvents(events, expected);
 
@@ -201,16 +211,23 @@ add_task(function* test_valueLimits() {
     if (event[3]) {
       event[3] = event[3].substr(0, LIMIT);
     }
     if (event[4]) {
       event[4].key1 = event[4].key1.substr(0, LIMIT);
     }
   }
 
+  // Strip off trailing null values to match the serialized events.
+  for (let e of expected) {
+    while ((e.length >= 3) && (e[e.length - 1] === null)) {
+      e.pop();
+    }
+  }
+
   // Check that the right events were recorded.
   let events = Telemetry.snapshotBuiltinEvents(OPTIN, true);
   Assert.equal(events.length, expected.length,
                "Should have recorded the expected number 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.");
   }
--- a/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
+++ b/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
@@ -720,16 +720,23 @@ add_task(function* test_checkSubsessionE
   let expected = [
     ["telemetry.test", "test1", "object1", "a", null],
     ["telemetry.test", "test1", "object1", null, {key1: "value"}],
   ];
   for (let event of expected) {
     Telemetry.recordEvent(...event);
   }
 
+  // Strip off trailing null values to match the serialized events.
+  for (let e of expected) {
+    while ((e.length >= 3) && (e[e.length - 1] === null)) {
+      e.pop();
+    }
+  }
+
   // Check that events are not available in classic pings but are in subsession
   // pings. Also clear the subsession.
   let classic = TelemetrySession.getPayload();
   let subsession = TelemetrySession.getPayload("environment-change", true);
 
   Assert.ok("events" in classic.processes.parent, "Should have an events field in classic payload.");
   Assert.ok("events" in subsession.processes.parent, "Should have an events field in subsession payload.");