1547331 - Legacy telemetry seems to be enabled in Fenix's GeckoView

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Alessio Placitelli [:Dexter]]

While debugging Fenix, I saw this in my logcat

2019-04-26 17:15:12.866 30286-30345/org.mozilla.fenix.debug E/GeckoConsole: [JavaScript Error: "1556291712866 Telemetry::CoveragePing ERROR no endpoint base set" {file: "resource://gre/modules/Log.jsm" line: 679}]
append@resource://gre/modules/Log.jsm:679:12
log@resource://gre/modules/Log.jsm:360:16
error@resource://gre/modules/Log.jsm:368:10
startup@resource://gre/modules/CoveragePing.jsm:40:11
setupTelemetry/this._delayedInitTask<@resource://gre/modules/TelemetryController.jsm:721:30

This seems to suggest that TelemetryController.jsm is being packaged with GeckoView (it wasn't and it shouldn't?) and it's running.

Comment 1

[Alessio Placitelli [:Dexter]]

Hey Eugen! I seem to remember that GeckoViewTelemetryController.jsm was the only file being packaged for GeckoView builds, when building Android stuff. Other Telemetry's JSM files were left out. However, I'm not able to find that file anymore and looks like legacy telemetry is getting run in GeckoView when it really shouldn't. Did something change recently?

Comment 2

[Alessio Placitelli [:Dexter]]

(In reply to Alessio Placitelli [:Dexter] from comment #1)

Hey Eugen! I seem to remember that GeckoViewTelemetryController.jsm was the only file being packaged for GeckoView builds, when building Android stuff. Other Telemetry's JSM files were left out. However, I'm not able to find that file anymore and looks like legacy telemetry is getting run in GeckoView when it really shouldn't. Did something change recently?

We used to have that in package-manifest.in, but a recent patch got rid of this. We're not looking for "not GeckoView" anymore. See here.

Comment 3

[Alessio Placitelli [:Dexter]]

This is sending data. We should act on it:

E/GeckoConsole: [JavaScript Error: "1556614549310 Toolkit.Telemetry ERROR TelemetrySend::_doPing - error making request to https://incoming.telemetry.mozilla.org/submit/telemetry/74f4ae01-8441-4afa-b340-b5c54d7477b9/modules/Fennec/68.0a1/nightly/20190422094240?v=4: eUnreachable" {file: "resource://gre/modules/Log.jsm" line: 679}]

Comment 4

[Alessio Placitelli [:Dexter]]

For context, this is the regressing changeset: https://phabricator.services.mozilla.com/D18437

Comment 5

[Alessio Placitelli [:Dexter]]

Hey :kmag, I saw that you worked on this changeset. That changed the behaviour of telemetry for GeckoView: while TelemetryStartup.js should be called for Firefox Desktop and Fennec, it should not be called for GeckoView. That's why it was wrapped by MOZ_GECKOVIEW_JAR.

As far as I know, we can't simply use MOZ_GECKOVIEW_JAR in .build files, as that's only provided at the packaging step, not at the build step. What's the right fix for this? Should the changeset be reverted?

Comment 6

[Kris Maglione [:kmag]]

I think perhaps just the category registration should be moved back to a component manifest file, and we can leave the component registration as-is. It would still wind up being packaged for GeckoView, but it wouldn't be loaded automatically.

I suppose there might be some way to change the packager to skip packaging that module for GeckoView, too, but the only trivial-ish way to do that that I can think of is to keep packaging it in the components/ directory but still load it as a JSM.

Comment 7

[Chris Peterson [:cpeterson]]

[Tracking Requested - why for this release]:

Alessio says this is a telemetry regression in 67 and is very important to fix in 68 for Fenix MVP.

Comment 8

[Alessio Placitelli [:Dexter]]

Attachment: Bug 1547331 - Don't initialize legacy telemetry in GeckoView. r?chutten,kmag

This changes the registration code so that categories are
still registered in the manifest (and thus not registered
in GeckoView), but the component is still always shipped.

Comment 9

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=07e841e88f171a70771edb2bf44d4d1cbb82e7ec

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c62af311d39b
Don't initialize legacy telemetry in GeckoView. r=chutten,kmag

Comment 11

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/c62af311d39b

Comment 12

[Julien Cristau [:jcristau]]

Please request uplift to beta when you get a chance.

Comment 13

[Chris Peterson [:cpeterson]]

We'll want to uplift this fix to GV 68 Beta so we can ship it in Fenix MVP.

Comment 14

[Alessio Placitelli [:Dexter]]

Comment on attachment 9069322 [details]
Bug 1547331 - Don't initialize legacy telemetry in GeckoView. r?chutten,kmag

Beta/Release Uplift Approval Request

• User impact if declined: Legacy telemetry would be enabled in Fenix in addition to Glean, sending rougue pings out throughTelemetryController. No change is expected for Desktop or Fennec.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is partially reverting bug 1524688 to the previous status, which worked. The changes should only affect GeckoView packaged gecko.
• String changes made/needed:

Comment 15

[Alessio Placitelli [:Dexter]]

(In reply to Chris Peterson [:cpeterson] from comment #13)

We'll want to uplift this fix to GV 68 Beta so we can ship it in Fenix MVP.

I manually checked this in the Reference Browser. I only see this line:

GeckoViewTelemetryController: setup - canRecordPrereleaseData true, canRecordReleaseData true

and not the one from comment 0, so this seems to be working as expected.

Comment 16

[Julien Cristau [:jcristau]]

Comment on attachment 9069322 [details]
Bug 1547331 - Don't initialize legacy telemetry in GeckoView. r?chutten,kmag

gv fix for 68.0b9

Comment 17

[Razvan Maries]

https://hg.mozilla.org/releases/mozilla-beta/rev/39f91a7407ab

Comment 18

[Florin Strugariu [:Bebe]]

== Change summary for alert #21334 (as of Thu, 06 Jun 2019 08:09:59 GMT) ==

Improvements:

5% raptor-scn-power-idle-geckoview-power android-hw-g5-7-0-arm7-api-16 pgo 88.44 -> 83.90

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=21334