While we won't fully ship on 40, we want to leave unified Telemetry behavior on for people who have "toolkit.telemetry.enabled" on (i.e. opted into Telemetry). While we have pref that toggles all the unified behavior, we don't have a pref that allows us leaving unified behavior on, but only for people that opted into Telemetry.

We just need to change one place for that (plus maybe tests): https://hg.mozilla.org/mozilla-central/annotate/e7e69cc8c07b/toolkit/components/telemetry/TelemetryController.jsm#l596 That should trigger the correct behavior for the rest of the code too. Suggesting to add a hidden pref for this, "toolkit.telemetry.unifiedIsOptIn".

Comment on attachment 8632057 [details] [diff] [review] bug1182424.patch Review of attachment 8632057 [details] [diff] [review]: ----------------------------------------------------------------- This looks good. I'd like to see this added to the documentation in a separate patch. ::: toolkit/components/telemetry/TelemetryController.jsm @@ +45,5 @@ > // Whether the FHR/Telemetry unification features are enabled. > // Changing this pref requires a restart. > const IS_UNIFIED_TELEMETRY = Preferences.get(PREF_UNIFIED, false); > +// This preference allows to leave unified Telemetry behavior on only for people that > +// opted into Telemetry. Add that it requires a restart.

Comment on attachment 8632085 [details] [diff] [review] part 2 - Flip the pref on in firefox.js Review of attachment 8632085 [details] [diff] [review]: ----------------------------------------------------------------- Note that we only want this patch on beta. ::: browser/app/profile/firefox.js @@ +1896,5 @@ > > // Telemetry settings. > // Determines if Telemetry pings can be archived locally. > pref("toolkit.telemetry.archive.enabled", true); > +// Whether the unified telemetry behavior is opt-in, requires a restart. "Whether Telemetry is opt-in even with unified Telemetry enabled, requires a restart."

Comment on attachment 8632086 [details] [diff] [review] part 3 - Update the docs. Review of attachment 8632086 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/docs/preferences.rst @@ +14,5 @@ > * Telemetry will send additional ``main`` pings. > > +``toolkit.telemetry.unifiedIsOptIn`` > + > + When true, this preference allows leaving unified behaviour on only for people that opted into Telemetry. How about: "When true, we enable the Telemetry system only for people that opted into Telemetry, even if unified Telemetry is enabled. Defaults to false & requires a restart."

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=06072924c286

Try had issues today, repushed: * beta simulation: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab4681c25892 * normal try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=61c1376502bc

Fixed test harness prefs: * beta simulation: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1c360df48e3 * normal push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=af224a72bc16

That was fun, try did have a lot of fallout for the trunk-as-try pushes. Todays try pushes look good: try from trunk with test fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee136b895371 try from beta plus "late beta" sim: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3d32dff3941 try from beta: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c1ec8fe7f9a

Comment on attachment 8632082 [details] [diff] [review] part 1 - Add the unifiedIsOptin pref. Approval Request Comment [Feature/regressing bug #]: Unified Telemetry [User impact if declined]: We made the call to not ship Unified Telemetry in 40. However, we want to collect the unified data from the small set of people that opted into Telemetry for validation of assumptions, data verification and to enable us shipping in the next cycle. This patch allows us to keep unified Telemetry on only for users who opted into Telemetry. [Describe test coverage new/current, TreeHerder]: Automation coverage exists but does not cover all real-world behavior, we took it through the basic test-cases for the feature. [Risks and why]: Low-risk: it's a very simple change to the Telemetry initialization logic, making it just dependent on one more pref. [String/UUID change made/needed]: None.

Comment on attachment 8632128 [details] [diff] [review] part 2 - Flip the pref on in firefox.js (BETA ONLY) Approval Request Comment [Feature/regressing bug #]: Unified Telemetry [User impact if declined]: We made the call to not ship Unified Telemetry in 40. However, we want to collect the unified data from the small set of people that opted into Telemetry for validation of assumptions, data verification and to enable us shipping in the next cycle. This patch flips the pref on. [Describe test coverage new/current, TreeHerder]: Automation coverage exists but does not cover all real-world behavior, we took it through the basic test-cases for the feature. [Risks and why]: Low-risk: Just flipping a pref. [String/UUID change made/needed]: None.

Comment on attachment 8632129 [details] [diff] [review] part 3 - Update the docs. Approval Request Comment [Feature/regressing bug #]: Unified Telemetry [User impact if declined]: We made the call to not ship Unified Telemetry in 40. However, we want to collect the unified data from the small set of people that opted into Telemetry for validation of assumptions, data verification and to enable us shipping in the next cycle. This patch adds documentation for the pref. [Describe test coverage new/current, TreeHerder]: Automation coverage exists but does not cover all real-world behavior, we took it through the basic test-cases for the feature. [Risks and why]: No risk: its just a documentation change. [String/UUID change made/needed]: None.

Comment on attachment 8632082 [details] [diff] [review] part 1 - Add the unifiedIsOptin pref. Sure, if we can help you without risk.

Why wasn't part 3 landed on fx-team?

Part 1 needs rebasing for Aurora (and presumably Beta) uplift.

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #20) > Why wasn't part 3 landed on fx-team? I just missed it.

Aurora Uplift: https://hg.mozilla.org/releases/mozilla-aurora/rev/c0079d1b9bc1 https://hg.mozilla.org/releases/mozilla-aurora/rev/8a4cf6cc9e80 Beta Uplift: https://hg.mozilla.org/releases/mozilla-beta/rev/6c4bb5acee12 https://hg.mozilla.org/releases/mozilla-beta/rev/fe0afe28c07c https://hg.mozilla.org/releases/mozilla-beta/rev/6141fafa9069 Treeherder for Beta https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=6141fafa9069

Verified that after updating 39RC and 40 beta to 40RC, telemetry wasn't enabled in non-modified builds while it remained enabled where the pref was manually modifed. Are there any other tests that should be performed? Thanks

Did you test it still behaved right after restarting? Otherwise that sounds fine.

There are no changes after restarting the builds.

Great, thanks.