-
Notifications
You must be signed in to change notification settings - Fork 27
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 navigationId to PerformanceEntry #192
Conversation
This is probably okay for technical review at this point, but I wouldn't merge this until some other parts are done -- we should have bfcache navs trigger a kind of NavigationTiming entry, and we should have a way to observe "all navigations" as a meta-category. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! a few nits
OK, Now I remember... We have an ongoing discussion on w3c/page-visibility#51 on better ways to hook into the HTML algorithm here, so you probably want to follow the path outlined there, rather than link to that HTML algorithm directly. Happy to chat about this. /cc @mmocny |
That approach makes it look like that the global's navigation id should be entirely defined in HTML; there would seem to be little benefit to declaring it here and then making HTML call an algorithm here just to increment it. |
Updated to move the navigation counter into HTML (Personal repo commit here) and use that instead of the session history document visibility change steps Note that this will always be 0 for workers without a relevant document, and should start at 1 for documents with their first load. |
This LGTM. Let's land the HTML parts first though before landing this. |
Looking at the discussions on whatwg/html#7370, it seems like it'd be better if we could reach an agreement to land the HTML related processing there... As currently (experimentally) implemented in SPA heuristics, the counter is not incremented on use of the Navigation API, but only when the heuristics are triggered. As SPA heuristics are not (yet) specified, maybe it's possible to convince the HTML folks to name this a navigation counter even if the only user is currently BFCache navigations? Alternatively, we could name it whatever they are comfortable with, and rename it once SPA heuristics are mature enough to be specified? |
After seeing how navigationId works with bfcache/softnavs, I'm surprised by the 1-indexed appearance.
What's the significance of a navId of 0? Would that be like.. Network Error Logging reporting on a failed navigation? (Can't be, as NEL isn't a perftimeline concept?) |
With the text as written, only events on a worker's timeline would have a navigationId of 0. A document's counter starts at 0, but is immediately incremented as the document loads. That said, WICG/soft-navigations#12 is relevant to this discussion, and we should probably be replacing this with a less-counter-y mechanism in any case. |
Taking the feedback from WICG/soft-navigations#12 into account, I've updated this PR. I expect that it doesn't need HTML integration support any more; I would instead change Navigation Timing to hook into this now, and call "Queue a navigation PerformanceEntry" rather than "Queue a PerformanceEntry". |
Q: Should we export "most recent navigation" for situations where a timeline entry might be started before a navigation event, but queued after it finishes? I'm thinking about the case where a resource or event timing entry spans a soft-navigation, and we want it to be attributed to the navigation preceding the soft nav, even if it completes afterwards. |
Ping @yoavweiss -- can you take a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for the delay! This fell through the cracks..
explainer.md
Outdated
When this API was originally designed, documents had a relatively simple lifecycle: they were | ||
loaded when the user navigated to them, and unloaded when the user navigated away, with the | ||
JavaScript environment being torn down at that time. Since then, the sitution has become more | ||
complex, with many browsers introducing a back-forwards-cache, with which a user can return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: back-forward cache
explainer.md
Outdated
loaded when the user navigated to them, and unloaded when the user navigated away, with the | ||
JavaScript environment being torn down at that time. Since then, the sitution has become more | ||
complex, with many browsers introducing a back-forwards-cache, with which a user can return | ||
a document wich they have previously navigated away from. The web has also seen a rise in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/wich/which/
explainer.md
Outdated
appears to the user as a navigation) can occur without the performance timeline being reset. | ||
In order to allow developers to reason about such events during the life of a page, some | ||
PerformanceEntry objects mark navigations, or navigation-like events. All PerformanceEntry | ||
objects include a navigation ID field, which links each PerformanceEntry to the most recent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/links/ties/
explainer.md
Outdated
In order to allow developers to reason about such events during the life of a page, some | ||
PerformanceEntry objects mark navigations, or navigation-like events. All PerformanceEntry | ||
objects include a navigation ID field, which links each PerformanceEntry to the most recent | ||
navigation entry which had occurred when the entry was generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/when/before/ ?
index.html
Outdated
@@ -244,6 +247,12 @@ <h2><dfn>Performance Timeline</dfn></h2> | |||
</ul> | |||
</li> | |||
</ul> | |||
<p>Each <a>Document</a> has:</p> | |||
<ul> | |||
<li>a <dfn>most recent navigation</dfn>, which is a <a>PerformanceEntry</a>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/a/A/
index.html
Outdated
<li>Let <var>type</var> be <var>entry</var>'s <a>entryType</a>.</li> | ||
<li>Let <var>startTime</var> be <var>entry</var>'s <a>startTime</a>. | ||
</li> | ||
<li>Return the [=string/concatenation=] of « <var>type</var>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we initially talked about a UUID, but this may be equivalent. A few things that may hamper that:
- Can we have multiple navigations per ms? Unlikely, but I'm not sure it's currently prohibited.
- Having a predictable structure may cause developers to create weird reliances on the ID's structure (but I'm not sure that would be the case)
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- We can't have two navigations at the same monotonic time.
- I think it's OK that people rely on this if it's well defined. It's only the URL and the start time which are anyway available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense!
FWIW, when working on resource timing initiator (which uses a similar ID concept), we got feedback that it might be more ergonomic to use an object rather than a concatenated string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we run into problems with object equality, if we use a structured object? Specifically, are there issues around key order or numeric precision, or just the fact that object equality is often conflated with object identity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed at the WG; there probably are issues with identity/equality here, and a string has better comparison behaviour, as long as we ensure it is unique.
Sticking with - will allow this pattern to be reused for other cases where we want to refer to timeline entries as well.
index.html
Outdated
<li>Let <var>startTime</var> be <var>entry</var>'s <a>startTime</a>. | ||
</li> | ||
<li>Return the [=string/concatenation=] of « <var>type</var>, | ||
<var>time</var> » using U+002D HYPHEN-MINUS.</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
concatenation is defined for a list of strings, so I think you need to convert the timestamp to a string first.
index.html
Outdated
<var>name</var> set to the method input <code>name</code> parameter, and | ||
<var>type</var> set to <code>null</code> if optional `entryType` is omitted, | ||
or set to the method's input <code>type</code> parameter otherwise.</p> | ||
</section> | ||
<section> | ||
<h2><dfn>getEntriesByNavigationId()</dfn> method</h2> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we discuss creation of a dedicated filter, compared to applying a JS filter on getEntries()
? I'm guessing this would have performance benefits, but it's worthwhile to articulate the reasoning (maybe in the explainer)
RE: Today's W3C WebPerf presentation by @clelland in which we saw an integer ("counter") and string idenitifier ( I believe startTime could theoretically clash in some unlikely pathalogical scenario where time doesn't move or things happen very fast during recovery from a blockage of sorts. Assuming navigationId has to be unique within a realm, and startTime isn't, does that mean the number would be altered or extended in some way in practice in the event of a conflict? If so, it might be worth reserving something in the format so that consumers won't be caught in surprise when an entry's navigationId varies notably from the assumed string format. For example It's not clear what value So perhaps |
It's monotonic time, so it should be unique % coarsening. |
* Fix typos * Remove getEntriesByNavigationId() * Convert timestamps to strings for concatenation
Thinking more about this issue this morning, @yoavweiss, @yashjoshi-dotcom and I came to the conclusion that However, there is a useful mechanism in event timing already, for generating interaction ids. I think we can reuse that logic here, to assign an integer id to every entry, and then navigationId will just use the id for the relevant navigation entry. It's monotonically increasing, but doesn't have a predictable start or increment1 so you'd have to go out of your way to try to depend on the exact sequence of values. (And it was the potential for those dependencies which was the reason we didn't want a simple counter.) Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % comment
Maybe you can do a 5 minute presentation on tomorrow's call to update folks on where we landed?
explainer.md
Outdated
@@ -59,6 +59,7 @@ The entry has the following attributes: | |||
* `entryType`: a string representing the type of performance data being exposed. It is also used to filter entries in the `getEntriesByType()` method and in the `PerformanceObserver`. | |||
* `startTime`: a timestamp representing the starting point for the performance data being recorded. The semantics of this attribute depend on the `entryType`. | |||
* `duration`: a time duration representing the duration of the performance data being recorded. The semantics of this one also depend on the `entryType`. | |||
* `navigationId`: a string identifying the `PerformanceEntry` object corresponding to the last navigation or navigation-like event that had occurred in the document at the time that this `PerformanceEntry` object was recorded. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
integer?
This adds the concept of a
current navigation id
to the global object, which starts at 0 and is incremented on every navigation event during a page's lifetime. This navigation id then gets exposed on PerformanceEntry objects, to allow those timeline entries to be segmented by user interaction.Preview | Diff
Preview | Diff