Last Comment Bug 667577 - Telemetry opt-in UI should only be on for Mozilla official builds
: Telemetry opt-in UI should only be on for Mozilla official builds
Status: VERIFIED FIXED
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: P1 normal (vote)
: ---
Assigned To: Mike Hommey [:glandium]
:
:
Mentors:
Depends on: 652657
Blocks: 659396
  Show dependency treegraph
 
Reported: 2011-06-27 12:34 PDT by (dormant account)
Modified: 2011-07-21 23:37 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Don't show the telemetry prompt unless the MOZ_TELEMETRY_REPORTING variable is defined (1.73 KB, patch)
2011-06-29 09:18 PDT, Mike Hommey [:glandium]
khuey: review+
asa: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Corresponding mozconfig change (for --enable-application=browser only, for mozilla2 only) (123.80 KB, patch)
2011-06-29 09:20 PDT, Mike Hommey [:glandium]
catlee: review-
Details | Diff | Splinter Review
Corresponding mozconfig change (29.73 KB, patch)
2011-07-07 06:58 PDT, Mike Hommey [:glandium]
catlee: review-
Details | Diff | Splinter Review
Corresponding mozconfig change (28.77 KB, patch)
2011-07-13 10:17 PDT, Mike Hommey [:glandium]
catlee: review+
Details | Diff | Splinter Review

Description (dormant account) 2011-06-27 12:34:11 PDT
khuey suggests that functionality be gated on MOZ_UPDATE_CHANNEL
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-06-27 16:46:14 PDT
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] 2011-06-27 16:49:46 PDT
(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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-06-27 16:54:24 PDT
It should be turned on if MOZ_UPDATE_CHANNEL != default, IMO.
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-06-27 17:15:00 PDT
So the goal is to disable it by default, but enable it for moco-shipped builds?
Comment 5 (dormant account) 2011-06-27 17:15:51 PDT
(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: gavin@gavinsharp.com] 2011-06-27 18:02:26 PDT
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-06-27 19:13:23 PDT
Why not?  That's how Test Pilot is done.  This seems similar to me.
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-06-27 19:25:09 PDT
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-06-27 19:27:25 PDT
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] 2011-06-28 00:16:08 PDT
How about MOZILLA_OFFICIAL?
Comment 11 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-06-28 10:29:07 PDT
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] 2011-06-29 01:21:18 PDT
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: gavin@gavinsharp.com] 2011-06-29 01:32:26 PDT
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] 2011-06-29 09:18:29 PDT
Created attachment 542837 [details] [diff] [review]
Don't show the telemetry prompt unless the MOZ_TELEMETRY_REPORTING variable is defined
Comment 15 Mike Hommey [:glandium] 2011-06-29 09:20:00 PDT
Created attachment 542838 [details] [diff] [review]
Corresponding mozconfig change (for --enable-application=browser only, for mozilla2 only)
Comment 16 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-06-29 19:15:01 PDT
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: gavin@gavinsharp.com] 2011-06-29 19:16:01 PDT
You could ifdef the _showTelemetryNotification implementation as well.
Comment 18 Chris AtLee [:catlee] 2011-07-07 06:46:14 PDT
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] 2011-07-07 06:58:10 PDT
Created attachment 544466 [details] [diff] [review]
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] 2011-07-07 13:03:46 PDT
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] 2011-07-13 10:17:14 PDT
Created attachment 545691 [details] [diff] [review]
Corresponding mozconfig change

Sorry for the previous mess.
Comment 22 Mike Hommey [:glandium] 2011-07-15 09:40:56 PDT
http://hg.mozilla.org/build/buildbot-configs/rev/29bf5a28e69e Landed the mozconfig changes.
Comment 24 Joe Drew (not getting mail) 2011-07-17 17:29:35 PDT
http://hg.mozilla.org/mozilla-central/rev/ff9c4941d4d5
Comment 25 Ed Morley [:emorley] 2011-07-18 01:41:54 PDT
(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] 2011-07-18 05:52:50 PDT
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] 2011-07-21 08:25:45 PDT
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] 2011-07-21 08:53:54 PDT
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] 2011-07-21 23:37:43 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/08ce839b81cc

Note You need to log in before you can comment on or make changes to this bug.