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

improve initiatorType #36

Closed
wants to merge 1 commit into from
Closed

improve initiatorType #36

wants to merge 1 commit into from

Conversation

igrigorik
Copy link
Member

  • 'fetch' for fetch() initiated requests
  • 'preflight' for CORS-preflight requests
  • 'other' to catch all other cases

closes #8

/cc @toddreifsteck @annevk

- 'fetch' for fetch() initiated requests
- 'preflight' for CORS-preflight requests
- 'other' to catch all other cases

closes #8
@annevk
Copy link
Member

annevk commented Sep 24, 2015

  1. It seems the layering here is wrong. All this timing stuff should be refactored in terms of Fetch.
  2. "preflight" seems too generic. We might have HSTS preflights soon, so I'd recommend "cors-preflight".
  3. I'm a bit worried about exposing preflights to content. Did that go through security review? What are the limitations for failed fetches and such, at what point are they exposed?

@toddreifsteck
Copy link
Member

The content within this update LGTM. Please follow up on @annevk's concerns before merging.

@igrigorik
Copy link
Member Author

It seems the layering here is wrong. All this timing stuff should be refactored in terms of Fetch.

Agree, and we should probably move this into a separate issue.. But, since we're on the subject, how do you see RT interacting with Fetch? Would it make sense to slot an extra step somewhere between steps 4-5 under HTTP fetch? There is some complexity to untangle here with redirects as well -- see #21.

Also, for initiatorType, how would we set that? The Request.initiator doesn't provide what we need here; Request.type is a bit closer but also doesn't give us exactly what we need.

"preflight" seems too generic. We might have HSTS preflights soon, so I'd recommend "cors-preflight".

Hmm, trying to wrap my head around what an HSTS preflight is/does. Any threads or references that I can catch up on?

I'm a bit worried about exposing preflights to content. Did that go through security review? What are the limitations for failed fetches and such, at what point are they exposed?

This is same discussion as #12.

@annevk
Copy link
Member

annevk commented Sep 25, 2015

I'm not sure how resource timing is done best. Perhaps "fetch groups" can be reused somehow and we annotate the "fetch records" in such groups with timing data? And then resource timing becomes just the API wrapper around those fetch groups?

I suspect initiatorType could be an annotation on a "fetch record". If it requires more, we'd need an extra field on request or maybe extend request's initiator field.

HSTS preflights are discussed here to some extent: https://lists.w3.org/Archives/Public/public-webappsec/2015Aug/thread.html#msg95

@igrigorik
Copy link
Member Author

I'm not sure how resource timing is done best. Perhaps "fetch groups" can be reused somehow and we annotate the "fetch records" in such groups with timing data? And then resource timing becomes just the API wrapper around those fetch groups?

Hmm.. Do redirect fetches, preflights, etc, get their own fetch records? How, if at all, is the navigation fetch surfaced? Also, a lot of the metrics we're talking about here are connection-oriented (DNS, TCP, etc), and don't have clean hooks into Fetch -- a lot of things to untangle here.

That said, I'm guessing some combination of fetch group + fetch observer (whatwg/fetch#65) should do the trick... ~ wait for notification that fetch has finished, create the RT entry and queue it for perf obsevers + relevant buffers.

HSTS preflights are discussed here to some extent: https://lists.w3.org/Archives/Public/public-webappsec/2015Aug/thread.html#msg95

Interesting. Thanks for the link.


Taking a step back here.. Seems like before we can merge this update we really ought to resolve #12 (i.e. whether surfacing cors preflights is something we ought to be doing), and investigate Fetch rewrite. As such, I propose we: close this, open a new bug for Fetch rewrite, and note in #12 that we should add new initiatorType once that's resolved (and if still relevant).

@annevk
Copy link
Member

annevk commented Sep 26, 2015

Redirects are part of the same overall fetch, since they're resolved somewhat atomically for security reasons. But just as redirects manipulate the url list, they could add information to fetch records quite easily. Would need careful scrutiny though.

I'm okay with splitting this up.

@igrigorik
Copy link
Member Author

Closing, let's continue in #39.

@igrigorik igrigorik closed this Oct 2, 2015
@igrigorik igrigorik deleted the initiator branch October 20, 2015 17:39
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.

Improve "initiator" reporting
3 participants