Closed Bug 1254550 Opened 4 years ago Closed 4 years ago

Make Telemetry configuration consistent across builds

Categories

(Toolkit :: Telemetry, defect, P1)

defect
Points:
2

Tracking

()

RESOLVED FIXED
Tracking Status
firefox45 --- unaffected
firefox46 --- fixed
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: gfritzsche, Assigned: Dexter)

References

(Blocks 1 open bug)

Details

(Whiteboard: [measurement:client])

Attachments

(1 file, 3 obsolete files)

As noticed in bug 1242084 and others, the Telemetry conditions & build flag interactions are too confusing.
From my perspective the important thing is that we don't have Telemetry *send* data from debug builds.

To not create more confusing rules i would propose:
(1) Telemetry defaults are the same between debug & release
(2) We have explicit Telemetry defaults per-channel
(3) we don't send Telemetry from debug & non-official builds (to not skew our analysis etc.)

(1) would give us proper test-coverage etc. and avoid unexpected behavior differences.
(2) would mean fixing the surprising and hard-to-find logic by avoiding the current libpref juggle at [0]
(3) would avoid sending Telemetry data from debug builds as a result of (1).

0: https://dxr.mozilla.org/mozilla-central/rev/05c087337043dd8e71cc27bdb5b9d55fd00aaa26/modules/libpref/Preferences.cpp#1322
We could approach it this way:
* set MOZ_TELEMETRY_REPORTING for release & debug build configs
* disable Telemetry sending if MOZ_DEBUG is set at [1]
* MOZ_TELEMETRY_ON_BY_DEFAULT is enabled specifically for "beta"/"aurora"/"nightly" channel in configure.in
* remove the libpref override for "toolkit.telemetry.enabled" at [0]

0: https://dxr.mozilla.org/mozilla-central/rev/05c087337043dd8e71cc27bdb5b9d55fd00aaa26/modules/libpref/Preferences.cpp#1322
1: https://dxr.mozilla.org/mozilla-central/rev/05c087337043dd8e71cc27bdb5b9d55fd00aaa26/toolkit/components/telemetry/TelemetrySend.jsm#1034
Benjamin Smedberg  [:bsmedberg] from bug 986582, comment 0):
> Kairo brought up a great point about turning Telemetry on by default on the
> beta channel. We send the final release build to the beta audience, which
> means that we cannot just build the default using a build flag.

Benjamin, do you remember the background here for doing this?
I wonder if we are running into any unexpected issues if we just enable MOZ_TELEMETRY_ON_BY_DEFAULT for specific channels.

Ryan, do you see any problems here?
Flags: needinfo?(ryanvm)
Flags: needinfo?(benjamin)
The requirement is that telemetry is on by default for users on the beta channel and off by default for users on the release channel. Since those are the exact same builds (we ship RCs to beta), you have to use the runtime detection. It doesn't have to be in Preferences.cpp, although that seemed expedient.
Flags: needinfo?(benjamin)
Thanks for the context, so we ship the same binaries to beta as to release here (and apparently the update channel is determined by the installer).
That explains the preference juggle, this doesn't seem to make sense to touch for now then.

(In reply to Georg Fritzsche [:gfritzsche] from comment #0)
> To not create more confusing rules i would propose:
> (1) Telemetry defaults are the same between debug & release
> (2) We have explicit Telemetry defaults per-channel
> (3) we don't send Telemetry from debug & non-official builds (to not skew
> our analysis etc.)

That means leaving (2) as-is for now.
The minimum we have to do for bug 1118794 is (3).
I'd like to see (2) as well (for coverage, less confusion and avoiding surprises like bug 1242084).
Priority: P3 → P2
Your plan sounds fine to me, assuming we don't end up with an 8-byte permaleak once we're done :P
Flags: needinfo?(ryanvm)
Points: --- → 2
Priority: P2 → P1
This patch makes sure no pings are sent by debug builds.
Assignee: nobody → alessio.placitelli
Status: NEW → ASSIGNED
Attachment #8729541 - Flags: review?(gfritzsche)
Comment on attachment 8729541 [details] [diff] [review]
Don't send Telemetry from debug builds

Review of attachment 8729541 [details] [diff] [review]:
-----------------------------------------------------------------

Lets make sure this runs fine on a full try build.
Is (1) coming up in a second patch?
Attachment #8729541 - Flags: review?(gfritzsche) → review+
This patch adds MOZ_TELEMETRY_REPORTING=1 to the debug/debug-asan mozconfig files for Firefox. I didn't add that to the debug-static-analysis mozconfig as MOZILLA_OFFICIAL doesn't seem to be set there either.

A try push is in progress here (I scrambled bug numbers so this bug didn't get spammed :-/): https://treeherder.mozilla.org/#/jobs?repo=try&revision=5dadea087ba4&selectedJob=18078376
Attachment #8730649 - Flags: review?(gfritzsche)
Comment on attachment 8730649 [details] [diff] [review]
Make Telemetry configuration consistent across release & debug

Review of attachment 8730649 [details] [diff] [review]:
-----------------------------------------------------------------

This looks ok to me, but i don't think i'm the right reviewer.

Glandium, can you review this?
The background here is that:
(1) we should have Telemetry enabled in debug builds too for coverage
(2) ... although we should not send Telemetry from debug builds to not (covered in a different patch here)

(2) was - as far as i know - the reason why had Telemetry disabled. As we fixed (2), i don't see why we should diverge in behavior for debug builds otherwise.
Attachment #8730649 - Flags: review?(mh+mozilla)
Attachment #8730649 - Flags: review?(gfritzsche)
Attachment #8730649 - Flags: feedback+
Attachment #8729541 - Attachment description: bug1254550.patch → Don't send Telemetry from debug builds
Attachment #8730649 - Attachment description: bug1254550_mozconfig.patch → Make Telemetry configuration consistent across release & debug
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #3)
> The requirement is that telemetry is on by default for users on the beta
> channel and off by default for users on the release channel. Since those are
> the exact same builds (we ship RCs to beta), you have to use the runtime
> detection. It doesn't have to be in Preferences.cpp, although that seemed
> expedient.

How is that going to work with release promotion, where releases will *actually* be beta builds?
The way it works currently, by doing a runtime check of the release channel?
oh, the update channel.
Comment on attachment 8730649 [details] [diff] [review]
Make Telemetry configuration consistent across release & debug

Review of attachment 8730649 [details] [diff] [review]:
-----------------------------------------------------------------

If the goal is to have all builds with telemetry enabled, you should set this in one of the common files included everywhere. Like browser/config/mozconfigs/common or build/mozconfig.common depending on how broad you want that.

That said, if we're doing a runtime check on the update channel, why not simply enable telemetry everywhere, as in, completely remove the MOZ_TELEMETRY_REPORTING condition?
Attachment #8730649 - Flags: review?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #13)
> That said, if we're doing a runtime check on the update channel, why not
> simply enable telemetry everywhere, as in, completely remove the
> MOZ_TELEMETRY_REPORTING condition?

We don't want Telemetry enabled in all products that are built from the source.
We specifically want Telemetry enabled for all Firefox builds (but only send to the servers in specific builds).
For the moment, we're only landing the first patch ("Don't send Telemetry pings from debug builds"). This is the try push for it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=133b47e9614c
Keywords: leave-open
https://hg.mozilla.org/integration/fx-team/rev/ae45c837f71ff925e848bb7f44353681a581207f
Bug 1254550 - Make Telemetry configuration consistent across builds. r=gfritzsche
Comment on attachment 8729541 [details] [diff] [review]
Don't send Telemetry from debug builds

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: None, but this is related to bug 1242084, which is needed for Release Promotion.
[Describe test coverage new/current, TreeHerder]: No new test coverage.
[Risks and why]: Low risk, this makes sure debug builds don't send telemetry.
[String/UUID change made/needed]: None
Attachment #8729541 - Flags: approval-mozilla-beta?
Attachment #8729541 - Flags: approval-mozilla-aurora?
(In reply to Alessio Placitelli [:Dexter] from comment #18)
> Comment on attachment 8729541 [details] [diff] [review]
> [User impact if declined]: None, but this is related to bug 1242084, which
> is needed for Release Promotion.

Errata: user impact would be Telemetry being incorrectly sent from debug builds without this
Comment on attachment 8734718 [details] [diff] [review]
Make Telemetry configuration consistent across release & debug

:glandium, I followed your suggestion and moved the MOZ_TELEMETRY_REPORTING to the browser-wide configuration file.

It took a bit to figure out the background of the l10n-mozconfig file, which seems to be used to generate localized release builds. After discussing the concerns with :Pike, it seems that there shouldn't be any problem with enabling MOZ_TELEMETRY_REPORTING there, as MOZILLA_OFFICIAL is defined as well.
Attachment #8734718 - Flags: review?(mh+mozilla)
Attachment #8734718 - Flags: feedback?(gfritzsche)
(In reply to Alessio Placitelli [:Dexter] from comment #21)
> Comment on attachment 8734718 [details] [diff] [review]
> Make Telemetry configuration consistent across release & debug

:Pike suggested to keep an eye on [1] when/if this patch gets to land.

[1] - https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&filter-job_group_symbol=L10n&filter-job_group_symbol=Update-3&filter-job_group_symbol=Update-1&filter-job_group_symbol=Update-2&exclusion_profile=false
Attachment #8734718 - Flags: review?(mh+mozilla) → review+
That's the try-push for "Make Telemetry configuration consistent across release & debug": https://treeherder.mozilla.org/#/jobs?repo=try&revision=35897c6455f1&selectedJob=18590458

The bc5 leak doesn't seem to be due to this patch, as it also happens without it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9575d945837&selectedJob=18592232
Comment on attachment 8734718 [details] [diff] [review]
Make Telemetry configuration consistent across release & debug

Benjamin, do you see any potential problem with making MOZ_TELEMETRY_REPORTING behave consistently across all the different mozconfigs?

Pings won't still be sent by debug or unofficial builds (not defining MOZILLA_OFFICIAL), but the data would still be gathered locally.
Attachment #8734718 - Flags: feedback?(gfritzsche) → feedback?(benjamin)
Comment on attachment 8734718 [details] [diff] [review]
Make Telemetry configuration consistent across release & debug

If a user uses a debug build, then an opt/release build, will that end up submitting the cached pings from the older build? That might be messy.

That said, I don't really care as long as the official builds have the guaranteed correct behavior and debug/self-builds don't submit by default.
Attachment #8734718 - Flags: feedback?(benjamin)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #25)
> If a user uses a debug build, then an opt/release build, will that end up
> submitting the cached pings from the older build? That might be messy.

We never store pings as "pending" from those builds (i.e. all non-official & debug builds), so that is fine.
Comment on attachment 8729541 [details] [diff] [review]
Don't send Telemetry from debug builds

OK to uplift to aurora and beta. This should land for Monday's beta 8 build.
Attachment #8729541 - Flags: approval-mozilla-beta?
Attachment #8729541 - Flags: approval-mozilla-beta+
Attachment #8729541 - Flags: approval-mozilla-aurora?
Attachment #8729541 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/integration/fx-team/rev/b3501e6ad98af3c4e6833bcc2725f374f171805b
Bug 1254550 - Set MOZ_TELEMETRY_REPORTING in the common browser configuration. r=glandium
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: leave-open
Resolution: --- → FIXED
It looks like this commit caused the talos numbers for linux64 to improve across the board:

https://treeherder.mozilla.org/perf.html#/alerts?id=729

Was this expected?
Flags: needinfo?(gfritzsche)
Hm, that is not expected.

We will need to understand:
* what exactly triggered the change (is MOZ_TELEMETRY_REPORTING now defined for those builds and wasnt before? or the other way around?)
* why this had a bigger performance impact (e.g. is Telemetry initialization not happening anymore etc.)
Flags: needinfo?(gfritzsche) → needinfo?(alessio.placitelli)
Mh, odd, this was unexpected. I digged into this issue, and found something interesting: the "common" config file [2] is only included win32/win64 and macos l10n mozconfigs. They are no

It looks the mozconfigs used to build FF for Linux are only including one common config file, found under build/ [3]. A similar thing is happening on Macos [4].

The comment "# This file is included by all browser mozconfigs" in the common mozconfig is not true. :glandium, do you have any suggestion on how to proceed?

[1] - https://dxr.mozilla.org/mozilla-central/search?q=%22.+\%22%24topsrcdir%2Fbrowser%2Fconfig%2Fmozconfigs%2Fcommon\%22%22&redirect=false&case=true
[2] - https://dxr.mozilla.org/mozilla-central/rev/a235bfcc8c411169b82420c503775c1a3e7edad5/browser/config/mozconfigs/common
[3] - https://dxr.mozilla.org/mozilla-central/rev/a235bfcc8c411169b82420c503775c1a3e7edad5/build/unix/mozconfig.linux
[4] - https://dxr.mozilla.org/mozilla-central/rev/a235bfcc8c411169b82420c503775c1a3e7edad5/browser/config/mozconfigs/macosx-universal/common-opt#3
Flags: needinfo?(alessio.placitelli) → needinfo?(mh+mozilla)
(In reply to Alessio Placitelli [:Dexter] from comment #34)
> Mh, odd, this was unexpected. I digged into this issue, and found something
> interesting: the "common" config file [2] is only included win32/win64 and
> macos l10n mozconfigs.

http://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=9bd90088875399347b05d87c67d3709e31539dcd&newProject=mozilla-central&newRev=d9f50aa0a1aaf90499b85c31e0f329b762e80fdd confirms that this bug "broke" OS X and Linux.
This is expected, as MOZ_TELEMETRY_REPORTING toggles that section.

:MattN, screenshot comparison is awesome.
(In reply to Alessio Placitelli [:Dexter] from comment #34)
> The comment "# This file is included by all browser mozconfigs" in the
> common mozconfig is not true. :glandium, do you have any suggestion on how
> to proceed?

mozconfigs are such a mess :( I don't have more to say than "use your best judgement". Maybe someone from releng would have something to say. Rail?
Flags: needinfo?(mh+mozilla) → needinfo?(rail)
(In reply to Mike Hommey [:glandium] from comment #39)
> (In reply to Alessio Placitelli [:Dexter] from comment #34)
> > The comment "# This file is included by all browser mozconfigs" in the
> > common mozconfig is not true. :glandium, do you have any suggestion on how
> > to proceed?
> 
> mozconfigs are such a mess :( I don't have more to say than "use your best
> judgement". Maybe someone from releng would have something to say. Rail?

Sorry for the silence. I have been trying to come up with something less messy, but mozconfigs win. Even though it requires some initial effort to adjust the mozconfigs for different build types, after that things should be smoother and everything stays in the tree.
Flags: needinfo?(rail)
Attachment #8734718 - Attachment is obsolete: true
Comment on attachment 8730649 [details] [diff] [review]
Make Telemetry configuration consistent across release & debug

Given the outcomes of the previous patch and the comment from :rail, should we just enable MOZ_TELEMETRY_REPORTING for debug builds changing the related .mozconfig files there?
Attachment #8730649 - Attachment is obsolete: false
Flags: needinfo?(gfritzsche)
Yes, lets just stay with what we do for other build flags too.
Sadly there doesn't seem to be a cleaner way.
Flags: needinfo?(gfritzsche)
:glandium, given the previous comments I changed the mozconfigs manually by adding MOZ_TELEMETRY_REPORTING where missing.
Attachment #8730649 - Attachment is obsolete: true
Attachment #8739948 - Flags: review?(mh+mozilla)
Comment on attachment 8739948 [details] [diff] [review]
Make Telemetry configuration consistent across release & debug

Review of attachment 8739948 [details] [diff] [review]:
-----------------------------------------------------------------

Why not keep it in the common files for mac and windows? It's not because our linux mozconfigs suck that we have to make things bad everywhere.
Attachment #8739948 - Flags: review?(mh+mozilla)
Comment on attachment 8739948 [details] [diff] [review]
Make Telemetry configuration consistent across release & debug

Review of attachment 8739948 [details] [diff] [review]:
-----------------------------------------------------------------

Why not keep it in the common files for mac and windows? It's not because our linux mozconfigs suck that we have to make things bad everywhere.
Attachment #8739948 - Flags: review+
See Also: → 1263854
Attachment #8739948 - Attachment is obsolete: true
I've moved the second patch to bug 1263854, since it will be riding different trains.
(In reply to Mike Hommey [:glandium] from comment #46)
> Comment on attachment 8739948 [details] [diff] [review]
> Make Telemetry configuration consistent across release & debug
> 
> Review of attachment 8739948 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why not keep it in the common files for mac and windows? It's not because
> our linux mozconfigs suck that we have to make things bad everywhere.

That's a good point. Unfortunately, unless I'm missing something (and I could be!), the browser/config/mozconfigs/common only really covers Windows [1]. We would end up modifying MacOS and Linux mozconfig files anyway :(

Were you thinking of specific files?

From what I can see, there are different "common" files for Mac and Linux, but they live under /build [2][3]. This should mean (please correct me if I'm wrong, as I'm not very familiar with this code) that any change in these files is reflected across all our products.


[1] - https://dxr.mozilla.org/mozilla-central/search?q=%22mozconfigs%2Fcommon%22+path%3Abrowser%2Fconfig%2Fmozconfig&redirect=false&case=true
[2] - https://dxr.mozilla.org/mozilla-central/source/build/macosx/universal/mozconfig.common
[3] - https://dxr.mozilla.org/mozilla-central/rev/e847cfcb315f511f4928b03fd47dcf57aad05e1e/build/unix/mozconfig.linux
Flags: needinfo?(mh+mozilla)
meh, live it as you did it...

That said, back to an old comment:

(In reply to Georg Fritzsche [:gfritzsche] from comment #14)
> (In reply to Mike Hommey [:glandium] from comment #13)
> > That said, if we're doing a runtime check on the update channel, why not
> > simply enable telemetry everywhere, as in, completely remove the
> > MOZ_TELEMETRY_REPORTING condition?
> 
> We don't want Telemetry enabled in all products that are built from the
> source.
> We specifically want Telemetry enabled for all Firefox builds (but only send
> to the servers in specific builds).

But that shouldn't that be covered by the existing runtime check?
Flags: needinfo?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #49)
> meh, live it as you did it...

s/live/leave/
(In reply to Mike Hommey [:glandium] from comment #49)
> meh, live it as you did it...
> 
> That said, back to an old comment:
> 
> (In reply to Georg Fritzsche [:gfritzsche] from comment #14)
> > (In reply to Mike Hommey [:glandium] from comment #13)
> > > That said, if we're doing a runtime check on the update channel, why not
> > > simply enable telemetry everywhere, as in, completely remove the
> > > MOZ_TELEMETRY_REPORTING condition?
> > 
> > We don't want Telemetry enabled in all products that are built from the
> > source.
> > We specifically want Telemetry enabled for all Firefox builds (but only send
> > to the servers in specific builds).
> 
> But that shouldn't that be covered by the existing runtime check?

Yes, sending is allowed only on Official, non-debug builds with Telemetry enabled.
Blocks: 1263854
I see some talos kraken regressions on aurora that appear to be coming from this change
(In reply to Joel Maher (:jmaher) from comment #52)
> I see some talos kraken regressions on aurora that appear to be coming from
> this change

That's interesting, is there any link to the regressions/bug?
I'm puzzled as this change should really just affect debug builds.
Flags: needinfo?(jmaher)
I think I have mistakenly picked this bug, sorry for the confusion.
Flags: needinfo?(jmaher)
You need to log in before you can comment on or make changes to this bug.