We need to update the event recording limits to the results from bug 1313592.

This updates the event implementation to the limits we based the estimations in bug 1313592 et al on.

Comment on attachment 8813200 [details] [diff] [review] Part 2 - Only serialize the events value & extra fields when needed Alessio, can you do a general review from the Telemetry perspective? Nathan, could you review the serializations changes in TelemetryEvent.cpp specifically? We decided that variable-length event entries are ok, so we only add the optional entries to individual events where needed. This saves a little bit on raw storage size. I expanded on the possible event forms that are output in a comment in TelemetryEvent.cpp: > // 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]

Comment on attachment 8813200 [details] [diff] [review] Part 2 - Only serialize the events value & extra fields when needed Review of attachment 8813200 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me, with the test message fixed. ::: toolkit/components/telemetry/tests/unit/test_TelemetryEvents.js @@ +22,5 @@ > + "Event element 4 should be null or a string."); > + } > + if (e.length > 5) { > + Assert.ok(e[5] === null || typeof(e[5]) == "object", > + "Event element 4 should be null or an object."); This comment should refer to element 5.

Comment on attachment 8813200 [details] [diff] [review] Part 2 - Only serialize the events value & extra fields when needed Given the thanksgiving holidays and the simple change here we probably don't need to block on Nathan here.

(In reply to Georg Fritzsche [:gfritzsche] from comment #3) > Comment on attachment 8813200 [details] [diff] [review] > Part 2 - Only serialize the events value & extra fields when needed > > Alessio, can you do a general review from the Telemetry perspective? I also had a look at the changes in TelemetryEvent.cpp and they look ok.

Pushed by georg.fritzsche@googlemail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f98ddece4b12 Part 1 - Use more strict size limits for event recording. r=dexter https://hg.mozilla.org/integration/mozilla-inbound/rev/84e97301a8c9 Part 2 - Only serialize the events value & extra fields when needed. r=dexter

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/dd01d903f823 for ASan test failures, https://treeherder.mozilla.org/logviewer.html#?job_id=40181720&repo=mozilla-inbound

Pushed by georg.fritzsche@googlemail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ce3b7997c139 Part 1 - Use more strict size limits for event recording. r=dexter https://hg.mozilla.org/integration/mozilla-inbound/rev/0d1d639a320c Part 2 - Only serialize the events value & extra fields when needed. r=dexter

https://hg.mozilla.org/mozilla-central/rev/ce3b7997c139 https://hg.mozilla.org/mozilla-central/rev/0d1d639a320c

Comment on attachment 8813199 [details] [diff] [review] Part 1 - Use more strict size limits for event recording Approval Request Comment [Feature/Bug causing the regression]: Event Telemetry. [User impact if declined]: Data of Event Telemetry behavior reaches us later, delaying our decision making for the project. [Is this code covered by automated tests?]: Yes. [Has the fix been verified in Nightly?]: Yes, bug 1302671. [Needs manual test from QE? If yes, steps to reproduce]: No. [List of other uplifts needed for the feature/fix]: The individual parts are: * bug 1302663 - basic Telemetry implementation * bug 1316810 - adjust event limits * bug 1318284 - improve metrics that track effects on ping sending * bug 1323628 - fix * bug 1316281 - record search event in Telemetry [Is the change risky?]: No. [Why is the change risky/not risky?]: It's validated on Nightly and the changes are selective and relatively well understood. [String changes made/needed]: No. This is part of an uplift need for the initial Event Telemetry [1][2] implementation to Firefox 52. We want to move this to Beta a little faster to see somewhat realistic data of an example event (search) coming in and verify that it matches our expectations. I validated that this behaves as expected on Nightly over the last two weeks in bug 1302671. 1: https://docs.google.com/document/d/1hNuS9lUJMvMqgntZXbFA6xZBU9zBpQgo7x73-sXKRpI/ 2: https://wiki.mozilla.org/Event_Telemetry

Comment on attachment 8813200 [details] [diff] [review] Part 2 - Only serialize the events value & extra fields when needed Approval Request Comment ... see comment 11.

Comment on attachment 8813199 [details] [diff] [review] Part 1 - Use more strict size limits for event recording add event telemetry for aurora52

Comment on attachment 8813200 [details] [diff] [review] Part 2 - Only serialize the events value & extra fields when needed add event telemetry for aurora52

https://hg.mozilla.org/releases/mozilla-aurora/rev/37b147f723bc https://hg.mozilla.org/releases/mozilla-aurora/rev/42ed25f0652c