Closed Bug 1344723 Opened 3 years ago Closed 2 years ago

Use the prefs defined in TelemetryUtils.jsm in TelemetryController.jsm

Categories

(Toolkit :: Telemetry, enhancement, P3)

enhancement
Points:
2

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox54 --- wontfix
firefox55 --- wontfix
firefox56 --- fixed

People

(Reporter: Dexter, Assigned: alejandro, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [measurement:client][lang=js])

Attachments

(1 file, 2 obsolete files)

Once bug 1344295 lands, we should change Telemetry code to use the preferences defined in the TelemetryUtils.jsm file rather then redefining them in every Telemetry JSM file.
Blocks: 1201022
Points: --- → 2
Depends on: 1344295
Priority: -- → P3
Whiteboard: [measurement:client]
Mentor: alessio.placitelli
Whiteboard: [measurement:client] → [measurement:client][lang=js]
Let's restrict this bug to changes to TelemetryController.jsm. After this lands, we can file more bugs for the other files (so we don't risk bitrotting here with a huge patch).
Summary: Use the prefs defined in TelemetryUtils.jsm in Telemetry code → Use the prefs defined in TelemetryUtils.jsm in TelemetryController.jsm
Assignee: nobody → alexrs95
I take this one.
There are several preferences such as PREF_BRANCH_LOG (and all the preferences that extends this one, PREF_LOG_LEVEL, PREF_LOG_DUMP...), or PREF_NEWPROFILE_PING_ENABLED that are not declared in TelemetryUtils.jsm. Do I need to create this preferences in TelemetryUtils.jsm?
Flags: needinfo?(alessio.placitelli)
(In reply to Alejandro Rodriguez Salamanca from comment #3)
> There are several preferences such as PREF_BRANCH_LOG (and all the
> preferences that extends this one, PREF_LOG_LEVEL, PREF_LOG_DUMP...), or
> PREF_NEWPROFILE_PING_ENABLED that are not declared in TelemetryUtils.jsm. Do
> I need to create this preferences in TelemetryUtils.jsm?

Yes please! If you see any preference that's missing, make sure to add it to the Utils file.
Flags: needinfo?(alessio.placitelli)
Comment on attachment 8887476 [details] [diff] [review]
Add missing preferences to TelemetryUtils and replace local preferences in TelemetryController

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

That's a good start, thanks Alejandro!

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +119,5 @@
>  
>  this.EXPORTED_SYMBOLS = ["TelemetryController"];
>  
>  this.TelemetryController = Object.freeze({
>    Constants: Object.freeze({

We now have the TelemetryUtils.Preferences object for all our preferences needs. Let's get rid of this "Constants" object and properly use TelemetryUtils where this was referenced. I think it isn't being used, otherwise you should have seen an error due to ":" being used ;)

@@ +880,5 @@
>     * Called whenever the FHR Upload preference changes (e.g. when user disables FHR from
>     * the preferences panel), this triggers sending the deletion ping.
>     */
>    _onUploadPrefChange() {
> +    const uploadEnabled = Preferences.get(TelemetryUtils.Preferences.HealthReportUploadEnabled, false);

Please use the pref that was already available here.

@@ +909,5 @@
>  
>    _attachObservers() {
>      if (IS_UNIFIED_TELEMETRY) {
>        // Watch the FHR upload setting to trigger deletion pings.
> +      Preferences.observe(TelemetryUtils.Preferences.HealthReportUploadEnabled, this._onUploadPrefChange, this);

Please use the pref that was already available here.

@@ +918,5 @@
>     * Remove the preference observer to avoid leaks.
>     */
>    _detachObservers() {
>      if (IS_UNIFIED_TELEMETRY) {
> +      Preferences.ignore(TelemetryUtils.Preferences.HealthReportUploadEnabled, this._onUploadPrefChange, this);

Please use the pref that was already available here.

::: toolkit/components/telemetry/TelemetryUtils.jsm
@@ +37,5 @@
> +    NewProfilePingEnabled: "toolkit.telemetry.newProfilePing.enabled",
> +    NewProfilePingDelay: "toolkit.telemetry.newProfilePing.delay",
> +
> +    // Log Preferences
> +    Log: "toolkit.telemetry.log.",

This log entry is only used once in the TelemetryController file, let's just leave it as a const there. Just move LogLevel and LogDump here.

@@ +50,5 @@
>      DataSubmissionEnabled: "datareporting.policy.dataSubmissionEnabled",
>      FhrUploadEnabled: "datareporting.healthreport.uploadEnabled",
>      MinimumPolicyVersion: "datareporting.policy.minimumPolicyVersion",
> +    HealthReportUploadEnabled: "datareporting.healthreport.uploadEnabled",
> +

This preference is already available a couple of lines above, as FhrUploadEnabled.
Attachment #8887476 - Flags: review?(alessio.placitelli) → feedback+
Attachment #8887526 - Flags: review?(alessio.placitelli)
Attachment #8887476 - Attachment is obsolete: true
Comment on attachment 8887526 [details] [diff] [review]
Remove redundant and useless code

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

This looks good with the nit below addressed! You won't need to request a new review after you attach the updated patch.
I will push it to try for you once you upload the updated patch!

::: toolkit/components/telemetry/TelemetryUtils.jsm
@@ +48,5 @@
>      CurrentPolicyVersion: "datareporting.policy.currentPolicyVersion",
>      DataSubmissionEnabled: "datareporting.policy.dataSubmissionEnabled",
>      FhrUploadEnabled: "datareporting.healthreport.uploadEnabled",
>      MinimumPolicyVersion: "datareporting.policy.minimumPolicyVersion",
> +

nit: please remove this empty blank line
Attachment #8887526 - Flags: review?(alessio.placitelli) → review+
Comment on attachment 8887526 [details] [diff] [review]
Remove redundant and useless code

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

Also please the commit message to something like "Bug 1344723 - Use the prefs defined in TelemetryUtils.jsm in TelemetryController.jsm. r?dexter"
Attachment #8887526 - Attachment is obsolete: true
Attachment #8887828 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/9599eab713ca6e7660e18307cf418378896f0363
Bug 1344723 - Use the prefs defined in TelemetryUtils.jsm in TelemetryController.jsm. r=dexter
Duplicate of this bug: 1375482
https://hg.mozilla.org/mozilla-central/rev/9599eab713ca
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
We are about to go to 55 RC. Mark 55 won't fix.
You need to log in before you can comment on or make changes to this bug.