w3c / performance-timeline

Performance Timeline
https://w3c.github.io/performance-timeline/
Other
111 stars 27 forks source link

Add navigationId to PerformanceEntry #192

Closed clelland closed 1 year ago

clelland commented 3 years ago

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

clelland commented 3 years ago

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.

yoavweiss commented 3 years ago

OK, Now I remember... We have an ongoing discussion on https://github.com/w3c/page-visibility/issues/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

clelland commented 3 years ago

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.

clelland commented 3 years ago

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.

yoavweiss commented 3 years ago

This LGTM. Let's land the HTML parts first though before landing this.

yoavweiss commented 2 years ago

Looking at the discussions on https://github.com/whatwg/html/pull/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?

paulirish commented 1 year ago

After seeing how navigationId works with bfcache/softnavs, I'm surprised by the 1-indexed appearance.

  • If relevantGlobal has an associated document:
    • Set newEntry's navigationId to the value of relevantGlobal's associated document's navigation id.
    • Otherwise, set newEntry's navigationId to 0.

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?) How would anything on the perf timeline get a navid of 0?

clelland commented 1 year ago

How would anything on the perf timeline get a navid of 0?

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, https://github.com/WICG/soft-navigations/issues/12 is relevant to this discussion, and we should probably be replacing this with a less-counter-y mechanism in any case.

clelland commented 1 year ago

Taking the feedback from https://github.com/WICG/soft-navigations/issues/12 into account, I've updated this PR. navigationId is now a string rather than a counter, and all of the algorithms to manage it are here in Performance Timeline.

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".

clelland commented 1 year ago

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.

clelland commented 1 year ago

Ping @yoavweiss -- can you take a look?

Krinkle commented 1 year ago

RE: Today's W3C WebPerf presentation by @clelland in which we saw an integer ("counter") and string idenitifier ({type}-{startTime}) proposed as value for the navigationId.

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 {type}-{startTime}-{number} where the latter would be 0 most cases but mediate would-be conflicts.

It's not clear what value startTime adds in that case, since it doesn't aid sorting (we'd need {startTime}-{type} instead of {type}-{startTime} to aid sorting), and is presumably not meant for machine-readable consumption either (the entry objects have .startTime for that).

So perhaps {type}-{number} would be simpler if we only want to reflect that it's a blackbox string that is neither an array index nor key for approx time order. Or a step further, and plain {number}, as blackbox integer, in which case we're back to a setTimeout-like integer/counter 😁

noamr commented 1 year ago

RE: Today's W3C WebPerf presentation by @clelland in which we saw an integer ("counter") and string idenitifier ({type}-{startTime}) proposed as value for the navigationId.

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.

It's monotonic time, so it should be unique % coarsening.

clelland commented 1 year ago

Thinking more about this issue this morning, @yoavweiss, @yashjoshi-dotcom and I came to the conclusion that {type}-{startTime} probably won't work as a general id mechanism for performance entries. We can't use it for resource timing or event timing, or even paint timing, as it's almost certain to generate collisions.

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 increment[^1] 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.)

[^1]: Should the increment be random each time? Or is a small consistent step better? That's a topic for bikeshedding