Closed
Bug 667577
Opened 13 years ago
Closed 13 years ago
Telemetry opt-in UI should only be on for Mozilla official builds
Categories
(Firefox :: General, defect, P1)
Firefox
General
Tracking
()
VERIFIED
FIXED
Tracking | Status | |
---|---|---|
firefox7 | --- | fixed |
People
(Reporter: taras.mozilla, Assigned: glandium)
References
Details
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
khuey
:
review+
asa
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
catlee
:
review+
|
Details | Diff | Splinter Review |
khuey suggests that functionality be gated on MOZ_UPDATE_CHANNEL
Reporter | ||
Updated•13 years ago
|
tracking-firefox6:
+ → ---
Comment 1•13 years ago
|
||
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•13 years ago
|
||
(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.
It should be turned on if MOZ_UPDATE_CHANNEL != default, IMO.
Comment 4•13 years ago
|
||
So the goal is to disable it by default, but enable it for moco-shipped builds?
Reporter | ||
Comment 5•13 years ago
|
||
(In reply to comment #4) > So the goal is to disable it by default, but enable it for moco-shipped > builds? yes
Comment 6•13 years ago
|
||
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.
Why not? That's how Test Pilot is done. This seems similar to me.
Comment 8•13 years ago
|
||
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.
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 11•13 years ago
|
||
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.
Assignee | ||
Comment 12•13 years ago
|
||
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•13 years ago
|
||
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).
Assignee | ||
Comment 14•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #542837 -
Flags: review?(khuey)
Attachment #542837 -
Flags: review?(khuey) → review+
Comment 16•13 years ago
|
||
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 18•13 years ago
|
||
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?
Attachment #542838 -
Flags: review?(catlee) → review-
Assignee | ||
Comment 19•13 years ago
|
||
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.
Attachment #542838 -
Attachment is obsolete: true
Attachment #544466 -
Flags: review?(catlee)
Comment 20•13 years ago
|
||
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
Attachment #544466 -
Flags: review?(catlee) → review-
Assignee | ||
Comment 21•13 years ago
|
||
Sorry for the previous mess.
Attachment #544466 -
Attachment is obsolete: true
Attachment #545691 -
Flags: review?(catlee)
Updated•13 years ago
|
Attachment #545691 -
Flags: review?(catlee) → review+
Assignee | ||
Comment 22•13 years ago
|
||
http://hg.mozilla.org/build/buildbot-configs/rev/29bf5a28e69e Landed the mozconfig changes.
Assignee | ||
Comment 23•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/ff9c4941d4d5
Whiteboard: [inbound]
Comment 24•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/ff9c4941d4d5
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Comment 25•13 years ago
|
||
(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•13 years ago
|
||
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.
Status: RESOLVED → VERIFIED
Assignee | ||
Comment 27•13 years ago
|
||
It might be better to get this in ff7, so that custom builds of the source tarballs don't yield the opt-in prompt.
tracking-firefox7:
--- → ?
Assignee | ||
Comment 28•13 years ago
|
||
Comment on attachment 542837 [details] [diff] [review] Don't show the telemetry prompt unless the MOZ_TELEMETRY_REPORTING variable is defined See comment 27
Attachment #542837 -
Flags: approval-mozilla-aurora?
Summary: Telemetry opt-in UI should only be on for release builds → Telemetry opt-in UI should only be on for Mozilla official builds
Updated•13 years ago
|
Attachment #542837 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 29•13 years ago
|
||
http://hg.mozilla.org/releases/mozilla-aurora/rev/08ce839b81cc
status-firefox7:
--- → fixed
tracking-firefox7:
? → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•