1549519 - Gather performance telemetry for reload stats

Status: RESOLVED FIXED

(Core :: Performance, enhancement)

Comment 1

[Sean Feng [:sefeng]]

Attachment: data_collection_review.txt

We need some page reload metrics to analyze the performance and improve the behaviours.

Comment 2

[Benjamin De Kosnik [:bdekoz]]

Hey Sean, is this associated with a particular PR? Can you give some backstory here? I'm having trouble understanding the context for the current differences in behavior between the toolbar reload button and F5, and so have to ask the stupid meta-question: wouldn't this be better fixed by having both of these actions use the same codepath, and not adding telemetry?

Given that, I think that adding a scalar or two scalars to count the number of reloads is a smart thing. This would be useful for existing telemetry scalars like total_uri_count, which include reloads in the total. With a separate reload count, it would be possible to disambiguate reloads and come up with a more accurate unique_uri_count. That's probably useful for data sci, apart from the reload issues that the perfteam is looking at.

Comment 3

[Sean Feng [:sefeng]]

bdekoz: I don't know the code paths on top of my head but I can provide my understanding of the issue.

Users can use different ways to reload a page, some of the options are F5 and the reload button. They work differently based on some historical factors (I am not exactly sure what the factors were, but Chrome did something similar here https://blog.chromium.org/2017/01/reload-reloaded-faster-and-leaner-page_26.html?m=1). The final solution might just be having both of these actions use the same codepath, but we need some telemetry to monitor and verify the user behaviours, then we can make the adjustment.

In terms of total_reload_uri_count, yeah as you said it probably doesn't help the issue we are looking, because we care which methods did the user use to reload. However, I can add it if we want to.

Let me know if anything is unclear.

Comment 4

[Benjamin De Kosnik [:bdekoz]]

Ah, I see. Thank you for the pointers to the backstory!

So we have something like

reload_cached_count
reload_refetched_count

or equivalent-ish? As two new scalars?

I like this, API-wise. If this sounds ok with you, can I rework the data collection review for you to comment on?

Comment 5

[Sean Feng [:sefeng]]

Yo Benjamin, I don't think I understand what you are proposing.

In the review form, I proposed 4 things.

A probe to count what users would prefer to reload a page
{
'F5/hotkey': 25,
'button': 12,
'other': 10
}

And also similar to the probe we have for page load
FX_RELOAD_PAGE_LOAD_MS_2

TIME_TO_RELOAD_FIRST_CONTENTFUL_PAINT_MS

TIME_TO_RELOAD_FIRST_INTERACTION_MS

Are you suggesting we add 2 more probes, ending up 6 in total?

I am also not sure what answers we can answer with reload_cached_count and reload_refetched_count.

Comment 6

[Benjamin De Kosnik [:bdekoz]]

Hmm. In proposed probe for counting 'reload invocation by the user' (is this an enumerated histogram?), what user input causes 'other'? Or, is this caused by non-user-input? (If there are only two cases, then just use a scalar...)

The existing reload user-facing interface seems not quite right, so I'm hesitant to sign-on to telemetry that uses it. See these other bug reports:

https://bugzilla.mozilla.org/show_bug.cgi?id=412244
https://bugzilla.mozilla.org/show_bug.cgi?id=1458912
https://bugzilla.mozilla.org/show_bug.cgi?id=324037

The point being, perhaps telemetry for this should focus on the measured browser behavior, not the way reload was called by the user? The rest of the probes proposed do measure behavior, not input/invocation.

If the goal is to measure reload behavior, and not user input preference (toolbar button or function key), then what are the cases that are important? From your reply above, I'm assuming that full-reload and cache-reload are the cases. Are there others? Do extensions/addons matter along with web content?

Certainly, beginning-middle-end type probes for reload behavior sound good. So

(whatever probe measures the type of reload)
TBD

(beginning)
TIME_TO_RELOAD_FIRST_CONTENTFUL_PAINT_MS
TIME_TO_RELOAD_FIRST_INTERACTION_MS

(end)
FX_PAGE_RELOAD_MS

Make sense. Was this what you were thinking as well, or were there other reasons for choosing the *_MS probes to use for reload?

Comment 7

[Corey Dow-Hygelund [:ccd]]

:bdekoz

You are correct in the rationale behind the reload analogs of the *_ms probes.

It makes sense to focus on the browser behavior in regards to the telemetry, given the nature of the other probes and the existing bugs. In that case should we split out the *_ms reload probes into two cases?

• FX_PAGE_RELOAD_CACHE_MS_2
• FX_PAGE_RELOAD_REFETCHED_MS_2
  ...

This seems like valuable information in regards to how reloads perform in the wild, and how to best instrument.

Comment 8

[Benjamin De Kosnik [:bdekoz]]

Yep. Agreed Corey, just drop the _2 suffix as there weren't RELOAD probes we have to redo with this patch. Histograms, both.

;)

Will be very curious to see this tracked in the wild, and see how this varies with device type.

Comment 9

[Sean Feng [:sefeng]]

Attachment: data_collection_review_v2.txt

Comment 10

[Sean Feng [:sefeng]]

Per our conversation, we are going to split reload related telemetry to 2 bugs.

1. Record reload stats from the performance perspective
2. Record related stats to fix UX issues for reload (to address Bug 1468476)

This bug focuses for 1.

bdekoz: I updated the data review collection form, can you please take a look again? Thanks

Comment 11

[Benjamin De Kosnik [:bdekoz]]

Comment on attachment 9065107 [details]
data_collection_review_v2.txt

Thanks Sean! Looks good

Comment 12

[Sean Feng [:sefeng]]

Attachment: Bug 1549519 - Add two histograms as page reload performance telemetry r=bdekoz

Comment 13

[Benjamin De Kosnik [:bdekoz]]

WIP tracks desktop only, via FX_PAGE_LOAD_MS_2 ish approach. To get at the percentage of cache hits for mobile, then this probe should also go in with a GV_PAGE_LOAD_MS ish approach to /mobile/android/chrome/geckoview/GeckoViewProgressChild.js.

Then, mobile and desktop would both have metrics for reload, which seems wise at this point.

(As Sean pointed out, there is only one way to reload on mobile so some of the other wrinkles (now in BZ 1551965) are not present for mobile)

Comment 14

[Sean Feng [:sefeng]]

Attachment: Bug 1549519 - Add page reload telemetry probe for geckoview

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/57e23c9c25d0
Add page reload telemetry probe for geckoview r=bdekoz,geckoview-reviewers,snorp

Comment 16

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/57e23c9c25d0

Comment 17

[Sean Feng [:sefeng]]

Reopen the bug as there's another patch that needs to be landed.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c8a19f92ba0d
Add two histograms as page reload perfromance telemetry r=bdekoz,Gijs

Comment 19

[Dorel Luca [:dluca]]

Backed out changeset c8a19f92ba0d (bug 1549519) for Eslint failure. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=250004122&repo=autoland&lineNumber=278

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=c8a19f92ba0d3f109aa0dbe2055299160d3af6c0

Backout:
https://hg.mozilla.org/integration/autoland/rev/5e7c2d9420574ed055721a9685ff0d048d15399c

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/05de6213944f
Add two histograms as page reload performance telemetry r=bdekoz,Gijs

Comment 21

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/05de6213944f

Comment 22

[Sean Feng [:sefeng]]

Clear my needinfo as everything landed.

Comment 23

[Tim Smith (inactive) 👨‍🔬 [:tdsmith]]

I think that by excluding certain pageloads, this change changed the interpretation of the FX_PAGE_LOAD_MS_2 histogram, without updating the documentation. Please consider letting probe owners and/or [e-mail] know before changing what an existing histogram collects, and please update the bug_numbers and probe descriptions in Histograms.json when you do.

I know this is an older change and that we've talked about this since, but just a note that data-review should be performed by one of the stewards listed at https://wiki.mozilla.org/Firefox/Data_Collection.

Comment 24

[Tim Smith (inactive) 👨‍🔬 [:tdsmith]]

Attachment: Bug 1549519 - Update documentation for FX_PAGE_LOAD_MS_2 r=chutten!,sefeng!

Additionally, stop indicating that we collect this in the content process (we don't).

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/35182e2258ef
Update documentation for FX_PAGE_LOAD_MS_2 r=chutten,sefeng

Comment 26

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/35182e2258ef