763991 - fennec build configs should define MOZ_TELEMETRY_REPORTING

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Nathan Froyd [:froydnj]]

bug 722240 disabled telemetry for non-official builds based on the presence of MOZ_TELEMETRY_REPORTING.  Unfortunately, the Android builds don't define MOZ_TELEMETRY_REPORTING, so bug 722240 completely disabled telemetry for them.

We should add the necessary logic in the mozconfigs.  Probably shouldn't uplift this until at least bug 762620 is resolved.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: patch to add MOZ_TELEMETRY_REPORTING

Wasn't sure about the android-{armv6,x86} builds, decided to add the necessary stanzas there too.

Wasn't sure about the l10n-* configs, decided to follow existing practice and not add them.

Comment 2

[Matt Brubeck (:mbrubeck)]

Attachment: Disable telemetry prompt in non-telemetry builds

In addition, we should disable the telemetry UI in builds without telemetry.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 632434 [details] [diff] [review]
Disable telemetry prompt in non-telemetry builds

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+#ifdef MOZ_TELEMETRY_REPORTING
>     Telemetry.init();
>+#endif

You missed a Telemetry.unint();

r+ with that change

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Matt Brubeck (:mbrubeck) from comment #2)
> Disable telemetry prompt in non-telemetry builds

Matt, do you want to land this patch prior to mine, or does it not really matter?

Comment 5

[Matt Brubeck (:mbrubeck)]

(In reply to Mark Finkle (:mfinkle) from comment #3)
> You missed a Telemetry.unint();

Fixed.

(In reply to Nathan Froyd (:froydnj) from comment #4)
> Matt, do you want to land this patch prior to mine, or does it not really
> matter?

Landed both patches together:
https://hg.mozilla.org/integration/mozilla-inbound/rev/112ab5ba3ac4
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b447b08eb73

Do we also want these on Aurora and Beta, or should we wait until bug 762620 is resolved?

Comment 6

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/112ab5ba3ac4
https://hg.mozilla.org/mozilla-central/rev/7b447b08eb73

Comment 7

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 632299 [details] [diff] [review]
patch to add MOZ_TELEMETRY_REPORTING

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 762590, Bug 764144 and Bug 762620
User impact if declined: No Telemetry on Fennec
Testing completed (on m-c, etc.): Has been in m-c for a while.
Risk to taking this patch (and alternatives if risky): Now that bug 762590 and 762620 are fixed, I'd say very low risk of accidentally using up battery/user data quota.

Comment 8

[Alex Keybl [:akeybl]]

Comment on attachment 632299 [details] [diff] [review]
patch to add MOZ_TELEMETRY_REPORTING

[Triage Comment]
I wish this had been nominated for uplift sooner - we can't risk any regressions to power consumption in 14.0.1 and I'm not comfortable with going out having only had the device coverage testing we've gotten on Nightlies so far. Let's get this into Aurora 15, however. If anybody disagrees, please email/ping me directly as we're hours away from going to build.

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 632299 [details] [diff] [review]
patch to add MOZ_TELEMETRY_REPORTING

Alex, the reason we scrambled to get idle-daily fixed was to turn on telemetry for 14.01, renominating.

Comment 10

[Alex Keybl [:akeybl]]

Comment on attachment 632299 [details] [diff] [review]
patch to add MOZ_TELEMETRY_REPORTING

[Triage comment]
Blassey and I spoke further about this. Based upon the risk in bug 762620, we actually backed that out. Given that, no need to land this on beta.

Comment 11

[Matt Brubeck (:mbrubeck)]

Pushed the patch to enable telemetry at build time:
https://hg.mozilla.org/releases/mozilla-aurora/rev/34379dbcf2cc

Note: I did not uplift the second patch, to disable the Telemetry UI in unofficial builds, because it is not approved and does not apply cleanly to Aurora.  Since that patch only affects unofficial builds, it's not a priority for Aurora uplift.