Closed
Bug 1452551
Opened 6 years ago
Closed 6 years ago
Clean up Telemetry initialization in GeckoView
Categories
(Toolkit :: Telemetry, enhancement, P1)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: Dexter, Assigned: Dexter)
References
Details
Attachments
(1 file)
When on GeckoView, we don't need to include all the JSM files that implement the Telemetry logic and delivery mechanism on Desktop. Let's investigate if we can simply exclude them from the packing stage while still finding a way to set the required Telemetry flags (canRecordRelease/Prerelease).
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 1•6 years ago
|
||
Note dump: - Telemetry is not being initialized in GeckoView [1], as TelemetryStartup [2] is not being packaged/loaded there (but it is on Fennec). - Does that mean that Telemetry *.JSM files are already being excluded? - |MOZ_GECKOVIEW_JAR| can be used in the manifest files to check for GeckoView. - We can introduce a GeckoView only startup JS file that takes care of setting the appropriate Telemetry prefs for enabling the Core. [1] - https://searchfox.org/mozilla-central/rev/7ccb618f45a1398e31a086a009f87c8fd3a790b6/mobile/android/installer/package-manifest.in#188 [2] - https://searchfox.org/mozilla-central/rev/7ccb618f45a1398e31a086a009f87c8fd3a790b6/toolkit/components/telemetry/TelemetryStartup.js
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•6 years ago
|
||
Note: TelemetryController is still invoked when starting up content processes - https://searchfox.org/mozilla-central/rev/6bfadf95b4a6aaa8bb3b2a166d6c3545983e179a/toolkit/components/processsingleton/ContentProcessSingleton.js#23
Comment hidden (mozreview-request) |
Comment 5•6 years ago
|
||
mozreview-review |
Comment on attachment 8967290 [details] Bug 1452551 - Initialize the Telemetry core in GeckoView. https://reviewboard.mozilla.org/r/235976/#review241846 Code analysis found 3 defects in this patch: - 3 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: toolkit/components/processsingleton/ContentProcessSingleton.js:26 (Diff revision 2) > observe(subject, topic, data) { > switch (topic) { > case "app-startup": { > Services.obs.addObserver(this, "xpcom-shutdown"); > + // Initialize Telemetry in the content process: use a different > + // controller depending on the platform. Error: Trailing spaces not allowed. [eslint: no-trailing-spaces] ::: toolkit/components/processsingleton/ContentProcessSingleton.js:29 (Diff revision 2) > Services.obs.addObserver(this, "xpcom-shutdown"); > + // Initialize Telemetry in the content process: use a different > + // controller depending on the platform. > + if (Services.prefs.getBoolPref("toolkit.telemetry.geckoview", false)) { > + GeckoViewTelemetryController.setup(); > + return; Error: Trailing spaces not allowed. [eslint: no-trailing-spaces] ::: toolkit/components/telemetry/geckoview/GeckoViewTelemetryStartup.js:7 (Diff revision 2) > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm"); > + > +XPCOMUtils.defineLazyModuleGetter(this, "GeckoViewTelemetryController", Error: Please use chromeutils.definemodulegetter instead of xpcomutils.definelazymodulegetter [eslint: mozilla/use-chromeutils-import]
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Summary: Do not pack/include JSM files from Telemetry when compiling for GeckoView → Clean up Telemetry initialization in GeckoView
Comment 8•6 years ago
|
||
mozreview-review |
Comment on attachment 8967290 [details] Bug 1452551 - Initialize the Telemetry core in GeckoView. https://reviewboard.mozilla.org/r/235976/#review241992 ::: toolkit/components/processsingleton/ContentProcessSingleton.js:31 (Diff revision 4) > + // controller depending on the platform. > + if (Services.prefs.getBoolPref("toolkit.telemetry.geckoview", false)) { > + GeckoViewTelemetryController.setup(); > + return; > + } > + // Initialize Desktop and Fennec Telemetry. Fennec doesn't have a content process. ::: toolkit/components/telemetry/geckoview/GeckoViewTelemetryController.jsm:48 (Diff revision 4) > + const isReleaseCandidateOnBeta = > + AppConstants.MOZ_UPDATE_CHANNEL === "release" && > + Services.prefs.getCharPref("app.update.channel", null) === "beta"; > + > + // Finally enable probe recording based on user preferences. > + Services.telemetry.canRecordBase = true; Will this stop all telemetry recording and can we control it at runtime? This would be an interesting setting to expose in the GV API.
Attachment #8967290 -
Flags: review?(esawin) → review+
Assignee | ||
Comment 9•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8967290 [details] Bug 1452551 - Initialize the Telemetry core in GeckoView. https://reviewboard.mozilla.org/r/235976/#review241992 > Fennec doesn't have a content process. Whoops, right, you already mentioned that to me :) My bad! > Will this stop all telemetry recording and can we control it at runtime? > > This would be an interesting setting to expose in the GV API. Yes, it controls the recording. However that should not be touched outside of this, without a good reason! As with Firefox Desktop, we always collect at least the "release" Telemetry probes, but we only send them out when data upload is enabled.
Comment hidden (mozreview-request) |
Comment 11•6 years ago
|
||
mozreview-review |
Comment on attachment 8967290 [details] Bug 1452551 - Initialize the Telemetry core in GeckoView. https://reviewboard.mozilla.org/r/235976/#review242278 ::: toolkit/components/telemetry/geckoview/GeckoViewTelemetryController.jsm:20 (Diff revision 5) > +XPCOMUtils.defineLazyGetter(this, "dump", () => > + ChromeUtils.import("resource://gre/modules/AndroidLog.jsm", > + {}).AndroidLog.d.bind(null, "GeckoViewTelemetry")); > + > +function debug(msg) { > + dump(msg); Looks like reviewboard had eaten my comment, but this should be commented out in production.
Comment hidden (mozreview-request) |
Comment 13•6 years ago
|
||
mozreview-review |
Comment on attachment 8967290 [details] Bug 1452551 - Initialize the Telemetry core in GeckoView. https://reviewboard.mozilla.org/r/235976/#review242534 This is a clear & focused solution! I only have smaller comments below. ::: mobile/android/app/geckoview-prefs.js:15 (Diff revision 6) > pref("dom.ipc.processCount", 1); > pref("dom.ipc.keepProcessesAlive.web", 1); > pref("dom.ipc.processPrelaunch.enabled", false); > + > +// Tell Telemetry that we're in GeckoView mode. > +pref("toolkit.telemetry.geckoview", true); Can we make the name more descriptive? `isGeckoviewMode`, `isGeckoview`, etc.? ::: toolkit/components/telemetry/geckoview/GeckoViewTelemetryController.jsm:1 (Diff revision 6) > +/* This Source Code Form is subject to the terms of the Mozilla Public I like the use of the `geckoview` sub directory. Going forward these things really help to manage our codebase. ::: toolkit/components/telemetry/geckoview/GeckoViewTelemetryController.jsm:15 (Diff revision 6) > +XPCOMUtils.defineLazyGetter(this, "dump", () => > + ChromeUtils.import("resource://gre/modules/AndroidLog.jsm", > + {}).AndroidLog.d.bind(null, "GeckoViewTelemetry")); > + > +function debug(msg) { > + // dump(msg); > +} Is this left intentionally? Do our Android engineers have recommendations on tracing usage and trace levels? ::: toolkit/components/telemetry/geckoview/GeckoViewTelemetryController.jsm:32 (Diff revision 6) > + // Enable extended Telemetry on pre-release channels and disable it > + // on Release/ESR. Does the logic below match our Desktop logic? Can we re-use/share any of that logic? ::: toolkit/components/telemetry/geckoview/GeckoViewTelemetryStartup.js:21 (Diff revision 6) > + classID: Components.ID("{0011654e-fd3a-4dca-85c1-827e5cd36048}"), > + > + QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver]), > + > + observe(aSubject, aTopic, aData) { > + // This initialized Telemetry for GeckoView only in the parent process. Nit: "initializes" ::: toolkit/components/telemetry/moz.build:196 (Diff revision 6) > + > + Nit: Double empty line.
Attachment #8967290 -
Flags: review?(gfritzsche)
Assignee | ||
Comment 14•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8967290 [details] Bug 1452551 - Initialize the Telemetry core in GeckoView. https://reviewboard.mozilla.org/r/235976/#review242534 > Is this left intentionally? > Do our Android engineers have recommendations on tracing usage and trace levels? Yes. As far as I can tell, all the GeckoView modules [use this technique](https://searchfox.org/mozilla-central/search?q=%7B%7D).AndroidLog.d.bind(null%2C&case=false®exp=false&path=gec). There's bug 1452200 to change that.... but hey, this just landed 5 hours ago! Let me follow up with the changes :) > Does the logic below match our Desktop logic? > Can we re-use/share any of that logic? Good point. I've refactored this out to `TelemetryUtils.jsm`.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 17•6 years ago
|
||
:jchen, any chance you could check GeckoViewTelemetryController.jsm for how I'm using the logging introduced in bug 1452200?
Flags: needinfo?(nchen)
Comment 18•6 years ago
|
||
mozreview-review |
Comment on attachment 8967290 [details] Bug 1452551 - Initialize the Telemetry core in GeckoView. https://reviewboard.mozilla.org/r/235976/#review242750 Looks good! ::: toolkit/components/telemetry/geckoview/GeckoViewTelemetryController.jsm:10 (Diff revision 8) > +"use strict"; > + > +ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm"); > + > +XPCOMUtils.defineLazyModuleGetters(this, { > + AppConstants: "resource://gre/modules/AppConstants.jsm", Not used? ::: toolkit/components/telemetry/geckoview/GeckoViewTelemetryController.jsm:11 (Diff revision 8) > + > +ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm"); > + > +XPCOMUtils.defineLazyModuleGetters(this, { > + AppConstants: "resource://gre/modules/AppConstants.jsm", > + GeckoViewUtils: "resource://gre/modules/GeckoViewUtils.jsm", Use `ChromeUtils.import("resource://gre/modules/GeckoViewUtils.jsm")` because `GeckoViewUtils` is used immediately below. ::: toolkit/components/telemetry/geckoview/GeckoViewTelemetryController.jsm:16 (Diff revision 8) > + GeckoViewUtils: "resource://gre/modules/GeckoViewUtils.jsm", > + Services: "resource://gre/modules/Services.jsm", > + TelemetryUtils: "resource://gre/modules/TelemetryUtils.jsm", > +}); > + > +GeckoViewUtils.initLogging("GeckoView.TelemetryController.[C]", this); You probably don't need the `.[C]` suffix because it looks like this is used in both the main and content processes. We are using `.[C]` to denote content-process code. ::: toolkit/components/telemetry/geckoview/GeckoViewTelemetryController.jsm:30 (Diff revision 8) > + debug `setup`; > + > + TelemetryUtils.setTelemetryRecordingFlags(); > + > + debug `setup - canRecordPrereleaseData ${Services.telemetry.canRecordPrereleaseData}, > + canRecordReleaseData ${Services.telemetry.canRecordReleaseData}`; You probably want the newline to be inside `${}`, otherwise the newline (and the indentation) will be included in the log output. debug `foo ${ bar } ${ baz }`; This looks a little weird, but it makes sure the newline/indentation are not included in the output.
Attachment #8967290 -
Flags: review+
Updated•6 years ago
|
Flags: needinfo?(nchen)
Comment 19•6 years ago
|
||
BTW, I think I slightly prefer initializing `GeckoViewTelemetryController.jsm` inside `GeckoViewStartup.js`, rather than having a separate `GeckoViewTelemetryStartup.js`, but it's not a huge deal.
Assignee | ||
Comment 20•6 years ago
|
||
(In reply to Jim Chen [:jchen] [:darchons] from comment #19) > BTW, I think I slightly prefer initializing > `GeckoViewTelemetryController.jsm` inside `GeckoViewStartup.js`, rather than > having a separate `GeckoViewTelemetryStartup.js`, but it's not a huge deal. Done! I don't have a strong opinion on this, so let's make this patch smaller by relying on GeckoViewStartup.js :)
Comment hidden (mozreview-request) |
Comment 22•6 years ago
|
||
mozreview-review |
Comment on attachment 8967290 [details] Bug 1452551 - Initialize the Telemetry core in GeckoView. https://reviewboard.mozilla.org/r/235976/#review242938 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: toolkit/components/telemetry/geckoview/GeckoViewTelemetryController.jsm:29 (Diff revision 9) > + debug `setup`; > + > + TelemetryUtils.setTelemetryRecordingFlags(); > + > + debug `setup - canRecordPrereleaseData ${Services.telemetry.canRecordPrereleaseData > + }, canRecordReleaseData ${Services.telemetry.canRecordReleaseData}`; Error: Unexpected tab character. [eslint: no-tabs]
Comment 23•6 years ago
|
||
mozreview-review |
Comment on attachment 8967290 [details] Bug 1452551 - Initialize the Telemetry core in GeckoView. https://reviewboard.mozilla.org/r/235976/#review243052 ::: commit-message-30d72:3 (Diff revision 9) > +Bug 1452551 - Initialize the Telemetry core in GeckoView. r?esawin,gfritzsche,jchen > + > +This additionally introduces a new pref (toolkit.telemetry.geckoview) isGeckoViewMode (although mode sounds weird, but might be OK for the telemetry context)
Comment 24•6 years ago
|
||
mozreview-review |
Comment on attachment 8967290 [details] Bug 1452551 - Initialize the Telemetry core in GeckoView. https://reviewboard.mozilla.org/r/235976/#review243236 This looks good, thanks. I have some comments/questions below that i want to see cleared up, but no major blockers. ::: toolkit/components/processsingleton/ContentProcessSingleton.js:27 (Diff revision 9) > + if (Services.prefs.getBoolPref("toolkit.telemetry.isGeckoViewMode", false)) { > + GeckoViewTelemetryController.setup(); The comment in GeckoViewStartup.js says we don't want to load the controller in all (GeckoView) content processes. Here we apparently load the `GeckoViewTelemetryController` in all GeckoView content processes. Does the comment above need clarification or am i missing something? ::: toolkit/components/telemetry/TelemetryController.jsm:578 (Diff revision 9) > > // Configure base Telemetry recording. > // Unified Telemetry makes it opt-out. If extended Telemetry is enabled, base recording > // is always on as well. > if (IS_UNIFIED_TELEMETRY) { > - // Enable extended Telemetry on pre-release channels and disable it > + TelemetryUtils.setTelemetryRecordingFlags(); This (plus the other coming changes) looks like we have to schedule a QA pass to make sure we don't impact any existing release or prerelease Desktop Telemetry. Can you file a bug on that? ::: toolkit/components/telemetry/geckoview/GeckoViewTelemetryController.jsm:18 (Diff revision 9) > +}); > + > +GeckoViewUtils.initLogging("GeckoView.TelemetryController", this); > + > +let GeckoViewTelemetryController = { > + Nit: Redundant empty line? ::: toolkit/components/telemetry/geckoview/GeckoViewTelemetryController.jsm:24 (Diff revision 9) > + /** > + * Setup the Telemetry recording flags. This must be called > + * in all the processes that need to collect Telemetry. > + */ > + setup() { > + debug `setup`; This (and below) looks unusual. Why use this style over `debug(...)`? How does this look in the console? Does `debug` actually add prefixes like "Telemetry::" ::: toolkit/components/telemetry/geckoview/GeckoViewTelemetryController.jsm:33 (Diff revision 9) > + debug `setup - canRecordPrereleaseData ${Services.telemetry.canRecordPrereleaseData > + }, canRecordReleaseData ${Services.telemetry.canRecordReleaseData}`; > + }, > +}; > + > +var EXPORTED_SYMBOLS = ["GeckoViewTelemetryController"]; Please put this on the top per our usual practices. It's easy to overlook here.
Attachment #8967290 -
Flags: review?(gfritzsche) → review+
Assignee | ||
Comment 25•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8967290 [details] Bug 1452551 - Initialize the Telemetry core in GeckoView. https://reviewboard.mozilla.org/r/235976/#review243052 > isGeckoViewMode (although mode sounds weird, but might be OK for the telemetry context) Good catch!
Assignee | ||
Comment 26•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8967290 [details] Bug 1452551 - Initialize the Telemetry core in GeckoView. https://reviewboard.mozilla.org/r/235976/#review243236 > The comment in GeckoViewStartup.js says we don't want to load the controller in all (GeckoView) content processes. > Here we apparently load the `GeckoViewTelemetryController` in all GeckoView content processes. > > Does the comment above need clarification or am i missing something? Good point, this comment made sense when we had `GeckoViewTelemetryStartup.js`. Since we moved initialization in `GeckoViewStartup.js`, as requested by jchen, this doesn't make sense anymore. We still want to init content process Telemetry in `ContentProcessSingleton.jsm` for consistency with Desktop and because we need to disable `TelemetryController.jsm` there anyway. > This (plus the other coming changes) looks like we have to schedule a QA pass to make sure we don't impact any existing release or prerelease Desktop Telemetry. > Can you file a bug on that? I filed bug 1454902 for this and sent a PI Request. > This (and below) looks unusual. > Why use this style over `debug(...)`? > How does this look in the console? Does `debug` actually add prefixes like "Telemetry::" Logging for GeckoView is only supported to [literals](https://searchfox.org/mozilla-central/rev/f65d7528e34ef1a7665b4a1a7b7cdb1388fcd3aa/mobile/android/modules/geckoview/GeckoViewUtils.jsm#260), see bug 1452200. The log line is prefixed with the name of the module, as shown below: ``` 04-18 12:15:05.391 25869-25888/org.mozilla.geckoview_example D/GeckoViewTelemetryController: setup - canRecordPrereleaseData true, canRecordReleaseData true ``` I guess we could create an helper function to change that for the code living in our modules. Should I file a bug?
Comment hidden (mozreview-request) |
Comment 28•6 years ago
|
||
Pushed by alessio.placitelli@gmail.com: https://hg.mozilla.org/integration/autoland/rev/b5f89882b966 Initialize the Telemetry core in GeckoView. r=esawin,gfritzsche,jchen
Assignee | ||
Comment 29•6 years ago
|
||
(In reply to Alessio Placitelli [:Dexter] from comment #26) > I guess we could create an helper function to change that for the code > living in our modules. Should I file a bug? I filed bug 1454951
Comment 30•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b5f89882b966
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee | ||
Comment 31•6 years ago
|
||
Hey Madalin! As discussed, this is the first bug that would need QA attention. We want to make sure that this change didn't cause any regression on the collection of release and pre-release data on Firefox Desktop. This should be similar to the QA performed in other bugs (e.g. bug 1406391). We want to make sure that the right data is recorded (pre-release data on pre-release channels and release data on the release channel).
Flags: needinfo?(madalin.cotetiu)
Comment 32•6 years ago
|
||
Removing the ni. Testing results in https://bugzilla.mozilla.org/show_bug.cgi?id=1454902
Flags: needinfo?(madalin.cotetiu)
You need to log in
before you can comment on or make changes to this bug.
Description
•