Closed
Bug 1254550
Opened 8 years ago
Closed 8 years ago
Make Telemetry configuration consistent across builds
Categories
(Toolkit :: Telemetry, defect, P1)
Toolkit
Telemetry
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)
1.85 KB,
patch
|
gfritzsche
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•8 years ago
|
||
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
Reporter | ||
Comment 2•8 years ago
|
||
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)
Reporter | ||
Updated•8 years ago
|
Blocks: release-promotion
Comment 3•8 years ago
|
||
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)
Reporter | ||
Comment 4•8 years ago
|
||
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).
Reporter | ||
Updated•8 years ago
|
Priority: P3 → P2
Reporter | ||
Updated•8 years ago
|
status-firefox45:
--- → unaffected
status-firefox46:
--- → affected
status-firefox47:
--- → affected
Comment 5•8 years ago
|
||
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)
Reporter | ||
Updated•8 years ago
|
Points: --- → 2
Reporter | ||
Updated•8 years ago
|
Priority: P2 → P1
Assignee | ||
Comment 6•8 years ago
|
||
This patch makes sure no pings are sent by debug builds.
Assignee: nobody → alessio.placitelli
Status: NEW → ASSIGNED
Attachment #8729541 -
Flags: review?(gfritzsche)
Reporter | ||
Comment 7•8 years ago
|
||
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+
Assignee | ||
Comment 8•8 years ago
|
||
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)
Reporter | ||
Comment 9•8 years ago
|
||
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+
Reporter | ||
Updated•8 years ago
|
Attachment #8729541 -
Attachment description: bug1254550.patch → Don't send Telemetry from debug builds
Reporter | ||
Updated•8 years ago
|
Attachment #8730649 -
Attachment description: bug1254550_mozconfig.patch → Make Telemetry configuration consistent across release & debug
Comment 10•8 years ago
|
||
(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?
Comment 11•8 years ago
|
||
The way it works currently, by doing a runtime check of the release channel?
Comment 12•8 years ago
|
||
oh, the update channel.
Comment 13•8 years ago
|
||
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)
Reporter | ||
Comment 14•8 years ago
|
||
(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).
Assignee | ||
Comment 15•8 years ago
|
||
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
Assignee | ||
Comment 16•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/ae45c837f71ff925e848bb7f44353681a581207f Bug 1254550 - Make Telemetry configuration consistent across builds. r=gfritzsche
Comment 17•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ae45c837f71f
Assignee | ||
Comment 18•8 years ago
|
||
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?
Assignee | ||
Comment 19•8 years ago
|
||
(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
Assignee | ||
Comment 20•8 years ago
|
||
Attachment #8730649 -
Attachment is obsolete: true
Assignee | ||
Comment 21•8 years ago
|
||
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)
Assignee | ||
Comment 22•8 years ago
|
||
(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
Updated•8 years ago
|
Attachment #8734718 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 23•8 years ago
|
||
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
Assignee | ||
Comment 24•8 years ago
|
||
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 25•8 years ago
|
||
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)
Reporter | ||
Comment 26•8 years ago
|
||
(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 27•8 years ago
|
||
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+
Updated•8 years ago
|
Assignee | ||
Comment 30•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/b3501e6ad98af3c4e6833bcc2725f374f171805b Bug 1254550 - Set MOZ_TELEMETRY_REPORTING in the common browser configuration. r=glandium
Comment 31•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b3501e6ad98a
Reporter | ||
Updated•8 years ago
|
Comment 32•8 years ago
|
||
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)
Reporter | ||
Comment 33•8 years ago
|
||
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)
Assignee | ||
Comment 34•8 years ago
|
||
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)
Reporter | ||
Comment 35•8 years ago
|
||
MattN also found fallout in the data choices tab: http://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=9bd90088875399347b05d87c67d3709e31539dcd&newProject=mozilla-central&newRev=d9f50aa0a1aaf90499b85c31e0f329b762e80fdd
Comment 36•8 years ago
|
||
(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.
Assignee | ||
Comment 37•8 years ago
|
||
This is expected, as MOZ_TELEMETRY_REPORTING toggles that section. :MattN, screenshot comparison is awesome.
Comment 38•8 years ago
|
||
Backout: https://hg.mozilla.org/integration/fx-team/rev/55f0de142be4
Comment 39•8 years ago
|
||
(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)
Comment 40•8 years ago
|
||
the backout has regressions (which seem to mirror the improvement from comment 32): https://treeherder.mozilla.org/perf.html#/compare?originalProject=fx-team&originalRevision=1725b460c3e0&newProject=fx-team&newRevision=55f0de142be4&framework=1 and list of alerts: https://treeherder.mozilla.org/perf.html#/alerts?id=779
Comment 41•8 years ago
|
||
(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)
Assignee | ||
Updated•8 years ago
|
Attachment #8734718 -
Attachment is obsolete: true
Assignee | ||
Comment 42•8 years ago
|
||
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)
Reporter | ||
Comment 43•8 years ago
|
||
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)
Assignee | ||
Comment 44•8 years ago
|
||
: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 45•8 years ago
|
||
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 46•8 years ago
|
||
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+
Assignee | ||
Updated•8 years ago
|
Attachment #8739948 -
Attachment is obsolete: true
Assignee | ||
Comment 47•8 years ago
|
||
I've moved the second patch to bug 1263854, since it will be riding different trains.
Assignee | ||
Comment 48•8 years ago
|
||
(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)
Comment 49•8 years ago
|
||
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)
Comment 50•8 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #49) > meh, live it as you did it... s/live/leave/
Assignee | ||
Comment 51•8 years ago
|
||
(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.
Comment 52•8 years ago
|
||
I see some talos kraken regressions on aurora that appear to be coming from this change
Assignee | ||
Comment 53•8 years ago
|
||
(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)
Comment 54•8 years ago
|
||
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.
Description
•