986582 - Figure out how to turn on telemetry by default even for the release-as-last-beta

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Benjamin Smedberg]

Kairo brought up a great point about turning Telemetry on by default on the beta channel. We send the final release build to the beta audience, which means that we cannot just build the default using a build flag.

It's not really acceptable for the default to flip in this case, because it will affect ongoing telemetry experiments and cause weird data hiccups. Instead what I'd like to do is have a coded default based on the channel or a build flag.

I expect this means that I'll be removing the weird distinction we have right now between "toolkit.telemetry.enabled" and "toolkit.telemetry.enabledPreRelease" and instead use the single pref "toolkit.telemetry.enabled" and set the default dynamically at startup instead of in all.js.

Comment 1

[Benjamin Smedberg]

Attachment: telemetry-enabledPreRelease

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 8394989 [details] [diff] [review]
telemetry-enabledPreRelease

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

Please split out the changes in m/a/b/background into a separate patch; they need to be upstreamed.

I think the GeckoPreferences change is sound, but I'd like to test migration from a previous version and correct reporting in Android FHR and in the Settings UI. Feel free to do that yourself, or I should be able to find time tonight.

Hooray for simplicity!

::: mobile/android/base/background/healthreport/EnvironmentBuilder.java
@@ +91,5 @@
>  
>      // Corresponds to Gecko pref "extensions.blocklist.enabled".
>      e.isBlocklistEnabled = (info.isBlocklistEnabled() ? 1 : 0);
>  
> +    // Corresponds to Gecko pref "toolkit.telemetry.enabled"

Nit: trailing period.

::: mobile/android/chrome/content/browser.js
@@ +5444,5 @@
>    PREF_BLOCKLIST_ENABLED: "extensions.blocklist.enabled",
>  
>    PREF_TELEMETRY_ENABLED:
>  #ifdef MOZ_TELEMETRY_REPORTING
>      // Telemetry pref differs based on build.

This comment can be removed.

::: services/healthreport/providers.jsm
@@ +56,5 @@
>  const DAILY_DISCRETE_NUMERIC_FIELD = {type: Metrics.Storage.FIELD_DAILY_DISCRETE_NUMERIC};
>  const DAILY_LAST_NUMERIC_FIELD = {type: Metrics.Storage.FIELD_DAILY_LAST_NUMERIC};
>  const DAILY_COUNTER_FIELD = {type: Metrics.Storage.FIELD_DAILY_COUNTER};
>  
>  // Preprocess to use the correct telemetry pref.

This comment can go.

Comment 3

[Vladan Djeric (:vladan)]

One thing to consider: if I install Nightly/Aurora/Beta with this patch on a new profile, and then switch to using Release, I will still be submitting data to Telemetry without ever opting in.

Is this ok? Should we run it by privacy people?

Comment 4

[Benjamin Smedberg]

I'm pretty sure that's not correct: we're setting default prefs here, which don't persist beyond the current session.

Comment 5

[Théo Chevalier]

This discussion reminds me the headache we had in bug 699806, maybe some of the comments can help here

Comment 6

[Vladan Djeric (:vladan)]

Comment on attachment 8394989 [details] [diff] [review]
telemetry-enabledPreRelease

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

::: modules/libpref/src/Preferences.cpp
@@ +1306,5 @@
> +  // If this build has MOZ_TELEMETRY_ON_BY_DEFAULT *or* we're on the beta
> +  // channel, telemetry is on by default, otherwise not. This is necessary
> +  // so that beta users who are testing final release builds don't flipflop
> +  // defaults.
> +  if (Preferences::GetDefaultType(kTelemetryPref) == PREF_INVALID) {

After this patch is applied, when will toolkit.telemetry.enabled actually have a default type? i.e. when will this if-check be false?

@@ +1318,5 @@
> +#endif
> +    PREF_SetBoolPref(kTelemetryPref, prerelease, true);
> +  }
> +  // Migrate the old prerelease telemetry pref
> +  if (!Preferences::GetBool(kOldTelemetryPref, true)) {

I wasn't super-comfortable with this patch, so I applied it to my tree and tested it out. It turns out that at this point in Firefox initialization, we actually haven't loaded the user's prefs.js file so this pref would actually never get migrated!

Comment 7

[Benjamin Smedberg]

Attachment: telemetry-enabledPreRelease

You're right! Running manual tests to verify this version, but I'm pretty sure the migration is now correct. I already ran a bunch of tests on the defaults for nightly/beta/release.

Comment 8

[Benjamin Smedberg]

> After this patch is applied, when will toolkit.telemetry.enabled actually have a default type?
>  i.e. when will this if-check be false?

The pref will not have a default for any Firefox build. It will still be possible to override the default in enterprise configs as well as non-Firefox applications.

Comment 9

[Richard Newman [:rnewman]]

Comment on attachment 8397843 [details] [diff] [review]
telemetry-enabledPreRelease

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

As I mentioned in Comment 2, please split out m/a/b/background into a separate patch for upstreaming.

r- to make sure that review comments actually get acted on this time.

::: mobile/android/base/background/healthreport/EnvironmentBuilder.java
@@ +91,5 @@
>  
>      // Corresponds to Gecko pref "extensions.blocklist.enabled".
>      e.isBlocklistEnabled = (info.isBlocklistEnabled() ? 1 : 0);
>  
> +    // Corresponds to Gecko pref "toolkit.telemetry.enabled"

Nit: trailing period.

::: mobile/android/chrome/content/browser.js
@@ +5629,5 @@
>    PREF_BLOCKLIST_ENABLED: "extensions.blocklist.enabled",
>  
>    PREF_TELEMETRY_ENABLED:
>  #ifdef MOZ_TELEMETRY_REPORTING
>      // Telemetry pref differs based on build.

As mentioned in Comment 2, please delete this comment.

::: services/healthreport/providers.jsm
@@ +56,5 @@
>  const DAILY_DISCRETE_NUMERIC_FIELD = {type: Metrics.Storage.FIELD_DAILY_DISCRETE_NUMERIC};
>  const DAILY_LAST_NUMERIC_FIELD = {type: Metrics.Storage.FIELD_DAILY_LAST_NUMERIC};
>  const DAILY_COUNTER_FIELD = {type: Metrics.Storage.FIELD_DAILY_COUNTER};
>  
>  // Preprocess to use the correct telemetry pref.

Delete this comment.

Comment 10

[Benjamin Smedberg]

Attachment: telemetry-enabledPreRelease

Comment 11

[Benjamin Smedberg]

Attachment: telemetry-enabledPreRelease-androidbackground

Comment 12

[Richard Newman [:rnewman]]

Comment on attachment 8400636 [details] [diff] [review]
telemetry-enabledPreRelease-androidbackground

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

Thanks for splitting the patch! I'll take care of upstreaming this.

Comment 13

[Richard Newman [:rnewman]]

Merged upstream, and landed the comment-only part to avoid divergence.

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

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8ac4315d8807

Comment 15

[Vladan Djeric (:vladan)]

Sorry for the delayed comment, I had a lot of things to wrap up before PTO.

I think the committed patch will do what we want, but should we rename MOZ_TELEMETRY_ON_BY_DEFAULT to something else to prevent people using it incorrectly in #ifdefs? It no longer means quite what its name suggests ("Telemetry is on by default on this channel")

Comment 16

[Benjamin Smedberg]

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

Comment 17

[Benjamin Smedberg]

I didn't see your comment before pushing: I'm happy to rename it but don't have a great alternative in mind.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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