1316810 - Change the event recording limits based on the size discussion

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Georg Fritzsche [:gfritzsche]]

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

Comment 1

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 1 - Use more strict size limits for event recording

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

Comment 2

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 2 - Only serialize the events value & extra fields when needed

Comment 3

[Georg Fritzsche [:gfritzsche]]

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 4

[Alessio Placitelli [:Dexter] - PTO until 26/July]

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 5

[Georg Fritzsche [:gfritzsche]]

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.

Comment 6

[Alessio Placitelli [:Dexter] - PTO until 26/July]

(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.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
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

[Phil Ringnalda (:philor)]

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso)]

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

Comment 11

[Georg Fritzsche [:gfritzsche]]

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 12

[Georg Fritzsche [:gfritzsche]]

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 13

[Julien Cristau [:jcristau]]

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

add event telemetry for aurora52

Comment 14

[Julien Cristau [:jcristau]]

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

add event telemetry for aurora52

Comment 15

[Georg Fritzsche [:gfritzsche]]

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