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 interim response times #366

Merged
merged 2 commits into from
May 9, 2023
Merged

Add interim response times #366

merged 2 commits into from
May 9, 2023

Conversation

noamr
Copy link
Contributor

@noamr noamr commented Dec 18, 2022

Add firstInterimResponseStart: the first response with an informational (1XX) status.

Bug: #345
Depends on whatwg/fetch#1483

For now not adding headers-end and body-start as they're not planned to be implemented.


Preview | Diff

@yoavweiss
Copy link
Contributor

Seems reasonable once the Fetch bits land.

index.html Outdated
Comment on lines 1019 to 1022
{{PerformanceResourceTiming/firstInterimResponseStart}},
{{PerformanceResourceTiming/responseStart}},
{{PerformanceResourceTiming/responseHeadersEnd}},
{{PerformanceResourceTiming/responseBodyStart}},

Choose a reason for hiding this comment

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

Thanks for making sure that the new timings require CORS. This is an essential requirement mentioned in the Chrome Security & Privacy intent review in order to avoid cross-origin leaks.

@annevk
Copy link
Member

annevk commented May 4, 2023

Why only the first 103? What about a 109?

@annevk
Copy link
Member

annevk commented May 4, 2023

Also, should we call this "interim" or "informational"? Is there any precedent in the web platform?

@noamr
Copy link
Contributor Author

noamr commented May 4, 2023

Why only the first 103? What about a 109?

Because it's the only informational/interim header processed by the web platform.

@noamr
Copy link
Contributor Author

noamr commented May 4, 2023

Also, should we call this "interim" or "informational"? Is there any precedent in the web platform?

That was the proposal on the issue and it seemed ok at the time. I don't really mind renaming it to informational. @tunetheweb @yoavweiss any objections?

@annevk
Copy link
Member

annevk commented May 4, 2023

Because it's the only informational/interim header processed by the web platform.

Well, we handle all of them. We have some special code for 101 and 103, but it's not like we bail out when we see a 102. (Also the current Fetch PR is not restricted to a 103.)

@tunetheweb
Copy link
Member

@LPardue from the IETF HTTP WG had some strong opinions on "informational":

I disagree with calling things informational responses. RFC 9110 doeant use that term, instead it defines interim responses, which use informational status codes. Terminology needs to be precise and consistent unless there is a good reason to diverge.

@noamr
Copy link
Contributor Author

noamr commented May 4, 2023

@LPardue from the IETF HTTP WG had some strong opinions on "informational":

I disagree with calling things informational responses. RFC 9110 doeant use that term, instead it defines interim responses, which use informational status codes. Terminology needs to be precise and consistent unless there is a good reason to diverge.

Right, RFC9110 does use the term "interim". @annevk does that satisfy "precedent in the web platform"?

@annevk
Copy link
Member

annevk commented May 4, 2023

It certainly does, thanks for digging that up.

@noamr
Copy link
Contributor Author

noamr commented May 4, 2023

Because it's the only informational/interim header processed by the web platform.

Well, we handle all of them. We have some special code for 101 and 103, but it's not like we bail out when we see a 102. (Also the current Fetch PR is not restricted to a 103.)

By handle I mean actually do something meaningful with them. Perhaps this could be named firstEarlyHintResponse and be specific to the first 103?

@tunetheweb
Copy link
Member

@LPardue had some strong views on that too :-)

Please don't prefix anything with early. If we want to support interim HTTP responses, call it that. Expect the HTTP to define more 1xx codes, because its been a part of HTTP since forever.

If we only want to record the time of the first interim response, that fine. But let's be clear. What these events measure. Maybe today first and last interim response are measured at the same time due to fetch. In the future, if fetch changes to support multiple interim responses, our first and last markers would be ready.

@annevk
Copy link
Member

annevk commented May 4, 2023

Right, but that means we should measure the first 1xx (if any), not the first 103. Which I tend to agree with.

@noamr
Copy link
Contributor Author

noamr commented May 4, 2023

@LPardue had some strong views on that too :-)

Please don't prefix anything with early. If we want to support interim HTTP responses, call it that. Expect the HTTP to define more 1xx codes, because its been a part of HTTP since forever.
If we only want to record the time of the first interim response, that fine. But let's be clear. What these events measure. Maybe today first and last interim response are measured at the same time due to fetch. In the future, if fetch changes to support multiple interim responses, our first and last markers would be ready.

"Early" here is for "Early hints" which is the term used in RFC8297. I think it's a misunderstanding of the context of early in this name. @LPardue I'm a bit confused why this is wrong. We're amending fetch to mark the timing when we got the first 103 Early Hints response.

@tunetheweb
Copy link
Member

I think @LPardue (and @annevk ?)'s point is that it shouldn't be limited to 103 (even if it is for now) in case we want to use it for any other 1XX responses in future.

@noamr
Copy link
Contributor Author

noamr commented May 4, 2023

I think @LPardue (and @annevk ?)'s point is that it shouldn't be limited to 103 (even if it is for now) in case we want to use it for any other 1XX responses in future.

OK with me. So in conclusion I'll change the description & spec & implementation to also include 100, and we'll keep the firstInterimResponseTime name?

noamr added 2 commits May 4, 2023 12:53
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 May 4, 2023

I think @LPardue (and @annevk ?)'s point is that it shouldn't be limited to 103 (even if it is for now) in case we want to use it for any other 1XX responses in future.

OK with me. So in conclusion I'll change the description & spec & implementation to also include 100, and we'll keep the firstInterimResponseTime name?

Done. Also removed the two unimplemented timings.

@annevk
Copy link
Member

annevk commented May 4, 2023

It seems fine to expose less, but #345 (comment) did have use cases for the others so that should still be tracked in some way I suppose?

Does this change also impact the request over at WebKit/standards-positions#109?

@noamr
Copy link
Contributor Author

noamr commented May 4, 2023

It seems fine to expose less, but #345 (comment) did have use cases for the others so that should still be tracked in some way I suppose?

There is a use case for the others but decided to implement them gradually. Currently only firstInterimResponseStart is implemented. I can add all of them to the spec but I thought this would be a bit fictional. Perhaps we can leave the issue open for the other ones.

Does this change also impact the request over at WebKit/standards-positions#109?

The standard position should consider that firstInterimResponseStart is the part of it that's currently implemented and the rest are in backlog for Chromium.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request May 7, 2023
Amending implementation based on spec change as per review
See discussion: w3c/resource-timing#366

Refactored HTTP1 & HTTP2 tests to be flexible as to whether to send
100 response.

Bug: 1402089
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request May 8, 2023
Amending implementation based on spec change as per review
See discussion: w3c/resource-timing#366

Refactored HTTP1 & HTTP2 tests to be flexible as to whether to send
100 response.

Bug: 1402089
Change-Id: If47bd5bf25425f9e5a012cd46be99bdaa1ec6969
@noamr noamr closed this May 8, 2023
@noamr noamr reopened this May 8, 2023
Copy link
Contributor

@yoavweiss yoavweiss left a comment

Choose a reason for hiding this comment

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

LGTM

@noamr noamr closed this May 9, 2023
@noamr noamr reopened this May 9, 2023
@noamr noamr merged commit 55724b9 into gh-pages May 9, 2023
aarongable pushed a commit to chromium/chromium that referenced this pull request May 9, 2023
Amending implementation based on spec change as per review
See discussion: w3c/resource-timing#366

Refactored HTTP1 & HTTP2 tests to be flexible as to whether to send
100 response.

Bug: 1402089
Change-Id: If47bd5bf25425f9e5a012cd46be99bdaa1ec6969
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4507473
Commit-Queue: Noam Rosenthal <[email protected]>
Reviewed-by: Yoav Weiss <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1141426}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request May 9, 2023
Amending implementation based on spec change as per review
See discussion: w3c/resource-timing#366

Refactored HTTP1 & HTTP2 tests to be flexible as to whether to send
100 response.

Bug: 1402089
Change-Id: If47bd5bf25425f9e5a012cd46be99bdaa1ec6969
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4507473
Commit-Queue: Noam Rosenthal <[email protected]>
Reviewed-by: Yoav Weiss <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1141426}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request May 9, 2023
Amending implementation based on spec change as per review
See discussion: w3c/resource-timing#366

Refactored HTTP1 & HTTP2 tests to be flexible as to whether to send
100 response.

Bug: 1402089
Change-Id: If47bd5bf25425f9e5a012cd46be99bdaa1ec6969
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4507473
Commit-Queue: Noam Rosenthal <[email protected]>
Reviewed-by: Yoav Weiss <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1141426}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request May 22, 2023
…t should include 100 responses, a=testonly

Automatic update from web-platform-tests
ResourceTiming: firstInterimResponseStart should include 100 responses

Amending implementation based on spec change as per review
See discussion: w3c/resource-timing#366

Refactored HTTP1 & HTTP2 tests to be flexible as to whether to send
100 response.

Bug: 1402089
Change-Id: If47bd5bf25425f9e5a012cd46be99bdaa1ec6969
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4507473
Commit-Queue: Noam Rosenthal <[email protected]>
Reviewed-by: Yoav Weiss <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1141426}

--

wpt-commits: fd5c04fb75f07887491c0c47999a5e95d40b190c
wpt-pr: 39881
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this pull request May 23, 2023
…t should include 100 responses, a=testonly

Automatic update from web-platform-tests
ResourceTiming: firstInterimResponseStart should include 100 responses

Amending implementation based on spec change as per review
See discussion: w3c/resource-timing#366

Refactored HTTP1 & HTTP2 tests to be flexible as to whether to send
100 response.

Bug: 1402089
Change-Id: If47bd5bf25425f9e5a012cd46be99bdaa1ec6969
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4507473
Commit-Queue: Noam Rosenthal <[email protected]>
Reviewed-by: Yoav Weiss <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1141426}

--

wpt-commits: fd5c04fb75f07887491c0c47999a5e95d40b190c
wpt-pr: 39881
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants