1499418 - Add GeckoView page load performance telemetry

Status: RESOLVED FIXED

(GeckoView :: General, defect, P1)

Description

[Eugen Sawin [:esawin]]

See bug 1497997: we want to add (and control) the actual and perceived page load performance probes for GeckoView for our performance dashboard.

Comment 1

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1499418-1.0-Add-GeckoView-page-load-performance-.patch

Add two probes: GV_LOAD_PROGRESS_COMPLETE_MS for perceived and GV_PAGE_LOAD_MS for actual page load time.

snorp: Do you think these probes would be adequate to represent the overall page load times?

chutten: Are the probe configs sane and do I need to add someone else to the mail alerts?

Comment 2

[Chris H-C :chutten]

Comment on attachment 9017585 [details] [diff] [review]
0001-Bug-1499418-1.0-Add-GeckoView-page-load-performance-.patch

Review of attachment 9017585 [details] [diff] [review]:
-----------------------------------------------------------------

Probe definitions and TelemetryStopwatch uses look sane. You'll need a Data Collection Review for the added collections: https://wiki.mozilla.org/Firefox/Data_Collection

Comment 3

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1499418-2.0-Add-GeckoView-startup-performance-te.patch

What do you think about this two probes: one times the total Gecko startup duration on the parent and content processes and the other times the parent module initialization.

This setup should be mostly safe with the preloading mechanics and if not, we should be able to easily filter them out as outliers.

The Java probes would need Telemetry.java and dependencies to be moved under geckoview/ or we could move the probes into native code instead.

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 9020461 [details] [diff] [review]
0002-Bug-1499418-2.0-Add-GeckoView-startup-performance-te.patch

Review of attachment 9020461 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/chrome/geckoview/geckoview.js
@@ +78,5 @@
>  
>        this._modules.clear();
>      });
> +
> +    MODULES_INIT_PROBE.finish();

Have you measured how long this typically takes? We don't really do any time-consuming stuff during the init stage, so I'm not sure if this is worth tracking.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoThread.java
@@ +8,5 @@
>  import org.mozilla.gecko.annotation.RobocopTarget;
>  import org.mozilla.gecko.annotation.WrapForJNI;
>  import org.mozilla.gecko.mozglue.GeckoLoader;
>  import org.mozilla.gecko.process.GeckoProcessManager;
> +import org.mozilla.gecko.Telemetry;

Should be at the top (above `RobocopTarget`)

@@ +142,5 @@
>      private GeckoProfile mProfile;
>      private String[] mArgs;
>      private Bundle mExtras;
>      private int mFlags;
> +    private static Telemetry.Timer sInitTimer;

Keep with other static fields

@@ +159,5 @@
>                                        final int prefsFd, final int prefMapFd,
>                                        final int ipcFd,
>                                        final int crashFd,
>                                        final int crashAnnotationFd) {
> +        sInitTimer = new Telemetry.UptimeTimer("GV_STARTUP_RUNTIME_MS");

Move to after the `mInitialized` check

::: toolkit/components/telemetry/Histograms.json
@@ +14224,5 @@
> +  },
> +  "GV_STARTUP_RUNTIME_MS": {
> +    "record_in_processes": ["main", "content"],
> +    "alert_emails": [
> +      "[e-mail]"

Should be a GV team-wide email address

@@ +14236,5 @@
> +  },
> +  "GV_STARTUP_MODULES_MS": {
> +    "record_in_processes": ["main"],
> +    "alert_emails": [
> +      "[e-mail]"

Same

::: widget/android/nsAppShell.cpp
@@ +433,5 @@
>          mozilla::GeckoProcessManager::Init();
>          mozilla::GeckoScreenOrientation::Init();
>          mozilla::GeckoSystemStateListener::Init();
>          mozilla::PrefsHelper::Init();
> +        mozilla::widget::Telemetry::Init();

`mozilla::widget::Telemetry` is not part of GeckoView ("Telemetry.h" is under "widget/android/fennec")

Comment 5

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1499418-1.1-Add-GeckoView-page-load-performance-.patch

Addressed comments.

Comment 6

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1499418-2.1-Add-GeckoView-startup-performance-te.patch

Addressed comments.

Comment 7

[Eugen Sawin [:esawin]]

Attachment: 0003-Bug-1499418-3.0-Move-and-split-Fennec-s-telemetry-cl.patch

Move the telemetry classes into the geckoview directory.
I've renamed Telemetry.java to TelemetryUtils.java and added a Telemetry.java wrapper for Fennec which handles the MMA stuff.

Comment 8

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 9022215 [details] [diff] [review]
0001-Bug-1499418-1.1-Add-GeckoView-page-load-performance-.patch

Review of attachment 9022215 [details] [diff] [review]:
-----------------------------------------------------------------

Just leaving two short drive-by comments.

::: mobile/android/modules/geckoview/GeckoViewTelemetry.jsm
@@ +5,5 @@
> +
> +ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetters(this, {
> +  TelemetryStopwatch: "resource://gre/modules/TelemetryStopwatch.jsm",

Note bug 1482091, which is converting `TelemetryStopwatch` to C++ & webidl.

@@ +11,5 @@
> +
> +var EXPORTED_SYMBOLS = ["HistogramStopwatch"];
> +
> +// A helper for histogram timer probes.
> +class HistogramStopwatch {

This looks like a good utility that could live in the Telemetry component directly.
Could you file a follow-up bug for us?

Comment 9

[Eugen Sawin [:esawin]]

Attachment: request.md

Comment 10

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 9022224 [details] [diff] [review]
0003-Bug-1499418-3.0-Move-and-split-Fennec-s-telemetry-cl.patch

Review of attachment 9022224 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/TelemetryUtils.java
@@ +24,5 @@
>   *   avoid timing a user activity when their phone is in their pocket!
>   *
>   * The majority of methods in this class are defined in terms of real time.
>   */
>  @RobocopTarget

Probably don't need this anymore?

@@ +196,5 @@
>          if (method == null) {
>              throw new IllegalArgumentException("Expected non-null method - use Method.NONE?");
>          }
>  
> +        if (true) {

This should still be `!AppConstants.RELEASE_OR_BETA`

Comment 11

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1499418-1.2-Add-GeckoView-page-load-performance-.patch

Addressed comment.

Comment 12

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1499418-2.1-Add-GeckoView-startup-performance-te.patch

Addressed comments.

Comment 13

[Eugen Sawin [:esawin]]

Attachment: 0003-Bug-1499418-3.1-Move-and-split-Fennec-s-telemetry-cl.patch

Addressed comments.

Comment 14

[Chris Peterson [:cpeterson]]

64=wontfix because this issue doesn't block Focus 8.0 from using GV 64.

Comment 15

[Chenxia Liu [:liuche]]

Comment on attachment 9022677 [details]
request.md

One nit: in addition to the GeckoView mailing list, can you also include an individual's email? That field is a list so you can have multiple items. Having an individual makes it easier for people to know a specific person to ping.


1) Is there or will there be **documentation** that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes, in Histograms.jsm

2) Is there a control mechanism that allows the user to turn the data collection on and off?

Yes, Histograms are controlled by user data controls


3) If the request is for permanent data collection, is there someone who will monitor the data over time?**

esawin

4) Using the **[category system of data types](https://wiki.mozilla.org/Firefox/Data_Collection)** on the Mozilla wiki, what collection type of data do the requested measurements fall under?  **

Type 1, technical data on GeckoView loading and initialization

5) Is the data collection request for default-on or default-off?

Included in library, data collection is up to app

6) Does the instrumentation include the addition of **any *new* identifiers** (whether anonymous or otherwise; e.g., username, random IDs, etc.  See the appendix for more details)?

No

7) Is the data collection covered by the existing Firefox privacy notice?
Yes

8) Does there need to be a check-in in the future to determine whether to renew the data? (Yes/No) (If yes, set a todo reminder or file a bug if appropriate)**

No, collections of basic performance of Gecko

Comment 16

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1499418-1.3-Add-GeckoView-page-load-and-startup-.patch

Addressed comment and merged patch 1 and 2.

Comment 17

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1499418-3.2-Refactor-and-move-Fennec-s-telemetry.patch

Addressed comments and fixed one Telemetry.stopUISession() call.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bc653b02e2f
[1.3] Add GeckoView page load and startup performance telemetry probes. r=snorp,chutten,jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/24e87b02707b
[3.2] Refactor and move Fennec's telemetry classes to geckoview/. r=jchen

Comment 19

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/8bc653b02e2f
https://hg.mozilla.org/mozilla-central/rev/24e87b02707b

Comment 20

[Chris Peterson [:cpeterson]]

64=affected because Eugen recommends we uplift these new telemetry probes to GV 64 Beta for the Focus 8.0 testing.

Comment 21

[Eugen Sawin [:esawin]]

Comment on attachment 9024767 [details] [diff] [review]
0001-Bug-1499418-1.3-Add-GeckoView-page-load-and-startup-.patch

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: No user impact, but we would miss page load and startup performance telemetry data.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

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): Adding new telemetry probes does not increase risk, but the refactoring of the telemetry classes may pose a low risk for Fennec's telemetry reporting.

String changes made/needed: None

Comment 22

[Eugen Sawin [:esawin]]

Comment on attachment 9024769 [details] [diff] [review]
0002-Bug-1499418-3.2-Refactor-and-move-Fennec-s-telemetry.patch

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: 

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: Yes

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): 

String changes made/needed:

Comment 23

[Julien Cristau [:jcristau]]

(In reply to Eugen Sawin [:esawin] from comment #21)
> Is this code covered by automated tests?: No
> 
> Has the fix been verified in Nightly?: No
> 
> 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): Adding new
> telemetry probes does not increase risk, but the refactoring of the
> telemetry classes may pose a low risk for Fennec's telemetry reporting.
> 
The above doesn't sound like a reasonable combination to me.  Is this something we could land on a GV relbranch to avoid affecting fennec?  Or are there steps we can take to validate fennec telemetry with this change?

Comment 24

[Eugen Sawin [:esawin]]

Sebastian, is there a way to validate Fennec telemetry (especially the MMA stuff) on Nightly?
I had to refactor some Fennec telemetry code (patch 2) to make it reusable for GeckoView and would like to see that uplifted without regressing Fennec.

Comment 25

[Eugen Sawin [:esawin]]

(In reply to Julien Cristau [:jcristau] from comment #23)
> The above doesn't sound like a reasonable combination to me.  Is this
> something we could land on a GV relbranch to avoid affecting fennec?  Or are
> there steps we can take to validate fennec telemetry with this change?

Landing that on a GV release branch would work, we don't need this changes in Fennec.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9024767 [details] [diff] [review]
0001-Bug-1499418-1.3-Add-GeckoView-page-load-and-startup-.patch

Gecko 64 will be merged to mozilla-release next week. We can create the GV64 relbranch and uplift it there at that time with an approval-mozilla-geckoview64 request (that request flag will also be created next Monday).

Comment 27

[Ryan VanderMeulen [:RyanVM]]

We don't have tracking-geckoview64 and status-geckoview64 flags yet, but will want to set this bug to + and affected, respectively, once they've been created next week.

Comment 28

[Chris Peterson [:cpeterson]]

The status-geckoview64 flag has been created. Setting status-geckoview64=affected because Eugen said he would like to uplift these telemetry probes.

Comment 29

[Chris Peterson [:cpeterson]]

status-geckoview64=wontfix

Comment 30

[Eugen Sawin [:esawin]]

Comment on attachment 9024767 [details] [diff] [review]
0001-Bug-1499418-1.3-Add-GeckoView-page-load-and-startup-.patch

[GeckoView Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for consideration: The new telemetry probes are required to track GeckoView page load and startup performance.

User impact if declined: No user impact, engineering impact would be the lack of page load performance telemetry.

Fix Landed on Version: 65

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): For GeckoView, this only adds some new telemetry probes.

String or UUID changes made by this patch: None

Comment 31

[Eugen Sawin [:esawin]]

Comment on attachment 9024769 [details] [diff] [review]
0002-Bug-1499418-3.2-Refactor-and-move-Fennec-s-telemetry.patch

[GeckoView Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for consideration: 

User impact if declined: 

Fix Landed on Version: 

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): 

String or UUID changes made by this patch:

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9024767 [details] [diff] [review]
0001-Bug-1499418-1.3-Add-GeckoView-page-load-and-startup-.patch

Per discussion with cpeterson, approved for GV64.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Pushed to GECKOVIEW_64_RELBRANCH:
https://hg.mozilla.org/releases/mozilla-release/rev/348872e1c4fe
https://hg.mozilla.org/releases/mozilla-release/rev/26d700d8010a