Report Navigation Timing into Telemetry

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
Performance Monitoring
RESOLVED FIXED
4 months ago
2 months ago

People

(Reporter: Harald, Assigned: wcpan)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [ps-radar][qf:p1])

Attachments

(2 attachments, 9 obsolete attachments)

8.88 KB, patch
wcpan
: review+
Details | Diff | Splinter Review
3.41 KB, patch
wcpan
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 months ago
To have counterparts to page load benchmarks we need to understand the breakdown of loading. We have new perceived measures landing, like non-blank paint (bug 1307242) but also need to know how it correlates to other milestones on the load lifecycle.

As a start we need probes for (naming per PerformanceTiming + proposed telemetry probe name):

- domLoading - TIME_TO_DOM_LOADING
- domInteractive - TIME_TO_DOM_INTERACTIVE
- domContentLoaded - …
- domComplete
- loadEvent

This should be only measures for the root document.
(Reporter)

Comment 1

4 months ago
:kanru, can your team help with that?
Flags: needinfo?(kchen)
Hi Harald, I'm curious, how do you want to use this data? We can add the corresponding performance timing data to telemetry but you can only compare the averages of the timings. It's probably a good way to have insights into real world numbers and catch regressions but is not helpful for diagnosing particular website.
Flags: needinfo?(kchen) → needinfo?(hkirschner)
Whiteboard: [ps-radar]
(Reporter)

Comment 3

3 months ago
This data would not be collected per site. But as these are measures that are collected by sites out there, we we can reach out to them and compare. This data is also reported by our page load benchmarks, so we need it to tweak the benchmark so it gets as close as possible to users.
Flags: needinfo?(hkirschner)
Whiteboard: [ps-radar] → [ps-radar][qf:p1]
(Reporter)

Comment 4

3 months ago
We might use evented telemetry for that later, but for now just having the distributions for the timings is useful.
Flags: needinfo?(kchen)
(Reporter)

Comment 5

3 months ago
responseStart should be also tracked as time to first byte metric. This only applies to top level document and should not be tracked for sub-resources.
Assignee: nobody → wpan
(Reporter)

Comment 6

3 months ago
Can we prioritize this in the work queue as it blocks setting performance targets. We already have data from benchmarks.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 9

3 months ago
mozreview-review
Comment on attachment 8853035 [details]
Bug 1344893 - Part 1: Report Navigation Timing into Telemetry.

https://reviewboard.mozilla.org/r/125146/#review128194

Horribly sorry that I'm behind my reviews.

::: dom/base/nsDOMNavigationTiming.h:127
(Diff revision 1)
>  
>    void Clear();
>  
> +  bool IsTopLevelContentDocument() const;
> +
> +  nsWeakPtr mDocShell;

mozilla::WeakPtr might make this all  a bit simpler

::: dom/base/nsDOMNavigationTiming.cpp:283
(Diff revision 1)
>    }
>    return 0;
>  }
> +
> +bool
> +nsDOMNavigationTiming::IsTopLevelContentDocument() const

This stuff could perhaps use the GetSameTypeRoot thingie from docshell.
Attachment #8853035 - Flags: review?(bugs)

Comment 10

3 months ago
mozreview-review
Comment on attachment 8853036 [details]
Bug 1344893 - Part 2: Add time to first byte metric.

https://reviewboard.mozilla.org/r/125148/#review128196

Similar stuff here as in the other patch.
And sorry about the delay!


If you update the patches, I can take another look (even if my review queue is closed).
Attachment #8853036 - Flags: review?(bugs)
Created attachment 8853556 [details] [diff] [review]
0001-Bug-1344893-Part-1-Report-Navigation-Timing-into-Tel.patch
Attachment #8853035 - Attachment is obsolete: true
Attachment #8853036 - Attachment is obsolete: true
Created attachment 8853557 [details] [diff] [review]
0001-Bug-1344893-Part-1-Report-Navigation-Timing-into-Tel.patch
Attachment #8853556 - Attachment is obsolete: true
Created attachment 8853558 [details] [diff] [review]
0002-Bug-1344893-Part-2-Add-time-to-first-byte-metric.-r-.patch
Flags: needinfo?(bugs)

Comment 14

3 months ago
Harald, do you need data about DOMContentLoaded start or end time? Same with load event.
I assume so.
Flags: needinfo?(bugs) → needinfo?(hkirschner)

Updated

3 months ago
Attachment #8853557 - Flags: review+

Comment 15

3 months ago
Comment on attachment 8853558 [details] [diff] [review]
0002-Bug-1344893-Part-2-Add-time-to-first-byte-metric.-r-.patch

oh, PerformanceTiming::PerformanceTiming is a bit hard to follow.
One needs to know that aHttpChannel is null for subresources.
Could you add a comment about why to check !aHttpChannel.

But I don't quite understand this.
We create PerformanceTiming only if needed. If the page never accesses .performance, nothing is added to telemetry, assuming I read the code right.

Am I missing something?
Attachment #8853558 - Flags: review-
(Reporter)

Comment 16

3 months ago
> Harald, do you need data about DOMContentLoaded start or end time? Same with load event.

With the multitude of discussions about diagnostic metrics in mind, I would probably collect the start-end durations for respective. This assumes engineers would benefit from this, as those durations are otherwise only slightly helpful to defer perceived performance but definitely help with tracking specific improvements.
Flags: needinfo?(hkirschner)
(In reply to Olli Pettay [:smaug] (review queue closed. Please ask reviews from other DOM peers) from comment #15)
> Comment on attachment 8853558 [details] [diff] [review]
> 0002-Bug-1344893-Part-2-Add-time-to-first-byte-metric.-r-.patch
> 
> oh, PerformanceTiming::PerformanceTiming is a bit hard to follow.
> One needs to know that aHttpChannel is null for subresources.
> Could you add a comment about why to check !aHttpChannel.
> 
> But I don't quite understand this.
> We create PerformanceTiming only if needed. If the page never accesses
> .performance, nothing is added to telemetry, assuming I read the code right.
> 
> Am I missing something?

I think we will create PerformanceTiming for each HttpChannel:
https://dxr.mozilla.org/mozilla-central/rev/720b9177c6856c1c4339d0fac1bf5149c0d53950/netwerk/protocol/http/HttpChannelChild.cpp#1027

Another concern is that if dom.enable_performance is disabled, the value will be zero.
I should also check the flag to avoid invalid data.
Since the pref is enabled by default, this should not affect probe collecting too much.

Comment 18

3 months ago
aha, I couldn't have imagined HttpChannel to make that call. 

And yes, would be good to not report non-valid data.
Created attachment 8855245 [details] [diff] [review]
0001-Bug-1344893-Part-1-Report-Navigation-Timing-into-Tel.patch

Rebase for latest m-c, nothing changed, carry over r=smaug.
Attachment #8853557 - Attachment is obsolete: true
Attachment #8853558 - Attachment is obsolete: true
Attachment #8855245 - Flags: review+
Created attachment 8855246 [details] [diff] [review]
0002-Bug-1344893-Part-2-Add-time-to-first-byte-metric.-r-.patch

1. added #include "nsITimedChannel.h" because it could not compile without unified build
2. changed the comment for !aHttpChannel
3. added pref checking
Flags: needinfo?(bugs)

Comment 21

3 months ago
(In reply to :Harald Kirschner :digitarald from comment #16)
> > Harald, do you need data about DOMContentLoaded start or end time? Same with load event.
> 
> With the multitude of discussions about diagnostic metrics in mind, I would
> probably collect the start-end durations for respective. This assumes
> engineers would benefit from this, as those durations are otherwise only
> slightly helpful to defer perceived performance but definitely help with
> tracking specific improvements.

Not quite sure what you mean with the comment. You want to track both start and end?
Flags: needinfo?(bugs) → needinfo?(hkirschner)

Comment 22

3 months ago
Comment on attachment 8855246 [details] [diff] [review]
0002-Bug-1344893-Part-2-Add-time-to-first-byte-metric.-r-.patch

>@@ -47,6 +52,15 @@ PerformanceTiming::PerformanceTiming(Performance* aPerformance,
>   }
> 
>   InitializeTimingInfo(aChannel);
>+
>+  // The aHttpChannel argument is null implies that this PerformanceTiming
>+  // object is being used for subresources, which is not relevant to this probe.
>+  if (!aHttpChannel &&
>+      nsContentUtils::IsPerformanceTimingEnabled() &&
>+      IsTopLevelContentDocument()) {
Isn't the comment backwards. aHttpChannel is non-null implies subresources.


But please wait for Harald's feedback on what data we want to collect here before landing.
Attachment #8855246 - Flags: review+
Harald, could you take a look at comment 21?
Flags: needinfo?(kchen)
(Reporter)

Comment 24

2 months ago
(In reply to Olli Pettay [:smaug] from comment #21)
> (In reply to :Harald Kirschner :digitarald from comment #16)
> > > Harald, do you need data about DOMContentLoaded start or end time? Same with load event.
> > 
> > With the multitude of discussions about diagnostic metrics in mind, I would
> > probably collect the start-end durations for respective. This assumes
> > engineers would benefit from this, as those durations are otherwise only
> > slightly helpful to defer perceived performance but definitely help with
> > tracking specific improvements.
> 
> Not quite sure what you mean with the comment. You want to track both start
> and end?

Yes, we should track start and end.
Flags: needinfo?(hkirschner)
(In reply to :Harald Kirschner :digitarald from comment #24)
> Yes, we should track start and end.

So we not only want to track "from navigation start to all load events end", but also "from navigation start to the first load event start"?
Flags: needinfo?(hkirschner)
(Reporter)

Comment 26

2 months ago
The 4 affected timings are:

- domContentLoadedEventStart
- domContentLoadedEventEnd
- loadEventStart
- loadEventEnd
Flags: needinfo?(hkirschner)
Created attachment 8859060 [details] [diff] [review]
0001-Bug-1344893-Part-1-Report-Navigation-Timing-into-Tel.patch

Add two probes to meature domContentLoadedEventStart and loadEventStart. (due to comment 26)

No other code changed, carry over r=smaug.
Attachment #8855245 - Attachment is obsolete: true
Attachment #8855246 - Attachment is obsolete: true
Attachment #8859060 - Flags: review+
Created attachment 8859062 [details] [diff] [review]
0002-Bug-1344893-Part-2-Add-time-to-first-byte-metric.-r-.patch

Correct comment mentioned in comment 22. Carry over r=smaug.
Attachment #8859062 - Flags: review+
Keywords: checkin-needed
Keywords: checkin-needed
Comment on attachment 8859060 [details] [diff] [review]
0001-Bug-1344893-Part-1-Report-Navigation-Timing-into-Tel.patch

Request approval for adding probes.
Attachment #8859060 - Flags: feedback?(benjamin)
Comment on attachment 8859062 [details] [diff] [review]
0002-Bug-1344893-Part-2-Add-time-to-first-byte-metric.-r-.patch

Request approval for adding probes.
Attachment #8859062 - Flags: feedback?(benjamin)
Comment on attachment 8859060 [details] [diff] [review]
0001-Bug-1344893-Part-1-Report-Navigation-Timing-into-Tel.patch

I have deep skepticism that this will be valuable telemetry, but I'm willing to collect it so Harald can experiment with it and demonstrate its value. data-r=me
Attachment #8859060 - Flags: feedback?(benjamin) → feedback+
Comment on attachment 8859062 [details] [diff] [review]
0002-Bug-1344893-Part-2-Add-time-to-first-byte-metric.-r-.patch

data-r=me
Attachment #8859062 - Flags: feedback?(benjamin) → feedback+
Created attachment 8859489 [details] [diff] [review]
0001-Bug-1344893-Part-1-Report-Navigation-Timing-into-Tel.patch

Add data-review=bsmedberg. Carry over r=smaug.
Attachment #8859060 - Attachment is obsolete: true
Attachment #8859062 - Attachment is obsolete: true
Attachment #8859489 - Flags: review+
Created attachment 8859490 [details] [diff] [review]
0002-Bug-1344893-Part-2-Add-time-to-first-byte-metric.-r-.patch

Add data-review=bsmedberg. Carry over r=smaug.
Attachment #8859490 - Flags: review+
Keywords: checkin-needed

Comment 35

2 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3559898682b9
Part 1: Report Navigation Timing into Telemetry. r=smaug, data-review=bsmedberg
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e22ac1beeda
Part 2: Add time to first byte metric. r=smaug, data-review=bsmedberg
Keywords: checkin-needed

Comment 36

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3559898682b9
https://hg.mozilla.org/mozilla-central/rev/3e22ac1beeda
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.