Call `finalize and report timing` automatically on end-of-body #1413

noamr · 2022-03-16T14:36:55Z

Closes #1208
Closes w3c/navigation-timing#131

A new method called conclude reports the timing for the resource.
The timing info used is the one attached to the fetch rather than
one attached to a response.

This is done to reduce ambiguities when timing info is attached to
the response, as it's unclear what happens to that timing info
when a response is cloned, reused, serialized or stored & restored
from cache.

Call-site patches:

Update resource/navigation to use conclude / resource-info: whatwg/html#7722
Update XHR to use conclude: whatwg/xhr#347
Update CSS to use conclude: w3c/csswg-drafts#7355
Update beacon to use conclude: w3c/beacon#75
Update resource timing to use resource info: w3c/resource-timing#321
Update navigation timing to use resource info: https://github.com/w3c/navigation-timing/pull/1760

(See WHATWG Working Mode: Changes for more details.)

Preview | Diff

noamr · 2022-03-16T14:39:13Z

(Note that this is a breaking change, not sure if to leave finalize response with a deprecated ref to avoid build breakage until refs are fixed? or commit this together with caller fixes?

annevk · 2022-03-18T13:59:09Z

I would prefer it if we attempt to swap it all at once. And we should also try to preserve working ID. Removing references seem fine as ideally we'd fix all of those as part of the swap.

noamr · 2022-03-18T14:40:35Z

I would prefer it if we attempt to swap it all at once. And we should also try to preserve working ID. Removing references seem fine as ideally we'd fix all of those as part of the swap.

OK, I'll add an alternate ID and work on the corresponding HTML/CSS/XHR patches

Depends on whatwg/fetch#1413 This clarifies that a resource timing entry is always for a fetch and not for a request or response.

Depends on whatwg/fetch#1413 This clarifies that resource-timing reports are specific to a fetch and not to a request or a response.

noamr · 2022-03-20T13:25:12Z

@annevk: I posted 6 related PRs...

Depends on whatwg/fetch#1413 This clarifies that resource-timing reports are specific to a fetch and not to a request or a response.

annevk

This generally looks like an improvement to me, but there's a couple of things that still need to be worked out.

fetch.bs

noamr · 2022-05-03T08:02:05Z

Another review, @annevk ?

annevk

As I mentioned on Matrix, I think it would be good if someone else looked at this before I give it a final pass. Maybe @yutakahirano is interested?

fetch.bs

yutakahirano · 2022-05-12T12:16:15Z

As I mentioned on Matrix, I think it would be good if someone else looked at this before I give it a final pass. Maybe @yutakahirano is interested?

Are you looking for a pure code review, or do you want to know how likely Chrome implements the spec change?

noamr · 2022-05-12T12:54:06Z

As I mentioned on Matrix, I think it would be good if someone else looked at this before I give it a final pass. Maybe @yutakahirano is interested?

Are you looking for a pure code review, or do you want to know how likely Chrome implements the spec change?

The current spec change is purely editorial but will allow subsequent behavior-affecting changes, such as defining how encoded/decoded body size works when service workers are involved

fetch.bs

yutakahirano · 2022-05-18T05:05:24Z

I suspect you've already discussed this, but I'm not very comfortable with allowing / asking external users (xhr, html, etc) to control controller's state (abort is an exception). Is it possible / desirable to call "conclude" in the fetch spec and allow external users to set parameters (to fetch params, for example)?

noamr · 2022-05-18T05:52:26Z

I suspect you've already discussed this, but I'm not very comfortable with allowing / asking external users (xhr, html, etc) to control controller's state (abort is an exception). Is it possible / desirable to call "conclude" in the fetch spec and allow external users to set parameters (to fetch params, for example)?

Yes it's been discussed months ago... Also terminating is something that external specs can do.
In most cases it's possible for fetch to report timing on its own, but in some cases the caller decides to avoid reporting or to report a network error based on the response. For example, script fetching checks for particular content-types, and style subresources are not reported when fetched from a cross-origin imported stylesheet.

We chose to do it this way because then it's clear when the reporting happens relative to other observable behaviors such as element load/error events.
This is actually not new in this PR, before that the concluded state was request's "done" flag which is set from outside in finalize and report timing - here I'm attempting to make this more readable and explicit.

noamr · 2022-05-18T06:28:11Z

I suspect you've already discussed this, but I'm not very comfortable with allowing / asking external users (xhr, html, etc) to control controller's state (abort is an exception). Is it possible / desirable to call "conclude" in the fetch spec and allow external users to set parameters (to fetch params, for example)?

Yes it's been discussed months ago... Also terminating is something that external specs can do. In most cases it's possible for fetch to report timing on its own, but in some cases the caller decides to avoid reporting or to report a network error based on the response. For example, script fetching checks for particular content-types, and style subresources are not reported when fetched from a cross-origin imported stylesheet.

We chose to do it this way because then it's clear when the reporting happens relative to other observable behaviors such as element load/error events. This is actually not new in this PR, before that the concluded state was request's "done" flag which is set from outside in finalize and report timing - here I'm attempting to make this more readable and explicit.

One thing I'm open to is to separate timing-reporting from the state change - allow fetch to set the "concluded" state on error/EOF but leave the reporting to the caller, which can only happen post-conclude. @annevk WDYT?

yutakahirano · 2022-05-18T13:52:05Z

One thing I'm open to is to separate timing-reporting from the state change - allow fetch to set the "concluded" state on error/EOF but leave the reporting to the caller, which can only happen post-conclude.

That sounds great to me.

noamr · 2022-05-18T13:53:57Z

One thing I'm open to is to separate timing-reporting from the state change - allow fetch to set the "concluded" state on error/EOF but leave the reporting to the caller, which can only happen post-conclude.

That sounds great to me.

@annevk does that sound ok to you?

annevk · 2022-05-23T16:27:17Z

Don't we set the "done flag" in "fetch response handover" today? Currently "finalize and report timing" does not make any state changes of that sort that I can see and it's called from one of the callback algorithms, at least in fetch(), which is what I'd expect.

noamr · 2022-06-08T14:43:18Z

Could you please triage the comment threads? Reading through the "Conversation" tab it's rather unclear what is still outstanding and you want a reviewer to have another look at and have them resolve and what you consider addressed.

Triaged again, I think all comments have been handled.

noamr · 2022-06-14T07:51:15Z

@annevk: ping for review?

annevk

Thanks, I think we're close, but there's at least one somewhat involved problem left.

fetch.bs

annevk · 2022-06-14T09:07:59Z

Triaged again, I think all comments have been handled.

To be clear, in the future please either resolve them (if you're a 100% sure) or add a comment (if you'd like another pass on that comment thread). I've now gone and resolved the older comments, but help with that is appreciated.

noamr · 2022-06-14T09:09:19Z

Triaged again, I think all comments have been handled.

To be clear, in the future please either resolve them (if you're a 100% sure) or add a comment (if you'd like another pass on that comment thread). I've now gone and resolved the older comments, but help with that is appreciated.

Sure, will do. Thanks!

annevk · 2022-06-14T12:05:49Z

Tentative commit message:

Inline "finalize and report timing"

As long as fetch callers pass in the necessary data through the request concept, they will not have to make additional calls to get timing reported accurately. Note that this does not work if callers want to use useParallelQueue.

Downstream PRs:
* https://github.com/whatwg/html/pull/7722
* https://github.com/whatwg/xhr/pull/347
* https://github.com/w3c/csswg-drafts/pull/7160
* https://github.com/w3c/beacon/pull/75
* https://github.com/w3c/resource-timing/pull/321
* https://github.com/w3c/navigation-timing/pull/1760

Closes #1208 and closes https://github.com/w3c/navigation-timing/pull/131.

annevk

@yutakahirano any final thoughts on this?

noamr · 2022-06-14T13:06:16Z

Tentative commit message:


Inline "finalize and report timing"



As long as fetch callers pass in the necessary data through the request concept, they will not have to make additional calls to get timing reported accurately. Note that this does not work if callers want to use useParallelQueue.



Downstream PRs:

* https://github.com/whatwg/html/pull/7722

* https://github.com/whatwg/xhr/pull/347

* https://github.com/w3c/csswg-drafts/pull/7160

* https://github.com/w3c/beacon/pull/75

* https://github.com/w3c/resource-timing/pull/321

* https://github.com/w3c/navigation-timing/pull/1760



Closes #1208 and closes https://github.com/w3c/navigation-timing/pull/131.

Looks good! Should I update the OP or unnecessary?

annevk · 2022-06-14T14:50:19Z

No need, I'll give @yutakahirano a couple more days before I merge this.

See whatwg/fetch#1413 Fetch now reports timing automatically if initiator is passed.

noamr · 2022-06-14T17:04:58Z

Tentative commit message:

Inline "finalize and report timing"

As long as fetch callers pass in the necessary data through the request concept, they will not have to make additional calls to get timing reported accurately. Note that this does not work if callers want to use useParallelQueue.

Downstream PRs:
* https://github.com/whatwg/html/pull/7722
* https://github.com/whatwg/xhr/pull/347
* https://github.com/w3c/csswg-drafts/pull/7160
* https://github.com/w3c/beacon/pull/75
* https://github.com/w3c/resource-timing/pull/321
* https://github.com/w3c/navigation-timing/pull/1760

Closes #1208 and closes https://github.com/w3c/navigation-timing/pull/131.

The correct downstream link for CSS is w3c/csswg-drafts#7355

yutakahirano

Still LGTM.

yutakahirano · 2022-06-15T08:37:34Z

fetch.bs

+     <a for="request">initiator type</a> is non-null and <var>fetchParams</var>'s
+     <a for="fetch params">request</a>'s <a for=request>client</a> is non-null, then run
+     <var>fetchParams</var>'s <a for="fetch params">controller</a>'s
+     <a for="fetch controller">report timing steps</a> given <var>fetchParams</var>'s


We need to give only one argument ("global") to report timing steps, right?

Right, removed extra arg, good catch

See whatwg/fetch#1413 Fetch now reports timing automatically if initiator is passed.

Depends on whatwg/fetch#1413.

noamr mentioned this pull request Mar 16, 2022

Followup: controller #1412

Open

4 tasks

noamr force-pushed the controller-conclude branch from d39af3e to 41bc255 Compare March 16, 2022 14:38

noamr added a commit to noamr/xhr that referenced this pull request Mar 20, 2022

Fetch: Use conclude instead of finalize

b3480b5

Depends on whatwg/fetch#1413 This clarifies that a resource timing entry is always for a fetch and not for a request or response.

noamr mentioned this pull request Mar 20, 2022

Editorial: Refactor for auto-reporting timing from fetch whatwg/xhr#347

Merged

noamr added a commit to noamr/html that referenced this pull request Mar 20, 2022

Fetch: Use 'conclude' instead of 'finalize'

23c967b

Depends on whatwg/fetch#1413 This clarifies that resource-timing reports are specific to a fetch and not to a request or a response.

This was referenced Mar 20, 2022

Editorial: refactor to depend on automatic timing reporting from fetch whatwg/html#7722

Closed

Pass response body info through w3c/navigation-timing#176

Merged

noamr mentioned this pull request Mar 20, 2022

workerStart and redirects w3c/navigation-timing#131

Open

noamr force-pushed the controller-conclude branch from 3f27782 to c18e61a Compare March 20, 2022 14:22

This was referenced Mar 21, 2022

Use response body info instead of timing info for body sizes w3c/resource-timing#321

Merged

[css-values] Use fetch 'conclude' instead of 'finalize' w3c/csswg-drafts#7160

Merged

noamr added a commit to noamr/html that referenced this pull request Apr 5, 2022

Fetch: Use 'conclude' instead of 'finalize'

cb21c61

Depends on whatwg/fetch#1413 This clarifies that resource-timing reports are specific to a fetch and not to a request or a response.

annevk reviewed Apr 20, 2022

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

whatwg deleted a comment from hamad8395 Apr 21, 2022

noamr force-pushed the controller-conclude branch from d497c09 to 4df5920 Compare April 22, 2022 07:46

annevk reviewed May 11, 2022

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

yutakahirano reviewed May 17, 2022

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

Associate initiator type with request

43be574

noamr force-pushed the controller-conclude branch from 9f14e94 to 43be574 Compare June 8, 2022 14:35

Spell out initiator-type values

448f3ae

annevk reviewed Jun 14, 2022

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Show resolved Hide resolved

fetch.bs Show resolved Hide resolved

noamr mentioned this pull request Jun 14, 2022

Add initiatorType values to CSP/destination table #1452

Open

noamr and others added 3 commits June 14, 2022 10:17

Change normative back to note

53f81ca

Dedup end-of-body task

c5614de

nits

ff7190c

annevk approved these changes Jun 14, 2022

View reviewed changes

annevk mentioned this pull request Jun 14, 2022

Add render-blocking boolean to fetch timing info #1449

Merged

3 tasks

noamr added a commit to noamr/csswg-drafts that referenced this pull request Jun 14, 2022

WIP: Editorial: adjust to fetch refactor

715cd5c

See whatwg/fetch#1413 Fetch now reports timing automatically if initiator is passed.

noamr mentioned this pull request Jun 14, 2022

[css-values-4] Editorial: adjust to fetch refactor w3c/csswg-drafts#7355

Merged

yutakahirano approved these changes Jun 15, 2022

View reviewed changes

noamr added 2 commits June 15, 2022 09:45

Remove leftover arg

622c604

Account for sync XHR

9f0745a

annevk mentioned this pull request Jun 15, 2022

processResponseConsumeBody and opaque responses #1446

Closed

annevk merged commit 4c93f89 into whatwg:main Jun 17, 2022

tabatkins pushed a commit to w3c/csswg-drafts that referenced this pull request Jun 17, 2022

WIP: Editorial: adjust to fetch refactor (#7355)

b507ff6

See whatwg/fetch#1413 Fetch now reports timing automatically if initiator is passed.

annevk pushed a commit to whatwg/xhr that referenced this pull request Aug 15, 2022

Editorial: refactor for reporting timing from Fetch

30dd504

Depends on whatwg/fetch#1413.

noamr mentioned this pull request Nov 10, 2022

Accidental reverts by navigation and session history rewrite whatwg/html#8491

Closed

huwyca mentioned this pull request Jan 11, 2024

🐛 Resource Timings are inaccurate when ServiceWorker is involved DataDog/browser-sdk#2566

Open

Bishwarupjee pushed a commit to Bishwarupjee/xhr that referenced this pull request Jan 31, 2024

Editorial: refactor for reporting timing from Fetch

2f9ae3b

Depends on whatwg/fetch#1413.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Call `finalize and report timing` automatically on end-of-body #1413

Call `finalize and report timing` automatically on end-of-body #1413

noamr commented Mar 16, 2022 •

edited by pr-preview bot

noamr commented Mar 16, 2022

annevk commented Mar 18, 2022

noamr commented Mar 18, 2022

noamr commented Mar 20, 2022 •

edited

annevk left a comment

noamr commented May 3, 2022

annevk left a comment

yutakahirano commented May 12, 2022

noamr commented May 12, 2022

yutakahirano commented May 18, 2022

noamr commented May 18, 2022

noamr commented May 18, 2022

yutakahirano commented May 18, 2022

noamr commented May 18, 2022

annevk commented May 23, 2022

noamr commented Jun 8, 2022

noamr commented Jun 14, 2022

annevk left a comment

annevk commented Jun 14, 2022

noamr commented Jun 14, 2022

annevk commented Jun 14, 2022

annevk left a comment

noamr commented Jun 14, 2022

annevk commented Jun 14, 2022

noamr commented Jun 14, 2022

yutakahirano left a comment

yutakahirano Jun 15, 2022

noamr Jun 15, 2022 •

edited

Call finalize and report timing automatically on end-of-body #1413

Call finalize and report timing automatically on end-of-body #1413

Conversation

noamr commented Mar 16, 2022 • edited by pr-preview bot

noamr commented Mar 16, 2022

annevk commented Mar 18, 2022

noamr commented Mar 18, 2022

noamr commented Mar 20, 2022 • edited

annevk left a comment

Choose a reason for hiding this comment

noamr commented May 3, 2022

annevk left a comment

Choose a reason for hiding this comment

yutakahirano commented May 12, 2022

noamr commented May 12, 2022

yutakahirano commented May 18, 2022

noamr commented May 18, 2022

noamr commented May 18, 2022

yutakahirano commented May 18, 2022

noamr commented May 18, 2022

annevk commented May 23, 2022

noamr commented Jun 8, 2022

noamr commented Jun 14, 2022

annevk left a comment

Choose a reason for hiding this comment

annevk commented Jun 14, 2022

noamr commented Jun 14, 2022

annevk commented Jun 14, 2022

annevk left a comment

Choose a reason for hiding this comment

noamr commented Jun 14, 2022

annevk commented Jun 14, 2022

noamr commented Jun 14, 2022

yutakahirano left a comment

Choose a reason for hiding this comment

yutakahirano Jun 15, 2022

Choose a reason for hiding this comment

noamr Jun 15, 2022 • edited

Choose a reason for hiding this comment

Call `finalize and report timing` automatically on end-of-body #1413

Call `finalize and report timing` automatically on end-of-body #1413

noamr commented Mar 16, 2022 •

edited by pr-preview bot

noamr commented Mar 20, 2022 •

edited

noamr Jun 15, 2022 •

edited