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

VERIFIED FIXED

Status

()

Firefox
General
P1
normal
VERIFIED FIXED
6 years ago
28 days ago

People

(Reporter: (dormant account), Assigned: glandium)

Tracking

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox7 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
khuey suggests that functionality be gated on MOZ_UPDATE_CHANNEL
(Reporter)

Updated

6 years ago
tracking-firefox6: + → ---
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)?
(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.
So the goal is to disable it by default, but enable it for moco-shipped builds?
(Reporter)

Comment 5

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

yes
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.
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.
How about MOZILLA_OFFICIAL?
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.
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).
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).
Created attachment 542837 [details] [diff] [review]
Don't show the telemetry prompt unless the MOZ_TELEMETRY_REPORTING variable is defined
Attachment #542837 - Flags: review?(khuey)
Created attachment 542838 [details] [diff] [review]
Corresponding mozconfig change (for --enable-application=browser only, for mozilla2 only)
Attachment #542838 - Flags: review?(catlee)
Attachment #542837 - Flags: review?(khuey) → review+
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?
You could ifdef the _showTelemetryNotification implementation as well.
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-
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.
Attachment #542838 - Attachment is obsolete: true
Attachment #544466 - Flags: review?(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
Attachment #544466 - Flags: review?(catlee) → review-
Created attachment 545691 [details] [diff] [review]
Corresponding mozconfig change

Sorry for the previous mess.
Attachment #544466 - Attachment is obsolete: true
Attachment #545691 - Flags: review?(catlee)

Updated

6 years ago
Attachment #545691 - Flags: review?(catlee) → review+
http://hg.mozilla.org/build/buildbot-configs/rev/29bf5a28e69e Landed the mozconfig changes.
http://hg.mozilla.org/integration/mozilla-inbound/rev/ff9c4941d4d5
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/ff9c4941d4d5
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
(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

6 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
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: --- → ?
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?

Updated

6 years ago
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

6 years ago
Attachment #542837 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.