Closed
Bug 1316810
Opened 8 years ago
Closed 8 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)
(deleted),
patch
|
Dexter
:
review+
jcristau
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
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•8 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•8 years ago
|
||
Assignee | ||
Comment 3•8 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•8 years ago
|
Attachment #8813199 -
Flags: review?(alessio.placitelli) → review+
Comment 4•8 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•8 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•8 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•8 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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ce3b7997c139
https://hg.mozilla.org/mozilla-central/rev/0d1d639a320c
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Assignee | ||
Updated•8 years ago
|
Whiteboard: [measurement:client] → [measurement:client] [measurement:client:uplift]
Assignee | ||
Comment 11•8 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•8 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•8 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•8 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•8 years ago
|
||
bugherder uplift |
Assignee | ||
Updated•8 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
•