Unknown category for SetEventRecordingEnabled: fxmonitor
Categories
(Firefox :: Firefox Monitor, defect, P1)
Tracking
()
People
(Reporter: robwu, Assigned: nhnt11)
References
(Regression)
Details
(Keywords: regression)
Attachments
(1 file)
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details | Review |
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.
Updated•6 years ago
|
Updated•6 years ago
|
Comment 1•6 years ago
|
||
Hi Nihanth, can we get a priority on this one? (Came up in regression triage) Thanks!
Assignee | ||
Comment 2•6 years ago
|
||
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.
Assignee | ||
Comment 3•6 years ago
|
||
Updated•6 years ago
|
Comment 4•6 years ago
|
||
Nihanth, what's the status of the patch on this bug? Should Luke take it over?
Updated•6 years ago
|
Assignee | ||
Comment 5•6 years ago
|
||
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 7•6 years ago
|
||
bugherder |
Comment 8•6 years ago
|
||
Is this something we should consider uplifting to Beta for Fx69 or can it ride to release with Fx70?
Assignee | ||
Comment 9•6 years ago
|
||
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!
Assignee | ||
Comment 10•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
bugherder uplift |
Updated•3 years ago
|
Description
•