Skip to content
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.

Sign up for GitHub

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

Jump to bottom

core: single network monitor kept on Driver #15055

Merged
merged 3 commits into from
Jul 21, 2023
Merged

core: single network monitor kept on Driver #15055

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1263672
core: single network monitor kept on Driver
connorjclark Jul 19, 2023
a60e6bf
always on
connorjclark Jul 19, 2023
073bc29
fix types
connorjclark Jul 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion core/gather/driver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import log from 'lighthouse-logger';
import {ExecutionContext} from './driver/execution-context.js';
import {TargetManager} from './driver/target-manager.js';
import {Fetcher} from './fetcher.js';
import {NetworkMonitor} from './driver/network-monitor.js';

/** @return {*} */
const throwNotConnectedFn = () => {
Expand Down Expand Up @@ -37,6 +38,8 @@ class Driver {
this._page = page;
/** @type {TargetManager|undefined} */
this._targetManager = undefined;
/** @type {NetworkMonitor|undefined} */
this._networkMonitor = undefined;
/** @type {ExecutionContext|undefined} */
this._executionContext = undefined;
/** @type {Fetcher|undefined} */
Expand All @@ -51,7 +54,6 @@ class Driver {
return this._executionContext;
}

/** @return {Fetcher} */
get fetcher() {
if (!this._fetcher) return throwNotConnectedFn();
return this._fetcher;
Expand All @@ -62,6 +64,11 @@ class Driver {
return this._targetManager;
}

get networkMonitor() {
if (!this._networkMonitor) return throwNotConnectedFn();
return this._networkMonitor;
}

/** @return {Promise<string>} */
async url() {
return this._page.url();
Expand All @@ -75,6 +82,8 @@ class Driver {
const cdpSession = await this._page.target().createCDPSession();
this._targetManager = new TargetManager(cdpSession);
await this._targetManager.enable();
this._networkMonitor = new NetworkMonitor(this._targetManager);
await this._networkMonitor.enable();
this.defaultSession = this._targetManager.rootSession();
this._executionContext = new ExecutionContext(this.defaultSession);
this._fetcher = new Fetcher(this.defaultSession);
Expand All @@ -85,6 +94,7 @@ class Driver {
async disconnect() {
if (this.defaultSession === throwingSession) return;
await this._targetManager?.disable();
await this._networkMonitor?.disable();
await this.defaultSession.dispose();
}
}
Expand Down
5 changes: 1 addition & 4 deletions core/gather/driver/navigation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@

import log from 'lighthouse-logger';

import {NetworkMonitor} from './network-monitor.js';
import {waitForFullyLoaded, waitForFrameNavigated, waitForUserToContinue} from './wait-for-condition.js'; // eslint-disable-line max-len
import * as constants from '../../config/constants.js';
import * as i18n from '../../lib/i18n/i18n.js';
Expand Down Expand Up @@ -89,10 +88,9 @@ async function gotoURL(driver, requestor, options) {
log.time(status);

const session = driver.defaultSession;
const networkMonitor = new NetworkMonitor(driver.targetManager);
const networkMonitor = driver.networkMonitor;

// Enable the events and network monitor needed to track navigation progress.
await networkMonitor.enable();
await session.sendCommand('Page.enable');
await session.sendCommand('Page.setLifecycleEventsEnabled', {enabled: true});

Expand Down Expand Up @@ -144,7 +142,6 @@ async function gotoURL(driver, requestor, options) {

// Bring `Page.navigate` errors back into the promise chain. See https://github.com/GoogleChrome/lighthouse/pull/6739.
await waitForNavigationTriggered;
await networkMonitor.disable();

if (options.debugNavigation) {
await waitForUserToContinue(driver);
Expand Down
1 change: 1 addition & 0 deletions core/gather/driver/network-monitor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {EventEmitter} from 'events';

import log from 'lighthouse-logger';

import * as LH from '../../../types/lh.js';
import {NetworkRecorder} from '../../lib/network-recorder.js';
import {NetworkRequest} from '../../lib/network-request.js';
import UrlUtils from '../../lib/url-utils.js';
Expand Down
5 changes: 1 addition & 4 deletions core/gather/gatherers/full-page-screenshot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
import BaseGatherer from '../base-gatherer.js';
import * as emulation from '../../lib/emulation.js';
import {pageFunctions} from '../../lib/page-functions.js';
import {NetworkMonitor} from '../driver/network-monitor.js';
import {waitForNetworkIdle} from '../driver/wait-for-condition.js';

// JPEG quality setting
Expand Down Expand Up @@ -89,15 +88,14 @@ class FullPageScreenshot extends BaseGatherer {
const height = Math.min(fullHeight, MAX_WEBP_SIZE);

// Setup network monitor before we change the viewport.
const networkMonitor = new NetworkMonitor(context.driver.targetManager);
const networkMonitor = context.driver.networkMonitor;
const waitForNetworkIdleResult = waitForNetworkIdle(session, networkMonitor, {
pretendDCLAlreadyFired: true,
networkQuietThresholdMs: 1000,
busyEvent: 'network-critical-busy',
idleEvent: 'network-critical-idle',
isIdle: recorder => recorder.isCriticalIdle(),
});
await networkMonitor.enable();

await session.sendCommand('Emulation.setDeviceMetricsOverride', {
mobile: deviceMetrics.mobile,
Expand All @@ -113,7 +111,6 @@ class FullPageScreenshot extends BaseGatherer {
waitForNetworkIdleResult.promise,
]);
waitForNetworkIdleResult.cancel();
await networkMonitor.disable();

// Now that new resources are (probably) fetched, wait long enough for a layout.
await context.driver.executionContext.evaluate(waitForDoubleRaf, {args: []});
Expand Down
1 change: 1 addition & 0 deletions core/lib/network-recorder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {EventEmitter} from 'events';

import log from 'lighthouse-logger';

import * as LH from '../../types/lh.js';
import {NetworkRequest} from './network-request.js';
import {PageDependencyGraph} from '../computed/page-dependency-graph.js';

Expand Down
8 changes: 8 additions & 0 deletions core/test/gather/driver/navigation-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ describe('.gotoURL', () => {

it('will track redirects through gotoURL load with warning', async () => {
mockDriver.defaultSession.on = mockDriver.defaultSession.once;
await driver.networkMonitor.enable();

const url = 'http://example.com';

Expand Down Expand Up @@ -91,6 +92,7 @@ describe('.gotoURL', () => {

it('backfills requestedUrl when using a callback requestor', async () => {
mockDriver.defaultSession.on = mockDriver.defaultSession.once;
await driver.networkMonitor.enable();

const requestor = () => Promise.resolve();

Expand All @@ -110,6 +112,7 @@ describe('.gotoURL', () => {

it('throws if no navigations found using a callback requestor', async () => {
mockDriver.defaultSession.on = mockDriver.defaultSession.once;
await driver.networkMonitor.enable();

const requestor = () => Promise.resolve();

Expand All @@ -129,6 +132,7 @@ describe('.gotoURL', () => {

it('does not add warnings when URLs are equal', async () => {
mockDriver.defaultSession.on = mockDriver.defaultSession.once;
await driver.networkMonitor.enable();

const url = 'https://www.example.com';

Expand All @@ -145,6 +149,7 @@ describe('.gotoURL', () => {

it('waits for Page.frameNavigated', async () => {
mockDriver.defaultSession.on = mockDriver.defaultSession.once;
await driver.networkMonitor.enable();

const url = 'https://www.example.com';

Expand All @@ -163,6 +168,7 @@ describe('.gotoURL', () => {

it('waits for page load', async () => {
mockDriver.defaultSession.on = mockDriver.defaultSession.once;
await driver.networkMonitor.enable();

const url = 'https://www.example.com';

Expand Down Expand Up @@ -192,6 +198,7 @@ describe('.gotoURL', () => {

it('waits for page FCP', async () => {
mockDriver.defaultSession.on = mockDriver.defaultSession.once;
await driver.networkMonitor.enable();

const url = 'https://www.example.com';

Expand Down Expand Up @@ -226,6 +233,7 @@ describe('.gotoURL', () => {

it('throws when asked to wait for FCP without waiting for load', async () => {
mockDriver.defaultSession.on = mockDriver.defaultSession.once;
await driver.networkMonitor.enable();

const url = 'https://www.example.com';

Expand Down
4 changes: 3 additions & 1 deletion core/test/gather/mock-driver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
} from './mock-commands.js';
import * as constants from '../../config/constants.js';
import {fnAny} from '../test-utils.js';
import {NetworkMonitor} from '../../gather/driver/network-monitor.js';

/** @typedef {import('../../gather/driver.js').Driver} Driver */
/** @typedef {import('../../gather/driver/execution-context.js')} ExecutionContext */
Expand Down Expand Up @@ -141,7 +142,7 @@ function createMockTargetManager(session) {
on: createMockOnFn(),
off: fnAny(),

/** @return {import('../../gather/driver/target-manager.js')} */
/** @return {LH.Gatherer.Driver['targetManager']} */
asTargetManager() {
// @ts-expect-error - We'll rely on the tests passing to know this matches.
return this;
Expand All @@ -168,6 +169,7 @@ function createMockDriver() {
fetcher: {
fetchResource: fnAny(),
},
networkMonitor: new NetworkMonitor(targetManager.asTargetManager()),

/** @return {Driver} */
asDriver() {
Expand Down
2 changes: 2 additions & 0 deletions types/gatherer.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {NetworkNode as _NetworkNode} from '../core/lib/dependency-graph/network-
import {CPUNode as _CPUNode} from '../core/lib/dependency-graph/cpu-node.js';
import {Simulator as _Simulator} from '../core/lib/dependency-graph/simulator/simulator.js';
import {ExecutionContext} from '../core/gather/driver/execution-context.js';
import {NetworkMonitor} from '../core/gather/driver/network-monitor.js';
import {Fetcher} from '../core/gather/fetcher.js';
import {ArbitraryEqualityMap} from '../core/lib/arbitrary-equality-map.js';

Expand Down Expand Up @@ -48,6 +49,7 @@ declare module Gatherer {
on(event: 'protocolevent', callback: (payload: Protocol.RawEventMessage) => void): void
off(event: 'protocolevent', callback: (payload: Protocol.RawEventMessage) => void): void
};
networkMonitor: NetworkMonitor;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know I should type just the public interface here, but then we get this error...

image

How can we resolve this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we pull waitForFullyLoaded (and whatever else...) into NetworkMonitor? expose as NetworkMonitor.waitForFullyLoaded(session, opts)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh, waitForFullyLoaded does other non-network stuff like CPU idle. I think it's fine this way personally.

}

interface Context<TDependencies extends DependencyKey = DefaultDependenciesKey> {
Expand Down
Loading