1561012 - Unknown category for SetEventRecordingEnabled: fxmonitor

Status: RESOLVED FIXED

(Firefox :: Firefox Monitor, defect, P1)

Description

[Rob Wu [:robwu]]

I've noticed the following warning in the global JS console when I start Firefox Nightly (buildID 20190618041042), and also reproduced this in Firefox Beta (68.0b12):

Unknown category for SetEventRecordingEnabled: fxmonitor

In Firefox 67.0.4:

Unkown category for SetEventRecordingEnabled.

This was introduced in bug 1525519 (which added the code), and exposed in bug 1531838 (which enabled FxMonitor by default in Fx68, which exposes this issue; apparently the feature is also enabled via Normandy, which triggers the issue in Firefox 67 as well).

This warning also shows that the extensions.fxmonitor.telemetryDisabled preference does not work at all. This preference was added when Fxmonitor was moved in-tree, because data review was pending (bug 1525977). Since the collection request has been approved, it should be fine to remove this preference.

The proper fix to this bug (besides potentially removing the preference) is to move the telemetry event registration to Events.yaml.

Comment 1

[Patricia Lawless]

Hi Nihanth, can we get a priority on this one? (Came up in regression triage) Thanks!

Comment 2

[(inactive) Nihanth Subramanya [:nhnt11]]

The issue is that the events are dynamically registered, and I seem to have missed that the documentation says that dynamically registered events can't be disabled.

The solution (without messing with Events.yaml) is to put the registration itself behind the pref rather than using the pref for enabling/disabling. I don't want to use Events.yaml until we implement Firefox Monitor as a direct browser feature rather than an extension - if we ever do.

Comment 3

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1561012 - Don't register fxmonitor events if telemetry is disabled. r=johannh,robwu

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Nihanth, what's the status of the patch on this bug? Should Luke take it over?

Comment 5

[(inactive) Nihanth Subramanya [:nhnt11]]

Bah, sorry, this got lost in my backlog since I was on PTO for 2 weeks. I just need to land it, I'll take care of that. Thanks for the bump!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/90dad26c3b63
Don't register fxmonitor events if telemetry is disabled. r=johannh,robwu

Comment 7

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/90dad26c3b63

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider uplifting to Beta for Fx69 or can it ride to release with Fx70?

Comment 9

[(inactive) Nihanth Subramanya [:nhnt11]]

It's not critical to uplift, but it's also easy to do so and eliminates some console spam, so maybe we should do it. I was worried that the patch may need rebasing due to the formatting changes but it seems like those are in beta already so we should be good. I'll request uplift. Thanks!

Comment 10

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 9075762 [details]
Bug 1561012 - Don't register fxmonitor events if telemetry is disabled. r=johannh,robwu

Beta/Release Uplift Approval Request

• User impact if declined: Not much - there will be some noise in the browser console.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch does some slight refactoring to avoid a function call that was redundant and caused an error in the console.
• String changes made/needed: none

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9075762 [details]
Bug 1561012 - Don't register fxmonitor events if telemetry is disabled. r=johannh,robwu

Fixes logspam by not trying to register fxmonitor events if Telemetry is disabled. Approved for 69.0b10.

Comment 12

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

https://hg.mozilla.org/releases/mozilla-beta/rev/6984cce40f4e