core(tracing): support missing TracingStartedInBrowser #7122
Conversation
patrickhulce
changed the title
|
I'm just gonna close this, it's messed up on |
FWIW, I'd be good with reopening. Refactoring to |
patrickhulce
changed the title
There was a problem hiding this comment.
LGTM, but @paulirish should we talk to some trace folks before landing this and forgetting about it?
|
Yes indeed @paulirish if you could pull in @a1ph or another DevTools tracing buff perhaps to sanity check this? :) |
| // Support the case where everything else fails, see https://github.com/GoogleChrome/lighthouse/issues/7118. | ||
| // If we can't find either TracingStarted event, then we'll fallback to the first navStart that | ||
| // looks like it was loading the main frame with a real URL. Because the schema for this event | ||
| // has changed across Chrome versions, we'll be extra defensive about finding this case. |
| @@ -237,6 +237,27 @@ class TraceProcessor { | |||
| } | |||
| } | |||
|
|
|||
| // Support the case where everything else fails, see https://github.com/GoogleChrome/lighthouse/issues/7118. | |||
| // If we can't find either TracingStarted event, then we'll fallback to the first navStart that | |||
There was a problem hiding this comment.
back in 2016, we used a diff hack.. which thread had the most events: 1c62db3#diff-a6b18ac2e75e02d6c1e53d586a319643R101
(it could be improved now to just consider renderer threads).
Since this is an uber-fallback case, I wonder if we should try that instead?
@aslushnikov points out that corp managed chrome's that have required chrome apps probably have very active extension threads. might mess this up. also agrees that "most active" is gross. :D
There was a problem hiding this comment.
Since this is an uber-fallback case, I wonder if we should try that instead?
Instead of instead, double check it with most events? :)
| const firstResourceSendEvt = events.find(e => e.name === 'ResourceSendRequest'); | ||
| // We know that these properties exist if we found the events, but TSC doesn't. | ||
| if (navStartEvt && navStartEvt.args && navStartEvt.args.data && | ||
| firstResourceSendEvt && firstResourceSendEvt.tid === navStartEvt.tid) { |
There was a problem hiding this comment.
tid's for renderer are usually the same, so you gotta compare pids as well.
oh, assuming navstart is sent on the same pid? is it? ugh i love this shit. :)
There was a problem hiding this comment.
heh, love is a..... word, haha lovehate? :)
There was a problem hiding this comment.
oops. early on the approval.
we gotta check pids here.
|
🤞 🤞 🤞 |
Summary
I'm not convinced we should actually merge this, butI think it's a good starting point for discussion with someone who knows tracing in and out. There are cases whereTracingStartedInBrowserjust isn't there. In newer chrome, navStart has a bunch of extra useful data we can leverage to try to find the main frame. We could even line up the URLs with the requested URL if we wanted.UPDATE: With m74 hitting stable this has become essential, ~70% of cases in DZL smoketests started failing with NO_TRACING_STARTED. We gotta do something, but DT team input obviously wanted :)
Related Issues/PRs
closes #7118