-
Notifications
You must be signed in to change notification settings - Fork 9.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
core(fr): collect OOPIF network data #12992
Changes from 6 commits
6af035a
75f4394
762f18b
1d464b3
8ee7513
c179026
bbb12bd
15d96ff
93787d5
5110856
1f3c187
b5d3d4a
f03370a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,6 +30,32 @@ global.require = originalRequire; | |
// @ts-expect-error - not worth giving test global an actual type. | ||
const lighthouse = global.runBundledLighthouse; | ||
|
||
/** | ||
* Clean the config of any additional audits that might not be able to be referenced. | ||
* A new audit is detected by finding any audit entry with a path and no options. | ||
* | ||
* @param {LH.Config.Json=} configJson | ||
*/ | ||
function cleanConfig(configJson) { | ||
if (!configJson) return; | ||
|
||
if (configJson.audits) { | ||
configJson.audits = configJson.audits.filter(audit => { | ||
// Just a string, is adding a new audit that won't be in the bundle, prune it. | ||
if (typeof audit === 'string') return false; | ||
// If it's the audit implementation directly, it's a new audit, prune it. (though not currently possible) | ||
if (typeof audit !== 'object') return false; | ||
// Keep the audit only if it is setting options, which is the signal that it's an existing core audit. | ||
return Boolean(audit.options); | ||
}); | ||
} | ||
|
||
if (configJson.categories) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what's going on here? does this not break There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. form-config is disabled because it doesn't work without the flag :) Other options I see:
Any preferences? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ha, sorry, missed that you had already replied |
||
// Categories are only defined for introduction of new categories. | ||
delete configJson.categories; | ||
} | ||
} | ||
|
||
/** | ||
* Launch Chrome and do a full Lighthouse run via the Lighthouse CLI. | ||
* @param {string} url | ||
|
@@ -42,6 +68,7 @@ async function runLighthouse(url, configJson, testRunnerOptions = {}) { | |
const launchedChrome = await ChromeLauncher.launch(); | ||
const port = launchedChrome.port; | ||
const connection = new ChromeProtocol(port); | ||
cleanConfig(configJson); | ||
|
||
// Run Lighthouse. | ||
const logLevel = testRunnerOptions.isDebug ? 'info' : undefined; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,7 @@ module.exports = { | |
}, | ||
}, | ||
artifacts: { | ||
_skipInBundled: true, | ||
IFrameElements: [ | ||
{ | ||
id: 'oopif', | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
/** | ||
* @license Copyright 2021 The Lighthouse Authors. All Rights Reserved. | ||
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 | ||
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. | ||
*/ | ||
'use strict'; | ||
|
||
module.exports = { | ||
meta: { | ||
id: 'iframe-elements', | ||
title: 'IFrame Elements', | ||
description: 'Audit to force the inclusion of IFrameElements artifact', | ||
requiredArtifacts: ['IFrameElements'], | ||
}, | ||
audit: () => ({score: 1}), | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,9 +12,11 @@ | |
|
||
const log = require('lighthouse-logger'); | ||
const {EventEmitter} = require('events'); | ||
const MessageLog = require('../devtools-log.js'); | ||
const NetworkRecorder = require('../../lib/network-recorder.js'); | ||
const NetworkRequest = require('../../lib/network-request.js'); | ||
const URL = require('../../lib/url-shim.js'); | ||
const TargetManager = require('./target-manager.js'); | ||
|
||
/** @typedef {import('../../lib/network-recorder.js').NetworkRecorderEvent} NetworkRecorderEvent */ | ||
/** @typedef {'network-2-idle'|'network-critical-idle'|'networkidle'|'networkbusy'|'network-critical-busy'|'network-2-busy'} NetworkMonitorEvent_ */ | ||
|
@@ -23,6 +25,8 @@ const URL = require('../../lib/url-shim.js'); | |
class NetworkMonitor extends EventEmitter { | ||
/** @type {NetworkRecorder|undefined} */ | ||
_networkRecorder = undefined; | ||
/** @type {TargetManager|undefined} */ | ||
_targetManager = undefined; | ||
/** @type {Array<LH.Crdp.Page.Frame>} */ | ||
_frameNavigations = []; | ||
|
||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. file naming may be off now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The restructuring seems totally fine, I was just talking about naming.
but now There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. gotcha 👍 protocol-monitor SGTM There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
lol I take it you changed your mind? :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) |
||
|
||
this._onTargetAttached = this._onTargetAttached.bind(this); | ||
|
||
/** @type {Map<string, LH.Gatherer.FRProtocolSession>} */ | ||
this._sessions = new Map(); | ||
|
||
/** @param {LH.Crdp.Page.FrameNavigatedEvent} event */ | ||
this._onFrameNavigated = event => this._frameNavigations.push(event.frame); | ||
|
||
/** @param {LH.Protocol.RawEventMessage} event */ | ||
this._onProtocolMessage = event => { | ||
this._messageLog.record(event); | ||
if (!this._networkRecorder) return; | ||
this._networkRecorder.dispatch(event); | ||
}; | ||
|
@@ -49,14 +61,29 @@ class NetworkMonitor extends EventEmitter { | |
this.off = (event, listener) => super.off(event, listener); | ||
} | ||
|
||
/** | ||
* @param {{target: {targetId: string}, session: LH.Gatherer.FRProtocolSession}} session | ||
*/ | ||
async _onTargetAttached({session, target}) { | ||
const targetId = target.targetId; | ||
if (this._sessions.has(targetId)) return; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. won't the targetManager already be ensuring this won't be called twice with the same targetId? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes in this version I don't believe this line is necessary anymore 👍 |
||
|
||
this._sessions.set(targetId, session); | ||
session.addProtocolMessageListener(this._onProtocolMessage); | ||
|
||
await session.sendCommand('Network.enable'); | ||
} | ||
|
||
/** | ||
* @return {Promise<void>} | ||
*/ | ||
async enable() { | ||
if (this._networkRecorder) return; | ||
if (this._targetManager) return; | ||
|
||
this._frameNavigations = []; | ||
this._sessions = new Map(); | ||
this._networkRecorder = new NetworkRecorder(); | ||
this._targetManager = new TargetManager(this._session); | ||
|
||
/** | ||
* Reemit the same network recorder events. | ||
|
@@ -71,22 +98,38 @@ class NetworkMonitor extends EventEmitter { | |
this._networkRecorder.on('requeststarted', reEmit('requeststarted')); | ||
this._networkRecorder.on('requestloaded', reEmit('requestloaded')); | ||
|
||
this._messageLog.reset(); | ||
this._messageLog.beginRecording(); | ||
|
||
this._session.on('Page.frameNavigated', this._onFrameNavigated); | ||
this._session.addProtocolMessageListener(this._onProtocolMessage); | ||
this._targetManager.addTargetAttachedListener(this._onTargetAttached); | ||
|
||
await this._session.sendCommand('Page.enable'); | ||
await this._session.sendCommand('Network.enable'); | ||
await this._targetManager.enable(); | ||
} | ||
|
||
/** | ||
* @return {Promise<void>} | ||
*/ | ||
async disable() { | ||
if (!this._targetManager) return; | ||
|
||
this._messageLog.reset(); | ||
this._messageLog.endRecording(); | ||
|
||
this._session.off('Page.frameNavigated', this._onFrameNavigated); | ||
this._session.removeProtocolMessageListener(this._onProtocolMessage); | ||
this._targetManager.removeTargetAttachedListener(this._onTargetAttached); | ||
|
||
for (const session of this._sessions.values()) { | ||
session.removeProtocolMessageListener(this._onProtocolMessage); | ||
} | ||
|
||
await this._targetManager.disable(); | ||
|
||
this._frameNavigations = []; | ||
this._networkRecorder = undefined; | ||
this._targetManager = undefined; | ||
this._sessions = new Map(); | ||
} | ||
|
||
/** @return {Promise<string | undefined>} */ | ||
|
@@ -103,6 +146,13 @@ class NetworkMonitor extends EventEmitter { | |
return finalNavigation && finalNavigation.url; | ||
} | ||
|
||
/** | ||
* @return {LH.DevtoolsLog} | ||
*/ | ||
getMessageLog() { | ||
return this._messageLog.messages; | ||
} | ||
|
||
/** | ||
* @return {Array<NetworkRequest>} | ||
*/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems like a lot of workaround for
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.Yes that's one of the options I outlined, is that your favorite one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess implicit in that is that any PR to make legacy filtered by default would have to fix the problem as well :)
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 :)