Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add more timing information about (interim) responses #1483

Merged
merged 5 commits into from
May 8, 2023

Conversation

noamr
Copy link
Contributor

@noamr noamr commented Sep 2, 2022

See w3c/resource-timing#345

Since early hints have landed, there are additional useful timestamp
that's currently not exposed:

  • First interim response start (e.g. when we received a 103)
    as a different timestamp from the final response start
    (e.g. when we received the 200)

Note that w3c/resource-timing#345 also mentions two additional
timestamps, which would be added incrementally when tests are authored for them.

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


Preview | Diff

@noamr
Copy link
Contributor Author

noamr commented Sep 2, 2022

@bdekoz, thoughts from the Mozilla side?

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To confirm, all these times are only exposed for CORSTAO-same-origin resources?

fetch.bs Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
@noamr

This comment was marked as resolved.

@annevk

This comment was marked as resolved.

@noamr
Copy link
Contributor Author

noamr commented Nov 28, 2022

To confirm, all these times are only exposed for TAO-same-origin resources?

Confirmed.

noamr added a commit to w3c/resource-timing that referenced this pull request Dec 18, 2022
Add 3 response times:
- firstInterimResponseStart: the first 103
- responseHeadersEnd: All headers have been received
- responseBodyStart: The body started streaming

Closes #345
Depends on whatwg/fetch#1483
@noamr
Copy link
Contributor Author

noamr commented Jan 25, 2023

@annevk could you spare time to finish reviewing this? Thanks!

@noamr
Copy link
Contributor Author

noamr commented Apr 24, 2023

@annevk could you spare time to finish reviewing this? Thanks!

Ping

@noamr
Copy link
Contributor Author

noamr commented May 1, 2023

@annevk could you spare time to finish reviewing this? Thanks!

Ping

Re-ping @annevk :)

@noamr noamr changed the title Add more timing iformation about (interim) responses Add more timing information about (interim) responses May 3, 2023
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Show resolved Hide resolved
noamr added 3 commits May 4, 2023 12:51
See w3c/resource-timing#345

Since early hints have landed, there are additional useful timestamps
that are currently not exposed:

- First interim response start (e.g. when we received a 103)
  as a different timestamp from the final response start
  (e.g. when we received the 200)

- Final headers received (when the last header has been received and
  we're ready for the body)

- First bytes of the body received

The naming in whatwg#345 is not finalized yet, but it's clear that these
are the 3 interesting timestamps. This PR exposes those for later
use by resource timing.
noamr added a commit to w3c/resource-timing that referenced this pull request May 4, 2023
Add 3 response times:
- firstInterimResponseStart: the first 103
- responseHeadersEnd: All headers have been received
- responseBodyStart: The body started streaming

Closes #345
Depends on whatwg/fetch#1483
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pushing this @noamr! One nit and tests remain and a bit of process feedback for next time.

I find it rather odd that the scope of this PR has been reduced based on what Chromium deemed worthy to implement right now. Especially without signals from other stakeholders that that seems reasonable. But perhaps this was discussed somewhere?

Either way, given the relatively small scope of this feature I'm okay with it, but OP will need updating.

As for tests:

  • Next time please land them as .tentative until specifications PRs land/are about to land.
  • Please add some tests where a non-103 1xx comes first. E.g., 1) 110 103 200, 2) 199 200.

fetch.bs Show resolved Hide resolved
@noamr
Copy link
Contributor Author

noamr commented May 7, 2023

Thanks for pushing this @noamr! One nit and tests remain and a bit of process feedback for next time.

I find it rather odd that the scope of this PR has been reduced based on what Chromium deemed worthy to implement right now. Especially without signals from other stakeholders that that seems reasonable. But perhaps this was discussed somewhere?

It wasn't discussed, it's perhaps my misunderstanding of how the process should look like.
Right now it seems like other vendors are supportive of the whole thing, but no vendor is actively implementing anything but firstInterimResponseStart. This includes WPT coverage. Given that, I thought it would be the right thing to first merge firstInterimResponseStart and then merge the other ones when there is a first implementer that's also going to spend the time on authoring WPTs for responseHeadersEnd and responseBodyStart.

Isn't merging this gradually a good thing, on the side of caution? I'm open to following the process in any other way. Perhaps I should have brought it up for discussion.

Either way, given the relatively small scope of this feature I'm okay with it, but OP will need updating.

As for tests:

  • Next time please land them as .tentative until specifications PRs land/are about to land.

Will do

  • Please add some tests where a non-103 1xx comes first. E.g., 1) 110 103 200, 2) 199 200.

On it

@noamr
Copy link
Contributor Author

noamr commented May 7, 2023

  • Please add some tests where a non-103 1xx comes first. E.g., 1) 110 103 200, 2) 199 200.

web-platform-tests/wpt#39881

@annevk annevk merged commit 76ee5f5 into whatwg:main May 8, 2023
@noamr noamr deleted the final-response-time branch May 8, 2023 08:51
@annevk
Copy link
Member

annevk commented May 8, 2023

I didn't realize we only had tests for a single feature. I agree that we need coverage for all features we add to the specification.

I think my issues with how this went are:

  1. I (and others) had put effort into reviewing all three features together. We'll now have to do that again at some future point for the remaining features.
  2. Other vendors might have started planning and if so that would have been impacted.

@noamr
Copy link
Contributor Author

noamr commented May 8, 2023

I didn't realize we only had tests for a single feature. I agree that we need coverage for all features we add to the specification.

I think my issues with how this went are:

  1. I (and others) had put effort into reviewing all three features together. We'll now have to do that again at some future point for the remaining features.
  2. Other vendors might have started planning and if so that would have been impacted.

Understood, apologies for the miscommunication. I should have clarified this when I realized I was going to implement them piecemeal. It seemed originally that it would be a one-go sort of thing.

Of course we're still interested in the other 2 metrics, just didn't get around to them yet and the first one was urgent to some folks, so if other vendors are already planning on adding them we'd support it and would be happy for those WPTs.

noamr added a commit to w3c/resource-timing that referenced this pull request May 9, 2023
* Add interim response times

Add 3 response times:
- firstInterimResponseStart: the first 103
- responseHeadersEnd: All headers have been received
- responseBodyStart: The body started streaming

Closes #345
Depends on whatwg/fetch#1483

* Remove unimplemented parts for now
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants