Closed
Bug 1316810
Opened 6 years ago
Closed 6 years ago
Change the event recording limits based on the size discussion
Categories
(Toolkit :: Telemetry, defect, P1)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla53
People
(Reporter: gfritzsche, Assigned: gfritzsche)
References
(Blocks 1 open bug)
Details
(Whiteboard: [measurement:client])
Attachments
(2 files)
10.17 KB,
patch
|
Dexter
:
review+
jcristau
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
10.16 KB,
patch
|
Dexter
:
review+
jcristau
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
We need to update the event recording limits to the results from bug 1313592.
Assignee | ||
Comment 1•6 years ago
|
||
This updates the event implementation to the limits we based the estimations in bug 1313592 et al on.
Attachment #8813199 -
Flags: review?(alessio.placitelli)
Assignee | ||
Comment 2•6 years ago
|
||
Assignee | ||
Comment 3•6 years ago
|
||
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]
Attachment #8813200 -
Flags: review?(nfroyd)
Attachment #8813200 -
Flags: review?(alessio.placitelli)
Updated•6 years ago
|
Attachment #8813199 -
Flags: review?(alessio.placitelli) → review+
Comment 4•6 years ago
|
||
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.
Attachment #8813200 -
Flags: review?(alessio.placitelli) → review+
Assignee | ||
Comment 5•6 years ago
|
||
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.
Attachment #8813200 -
Flags: review?(nfroyd)
Comment 6•6 years ago
|
||
(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
Comment 8•6 years ago
|
||
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
Comment 10•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ce3b7997c139 https://hg.mozilla.org/mozilla-central/rev/0d1d639a320c
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Assignee | ||
Updated•6 years ago
|
Whiteboard: [measurement:client] → [measurement:client] [measurement:client:uplift]
Assignee | ||
Comment 11•6 years ago
|
||
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
Attachment #8813199 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 12•6 years ago
|
||
Comment on attachment 8813200 [details] [diff] [review] Part 2 - Only serialize the events value & extra fields when needed Approval Request Comment ... see comment 11.
Attachment #8813200 -
Flags: approval-mozilla-aurora?
Comment 13•6 years ago
|
||
Comment on attachment 8813199 [details] [diff] [review] Part 1 - Use more strict size limits for event recording add event telemetry for aurora52
Attachment #8813199 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 14•6 years ago
|
||
Comment on attachment 8813200 [details] [diff] [review] Part 2 - Only serialize the events value & extra fields when needed add event telemetry for aurora52
Attachment #8813200 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 15•6 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/37b147f723bc https://hg.mozilla.org/releases/mozilla-aurora/rev/42ed25f0652c
Assignee | ||
Updated•6 years ago
|
Whiteboard: [measurement:client] [measurement:client:uplift] → [measurement:client]
You need to log in
before you can comment on or make changes to this bug.
Description
•