1794652 - Consider removing "insufficient" page load timing probes like TIME_TO_*

Status: NEW

(Core :: Performance Engineering, task, P3)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

In the commit summary for bug 1671729, :denispal suggests:

Current page load telemetry probes are insufficient in performance RUM testing. FX_PAGE_LOAD_MS_2 will stop the timer when the user switches tabs or navigates off the page, while the current navigation probes [ed: including TIME_TO_*] include all content including about:blank, about:newtab, moz-extension, etc.

:denispal implemented PERF_* probes to replace them.

Looking at the bugs listed for the implementation of TIME_TO_DOM_COMPLETE_MS – bug 1344893, bug 1489524, bug 1580077 – this probe appears to have been initially implemented for performance monitoring purposes so I speculate the others are as well.

If, as :denispal suggests, these probes are insufficient for performance RUM, are they useful to anyone? We should answer that question and consider removing them. The most important reason is to avoid confusion when analyzing performance telemetry. There are currently 11 TIME_TO_* probes, plus some FX_* probes I didn't look into.

If we remove them, make sure we have appropriate replacements for all of the measurements we want to take. This is especially true for Android where probes are not transmitted by default: we have to explicitly make sure the replacement probes are exported.

---

:denispal, does this seem like a reasonable thing to do?

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

If we want to investigate removing them but we don't have time to to do so, it might be useful to link to this issue from the description in each of the probes so someone using those probes has a chance of being aware of this context.

Comment 2

[Denis Palmeiro [:denispal]]

I think it's fair to remove these.

Comment 3

[Florian Quèze [:florian]]

(In reply to Denis Palmeiro [:denispal] from comment #2)

I think it's fair to remove these.

Do you have a list in mind of which probes are removable? I'm willing to do the patches to remove them if someone can give me a list of the probes to remove.

And more specifically, can we remove FX_REFRESH_DRIVER_SYNC_SCROLL_FRAME_DELAY_MS? I'm interested specifically in that one because it is the only one that uses the setHistogramRecordingEnabled method of nsITelemetry, and recording often to a disabled probe will cause noisy profiler markers with my patch from bug 1917586.

Comment 4

[Denis Palmeiro [:denispal]]

(In reply to Florian Quèze [:florian] from comment #3)

(In reply to Denis Palmeiro [:denispal] from comment #2)

I think it's fair to remove these.

Do you have a list in mind of which probes are removable? I'm willing to do the patches to remove them if someone can give me a list of the probes to remove.

And more specifically, can we remove FX_REFRESH_DRIVER_SYNC_SCROLL_FRAME_DELAY_MS? I'm interested specifically in that one because it is the only one that uses the setHistogramRecordingEnabled method of nsITelemetry, and recording often to a disabled probe will cause noisy profiler markers with my patch from bug 1917586.

Looking over the existing probes, these ones seem not useful anymore. Either nobody is looking at them, or we have duplicates for many of them already. However, I should probably ask around first if they are still useful to anyone.

HTTP3_PERF_PAGE_LOAD_TIME_MS
HTTP3_PERF_FIRST_CONTENTFUL_PAINT_MS

H3P_PERF_PAGE_LOAD_TIME_MS
H3P_PERF_FIRST_CONTENTFUL_PAINT_MS

FX_PAGE_LOAD_MS_2
FX_PAGE_RELOAD_NORMAL_MS

PERF_REQUEST_ANIMATION_CALLBACK_NON_PAGELOAD_MS
PERF_REQUEST_ANIMATION_CALLBACK_PAGELOAD_MS

TIME_TO_NON_BLANK_PAINT_MS
TIME_TO_FIRST_CONTENTFUL_PAINT_MS
TIME_TO_DOM_LOADING_MS
TIME_TO_DOM_INTERACTIVE_MS
TIME_TO_DOM_CONTENT_LOADED_START_MS
TIME_TO_DOM_CONTENT_LOADED_END_MS
TIME_TO_DOM_COMPLETE_MS
TIME_TO_LOAD_EVENT_START_MS
TIME_TO_LOAD_EVENT_END_MS

GV_PAGE_LOAD_PROGRESS_MS
GV_PAGE_LOAD_MS
GV_PAGE_RELOAD_MS

I have never used FX_REFRESH_DRIVER_SYNC_SCROLL_FRAME_DELAY_MS, but maybe the gfx team is still using it.

Comment 5

[Florian Quèze [:florian]]

(In reply to Denis Palmeiro [:denispal] from comment #4)

I have never used FX_REFRESH_DRIVER_SYNC_SCROLL_FRAME_DELAY_MS, but maybe the gfx team is still using it.

Jeff, do you know if this probe is still in active use? (as I explained in comment 3, it is implemented in a way that requires a custom handling by telemetry, so it would be nice to remove or replace the implementation of that probe to be able to remove the custom handling that can't be migrated to Glean.)

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Botond, do we still do sync scrolls? Can you imagine us caring about this metric?

Comment 7

[Botond Ballo [:botond]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #6)

Botond, do we still do sync scrolls?

There are cases where we still run a smooth scroll animation on the main thread. Bug 1799238 is an example. That said...

Can you imagine us caring about this metric?

... I wasn't even aware this metric existed. Hiro and I chatted about it briefly on Matrix -- based on that discussion, the metric doesn't seem particularly useful in its current form and we'd be fine with removing it.