Closed Bug 1452551 Opened 6 years ago Closed 6 years ago

Clean up Telemetry initialization in GeckoView

Categories

(Toolkit :: Telemetry, enhancement, P1)

enhancement

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: nobody → alessio.placitelli
Blocks: 1452550
Priority: -- → P1
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 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]
Summary: Do not pack/include JSM files from Telemetry when compiling for GeckoView → Clean up Telemetry initialization in GeckoView
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+
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 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.
Blocks: 1437551
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)
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&regexp=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`.
:jchen, any chance you could check GeckoViewTelemetryController.jsm for how I'm using the logging introduced in bug 1452200?
Flags: needinfo?(nchen)
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+
Flags: needinfo?(nchen)
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.
(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 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 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 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+
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!
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?
Pushed by alessio.placitelli@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b5f89882b966
Initialize the Telemetry core in GeckoView. r=esawin,gfritzsche,jchen
(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
https://hg.mozilla.org/mozilla-central/rev/b5f89882b966
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
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)
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.