Add a mitigation strategy for the "event" ping

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P1
normal
RESOLVED FIXED
Last year
11 months ago

People

(Reporter: chutten, Assigned: chutten)

Tracking

Trunk
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed, firefox63 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

Last year
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
Assignee

Comment 1

Last year
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"
Priority: -- → P2
Assignee

Updated

Last year
Assignee: nobody → chutten
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment hidden (mozreview-request)

Comment 3

Last year
mozreview-review
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?
Assignee

Comment 4

Last year
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.
Flags: needinfo?(alessio.placitelli)
(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
Flags: needinfo?(alessio.placitelli)
Comment hidden (mozreview-request)

Comment 7

Last year
mozreview-review
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
Attachment #8988166 - Flags: review?(alessio.placitelli) → review+

Comment 8

Last year
Pushed by chutten@mozilla.com:
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

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/a034391b5c15
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
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
Flags: needinfo?(chutten)
Assignee

Comment 11

Last year
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
Flags: needinfo?(chutten) → needinfo?(pascalc)
Assignee

Comment 12

Last year
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.
Flags: needinfo?(pascalc)
Attachment #8988166 - Flags: approval-mozilla-beta?
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.
Attachment #8988166 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Note for sheriffs, please uplift patches from bug 1460595 first.
Flags: needinfo?(aryx.bugmail)
Flags: needinfo?(aryx.bugmail)
You need to log in before you can comment on or make changes to this bug.