1178456 - Experiments service shouldn't use FHR prefs

Status: VERIFIED FIXED

(Toolkit :: Telemetry, defect)

Description

[Benjamin Smedberg]

The experiments service currently uses datareporting.healthreport.service.enabled

We should check other prefs or ideally just use the helper functions in the telemetry code.

Comment 1

[Alessio Placitelli [:Dexter]]

Attachment: bug1178456.patch

This patch removes the dependency and uses Telemetry.canRecordBase to check if Telemetry is on.

Comment 2

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8627811 [details] [diff] [review]
bug1178456.patch

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

::: browser/experiments/ExperimentsService.js
@@ +27,5 @@
>    });
>  
>  XPCOMUtils.defineLazyGetter(
>    this, "gExperimentsEnabled", () => {
> +    return gPrefs.get(PREF_EXPERIMENTS_ENABLED, false) && Services.telemetry.canRecordBase;

This actually changes semantics here - experiments should only be enabled when the user opted into Telemetry.

To keep things working properly and be able to switch between old FHR and unified Telemetry, we should hang it off the right prefs:

EXPERIMENTS_ENABLED && (UNIFIED_TELEMETRY || HEALTHREPORT_ENABLED) && TELEMETRY_ENABLED

Comment 3

[Alessio Placitelli [:Dexter]]

Attachment: bug1178456.patch - v2

Comment 4

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8628218 [details] [diff] [review]
bug1178456.patch - v2

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

Please take this through some basic manual testing to make sure Experiments initializes (or not) as expected.
Confirming via logging should be sufficient.

::: browser/experiments/ExperimentsService.js
@@ +35,5 @@
>  
>  XPCOMUtils.defineLazyGetter(
>    this, "gExperimentsEnabled", () => {
> +    // We can enable experiments if either unified telemetry or FHR is on, and user
> +    // has opted into Telemetry.

Nits: upper-case Telemetry, "the user"

Comment 5

[Alessio Placitelli [:Dexter]]

Attachment: bug1178456.patch - v3

That's what I've tested so far:

- DRS and Unified Telemetry both ON, the user opted into Telemetry -> Experiments Enabled
- DRS ON, Unified OFF, the user opted into Telemetry -> Experiments Enabled
- DRS OFF, Unified ON, the user opted into Telemetry -> Experiments Enabled
- DRS OFF, Unified OFF -> Experiments Disabled
- DRS On, Unified ON, the user didn't opt into Telemetry -> Experiments disabled
- DRS On, Unified Off, the user didn't opt into Telemetry -> Experiments disabled

Comment 6

[Alessio Placitelli [:Dexter]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3dfd8f91f473

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/b9b256c42a62

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b9b256c42a62

Comment 9

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8628228 [details] [diff] [review]
bug1178456.patch - v3

Approval Request Comment
[Feature/regressing bug #]:
This is part of the uplift3 batch for the unified Telemetry project: http://bit.ly/1TCl4r8
This is targetting 41 now and these are the last minor "feature" patches blocking shipping - any further patches will be fixes for data quality etc. with very limited scope & impact.
[User impact if declined]:
Unified Telemetry can't ship.
This is a fix needed to keep Telemetry Experiments working.
[Describe test coverage new/current, TreeHerder]:
This has good automated test-coverage.
[Risks and why]:
Low-risk, well understood small behavior change limited to Experiments code.
[String/UUID change made/needed]:
No string changes.

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8628228 [details] [diff] [review]
bug1178456.patch - v3

Seems safe to uplift as it has been in m-c for 3 weeks.

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked. Requesting QE team to verify the fix whenever possible. Thanks!

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6b359b569710

Comment 13

[Andrei Vaida [:avaida]]

I see that the verification of this bug implies having _only_ Unified Telemetry disabled in some cases. Alessio, is there any way to manually disable just Unified Telemetry on Beta 41? It was my understanding that on pre-release builds, Unified Telemetry is always enabled and the "toolkit.telemetry.unified" pref doesn't control its state.

Comment 14

[Alessio Placitelli [:Dexter]]

Andrei, thanks for testing this! We are not planning to turn toolkit.telemetry.unified off anymore, so testing with FHR on/off should be enough!

Comment 15

[Andrei Vaida [:avaida]]

Verified fixed on Firefox 41.0b8 (20150907144446), using Windows 7 x64, Ubuntu 14.04 and Mac OS X 10.9.5.

Confirmed that the state of datareporting.healthreport.service.enabled has no effect over the experiments feature. I've used the guidelines from the Telemetry Test Plan [1] to do this.

[1] https://wiki.mozilla.org/QA/Telemetry#Telemetry_Experiments