core(trace-processor): refactor processEvents and frameEvents #14287
Conversation
This reverts commit e283d76.
| expect(frameTreeEventOutput).toMatchInlineSnapshot(` | ||
| Array [ | ||
| "navigationStart - ROOT_FRAME", | ||
| "FrameCommittedInBrowser - ROOT_FRAME", |
There was a problem hiding this comment.
diff-wise: the two FrameCommittedInBrowser items here are the new rows.
I added the frame identifier to spell out the tree.
|
|
||
| const pwaTrace = readJson('../../fixtures/traces/progressive-app.json', import.meta); | ||
| const badNavStartTrace = readJson('../../fixtures/traces/bad-nav-start-ts.json', import.meta); | ||
| const lateTracingStartedTrace = readJson('../../fixtures/traces/tracingstarted-after-navstart.json', import.meta); |
There was a problem hiding this comment.
Turns out startedAfterNavstartTrace (which is already required in this test) is the same asset. I liked that name better.
There was a problem hiding this comment.
Really nice!
- do we have a test trace that switches processes? I assume no but it would be good to have one
- one thing that gets us still is if somehow a
pidgets re-used (however unlikely that is while tracing). Having just a map ofpid->tidsays nothing about the timing. Seems like we might need a temporal aspect to the tracking as well? Or just step though the trace, subsetting in chunks between anyFrameCommittedInBrowserevents. That might be what you mean by "determine the "inspected" pids/frames in one pass before doing all the subsetting", though?
| let tid = threadNameEvt?.tid; | ||
|
|
||
| // Fallback as many test/fixture traces are missing metadata blocks | ||
| if (!tid) { |
There was a problem hiding this comment.
Would there be an easy way to make sure we don't fall back to this in new traces? If somehow the thread_name event changed format it would be good if the change wasn't masked by the fallback still working.
On the other hand we don't want to throw for users if the fallback works fine still, so probably not a good idea to throw directly, but would there be a way to alert to changes to the event in our ToT tests?
|
Can you extract the followup work to a new issue? |
That resolves the only remaining open threads on this PR. |
paulirish
changed the title
5 participants
As discovered in #14264 we were severely excluding events in our
processEventsarray. And alsoframeEvents,frameTreeEvents, andmainThreadEvents.The primary changes here are in trace-processor and this pr contains mostly bug fixes. I envision a second PR which refactors the traceprocessor functions and flow a bit (and wouldn't adjust test results). I can do that here, too, but i'll leave that to the reviewer to opt-in to.
Observations & changes:
findMainFrameIds()returns the "starting" pid, but that will not be the pid of the primary navigation necessarily. that needs to be found afterwards.FrameCommittedInBrowserevent is great and spells out the potentially-newprocessIdof the renderer in the new navigation.args.data.frameorargs.frame. No consistency. Where I saw our code depending on one location, I expanded it to both.Followup work:
edit: filed trace processor - cleanup and fixes ☂️ · #14708