core(trace): use tracing-started event for frame tree info #13913
Conversation
|
|
||
| // Update known frames if FrameCommittedInBrowser events come in, typically | ||
| // with updated `url` (as well as pid, etc). Some traces (like timespans) may | ||
| // not have any committed frames. |
There was a problem hiding this comment.
somewhat debatable if we really need to "update" the known frames if there are FrameCommittedInBrowser events, because only the url changes if there are entries for a frame in both TracingStartedInBrowser and a FrameCommittedInBrowser event, and as far as I can tell we never use those urls anywhere :)
However, it seems good to do because it makes sense conceptually, not updating doesn't really make this code any simpler, this is like a few dozen events at most, so 🤷
| @@ -304,7 +304,6 @@ describe('Metrics: CLS', () => { | |||
| const cat = 'loading,rail,devtools.timeline'; | |||
| trace.traceEvents.push( | |||
| /* eslint-disable max-len */ | |||
| {name: 'FrameCommittedInBrowser', cat, args: {data: {frame: mainFrame, url: 'https://example.com'}}}, | |||
There was a problem hiding this comment.
Noticed these extra test entries when comparing the before/after. They don't hurt anything with our current setup, but createTestTrace creates a mainframe FrameCommittedInBrowser event already, so these end up doubled up
| @@ -7850,7 +7850,7 @@ | |||
| "description": "Consider reducing the time spent parsing, compiling, and executing JS. You may find delivering smaller JS payloads helps with this. [Learn more](https://web.dev/bootup-time/).", | |||
| "score": 1, | |||
| "scoreDisplayMode": "numeric", | |||
| "numericValue": 274.5320000000002, | |||
| "numericValue": 275.90400000000034, | |||
There was a problem hiding this comment.
@adamraine this looks right and the timespan wasn't looking cross frame for bootup time, but haven't verified yet. Hard to find where each step begins and ends in the big json files :)
There was a problem hiding this comment.
Yeah trace processor was producing an empty list for frames in timespan
There was a problem hiding this comment.
Hard to find where each step begins and ends in the big json files :)
I can open an issue for an assetSaver.saveFlowArtifacts that separates this stuff into separate files. Wouldn't hurt to have another 800,000+ line PR on my resume 😎
| @@ -612,21 +612,41 @@ class TraceProcessor { | |||
| // Find the inspected frame | |||
| const mainFrameIds = this.findMainFrameIds(keyEvents); | |||
|
|
|||
| const frames = keyEvents | |||
| /** @type {Map<string, {id: string, url: string, parent?: string}>} */ | |||
| const framesById = new Map(); | |||
There was a problem hiding this comment.
I think we can remove this warning now:
lighthouse/lighthouse-core/lib/tracehouse/trace-processor.js
Lines 645 to 647 in 6ba8dc8
There was a problem hiding this comment.
I can update the message maybe? I feel like it's really just for the outdated tests now
There was a problem hiding this comment.
I can update the message maybe?
SGTM
| parent: frame.parent, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
super nit: could reduce indentation if L621 did || [].
There was a problem hiding this comment.
I had that originally but it was bordering on unreadable. The indent isn't ideal but it doesn't seem too bad to me
| @@ -8537,12 +8543,12 @@ | |||
| ], | |||
| "items": [ | |||
| { | |||
| "url": "https://www.mikescerealshack.co/_next/static/chunks/framework.9d524150d48315f49e80.js", | |||
| "url": "https://www.mikescerealshack.co/", | |||
There was a problem hiding this comment.
what audit is this second chunk?
There was a problem hiding this comment.
long-tasks. Similar to bootup-time, it was getting an empty frames array here:
Part of the responsiveness in timespans implementation.
We've previously relied on
FrameCommittedEvents to generate our knowledge of the frame tree, but for traces without navigations (like in a timespan), there typically aren't anyFrameCommittedEvents generated because all the frames are already around. As a result, metric events from outside of the main frame wouldn't be included.TracingStartedInBrowserevents also have frame tree information, but for a typical navigation the main frame URL will change (fromabout:blankto the test url) and child frames won't be known at all, so that frame tree is useless for cross-frame metrics in navigations.This PR combines the two sources, starting with the set from
TracingStartedInBrowserand updating asFrameCommittedEvents come in (if they do). Navigationful and navigationless traces should now have their frame tree correctly figured out.No test changes were needed for all the existing cross-frame tests, and I verified that for all existing tests the set of frames found in the trace are the same before and after this change. This PR includes a test that would fail without this change, but an upcoming PR with the responsiveness metric (edit: #13917) will add a real timespan trace that fails without this change.