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

Introduce performance timeline API integration #125

Merged
merged 2 commits into from
Jun 4, 2021
Merged

Conversation

domenic
Copy link
Collaborator

@domenic domenic commented May 27, 2021

This replaces our previous broken idea of using currentchange for this. Closes #59. Closes #33. #14 remains to track whether currentchange is actually useful.

The main question is, what information should be included here, beyond the basics of URL/start time/duration? So far I added success, but I'm not sure whether that's useful. Other ideas include navigationType, navigateInfo, userInitiated, and referrer (or some other name for the previous URL).

See also #124 since I think interrupted navigations counting as failures would be pretty noisy to send to an analytics provider unless we also fix that.

@yoavweiss, I'd love your thoughts on this!

This replaces our previous broken idea of using currentchange for this. Closes #59. Closes #33. #14 remains to track whether currentchange is actually useful.
@yoavweiss
Copy link

Apologies for the delay in reviewing this! Might be interesting to discuss this with the WG.
One obvious question is whether we want a new entry type or if this can fit as a new NavigationType on an PerformanceNavigationTiming entry.
That's at least the route we said we'd take with BFCache navigations, so might be good to coalesce all navigations on a similar API shape.

WDYT?

@domenic
Copy link
Collaborator Author

domenic commented Jun 2, 2021

One obvious question is whether we want a new entry type or if this can fit as a new NavigationType on an PerformanceNavigationTiming entry.

Do you have thoughts on how that would work? In particular:

  • Single-page app navigations can be single-page variants of each navigation type (and can be a replace as well, which is not captured by NavigationType). I.e. they are not a new type of navigation that sits alongside those.

  • None of the fields on PerformanceNavigationTiming and PerformanceResourceTiming, except maybe type, make sense for single-page app navigations.

@yoavweiss
Copy link

What we talked about for BFCache is that it'd be interesting to add a NavigationID to various PerformanceEntries, to be able to correlate them with the navigation that kicked them off. The current thinking should be summed up in this design doc

  • Single-page app navigations can be single-page variants of each navigation type (and can be a replace as well, which is not captured by NavigationType). I.e. they are not a new type of navigation that sits alongside those.

Looking at the list, I can see how they can be intermixed with "navigate" and "back_forward". That seems solvable (e.g. by adding 2 types for same-document navs)

  • None of the fields on PerformanceNavigationTiming and PerformanceResourceTiming, except maybe type, make sense for single-page app navigations.

The same is largely true for BFCache navigations. That's indeed unfortunate, but there were multiple voices among the API users that preferred the ergonomics and discoverability advantages of reusing the current "navigation" observers over having to kick off an observer for each new navigation type. I'm happy to re-open that discussion if you disagree with that decision, but it'd be good to align both proposals on the same pattern.

@domenic
Copy link
Collaborator Author

domenic commented Jun 2, 2021

Looking at the list, I can see how they can be intermixed with "navigate" and "back_forward". That seems solvable (e.g. by adding 2 types for same-document navs)

I don't quite understand the proposal. Are you saying we should expand NavigationType from navigate, reload, back_forward, prerender to also include same-document-navigate, same-document-reload, same-document-replace, and same-document-back_forward? That seems pretty awkward compared creating an orthogonal event...

The same is largely true for BFCache navigations. That's indeed unfortunate, but there were multiple voices among the API users that preferred the ergonomics and discoverability advantages of reusing the current "navigation" observers over having to kick off an observer for each new navigation type. I'm happy to re-open that discussion if you disagree with that decision, but it'd be good to align both proposals on the same pattern.

I think it'd be good to reopen that discussion, especially since you could get the same benefits by using a fresh subclass of PerformanceEntry but reusing the same entryType value.

But even beyond that, I would think that, given how same-document navigations are very different from cross-document ones, you'd want to use a separate entryType for those.

@yoavweiss
Copy link

same-document-reload, same-document-replace, and same-document-back_forward? That seems pretty awkward compared creating an orthogonal event...

I didn't really consider "reload" for same document navs and didn't consider "replace" to be a separate type. Agree that as you outlined it may be awkward, and it may be better to split that information to a separate attribute (e.g. a "same-document navigation" boolean). I'm not sure it justifies a completely separate entry though.

I think it'd be good to reopen that discussion, especially since you could get the same benefits by using a fresh subclass of PerformanceEntry but reusing the same entryType value.

Can you expand on that?

Would you be interested in presenting to the WebPerfWG your proposal in one of the future calls?

@domenic
Copy link
Collaborator Author

domenic commented Jun 3, 2021

I didn't really consider "reload" for same document navs and didn't consider "replace" to be a separate type.

Yeah, replace is a very separate type in specs and implementations, and we have some evidence developers want to handle them differently. So for single-page navs at least they should be separate.

Can you expand on that?

What would you like me to expand on? The suggestion is to just have a subclass of PerformanceEntry, e.g. BFCacheNavigationEntry, which has entryType === "navigation". Then all the entryType-based filtering/observing APIs would work, but you wouldn't have a violation of the Liskov Substitution Principle.

Would you be interested in presenting to the WebPerfWG your proposal in one of the future calls?

Is there any way to have the discussion asynchronously?

@domenic
Copy link
Collaborator Author

domenic commented Jun 4, 2021

Based on some offline discussion with web developers, I got a few points of feedback:

  • They reiterated that single-page navigations are very different from cross-document navigations, and they'd be surprised by getting a PerformanceNavigationTiming with a bunch of zero fields for something that doesn't apply. They have separate (usually application-specific) metrics they want to track for single-page navigations, and having a bunch of useless fields would make it confusing.
  • To my point in the OP about "what information to include", they thought that just success was fine. The other things in the OP are kind of "facts about the navigation", which is best captured by listening for the navigate event. Whereas the performance timing integration should focus on performance and success/failure statistics.

I'll merge this for now, but happy to continue discussion, especially as we start implementing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants