core(fr): collect OOPIF network data #12992
Conversation
patrickhulce
requested review from
connorjclark
and removed request for
a team
|
friendly ping here :) |
| }); | ||
| } | ||
|
|
||
| if (configJson.categories) { |
There was a problem hiding this comment.
what's going on here? does this not break form-config / form smoke?
There was a problem hiding this comment.
form-config is disabled because it doesn't work without the flag :)
Other options I see:
- hardcode this function to exclude only this oopif audit
- Add this as a default audit
- Include this audit in the DT bundle
- Test a different bundle than the DT bundle that does include this audit
Any preferences?
There was a problem hiding this comment.
ha, sorry, missed that you had already replied
lighthouse-cli/test/smokehouse/test-definitions/oopif/oopif-config.js
Outdated
Show resolved
Hide resolved
| @@ -30,6 +30,32 @@ global.require = originalRequire; | |||
| // @ts-expect-error - not worth giving test global an actual type. | |||
| const lighthouse = global.runBundledLighthouse; | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
this seems like a lot of workaround for
in Fraggle Rock unused artifacts are filtered out of the config
if the goal is for legacy and FR to match soon, it seems like this is the thing to resolve rather than adding a bunch of new test stuff to support both? Downside is I found the two halves to make it work really hard to follow :)
If we reallllllly want to put off that decision, is this issue resolvable by injecting oopif-iframe-audit into the dt bundle?
There was a problem hiding this comment.
it seems like this is the thing to resolve rather than adding a bunch of new test stuff to support both?
I'm not sure how this solves our problem given that we wanted to support unused artifact filtering for awhile. Aligning on this removes the _skipInBundled logic but still has this underlying issue that iframeelements is unused.
If we reallllllly want to put off that decision, is this issue resolvable by injecting oopif-iframe-audit into the dt bundle?
Yes that's one of the options I outlined, is that your favorite one?
There was a problem hiding this comment.
. Aligning on this removes the _skipInBundled logic but still has this underlying issue that iframeelements is unused.
I guess implicit in that is that any PR to make legacy filtered by default would have to fix the problem as well :)
Yes that's one of the options I outlined, is that your favorite one?
if we were already filtering by default when we landed the iframeelements artifact, it does feel like we'd either have done that or added an outright pubads smoke test (since that's also already in the dt bundle). I'm cool with adding the test audit to the bundle. It could even be the clearinghouse audit specifically for including artifacts not being used by the default audits :)
| @@ -31,11 +35,19 @@ class NetworkMonitor extends EventEmitter { | |||
| super(); | |||
| this._session = session; | |||
|
|
|||
| this._messageLog = new MessageLog(/^(Page|Network)\./); | |||
There was a problem hiding this comment.
file naming may be off now. network-monitor now doing devtools-log duties (recording Page events too). devtools-monitor?
There was a problem hiding this comment.
the page events are largely network related (lifecycle networkidle) and aren't even used, so I wasn't very concerned about the extra bit. if folks feel it's a candidate for confusion, I can emit the messages and have the gatherer merge with page events instead, but that extra level of indirection felt unnecessary
There was a problem hiding this comment.
The restructuring seems totally fine, I was just talking about naming.
the page events are largely network related (lifecycle networkidle) and aren't even used
but now DevtoolsLog the Gatherer gets the devtools log from NetworkMonitor. A rename might be in order?
There was a problem hiding this comment.
gotcha 👍 protocol-monitor SGTM
There was a problem hiding this comment.
gotcha 👍 protocol-monitor SGTM
lol I take it you changed your mind? :)
There was a problem hiding this comment.
yeah I got in there and it just didn't feel right, everything is so network-focused.
just reemitting the messages and letting the log continue to be captured by the gatherer :) (also bonus we get strict event listener types for the monitor now)
| const target = await session.sendCommand('Target.getTargetInfo').catch(() => null); | ||
| const targetType = target && target.targetInfo && target.targetInfo.type; | ||
| const hasValidTargetType = targetType === 'page' || targetType === 'iframe'; | ||
| if (!target || !hasValidTargetType) return; | ||
|
|
||
| const targetId = target.targetInfo.targetId; | ||
| session.setTargetInfo(target.targetInfo); |
There was a problem hiding this comment.
could session take care of this itself rather than requiring an external class to query and set the targetInfo?
There was a problem hiding this comment.
it could, although there is no async initialization lifecycle on sessions today. having an async init that could fail to be called felt similarly brittle while also violating the lightweight connection invariant we have currently. passing the targetInfo in at the constructor was the closest I'm certainly up for, but that's still an external class querying and setting the targetInfo if that's where your concern is.
There was a problem hiding this comment.
it could, although there is no async initialization lifecycle on sessions today
my point was that if this is the only use of subsession initialization and it's async to any observer of target-manager, might as well encapsulate session state in session, but sounds like that might be premature and the shape of things is still developing.
| log.verbose('target-manager', `target ${targetName} attached`); | ||
|
|
||
| const targetWithSession = {target: target.targetInfo, session}; | ||
| this._targets.set(targetId, targetWithSession); |
There was a problem hiding this comment.
is targetWithSession in this map eventually needed or could it just be a Set of targetIds?
There was a problem hiding this comment.
and is the plan still to eventually have multiple sessions per target?
There was a problem hiding this comment.
and is the plan still to eventually have multiple sessions per target?
yep
is targetWithSession in this map eventually needed or could it just be a Set of targetIds?
I toyed around with versions that exposed the targets to consumers which is why they're here, and I think we'll eventually do that for creating new sessions/exploring targets, but if it's concerning we can switch to just the set.
There was a problem hiding this comment.
I toyed around with versions that exposed the targets to consumers which is why they're here, and I think we'll eventually do that for creating new sessions/exploring targets, but if it's concerning we can switch to just the set.
just live blogging me reading through the code and reasoning through what things are used for what. Set is probably the right thing to use for the current code, but it's not like there's going to be some deep YAGNI penalty for using a map with unused (for now) values over a set :)
| /** @param {LH.Gatherer.FRProtocolSession} session */ | ||
| constructor(session) { | ||
| this._enabled = false; | ||
| this._session = session; |
There was a problem hiding this comment.
it feels like if the intention is we have a root session that maybe we should start naming it something special since it's not like the other sessions, but no rush to even think about that right now.
| */ | ||
| async _onTargetAttached({session, target}) { | ||
| const targetId = target.targetId; | ||
| if (this._sessions.has(targetId)) return; |
There was a problem hiding this comment.
won't the targetManager already be ensuring this won't be called twice with the same targetId?
There was a problem hiding this comment.
yes in this version I don't believe this line is necessary anymore 👍
Co-authored-by: Brendan Kenny <bckenny@gmail.com>
Summary
Adds OOPIF support to Fraggle Rock.
I'm still working on tests, but I lost too many days to brittle refactors, so I'm just putting up this as-is for feedback.Bringing support here involved a number of components...connection()support.sessionattachedevents onFRProtocolSession.sessionIdfromtargetInfodata on each session protocol event.TargetManagerclass for managing the many sessions/targets we have going on.NetworkMonitorto leverageTargetManagerfor its network listeners.DevtoolsLoggatherer to useNetworkMonitorto avoid duplication of this complex logic.Related Issues/PRs
ref #12861 #11313