1236580 - Remove the IS_UNIFIED_TELEMETRY constant and the related preferences

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Alessio Placitelli [:Dexter]]

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

Comment 1

[Georg Fritzsche [:gfritzsche]]

(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.

Comment 2

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Remove the unused constants

Comment 3

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Fix the tests and update the docs

Comment 4

[Georg Fritzsche [:gfritzsche]]

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.?

Comment 5

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a8e75637110

Comment 6

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Remove the unused constants

Comment 7

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f98c379f075

Comment 8

[Alessio Placitelli [:Dexter]]

(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.

Comment 9

[Georg Fritzsche [: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.

Comment 10

[Alessio Placitelli [:Dexter]]

(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?

Comment 11

[Georg Fritzsche [:gfritzsche]]

Nevermind, i missed part 2.

Comment 12

[Georg Fritzsche [:gfritzsche]]

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.

Comment 13

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Fix the tests and update the docs

Comment 14

[Alessio Placitelli [:Dexter]]

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

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fa59fd64e745
https://hg.mozilla.org/mozilla-central/rev/d359f149bf63