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.

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

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 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"

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

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

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 on attachment 8628228 [details] [diff] [review] bug1178456.patch - v3 Seems safe to uplift as it has been in m-c for 3 weeks.

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

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.

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!

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