Translation Telemetry shouldn't check the toolkit.telemetry.enabled pref

RESOLVED FIXED in Firefox 59

Status

()

Toolkit
Telemetry
P3
normal
RESOLVED FIXED
a month ago
3 days ago

People

(Reporter: chutten, Assigned: Aditya Bharti, Mentored)

Tracking

(Blocks: 1 bug)

Trunk
mozilla59
Points:
---

Firefox Tracking Flags

(firefox58 wontfix, firefox59 fixed)

Details

(Whiteboard: [good next bug][lang=js])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

a month ago
browser/components/translation/Translation.jsm checks the extended telemetry pref before collecting telemetry. This is wasteful and unnecessary as Telemetry does it for you.

Remove all uses of the pref from Translation.jsm and its tests. (browser_translation_telemetry.js sets and resets the pref unnecessarily already)
Priority: -- → P3
(Assignee)

Comment 1

9 days ago
Hi, I would like to start working on this bug.

I'm looking at the Translation.jsm file and I see multiple calls to the _canRecord function, which calls the Services.prefs.getBoolPref("toolkit.telemetry.enabled"). Is this the pref whose uses must be removed?

Also, just to ensure I understand correctly, this check is not needed because the Services.telemetry module performs the check for you? If that is the case, I'm assuming no data is returned if the pref is not enabled. Will the application handle this case correctly?
Flags: needinfo?(chutten)
(Reporter)

Comment 2

8 days ago
You are right: _canRecord can go, as can all of the early returns that use it.

You are also right in that Telemetry handles whether or not to record or send this data internally.

I have assigned this bug to you.
Assignee: nobody → adibhar97
Status: NEW → ASSIGNED
Flags: needinfo?(chutten)
(Assignee)

Comment 3

8 days ago
Created attachment 8929048 [details] [diff] [review]
Removed definition and all uses of _canRecord from Translation.jsm

I have removed all uses of the toolkit.telemetry.enabled pref from Translation.jsm. The about:telemetry page works and nothing, as far as I can see, breaks. 

I'm not sure about which test file needs modification so will remove the pref from testfile browser_telemetry.js (https://searchfox.org/mozilla-central/source/browser/components/preferences/in-content/tests/browser_telemetry.js#6) in next patch file. Waiting for further info on this before proceeding.

Meanwhile, please take a look at these changes. Any other test files which need modification?
Flags: needinfo?(chutten)
(Reporter)

Comment 4

8 days ago
The patch looks like a good draft!

Don't go ahead with changing browser_telemetry.js. Instead, the tests you change should be limited to just the translation tests found in browser_translation_telemetry.js (browser/components/translation/test/browser_translation_telemetry.js)
Flags: needinfo?(chutten)
(Assignee)

Comment 5

8 days ago
How do I know which tests to change? There don't seem to be any failing tests. I ran mochitests on that single file (browser/components/translation/test/browser_translation_telemetry.js) and all 24 tests passed. Plus, I'm not seeing the pref toolkit.telemetry.enabled being explicitly checked anywhere in the file?

Can you give me a little more info on which tests require a change and why? Thanks.
Flags: needinfo?(chutten)
(Reporter)

Comment 6

8 days ago
Oh. Looks like I took care of that file during bug 1406391. :S

Sorry about that, looks like all you needed was the changes to the jsm itself. Carry on!
Flags: needinfo?(chutten)
(Assignee)

Comment 7

8 days ago
Created attachment 8929076 [details] [diff] [review]
Changed file Translation.jsm

Since Translation.jsm was the only file that needed to be changed, uploading a patch for review.
Attachment #8929048 - Attachment is obsolete: true
Attachment #8929076 - Flags: review?(chutten)
(Reporter)

Comment 8

8 days ago
Comment on attachment 8929076 [details] [diff] [review]
Changed file Translation.jsm

Review of attachment 8929076 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!
Attachment #8929076 - Flags: review?(chutten) → review+
(Reporter)

Comment 9

8 days ago
We're going to have to find you a harder one for next time. Does anything in this list of "good second bugs" appeal to you? https://bugzilla.mozilla.org/buglist.cgi?cmdtype=runnamed&namedcmd=goodSecondBug%20-%20unassigned&list_id=13885366
Keywords: checkin-needed

Comment 10

8 days ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/56c03f806d36
Fix Translation Telemetry shouldn't check toolkit.telemetry.enabled pref. r=chutten
Keywords: checkin-needed

Comment 11

7 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/56c03f806d36
Status: ASSIGNED → RESOLVED
Last Resolved: 7 days ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
status-firefox58: affected → wontfix
You need to log in before you can comment on or make changes to this bug.