Closed Bug 1316810 Opened 3 years ago Closed 3 years ago

Change the event recording limits based on the size discussion

Categories

(Toolkit :: Telemetry, defect, P1)

defect
Points:
1

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: gfritzsche, Assigned: gfritzsche)

References

(Blocks 1 open bug)

Details

(Whiteboard: [measurement:client])

Attachments

(2 files)

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.
Attachment #8813199 - Flags: review?(alessio.placitelli)
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)
Attachment #8813199 - Flags: review?(alessio.placitelli) → review+
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+
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)
(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
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
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Whiteboard: [measurement:client] → [measurement:client] [measurement:client:uplift]
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?
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 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 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+
Whiteboard: [measurement:client] [measurement:client:uplift] → [measurement:client]
You need to log in before you can comment on or make changes to this bug.