667577 - Telemetry opt-in UI should only be on for Mozilla official builds

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[(dormant account)]

khuey suggests that functionality be gated on MOZ_UPDATE_CHANNEL

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Why should this behavior differ based on the update channel? How do you see it differing (always on for non-release, opt-in for release)?

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to comment #1)
> How do you see
> it differing (always on for non-release, opt-in for release)?

IIUC, we'd want always *off* for non-release (that is -- for local debug builds, one-off tryserver builds with random hacky changes, etc), and opt-in for release.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

It should be turned on if MOZ_UPDATE_CHANNEL != default, IMO.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

So the goal is to disable it by default, but enable it for moco-shipped builds?

Comment 5

[(dormant account)]

(In reply to comment #4)
> So the goal is to disable it by default, but enable it for moco-shipped
> builds?

yes

Comment 6

[:Gavin Sharp [email: [e-mail]]]

That's probably best done with a separate configure flag that we set in the MoCo build configs. I don't think it makes much sense to tie it to the update channel.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Why not?  That's how Test Pilot is done.  This seems similar to me.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Well, I'm not a fan of that setup either, but aside from that I think the requirements are somewhat different. Don't we want telemetry on in official Nightly builds?

Tying stuff to the update channel because it happens to mostly match the scenarios we care about is hacky, and doing this properly isn't particularly hard.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

It exactly matches the scenarios we care about.  All MoCo branded versions are shipped with MOZ_UPDATE_CHANNEL != "default".

I don't care enough to argue it beyond this last comment though.  I'll r+ a patch for any reasonable solution.

Comment 10

[Mike Hommey [:glandium]]

How about MOZILLA_OFFICIAL?

Comment 11

[:Gavin Sharp [email: [e-mail]]]

That conflicts with bug 588862 (admittedly I have a bit of a bias here!). The proliferation of magical overloaded variables that mean different things to different people ends up being much more confusing than the proliferation of configure options, IMO.

Comment 12

[Mike Hommey [:glandium]]

The problem with adding configure options or env variables or whatever, is that it would require a modification of our mozconfigs, and further modifications to propagate across branches at merge time. Note that in practice, MOZ_OFFICIAL_BRANDING can't replace MOZILLA_OFFICIAL, since the latter is defined for all our builds, while the former is only defined for betas and releases.
MOZ_UPDATE_CHANNEL could work for us here, but it wouldn't work for Linux distros (except if we don't want to get telemetry from them, but I doubt it).

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Adding things to the MoCo mozconfigs isn't difficult, and there's no need to tie it to merge dates (it can just be added now to all configs).

Comment 14

[Mike Hommey [:glandium]]

Attachment: Don't show the telemetry prompt unless the MOZ_TELEMETRY_REPORTING variable is defined

Comment 15

[Mike Hommey [:glandium]]

Attachment: Corresponding mozconfig change (for --enable-application=browser only, for mozilla2 only)

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 542838 [details] [diff] [review]
Corresponding mozconfig change (for --enable-application=browser only, for mozilla2 only)

Hmm, I thought there was a way to set a config more globally (without having to change every single mozconfig file). In some buildbot config/script, maybe?

Comment 17

[:Gavin Sharp [email: [e-mail]]]

You could ifdef the _showTelemetryNotification implementation as well.

Comment 18

[Chris AtLee [:catlee]]

Comment on attachment 542838 [details] [diff] [review]
Corresponding mozconfig change (for --enable-application=browser only, for mozilla2 only)

This is getting set on branches that don't even support telemetry (e.g. 1.9.1, 1.9.2, 2.0)

And do you want telemetry enabled for debug or qt builds?

Comment 19

[Mike Hommey [:glandium]]

Attachment: Corresponding mozconfig change

Stripped down to release, nightly and nightly-rpm on mozilla-aurora, mozilla-beta, mozilla-central and mozilla-release. That could be mozilla-central only for now, but it doesn't change anything when the build patch is not applied and avoids having to care about the propagation of this particular change when merging from central to aurora, then aurora to beta, etc.

Comment 20

[Chris AtLee [:catlee]]

Comment on attachment 544466 [details] [diff] [review]
Corresponding mozconfig change

>diff --git a/mozilla2/linux/mozilla-central/nightly-rpm/mozconfig b/mozilla2/linux/mozilla-central/nightly-rpm/mozconfig
>--- a/mozilla2/linux/mozilla-central/nightly-rpm/mozconfig
>+++ b/mozilla2/linux/mozilla-central/nightly-rpm/mozconfig
>@@ -24,16 +24,22 @@ export CXXFLAGS="-gdwarf-2"
> 
> # For NSS symbols
> export MOZ_DEBUG_SYMBOLS=1
> ac_add_options --enable-debug-symbols="-gdwarf-2"
> 
> # Needed to enable breakpad in application.ini
> export MOZILLA_OFFICIAL=1
> 
>+export MOZ_TELEMETRY_REPORTING=1
>+
>+export MOZ_TELEMETRY_REPORTING=1
>+
>+export MOZ_TELEMETRY_REPORTING=1

Looks like something went horribly wrong here

Comment 21

[Mike Hommey [:glandium]]

Attachment: Corresponding mozconfig change

Sorry for the previous mess.

Comment 22

[Mike Hommey [:glandium]]

http://hg.mozilla.org/build/buildbot-configs/rev/29bf5a28e69e Landed the mozconfig changes.

Comment 23

[Mike Hommey [:glandium]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/ff9c4941d4d5

Comment 24

[Joe Drew (not getting mail)]

http://hg.mozilla.org/mozilla-central/rev/ff9c4941d4d5

Comment 25

[Ed Morley [:emorley]]

(In reply to comment #2)
> IIUC, we'd want always *off* for non-release (that is -- for local debug
> builds, one-off tryserver builds with random hacky changes, etc), and opt-in
> for release.

If the reason for this is to avoid misleading telemetry pings for potentially broken/alternative builds, then surely not just the UI, but also the ping itself needs to be made conditional on MOZ_TELEMETRY_REPORTING - since an existing profile with toolkit.telemetry.enabled set (either from before this change, or one shared with an official release), will still send telemetry pings.

Shall I file a follow-up?

Comment 26

[AndreiD[QA]]

Build id: Mozilla/5.0 (Windows NT 6.1; rv:8.0a1) Gecko/20110717 Firefox/8.0a1

Setting this as verified fixed since the changes related to the  MOZ_TELEMETRY_REPORTING  definition and use (opt-in ui for release builds only), landed on central.

Comment 27

[Mike Hommey [:glandium]]

It might be better to get this in ff7, so that custom builds of the source tarballs don't yield the opt-in prompt.

Comment 28

[Mike Hommey [:glandium]]

Comment on attachment 542837 [details] [diff] [review]
Don't show the telemetry prompt unless the MOZ_TELEMETRY_REPORTING variable is defined

See comment 27

Comment 29

[Mike Hommey [:glandium]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/08ce839b81cc