Closed Bug 1547331 Opened 1 year ago Closed 1 year ago

Legacy telemetry seems to be enabled in Fenix's GeckoView

Categories

(Toolkit :: Telemetry, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox-esr60 --- unaffected
firefox67 --- wontfix
firefox67.0.1 --- wontfix
firefox68 + fixed
firefox69 --- fixed

People

(Reporter: Dexter, Assigned: Dexter)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression, Whiteboard: [geckoview:fenix:m6])

Attachments

(1 file)

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.

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?

Flags: needinfo?(esawin)

(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.

Flags: needinfo?(esawin)
Regressed by: 1524688

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}]

Priority: -- → P1
Assignee: nobody → alessio.placitelli

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

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?

Assignee: alessio.placitelli → nobody
Blocks: 1552884
Flags: needinfo?(kmaglione+bmo)

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.

Flags: needinfo?(kmaglione+bmo)

[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.

Whiteboard: [geckoview:fenix:m6]
Assignee: nobody → alessio.placitelli

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.

Pushed by aplacitelli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c62af311d39b
Don't initialize legacy telemetry in GeckoView. r=chutten,kmag
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

Please request uplift to beta when you get a chance.

Flags: needinfo?(alessio.placitelli)

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

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:
Attachment #9069322 - Flags: approval-mozilla-beta?

(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.

Flags: needinfo?(alessio.placitelli)
See Also: → 1558156

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

gv fix for 68.0b9

Attachment #9069322 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

== 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

You need to log in before you can comment on or make changes to this bug.