Bug 1316810 - Part 1 - Use more strict size limits for event recording. r=dexter
☠☠ backed out by dd01d903f823 ☠ ☠
authorGeorg Fritzsche <georg.fritzsche@googlemail.com>
Thu, 01 Dec 2016 14:57:32 +0100
changeset 324917 f98ddece4b12d3129bc8875830eb59f8452683a8
parent 324916 89d204e96e8d7715da4c309f8861c5a51184c690
child 324918 84e97301a8c94aaa7279c624576983f3f3cac60f
push id84550
push usergeorg.fritzsche@googlemail.com
push dateThu, 01 Dec 2016 13:57:49 +0000
treeherdermozilla-inbound@2c711bb8e373 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdexter
bugs1316810
milestone53.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 1316810 - Part 1 - Use more strict size limits for event recording. r=dexter
toolkit/components/telemetry/Events.yaml
toolkit/components/telemetry/TelemetryEvent.cpp
toolkit/components/telemetry/parse_events.py
toolkit/components/telemetry/tests/unit/test_TelemetryEvents.js
--- a/toolkit/components/telemetry/Events.yaml
+++ b/toolkit/components/telemetry/Events.yaml
@@ -5,38 +5,38 @@ telemetry.test:
   objects: ["object1", "object2"]
   bug_numbers: [1286606]
   notification_emails: ["telemetry-client-dev@mozilla.com"]
   description: This is a test entry for Telemetry.
   expiry_date: never
   extra_keys:
     key1: This is just a test description.
     key2: This is another test description.
-- methods: ["test_optout"]
+- methods: ["optout"]
   objects: ["object1", "object2"]
   bug_numbers: [1286606]
   notification_emails: ["telemetry-client-dev@mozilla.com"]
   description: This is an opt-out test entry.
   expiry_date: never
   release_channel_collection: opt-out
   extra_keys:
     key1: This is just a test description.
-- methods: ["test_expired_version"]
+- methods: ["expired_version"]
   objects: ["object1", "object2"]
   bug_numbers: [1286606]
   notification_emails: ["telemetry-client-dev@mozilla.com"]
   description: This is a test entry with an expired version.
   expiry_version: "3.6"
-- methods: ["test_expired_date"]
+- methods: ["expired_date"]
   objects: ["object1", "object2"]
   bug_numbers: [1286606]
   notification_emails: ["telemetry-client-dev@mozilla.com"]
   description: This is a test entry with an expired date.
   expiry_date: 2014-01-28
-- methods: ["test_not_expired_optout"]
+- methods: ["not_expired_optout"]
   objects: ["object1"]
   bug_numbers: [1286606]
   notification_emails: ["telemetry-client-dev@mozilla.com"]
   description: This is an opt-out test entry with unexpired date and version.
   release_channel_collection: opt-out
   expiry_date: 2099-01-01
   expiry_version: "999.0"
 
--- a/toolkit/components/telemetry/TelemetryEvent.cpp
+++ b/toolkit/components/telemetry/TelemetryEvent.cpp
@@ -82,21 +82,21 @@ const uint32_t kEventCount = mozilla::Te
 // This is a special event id used to mark expired events, to make expiry checks
 // faster at runtime.
 const uint32_t kExpiredEventId = kEventCount + 1;
 static_assert(kEventCount < kExpiredEventId, "Should not overflow.");
 
 // This is the hard upper limit on the number of event records we keep in storage.
 // If we cross this limit, we will drop any further event recording until elements
 // are removed from storage.
-const uint32_t kMaxEventRecords = 10000;
+const uint32_t kMaxEventRecords = 1000;
 // Maximum length of any passed value string, in UTF8 byte sequence length.
-const uint32_t kMaxValueByteLength = 100;
+const uint32_t kMaxValueByteLength = 80;
 // Maximum length of any string value in the extra dictionary, in UTF8 byte sequence length.
-const uint32_t kMaxExtraValueByteLength = 100;
+const uint32_t kMaxExtraValueByteLength = 80;
 
 typedef nsDataHashtable<nsCStringHashKey, uint32_t> EventMapType;
 typedef nsClassHashtable<nsCStringHashKey, nsCString> StringMap;
 
 enum class RecordEventResult {
   Ok,
   UnknownEvent,
   InvalidExtraKey,
--- a/toolkit/components/telemetry/parse_events.py
+++ b/toolkit/components/telemetry/parse_events.py
@@ -4,21 +4,21 @@
 
 import re
 import yaml
 import itertools
 import datetime
 import string
 from shared_telemetry_utils import add_expiration_postfix
 
-MAX_CATEGORY_NAME_LENGTH = 100
-MAX_METHOD_NAME_LENGTH = 40
-MAX_OBJECT_NAME_LENGTH = 40
-MAX_EXTRA_KEYS_COUNT = 20
-MAX_EXTRA_KEY_NAME_LENGTH = 20
+MAX_CATEGORY_NAME_LENGTH = 30
+MAX_METHOD_NAME_LENGTH = 20
+MAX_OBJECT_NAME_LENGTH = 20
+MAX_EXTRA_KEYS_COUNT = 10
+MAX_EXTRA_KEY_NAME_LENGTH = 15
 
 IDENTIFIER_PATTERN = r'^[a-zA-Z][a-zA-Z0-9_.]+[a-zA-Z0-9]$'
 DATE_PATTERN = r'^[0-9]{4}-[0-9]{2}-[0-9]{2}$'
 
 def nice_type_name(t):
     if isinstance(t, basestring):
         return "string"
     return t.__name__
@@ -121,18 +121,18 @@ def type_check_event_fields(category, de
 
     # Type-check fields.
     for k,v in definition.iteritems():
         ALL_FIELDS[k].check(k, v)
 
 def string_check(category, field_name, value, min_length, max_length, regex=None):
     # Length check.
     if len(value) > max_length:
-        raise ValueError("Value for %s in %s exceeds maximum length of %d" %\
-                         (field_name, category, max_length))
+        raise ValueError("Value '%s' for %s in %s exceeds maximum length of %d" %\
+                         (value, field_name, category, max_length))
     # Regex check.
     if regex and not re.match(regex, value):
         raise ValueError, 'String value for %s in %s is not matching pattern "%s": %s' % \
                           (field_name, category, regex, value)
 
 class EventData:
     """A class representing one event."""
 
--- a/toolkit/components/telemetry/tests/unit/test_TelemetryEvents.js
+++ b/toolkit/components/telemetry/tests/unit/test_TelemetryEvents.js
@@ -39,17 +39,17 @@ add_task(function* test_recording() {
     {optout: false, event: ["telemetry.test", "test1", "object1"]},
     {optout: false, event: ["telemetry.test", "test2", "object2"]},
 
     {optout: false, event: ["telemetry.test", "test1", "object1", "value"]},
     {optout: false, event: ["telemetry.test", "test1", "object1", "value", null]},
     {optout: false, event: ["telemetry.test", "test1", "object1", null, {"key1": "value1"}]},
     {optout: false, event: ["telemetry.test", "test1", "object1", "value", {"key1": "value1", "key2": "value2"}]},
 
-    {optout: true,  event: ["telemetry.test", "test_optout", "object1"]},
+    {optout: true,  event: ["telemetry.test", "optout", "object1"]},
     {optout: false, event: ["telemetry.test.second", "test", "object1"]},
     {optout: false, event: ["telemetry.test.second", "test", "object1", null, {"key1": "value1"}]},
   ];
 
   for (let entry of expected) {
     entry.tsBefore = Math.floor(Telemetry.msSinceProcessStart());
     try {
       Telemetry.recordEvent(...entry.event);
@@ -118,27 +118,27 @@ add_task(function* test_clear() {
   events = Telemetry.snapshotBuiltinEvents(OPTIN, false);
   Assert.equal(events.length, 0, `Should have cleared the events.`);
 });
 
 add_task(function* test_expiry() {
   Telemetry.clearEvents();
 
   // Recording call with event that is expired by version.
-  Telemetry.recordEvent("telemetry.test", "test_expired_version", "object1");
+  Telemetry.recordEvent("telemetry.test", "expired_version", "object1");
   let events = Telemetry.snapshotBuiltinEvents(OPTIN, true);
   Assert.equal(events.length, 0, "Should not record event with expired version.");
 
   // Recording call with event that is expired by date.
-  Telemetry.recordEvent("telemetry.test", "test_expired_date", "object1");
+  Telemetry.recordEvent("telemetry.test", "expired_date", "object1");
   events = Telemetry.snapshotBuiltinEvents(OPTIN, true);
   Assert.equal(events.length, 0, "Should not record event with expired date.");
 
   // Recording call with event that has expiry_version and expiry_date in the future.
-  Telemetry.recordEvent("telemetry.test", "test_not_expired_optout", "object1");
+  Telemetry.recordEvent("telemetry.test", "not_expired_optout", "object1");
   events = Telemetry.snapshotBuiltinEvents(OPTOUT, true);
   Assert.equal(events.length, 1, "Should record event when date and version are not expired.");
 });
 
 add_task(function* test_invalidParams() {
   Telemetry.clearEvents();
 
   // Recording call with wrong type for value argument.
@@ -161,53 +161,53 @@ add_task(function* test_invalidParams() 
   events = Telemetry.snapshotBuiltinEvents(OPTIN, true);
   Assert.equal(events.length, 0, "Should not record event when extra argument with invalid value type is passed.");
 });
 
 add_task(function* test_storageLimit() {
   Telemetry.clearEvents();
 
   // Record more events than the storage limit allows.
-  let LIMIT = 10000;
+  let LIMIT = 1000;
   let COUNT = LIMIT + 10;
   for (let i = 0; i < COUNT; ++i) {
     Telemetry.recordEvent("telemetry.test", "test1", "object1", String(i));
   }
 
   // Check that the right events were recorded.
   let events = Telemetry.snapshotBuiltinEvents(OPTIN, true);
   Assert.equal(events.length, LIMIT, `Should have only recorded ${LIMIT} events`);
   Assert.ok(events.every((e, idx) => e[4] === String(idx)),
             "Should have recorded all events from before hitting the limit.");
 });
 
 add_task(function* test_valueLimits() {
   Telemetry.clearEvents();
 
   // Record values that are at or over the limits for string lengths.
-  let LIMIT = 100;
+  let LIMIT = 80;
   let expected = [
     ["telemetry.test", "test1", "object1", "a".repeat(LIMIT - 10), null],
     ["telemetry.test", "test1", "object1", "a".repeat(LIMIT     ), null],
     ["telemetry.test", "test1", "object1", "a".repeat(LIMIT +  1), null],
     ["telemetry.test", "test1", "object1", "a".repeat(LIMIT + 10), null],
 
     ["telemetry.test", "test1", "object1", null, {key1: "a".repeat(LIMIT - 10)}],
     ["telemetry.test", "test1", "object1", null, {key1: "a".repeat(LIMIT     )}],
     ["telemetry.test", "test1", "object1", null, {key1: "a".repeat(LIMIT +  1)}],
     ["telemetry.test", "test1", "object1", null, {key1: "a".repeat(LIMIT + 10)}],
   ];
 
   for (let event of expected) {
     Telemetry.recordEvent(...event);
     if (event[3]) {
-      event[3] = event[3].substr(0, 100);
+      event[3] = event[3].substr(0, LIMIT);
     }
     if (event[4]) {
-      event[4].key1 = event[4].key1.substr(0, 100);
+      event[4].key1 = event[4].key1.substr(0, LIMIT);
     }
   }
 
   // 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) {