Closed Bug 1463439 Opened 3 years ago Closed 2 years ago

Add a mitigation strategy for the "event" ping

Categories

(Toolkit :: Telemetry, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox62 --- fixed
firefox63 --- fixed

People

(Reporter: chutten, Assigned: chutten)

References

Details

Attachments

(1 file)

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
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: nobody → chutten
Status: NEW → ASSIGNED
Priority: P2 → P1
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?
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 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+
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
https://hg.mozilla.org/mozilla-central/rev/a034391b5c15
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
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)
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)
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.