Closed Bug 1182424 Opened 9 years ago Closed 9 years ago

Always leave unified Telemetry behavior on for people who opted into Telemetry

Categories

(Toolkit :: Telemetry, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla42
Tracking Status
firefox39 --- unaffected
firefox40 --- verified
firefox41 --- fixed
firefox42 --- fixed

People

(Reporter: gfritzsche, Assigned: Dexter)

References

Details

(Whiteboard: [b5] [unifiedTelemetry] [uplift1.5])

Attachments

(3 files, 3 obsolete files)

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".
Blocks: 1120356
Attached patch bug1182424.patch (obsolete) — Splinter Review
Attachment #8632057 - Flags: feedback?(gfritzsche)
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.
Attachment #8632057 - Flags: feedback?(gfritzsche) → review+
Attachment #8632057 - Attachment is obsolete: true
Attachment #8632082 - Flags: review+
Attachment #8632085 - Flags: review?(gfritzsche)
Attached patch part 3 - Update the docs. (obsolete) — Splinter Review
Attachment #8632086 - Flags: review?(gfritzsche)
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."
Attachment #8632085 - Flags: review?(gfritzsche) → review+
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."
Attachment #8632086 - Flags: review?(gfritzsche) → review+
Attachment #8632085 - Attachment is obsolete: true
Attachment #8632128 - Flags: review+
Attachment #8632086 - Attachment is obsolete: true
Attachment #8632129 - Flags: review+
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.
Attachment #8632082 - Flags: approval-mozilla-beta?
Attachment #8632082 - Flags: approval-mozilla-aurora?
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.
Attachment #8632128 - Flags: approval-mozilla-beta?
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.
Attachment #8632129 - Flags: approval-mozilla-beta?
Attachment #8632129 - Flags: approval-mozilla-aurora?
Comment on attachment 8632082 [details] [diff] [review]
part 1 - Add the unifiedIsOptin pref.

Sure, if we can help you without risk.
Attachment #8632082 - Flags: approval-mozilla-beta?
Attachment #8632082 - Flags: approval-mozilla-beta+
Attachment #8632082 - Flags: approval-mozilla-aurora?
Attachment #8632082 - Flags: approval-mozilla-aurora+
Attachment #8632128 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8632129 - Flags: approval-mozilla-beta?
Attachment #8632129 - Flags: approval-mozilla-beta+
Attachment #8632129 - Flags: approval-mozilla-aurora?
Attachment #8632129 - Flags: approval-mozilla-aurora+
Why wasn't part 3 landed on fx-team?
Flags: needinfo?(gfritzsche)
Part 1 needs rebasing for Aurora (and presumably Beta) uplift.
Flags: needinfo?(alessio.placitelli)
https://hg.mozilla.org/mozilla-central/rev/d6cefdb86d88
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #20)
> Why wasn't part 3 landed on fx-team?

I just missed it.
Flags: needinfo?(gfritzsche)
Whiteboard: [b5] [unifiedTelemetry] [uplift2] → [b5] [unifiedTelemetry] [uplift1.5]
Flags: qe-verify+
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.
Flags: qe-verify+
Blocks: 1192906
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.