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

VERIFIED FIXED in Firefox 40

Status

()

Toolkit
Telemetry
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: gfritzsche, Assigned: Dexter)

Tracking

Trunk
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 unaffected, firefox40 verified, firefox41 fixed, firefox42 fixed)

Details

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

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
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
(Reporter)

Updated

2 years ago
status-firefox39: --- → unaffected
status-firefox40: --- → affected
status-firefox41: --- → affected
(Assignee)

Comment 2

2 years ago
Created attachment 8632057 [details] [diff] [review]
bug1182424.patch
Attachment #8632057 - Flags: feedback?(gfritzsche)
(Reporter)

Comment 3

2 years ago
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+
(Assignee)

Comment 4

2 years ago
Created attachment 8632082 [details] [diff] [review]
part 1 - Add the unifiedIsOptin pref.
Attachment #8632057 - Attachment is obsolete: true
Attachment #8632082 - Flags: review+
(Assignee)

Comment 5

2 years ago
Created attachment 8632085 [details] [diff] [review]
part 2 - Flip the pref on in firefox.js
Attachment #8632085 - Flags: review?(gfritzsche)
(Assignee)

Comment 6

2 years ago
Created attachment 8632086 [details] [diff] [review]
part 3 - Update the docs.
Attachment #8632086 - Flags: review?(gfritzsche)
(Reporter)

Comment 7

2 years ago
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+
(Reporter)

Comment 8

2 years ago
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+
(Assignee)

Comment 9

2 years ago
Created attachment 8632128 [details] [diff] [review]
part 2 - Flip the pref on in firefox.js (BETA ONLY)
Attachment #8632085 - Attachment is obsolete: true
Attachment #8632128 - Flags: review+
(Assignee)

Comment 10

2 years ago
Created attachment 8632129 [details] [diff] [review]
part 3 - Update the docs.
Attachment #8632086 - Attachment is obsolete: true
Attachment #8632129 - Flags: review+
(Assignee)

Comment 11

2 years ago
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=06072924c286
(Reporter)

Comment 12

2 years ago
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
(Reporter)

Comment 13

2 years ago
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
(Reporter)

Comment 14

2 years ago
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 15

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/d6cefdb86d88
(Reporter)

Comment 16

2 years ago
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?
(Reporter)

Comment 17

2 years ago
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?
(Reporter)

Comment 18

2 years ago
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
Last Resolved: 2 years ago
status-firefox42: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
(Reporter)

Comment 23

2 years ago
(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)

Comment 24

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/47fb1123fbb8
(Assignee)

Comment 25

2 years ago
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
Flags: needinfo?(alessio.placitelli)
status-firefox40: affected → fixed
status-firefox41: affected → fixed
https://hg.mozilla.org/mozilla-central/rev/47fb1123fbb8
(Reporter)

Updated

2 years ago
Whiteboard: [b5] [unifiedTelemetry] [uplift2] → [b5] [unifiedTelemetry] [uplift1.5]
(Reporter)

Updated

2 years ago
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
(Reporter)

Comment 28

2 years ago
Did you test it still behaved right after restarting?
Otherwise that sounds fine.
status-firefox40: fixed → affected
There are no changes after restarting the builds.
(Reporter)

Comment 30

2 years ago
Great, thanks.
status-firefox40: affected → verified
Flags: qe-verify+
(Reporter)

Updated

2 years ago
Blocks: 1192906
(Reporter)

Updated

2 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.