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 "initiator" reporting #8

Open
vayam opened this issue Feb 16, 2015 · 12 comments
Open

Improve "initiator" reporting #8

vayam opened this issue Feb 16, 2015 · 12 comments
Milestone

Comments

@vayam
Copy link

vayam commented Feb 16, 2015

We at Vimeo are working on uploading straight to Google Cloud Storage. Occasionally I notice OPTIONS requests taking a long time to respond (10-30 sec).

Here is one example: https://gist.github.com/vayam/d752cf31eeedc4849766#file-gistfile1-txt-L2164

I would like to log OPTIONS request timings so I can report to Google Cloud support. Unfortunately, the HTTP method is not recorded in the resource timing spec. Right now I can't distinguish b/w PUT vs OPTIONS.

From the spec The name attribute must return the resolved URL of the requested resource.
I propose adding method attribute to it.

I would like to hear your feedback on this.

/cc @toddreifsteck

@igrigorik
Copy link
Member

I'm assuming you're using XHR to invoke the upload - correct? Is there any reason why you can't instrument the upload logic to get the method and/or any other variables from the request object itself?

@vayam
Copy link
Author

vayam commented Feb 17, 2015

The only XHR request I am making is a PUT. I don't have any control on OPTIONS request that browser is internally making for CORS. At least I am not aware of any way I can grab the response times of OPTIONS request. Can you elaborate?

@igrigorik
Copy link
Member

On further thought.. I think this a subset of multi-request case that we need to address, see: https://lists.w3.org/Archives/Public/public-web-perf/2015Feb/0059.html

@vayam
Copy link
Author

vayam commented Feb 18, 2015

c) A preflight request is triggered for a cross-origin request:

performance.getEntriesByType("resource") -> [
   PerformanceResourceTiming{name: "https://other.com/thing.js", <timing
attrs>},
   PerformanceResourceTiming{name: "https://other.com/thing.js", <timing
attrs>},
   ... <other entries > ...
]

Similar to above subrequest case, but the request URL remains the same
across multiple requests required to fulfill this fetch. The application
would:

reqs = performance.getEntriesByName("https://other.com/thing.js") //
returns an array with two PerformanceResourceTiming entries
processTiming(reqs)

(note: there is an implicit assumption here that requests are recorded in
the timeline in the same order as they occur)

@igrigorik that looks great and handles lot of cases. It still doesn't resolve this issue completely. Two thing If I am doing multiple PUT request to same resource as in case of upload there are situations browser skips an OPTIONS request. So you cannot assume there is always preflight. I have seen this behavior in all browsers. here is an example of chrome skipping preflight for PUT to same resource.
as you can see the 2nd PUT is sent without preflight OPTIONS.

I can potentially end with an odd numbers of timing entries and I can't distinguish which ones are preflight.

Here is another thought, the spec could explain initiatorType field better. As of now chrome sets initiatorType=xmlhttprequest' where as IE11 sets it topreflight`.

ie11

vs
chrome

I still feel adding method attribute would make it unambiguous.
What do you think?

@igrigorik
Copy link
Member

The behavior you're describing is correct: browsers cache CORS permissions responses to avoid having to send an OPTIONS request for each cross-origin fetch. Hence the "odd" number of requests.

That said, you're right, just "name" and "nextName" may be insufficient when you have multiple app-level requests against the same URL. We may need to introduce some concept of a request ID.

Last but not least, I actually really like IE's behavior. I think we should take that to the mailing list and see if we can put that in the spec. The combination of initiator + request ID would address all of the above issues.

@vayam
Copy link
Author

vayam commented Feb 20, 2015

The combination of initiator + request ID would address all of the above issues.
@igrigorik 👍

@igrigorik igrigorik changed the title Add Request Method attribute Improve "initiator" reporting May 14, 2015
@igrigorik
Copy link
Member

Discussed on perf call (26/08/15): @toddreifsteck to followup with list of initiators implemented in Edge.

@toddreifsteck
Copy link
Member

Here is the list of currently implemented Microsoft Edge initiatorTypes.

link: link requests
css: requests initiated from within css
script: script
img: image
object: downloads from object tag
subdocument: IFrame, Document.Write and COM edits that trigger a resource pull from network
preflight: CORS preflight
xmlhttprequest: xhr
svg: svg
other: Any request not in a category above or any code missing categorization

@toddreifsteck toddreifsteck removed their assignment Sep 16, 2015
@igrigorik
Copy link
Member

@toddreifsteck thanks! Looking at the list, RT's current definition covers: css, {link, script, img, object, svg} via localName, and xhr. That leaves us with:

  • preflight - seems like a reasonable case we should define in RT.
  • subdocument - for iframe's why that instead of localName value (i.e. iframe.. that's what we report in Chrome); wouldn't localName also work for elements injected via doc.write?

Also, we don't have a definition for fetch().. seems like something we need to as well.

Any other cases we're overlooking here?

@toddreifsteck
Copy link
Member

Agreed on preflight and fetch. Unclear that adding subdocument or iframe to spec has clear value.

igrigorik added a commit that referenced this issue Sep 23, 2015
- 'fetch' for fetch() initiated requests
- 'preflight' for CORS-preflight requests
- 'other' to catch all other cases

closes #8
@plehegar plehegar added this to the V1 milestone Mar 3, 2016
@plehegar
Copy link
Member

plehegar commented Mar 3, 2016

Should this be closed in favor of #39 ?

@igrigorik
Copy link
Member

Hmm, probably not. Fetch initiator [1] is not granular enough for our purposes, we'll have to maintain some of our own logic.

And, if we agree on that, than we don't need to block on #39.

https://fetch.spec.whatwg.org/#concept-request-initiator

@plehegar plehegar modified the milestones: V2, V1 Apr 26, 2016
@igrigorik igrigorik modified the milestones: Level 3, Level 2 Jun 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants