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 325009 f98ddece4b12d3129bc8875830eb59f8452683a8
parent 325008 89d204e96e8d7715da4c309f8861c5a51184c690
child 325010 84e97301a8c94aaa7279c624576983f3f3cac60f
push id24
push usermaklebus@msu.edu
push dateTue, 20 Dec 2016 03:11:33 +0000
reviewersdexter
bugs1316810
milestone53.0a1
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) {