1463439 - Add a mitigation strategy for the "event" ping

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P1)

Description

[Chris H-C :chutten]

If everything goes pear-shaped and we start recording far more events on a release than we'd like, we should have sensible limits and easy-to-reach shutoff levers for the "event" ping.

Concerns:
- Unbounded memory storage for events
- Difficult to shut down event reporting because of many prefs with odd names
- High frequency, large "event" pings

Comment 1

[Chris H-C :chutten]

Another mitigation might be to set a factor by which we are not going to go over in recording events. 2x the event limit ought to be enough to account for excess leftover events in c++ until the event ping gets around to counting them as "lost"

Comment 2

[Chris H-C :chutten]

Attachment: bug 1463439 - Add a pref to enable 'event' ping. Defaults to true, except for GV.

When it's false we also disable collecting events completely, in case the
reason we're disabling it is due to storage issues.

Review commit: https://reviewboard.mozilla.org/r/253428/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/253428/

Comment 3

[Alessio Placitelli [:Dexter]]

Comment on attachment 8988166 [details]
bug 1463439 - Add a pref to enable 'event' ping. Defaults to true, except for GV.

https://reviewboard.mozilla.org/r/253428/#review260012

::: toolkit/components/telemetry/TelemetryEvent.cpp:497
(Diff revision 1)
>    if (!gEnabledCategories.GetEntry(GetCategory(lock, *eventKey))) {
>      return RecordEventResult::Ok;
>    }
>  
> +  static bool sEventPingEnabled =
> +    mozilla::Preferences::GetBool("toolkit.telemetry.eventping.enabled", true);

In the docs you mention that the default is `false`, but here is `true`. I think this should be `false` so that we don't mistakenly collect events on products we don't want to (e.g. GeckoView). You can add an entry [here](https://searchfox.org/mozilla-central/rev/14cb8f1c238735ba1abe18ad44e39808983c2572/browser/app/profile/firefox.js#1460-1476), close to the other Telemetry prefs, if you want this to be true.

::: toolkit/components/telemetry/docs/internals/preferences.rst:148
(Diff revision 1)
>    :ref:`keyed scalars <scalars.keyed-scalars>`. Default is 500. Change requires restart.
>  
> +``toolkit.telemetry.eventping.enabled``
> +
> +  Whether the :doc:`../data/event-ping` is enabled.
> +  Default is false. Change requires restart.

It is? Are we shipping this disabled by default?

Comment 4

[Chris H-C :chutten]

Argh, this is a confusion from an earlier patch. The idea is to ship it enabled by default.

I was taking as an example the "health" ping. It, too, is enabled by default on all platforms where it isn't specifically disabled. How strongly do you feel that events should be disabled by default? I'm leaning towards just changing the doc.

Comment 5

[Alessio Placitelli [:Dexter]]

(In reply to Chris H-C :chutten from comment #4)
> Argh, this is a confusion from an earlier patch. The idea is to ship it
> enabled by default.
> 
> I was taking as an example the "health" ping. It, too, is enabled by default
> on all platforms where it isn't specifically disabled. How strongly do you
> feel that events should be disabled by default? I'm leaning towards just
> changing the doc.

I think it's fine to ship this enabled by default, since that's how it worked before, right? However, since we're not sending events from GeckoView I wouldn't mind having events disabled on that platform by default. Would you kindly add a pref there [1], mentioning that it's disabled because we're not sending/persisting events on GV? It would be great if you could mention that in the docs too.

[1] - https://searchfox.org/mozilla-central/rev/14cb8f1c238735ba1abe18ad44e39808983c2572/mobile/android/app/geckoview-prefs.js#15

Comment 6

[Chris H-C :chutten]

Comment on attachment 8988166 [details]
bug 1463439 - Add a pref to enable 'event' ping. Defaults to true, except for GV.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253428/diff/1-2/

Comment 7

[Alessio Placitelli [:Dexter]]

Comment on attachment 8988166 [details]
bug 1463439 - Add a pref to enable 'event' ping. Defaults to true, except for GV.

https://reviewboard.mozilla.org/r/253428/#review260064

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a034391b5c15
Add a pref to enable 'event' ping. Defaults to true, except for GV. r=Dexter

Comment 9

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/a034391b5c15

Comment 10

[Pascal Chevrel:pascalc]

Hi Chris, is there a benefit in having this patch uplifted to 62 beta or can I change the tracking flag for Firefox 62 from affected to wontfix?

Thanks

Comment 11

[Chris H-C :chutten]

62 shouldn't be affected, as the event ping only landed in 63 last week.

I -am- planning on requesting uplift for both the ping and this, though, so I'm not sure what flag statuses apply :S

Comment 12

[Chris H-C :chutten]

Comment on attachment 8988166 [details]
bug 1463439 - Add a pref to enable 'event' ping. Defaults to true, except for GV.

Approval Request Comment
[Feature/Bug causing the regression]:
No regression
[User impact if declined]:
The new "event" pings may not be easily disable-able.
[Is this code covered by automated tests?]:
Yes.
[Has the fix been verified in Nightly?]:
Yes
[Needs manual test from QE? If yes, steps to reproduce]: 
No
[List of other uplifts needed for the feature/fix]:
bug 1460595 first, then this one and bug 1470493 in either order.
[Is the change risky?]:
No.
[Why is the change risky/not risky?]:
Only adds a pref for disabling the "event" ping. 
[String changes made/needed]:
None.

Comment 13

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8988166 [details]
bug 1463439 - Add a pref to enable 'event' ping. Defaults to true, except for GV.

OK, this should let us disable event pings if needed in 62.

Comment 14

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Note for sheriffs, please uplift patches from bug 1460595 first.

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/3281d0590870