Improve reliability of finding navStart #1895
Conversation
|
|
||
| // The first TracingStartedInPage in the trace is definitely our renderer thread of interest | ||
| // Beware: the tracingStartedInPage event can appear slightly after a navigationStart | ||
| const startedInPageEvt = keyEvents.find(e => e.name === 'TracingStartedInPage'); | ||
| // Filter to just events matching the frame ID for sanity | ||
| const frameEvents = keyEvents.filter(e => e.args.frame === startedInPageEvt.args.data.page); | ||
|
|
||
| // Our navStart will be the last frame navigation in the trace | ||
| const navigationStart = frameEvents.filter(e => e.name === 'navigationStart').pop(); |
There was a problem hiding this comment.
can we throw here if it's undefined so we get slightly more friendly error?
| return e.pid === startedInPageEvt.pid; | ||
| }).sort((event0, event1) => event0.ts - event1.ts); | ||
| const processEvents = trace.traceEvents | ||
| .filter(e => { |
|
|
||
| return { | ||
| processEvents, | ||
| startedInPageEvt: startedInPageEvt, | ||
| navigationStartEvt: navigationStart, | ||
| firstPaintEvt: firstPaint, | ||
| firstContentfulPaintEvt: firstContentfulPaint, | ||
| firstMeaningfulPaintEvt: firstMeaningfulPaint | ||
| firstMeaningfulPaintEvt: firstMeaningfulPaint, |
There was a problem hiding this comment.
I started using https://github.com/prettier/prettier on demand in this PR. I'm already in love.
| // Our navStart will be the latest one before fP. | ||
| const navigationStart = frameEvents.filter(e => | ||
| e.name === 'navigationStart' && e.ts < firstPaint.ts).pop(); | ||
| const firstPaint = frameEvents.find(e => e.name === 'firstPaint' && e.ts > navigationStart.ts); |
There was a problem hiding this comment.
yeah. if FP was at the same time, i would have BIG concerns about the trace and i'd need to debug it. because that's definitely not correct.
| const firstContentfulPaint = frameEvents.find(e => | ||
| e.name === 'firstContentfulPaint' && e.ts >= firstPaint.ts); | ||
| const firstContentfulPaint = frameEvents.find( | ||
| e => e.name === 'firstContentfulPaint' && e.ts > navigationStart.ts |
There was a problem hiding this comment.
any significance of dropping the e.ts >= firstPaint.ts and using relative to navigationStart instead?
There was a problem hiding this comment.
we were using FP as the reference event before. Honestly this line could been asking for > navStart rather than FP, but it was just all FP for consistency.
| .filter(e => { | ||
| return e.pid === startedInPageEvt.pid; | ||
| }) | ||
| .sort((event0, event1) => event0.ts - event1.ts); |
There was a problem hiding this comment.
this formatting change makes the diff hard to read :P no difference, right?
| @@ -148,19 +138,19 @@ class Metrics { | |||
| * @param {{ts: number, id: string, name: string}} navStart | |||
There was a problem hiding this comment.
delete navStart from jsdoc
|
thanks! updated. |
| @@ -37,64 +36,64 @@ class Metrics { | |||
| getTs: auditResults => { | |||
| const fmpExt = auditResults['first-meaningful-paint'].extendedInfo; | |||
| return fmpExt.value.timestamps.navStart; | |||
| } | |||
| }, | |||
There was a problem hiding this comment.
this is going to get old :P does it do the whole file when you change anything in it and save?
There was a problem hiding this comment.
Yah the whole file. I can avoid this if we want tho. :)
|
thx guys 👍 |
|
is this fix already released? |
Fixes #1253 (the "ts of undefined" bug)