Closed Bug 1236580 Opened 4 years ago Closed 4 years ago

Remove the IS_UNIFIED_TELEMETRY constant and the related preferences

Categories

(Toolkit :: Telemetry, defect, P1)

defect
Points:
2

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox46 --- affected
firefox47 --- fixed

People

(Reporter: Dexter, Assigned: Dexter)

Details

(Whiteboard: [measurement:client])

Attachments

(2 files, 2 obsolete files)

Now that Unified Telemetry is up and running, and that we're removing FHR, we can get rid of all those preferences and constants that allowed to switch back and forth between FHR (v2) and Unified Telemetry (v4).

- IS_UNIFIED_TELEMETRY, "toolkit.telemetry.unified"
- IS_UNIFIED_OPTIN, "toolkit.telemetry.unifiedIsOptIn", "toolkit.telemetry.optoutSample" and the related [0]
- IS_V4
- "datareporting.healthreport.about.reportUrlUnified"

[0] - https://dxr.mozilla.org/mozilla-central/rev/d7a0ad85d9fb77916f9d77d62697b852f3dc63e6/toolkit/components/telemetry/TelemetryController.jsm#626
Whiteboard: [measurement:client]
Priority: -- → P3
(In reply to Alessio Placitelli [:Dexter] from comment #0)
> Now that Unified Telemetry is up and running, and that we're removing FHR,
> we can get rid of all those preferences and constants that allowed to switch
> back and forth between FHR (v2) and Unified Telemetry (v4).
> 
> - , "toolkit.telemetry.unified"

IS_UNIFIED_TELEMETRY currently toggles behavior on Fennec, that can't be removed without investigation/planning/cleanup work first.
Points: --- → 2
Priority: P3 → P1
Assignee: nobody → alessio.placitelli
Attachment #8709116 - Flags: review?(gfritzsche)
Status: NEW → ASSIGNED
Comment on attachment 8709116 [details] [diff] [review]
part 1 - Remove the unused constants

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

::: testing/mozbase/mozprofile/mozprofile/profile.py
@@ -386,5 @@
>                     # needed as Telemetry sends pings also if FHR upload is enabled.
>                     'toolkit.telemetry.server' : 'http://%(server)s/telemetry-dummy/',
> -                   # Our current tests expect the unified Telemetry feature to be opt-out,
> -                   # which is not true while we hold back shipping it.
> -                   'toolkit.telemetry.unifiedIsOptIn': True,

This flips Telemetry to opt-out in tests.
Will this break anything with our tests?
Will this break Thunderbird tests etc.?
Attachment #8709116 - Flags: review?(gfritzsche) → feedback+
(In reply to Georg Fritzsche [:gfritzsche] from comment #4)
> > -                   # Our current tests expect the unified Telemetry feature to be opt-out,
> > -                   # which is not true while we hold back shipping it.
> > -                   'toolkit.telemetry.unifiedIsOptIn': True,
> 
> This flips Telemetry to opt-out in tests.
> Will this break anything with our tests?

After checking the test again (and with the help of the previous try push) it seems like the tests don't really expect UT to be opt-out.

The crash tests were failing due to an error in my patch.

> Will this break Thunderbird tests etc.?

I don't think so. I digged through the TB Daily/Earlybird prefs and it seems like UT should behave the same, with the exception of the upload being disabled there.
Attachment #8712574 - Flags: review?(gfritzsche)
Comment on attachment 8712574 [details] [diff] [review]
part 1 - Remove the unused constants

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

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ -1087,5 @@
>      this._currentEnvironment.settings = {
>        blocklistEnabled: Preferences.get(PREF_BLOCKLIST_ENABLED, true),
>        e10sEnabled: Services.appinfo.browserTabsRemoteAutostart,
>        telemetryEnabled: Utils.isTelemetryEnabled,
> -      isInOptoutSample: TelemetryController.isInOptoutSample,

This needs a documentation update.
Attachment #8712574 - Flags: review?(gfritzsche) → review+
(In reply to Georg Fritzsche [:gfritzsche] from comment #9)
> This needs a documentation update.

That's in part 2. Do you want me to put the doc changes in a separate patch?
Nevermind, i missed part 2.
Comment on attachment 8709117 [details] [diff] [review]
part 2 - Fix the tests and update the docs

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

::: toolkit/components/telemetry/docs/preferences.rst
@@ +20,5 @@
>    * Telemetry will send additional ``main`` pings.
>  
>  ``toolkit.telemetry.unifiedIsOptIn``
>  
> +  **Do not use, obsolete.** When true, we enable the Telemetry system only for people that opted into Telemetry, even if unified Telemetry is enabled.

We removed this, so lets remove the documentation on it too.
Ditto for the ones below.
Attachment #8709117 - Flags: review?(gfritzsche) → review+
https://hg.mozilla.org/integration/fx-team/rev/fa59fd64e745b3af37b36c813f1b2e2e779a9fa6
Bug 1236580 - Remove the IS_UNIFIED_TELEMETRY constant and the related preferences. r=gfritzsche

https://hg.mozilla.org/integration/fx-team/rev/d359f149bf63826eefba0276ddb6050141af158c
Bug 1236580 - Fix the tests and update the documentation. r=gfritzsche
https://hg.mozilla.org/mozilla-central/rev/fa59fd64e745
https://hg.mozilla.org/mozilla-central/rev/d359f149bf63
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.