Initial resource timing integration. #1185

noamr · 2021-03-02T14:56:17Z

Created a "fetch timing info" struct to hold the bookkeeping necessary for resource timing.

Populated it with some of the required values, leaving some of them for later patches as this is a big undertaking.

See w3c/resource-timing#252

Preview | Diff

noamr · 2021-03-02T15:00:33Z

@annevk @yoavweiss

fetch.bs

"global" is a term already used to describe the global object, while this monotonic clock is shared across globals (e.g. workers and the window). Also expose "current universal time", for use by Fetch. See whatwg/fetch#1185

fetch.bs

annevk

It would help a lot if you checked the final diff and read through it at least once to make sure there are no obvious mistakes there. At least I personally tend to spot a lot of mistakes when doing that in my own writing.

Overall I think this approach looks good. I'm a little worried about it also relying on the "done flag" badness. Perhaps the solution there is to accept that implementations are not full duplex and so once the response stream is closed it's really all over.

One thing I haven't done in detail is ensuring that the point of capture matches RT and matches implementations. For that it would be helpful to have some pointers to tests. As I also noted I would prefer to not define fields we're unsure of or that can be inferred from other fields.

(I also do not see transfer size, was that omitted intentionally?)

fetch.bs

noamr · 2021-03-07T06:56:32Z

It would help a lot if you checked the final diff and read through it at least once to make sure there are no obvious mistakes there. At least I personally tend to spot a lot of mistakes when doing that in my own writing.

Sorry for that. I will double check next time.

Overall I think this approach looks good. I'm a little worried about it also relying on the "done flag" badness. Perhaps the solution there is to accept that implementations are not full duplex and so once the response stream is closed it's really all over.

One thing I haven't done in detail is ensuring that the point of capture matches RT and matches implementations. For that it would be helpful to have some pointers to tests. As I also noted I would prefer to not define fields we're unsure of or that can be inferred from other fields.

(I also do not see transfer size, was that omitted intentionally?)

Yes. transfer size will become encoded body size + some constant number, and that will be done in RT.

noamr · 2021-03-08T15:16:57Z

See issue for adding connection tests: web-platform-tests/wpt#27935

fetch.bs

annevk

There's still a lot of style issues left that I mostly tried to read over. I left some questions for things that are unclear. I checked some of the timings, but that could use another look as well. Is @yoavweiss also looking at those?

fetch.bs

annevk · 2021-03-10T12:57:34Z

I would like to see a test for a synthetic response from a service worker. Per the current set of definitions most fields would end up being 0, but is that what implementations do?

Similarly, testing data: URLs and blob: URLs might also highlight some problems.

fetch.bs

Co-authored-by: Anne van Kesteren <annevk@annevk.nl>

annevk · 2021-03-24T15:52:49Z

fetch.bs

+
+ <li><p>If <var>timingInfo</var>'s <a for="fetch timing info">redirect start time</a> is not 0, then
+ set <var>startTime</var> to <var>timingInfo</var>'s
+ <a for="fetch timing info">redirect start time</a>.


Sorry I missed this.

Why would we do this for the case where we cannot reveal information?

Not sure what you're asking. The redirect start time is not revealing, as it occurs before the first redirect and keeps all the redirect info obscure.

But doesn't that reveal there was a redirect?

No. It sets startTime to be the redirect start time, so it exposes a startTime, and the caller doesn't know that its from a redirect

If I do performance.now() and then invoke fetch() it seems I would be able to tell quite easily it's not the actual start time and instead something from the network.

w3c/resource-timing#260 :)
This PR actually fixes that, as startTime would mean redirectStart, the time before the first redirect, and not fetchStart, the time after the last redirect.

noamr · 2021-03-24T17:10:22Z

Commit message:

Take note of the different timings required for the resource timing spec.

The particular points where time is measured is based on the current Resource Timing spec, and the missing parts reflect current implementations:

fetchStart, requestStart, responseStart, redirectStart, redirectEnd, workerStart, encodedBodySize and decodedBodySize are handled as part of the FETCH processing model
the rest of the metrics are handled as requirements given to the HTTP connection obtaining processing model, which is not in itself part of the FETCH spec.

Note that FETCH doesn't decide when to enqueue the resource timing entry, but rather creates the data structure that holds the different timings, and it's up to the caller to enqueue. FETCH sends a new event, processResponseDone, when the response is closed or EOFs, which lets callers like the fetch() API enqueue the entry at the right time.
This change also makes that call as part of fetch().

It's the first phase for addressing w3c/resource-timing#252, which contains a link to the other required multi-repo steps to address RT/NT/HTML/Fetch integration.

annevk · 2021-03-25T08:38:30Z

That description misses: startTime and responseEnd.

annevk · 2021-03-25T08:52:28Z

Outstanding issues:

start time isn't set
if TAO isn't there we should reset to start time, not fetch start time
I think we should rename fields to more accurately capture their semantics:
- fetch start time -> final fetch start time (even that isn't super clear, but I'm hesitant to use final main fetch start time; one thing that is weird with final fetch start time is that with a 401 scenario it wouldn't capture the time the subsequent authenticated fetch started, as that doesn't go through main fetch)
- request start time -> network request start time (it's 0 if there's a service worker involved)
- response start time -> network response start time (it's 0 if there's a service worker involved)
- response end time -> end time (aligns with start time and is clearer if there's a service worker involved as it'll be non-0)

noamr · 2021-03-25T08:58:16Z

Outstanding issues:

start time isn't set

if TAO isn't there we should reset to start time, not fetch start time

Not sure what you mean. when TAO isn't there we reset both fetch start time and start time, to avoid exposing the fact that there were redirects.

I think we should rename fields to more accurately capture their semantics:

fetch start time -> final fetch start time (even that isn't super clear, but I'm hesitant to use final main fetch start time; one thing that is weird with final fetch start time is that with a 401 scenario it wouldn't capture the time the subsequent authenticated fetch started, as that doesn't go through main fetch)

Right, it would capture that start of that fetch, as 401 is not exactly a redirect. It's more like a "post-redirect fetch start time".

request start time -> network request start time (it's 0 if there's a service worker involved)

response start time -> network response start time (it's 0 if there's a service worker involved)

response end time -> end time (aligns with start time and is clearer if there's a service worker involved as it'll be non-0)

OK. I don't mind the renames

annevk · 2021-03-25T09:04:44Z

In https://whatpr.org/fetch/1185.html#finalize-and-report-timing step 5 we initialize startTime to fetch start time. At that point fetch start time is not guaranteed to be equal to start time, right? Then step 6 compensates for that. Then step 7 does the reset. But why not initialize startTime to start time and drop step 6? It seems that would be a lot clearer.

I really like post-redirect fetch start time by the way, let's use that! Or maybe even post-redirect start time. No need to mention fetch.

yoavweiss

Still LGTM

I like the new names, even if they diverge from the web exposed ones, as they are indeed more descriptive.

Create a "document load timing info" struct, associated with a document, which receives its information from several sources: - fetch timing of the navigation response - Load/DOMContentLoaded event dispatches - Document readyness state changes - sending unload events to the previous document, if it comes from the same origin. See w3c/navigation-timing#136 Depends on whatwg/fetch#1185

When response is a network error or body is fully read, finalize and report the response. Depends on whatwg/fetch#1185 See: w3c/resource-timing#261 and w3c/resource-timing#252

When response is a network error or body is fully read, finalize and report the response. Depends on whatwg/fetch#1185. Also see w3c/resource-timing#261 and w3c/resource-timing#252.

yoavweiss reviewed Mar 2, 2021

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

yoavweiss reviewed Mar 2, 2021

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

noamr mentioned this pull request Mar 3, 2021

Change "global monotonic clock" to "shared" and expose new algorithm "unsafe shared current time" w3c/hr-time#107

Merged

yoavweiss reviewed Mar 3, 2021

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

yoavweiss reviewed Mar 3, 2021

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

yoavweiss reviewed Mar 3, 2021

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

annevk reviewed Mar 4, 2021

View reviewed changes

noamr force-pushed the fetch-resource-timing-initial branch 3 times, most recently from 7496b4a to d0e2408 Compare March 7, 2021 15:24

noamr force-pushed the fetch-resource-timing-initial branch 3 times, most recently from e1a3e17 to 526a3d1 Compare March 8, 2021 17:07

noamr commented Mar 8, 2021

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

noamr mentioned this pull request Mar 9, 2021

[resource-timing] Add a test for encodedBodySize with no Content-Length header web-platform-tests/wpt#27952

Closed

annevk reviewed Mar 9, 2021

View reviewed changes

yoavweiss reviewed Mar 9, 2021

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

noamr force-pushed the fetch-resource-timing-initial branch 3 times, most recently from 0538a2f to 10cb532 Compare March 9, 2021 16:11

noamr force-pushed the fetch-resource-timing-initial branch 5 times, most recently from b19b46e to a96f625 Compare March 11, 2021 11:48

noamr and others added 7 commits March 24, 2021 13:17

Fix nit

cb1fb7c

Added a note about 10x

699875f

nits + TODOs

dddf4e0

address CI issues

470e9cf

rename worker start and clean up ALPN Protocol ID

b111631

coarsen times early

21e258c

put connection timings under Connections

8fddbb6

annevk reviewed Mar 24, 2021

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

Simplify redirects

b37ef51

noamr force-pushed the fetch-resource-timing-initial branch from 903f911 to b37ef51 Compare March 24, 2021 11:25

noamr and others added 2 commits March 24, 2021 13:32

Update fetch.bs

ce317dc

Co-authored-by: Anne van Kesteren <annevk@annevk.nl>

trigger GitHub actions

2b6e0cc

annevk reviewed Mar 24, 2021

View reviewed changes

annevk added 2 commits March 25, 2021 11:31

Address remaining issues

52fd63b

nits

66772da

yoavweiss approved these changes Mar 25, 2021

View reviewed changes

annevk merged commit 73f01f7 into whatwg:main Mar 25, 2021

annevk mentioned this pull request Mar 25, 2021

Current definition of startTime exposes total redirect time even when TAO fails w3c/resource-timing#260

Closed

This was referenced Mar 29, 2021

4.6 Integrate the processing model with Fetch w3c/resource-timing#252

Closed

Create resource timing entries for HTML resources whatwg/html#6542

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial resource timing integration. #1185

Initial resource timing integration. #1185

noamr commented Mar 2, 2021 •

edited by pr-preview bot

Loading

noamr commented Mar 2, 2021

annevk left a comment

noamr commented Mar 7, 2021

noamr commented Mar 8, 2021

annevk left a comment

annevk commented Mar 10, 2021 •

edited

Loading

annevk Mar 24, 2021

noamr Mar 24, 2021

annevk Mar 24, 2021

noamr Mar 24, 2021

annevk Mar 24, 2021

noamr Mar 24, 2021

noamr commented Mar 24, 2021 •

edited

Loading

annevk commented Mar 25, 2021

annevk commented Mar 25, 2021 •

edited

Loading

noamr commented Mar 25, 2021 •

edited

Loading

annevk commented Mar 25, 2021 •

edited

Loading

yoavweiss left a comment

Initial resource timing integration. #1185

Initial resource timing integration. #1185

Conversation

noamr commented Mar 2, 2021 • edited by pr-preview bot Loading

noamr commented Mar 2, 2021

annevk left a comment

Choose a reason for hiding this comment

noamr commented Mar 7, 2021

noamr commented Mar 8, 2021

annevk left a comment

Choose a reason for hiding this comment

annevk commented Mar 10, 2021 • edited Loading

annevk Mar 24, 2021

Choose a reason for hiding this comment

noamr Mar 24, 2021

Choose a reason for hiding this comment

annevk Mar 24, 2021

Choose a reason for hiding this comment

noamr Mar 24, 2021

Choose a reason for hiding this comment

annevk Mar 24, 2021

Choose a reason for hiding this comment

noamr Mar 24, 2021

Choose a reason for hiding this comment

noamr commented Mar 24, 2021 • edited Loading

annevk commented Mar 25, 2021

annevk commented Mar 25, 2021 • edited Loading

noamr commented Mar 25, 2021 • edited Loading

annevk commented Mar 25, 2021 • edited Loading

yoavweiss left a comment

Choose a reason for hiding this comment

noamr commented Mar 2, 2021 •

edited by pr-preview bot

Loading

annevk commented Mar 10, 2021 •

edited

Loading

noamr commented Mar 24, 2021 •

edited

Loading

annevk commented Mar 25, 2021 •

edited

Loading

noamr commented Mar 25, 2021 •

edited

Loading

annevk commented Mar 25, 2021 •

edited

Loading