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.

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): add artifact dependency support #12045

Merged
merged 4 commits into from
Feb 8, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
50 changes: 48 additions & 2 deletions lighthouse-core/fraggle-rock/config/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,13 @@ const log = require('lighthouse-logger');
const Runner = require('../../runner.js');
const defaultConfig = require('./default-config.js');
const {defaultNavigationConfig} = require('../../config/constants.js');
const {isFRGathererDefn} = require('./validation.js');
const {
isFRGathererDefn,
throwInvalidDependencyOrder,
isValidArtifactDependency,
throwInvalidArtifactDependency,
assertArtifactTopologicalOrder,
} = require('./validation.js');
const {filterConfigByGatherMode} = require('./filters.js');
const {
deepCloneConfigJson,
Expand Down Expand Up @@ -47,6 +53,35 @@ function resolveWorkingCopy(configJSON, context) {
};
}

/**
* Looks up the required artifact IDs for each dependency, throwing if no earlier artifact satisfies the dependency.
*
* @param {LH.Config.ArtifactJson} artifact
* @param {LH.Config.FRGathererDefn} gatherer
* @param {Map<Symbol, LH.Config.ArtifactDefn>} artifactDefnsBySymbol
* @return {LH.Config.ArtifactDefn['dependencies']}
*/
function resolveArtifactDependencies(artifact, gatherer, artifactDefnsBySymbol) {
if (!('dependencies' in gatherer.instance.meta)) return undefined;

const dependencies = Object.entries(gatherer.instance.meta.dependencies).map(
([dependencyName, artifactSymbol]) => {
const dependency = artifactDefnsBySymbol.get(artifactSymbol);

// Check that dependency was defined before us.
if (!dependency) throwInvalidDependencyOrder(artifact.id, dependencyName);

// Check that the phase relationship is OK too.
const validDependency = isValidArtifactDependency(gatherer, dependency.gatherer);
if (!validDependency) throwInvalidArtifactDependency(artifact.id, dependencyName);

return [dependencyName, {id: dependency.id}];
}
);

return Object.fromEntries(dependencies);
}

/**
*
* @param {LH.Config.ArtifactJson[]|null|undefined} artifacts
Expand All @@ -59,6 +94,9 @@ function resolveArtifactsToDefns(artifacts, configDir) {
const status = {msg: 'Resolve artifact definitions', id: 'lh:config:resolveArtifactsToDefns'};
log.time(status, 'verbose');

/** @type {Map<Symbol, LH.Config.ArtifactDefn>} */
const artifactDefnsBySymbol = new Map();

const coreGathererList = Runner.getGathererList();
const artifactDefns = artifacts.map(artifactJson => {
/** @type {LH.Config.GathererJson} */
Expand All @@ -70,10 +108,16 @@ function resolveArtifactsToDefns(artifacts, configDir) {
throw new Error(`${gatherer.instance.name} gatherer does not support Fraggle Rock`);
}

return {
/** @type {LH.Config.ArtifactDefn<LH.Gatherer.DependencyKey>} */
const artifact = {
id: artifactJson.id,
gatherer,
dependencies: resolveArtifactDependencies(artifactJson, gatherer, artifactDefnsBySymbol),
};

const symbol = artifact.gatherer.instance.meta.symbol;
if (symbol) artifactDefnsBySymbol.set(symbol, artifact);
return artifact;
});

log.timeEnd(status);
Expand Down Expand Up @@ -109,6 +153,8 @@ function resolveNavigationsToDefns(navigations, artifactDefns) {
return {...navigationWithDefaults, artifacts};
});

assertArtifactTopologicalOrder(navigationDefns);

log.timeEnd(status);
return navigationDefns;
}
Expand Down
76 changes: 75 additions & 1 deletion lighthouse-core/fraggle-rock/config/validation.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,78 @@ function isFRGathererDefn(gathererDefn) {
return 'meta' in gathererDefn.instance;
}

module.exports = {isFRGathererDefn};
/**
* Determines if the artifact dependency direction is valid.
* A timespan artifact cannot depend on a snapshot/navigation artifact because snapshot runs after timespan.
* A snapshot artifact cannot depend on a navigation artifact because it might be run without a navigation.
* In other words, the dependency's minimum supported mode must be less than or equal to the dependent's.
*
* @param {LH.Config.FRGathererDefn} dependent The artifact that depends on the other.
* @param {LH.Config.FRGathererDefn} dependency The artifact that is being depended on by the other.
* @return {boolean}
*/
function isValidArtifactDependency(dependent, dependency) {
const levels = {timespan: 0, snapshot: 1, navigation: 2};
const dependentLevel = Math.min(...dependent.instance.meta.supportedModes.map(l => levels[l]));
const dependencyLevel = Math.min(...dependency.instance.meta.supportedModes.map(l => levels[l]));
return dependencyLevel <= dependentLevel;
}

/**
* Asserts that artifacts are in a valid dependency order that can be computed.
*
* @param {Array<LH.Config.NavigationDefn>} navigations
*/
function assertArtifactTopologicalOrder(navigations) {
const availableArtifacts = new Set();

for (const navigation of navigations) {
for (const artifact of navigation.artifacts) {
availableArtifacts.add(artifact.id);
if (!artifact.dependencies) continue;

for (const [dependencyKey, {id: dependencyId}] of Object.entries(artifact.dependencies)) {
if (availableArtifacts.has(dependencyId)) continue;
throwInvalidDependencyOrder(artifact.id, dependencyKey);
}
}
}
}

/**
* @param {string} artifactId
* @param {string} dependencyKey
* @return {never}
*/
function throwInvalidDependencyOrder(artifactId, dependencyKey) {
throw new Error(
[
`Failed to find dependency "${dependencyKey}" for "${artifactId}" artifact`,
`Check that...`,
` 1. A gatherer exposes a matching Symbol that satisfies "${dependencyKey}".`,
` 2. "${dependencyKey}" is configured to run before "${artifactId}"`,
].join('\n')
);
}

/**
* @param {string} artifactId
* @param {string} dependencyKey
* @return {never}
*/
function throwInvalidArtifactDependency(artifactId, dependencyKey) {
throw new Error(
[
`Dependency "${dependencyKey}" for "${artifactId}" artifact is invalid.`,
`A timespan artifact cannot depend on a snapshot artifact.`,
].join('\n')
);
}

module.exports = {
isFRGathererDefn,
isValidArtifactDependency,
assertArtifactTopologicalOrder,
throwInvalidDependencyOrder,
throwInvalidArtifactDependency,
};
6 changes: 3 additions & 3 deletions lighthouse-core/fraggle-rock/gather/base-gatherer.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ class FRGatherer {
* @return {Promise<LH.Gatherer.PhaseResultNonPromise>}
*/
async beforePass(passContext) {
await this.beforeTimespan(passContext);
await this.beforeTimespan({...passContext, dependencies: {}});
}

/**
Expand All @@ -73,10 +73,10 @@ class FRGatherer {
*/
async afterPass(passContext, loadData) {
if (this.meta.supportedModes.includes('timespan')) {
return this.afterTimespan(passContext);
return this.afterTimespan({...passContext, dependencies: {}});
}

return this.snapshot(passContext);
return this.snapshot({...passContext, dependencies: {}});
}
}

Expand Down
23 changes: 18 additions & 5 deletions lighthouse-core/fraggle-rock/gather/navigation-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

const Driver = require('./driver.js');
const Runner = require('../../runner.js');
const {collectArtifactDependencies} = require('./runner-helpers.js');
const {defaultNavigationConfig} = require('../../config/constants.js');
const {initializeConfig} = require('../config/config.js');
const {getBaseArtifacts} = require('./base-artifacts.js');
Expand Down Expand Up @@ -56,7 +57,11 @@ async function _beforeTimespanPhase(navigationContext, artifacts) {
if (!gatherer.meta.supportedModes.includes('timespan')) continue;

const artifactPromise = Promise.resolve().then(() =>
gatherer.beforeTimespan({driver: navigationContext.driver, gatherMode: 'navigation'})
gatherer.beforeTimespan({
driver: navigationContext.driver,
gatherMode: 'navigation',
dependencies: {},
})
);
artifacts[artifactDefn.id] = artifactPromise;
await artifactPromise.catch(() => {});
Expand Down Expand Up @@ -84,8 +89,12 @@ async function _afterTimespanPhase(navigationContext, artifacts) {
const gatherer = artifactDefn.gatherer.instance;
if (!gatherer.meta.supportedModes.includes('timespan')) continue;

const artifactPromise = (artifacts[artifactDefn.id] || Promise.resolve()).then(() =>
gatherer.afterTimespan({driver: navigationContext.driver, gatherMode: 'navigation'})
const artifactPromise = (artifacts[artifactDefn.id] || Promise.resolve()).then(async () =>
gatherer.afterTimespan({
driver: navigationContext.driver,
gatherMode: 'navigation',
dependencies: await collectArtifactDependencies(artifactDefn, artifacts),
})
);
artifacts[artifactDefn.id] = artifactPromise;
await artifactPromise.catch(() => {});
Expand All @@ -101,8 +110,12 @@ async function _snapshotPhase(navigationContext, artifacts) {
const gatherer = artifactDefn.gatherer.instance;
if (!gatherer.meta.supportedModes.includes('snapshot')) continue;

const artifactPromise = Promise.resolve().then(() =>
gatherer.snapshot({driver: navigationContext.driver, gatherMode: 'navigation'})
const artifactPromise = Promise.resolve().then(async () =>
gatherer.snapshot({
driver: navigationContext.driver,
gatherMode: 'navigation',
dependencies: await collectArtifactDependencies(artifactDefn, artifacts),
})
);
artifacts[artifactDefn.id] = artifactPromise;
await artifactPromise.catch(() => {});
Expand Down
36 changes: 36 additions & 0 deletions lighthouse-core/fraggle-rock/gather/runner-helpers.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/**
* @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';

/**
* @param {LH.Config.ArtifactDefn} artifact
* @param {Record<string, LH.Gatherer.PhaseResult>} artifactsById
* @return {Promise<LH.Gatherer.FRTransitionalContext<LH.Gatherer.DependencyKey>['dependencies']>}
*/
async function collectArtifactDependencies(artifact, artifactsById) {
Copy link
Member

Choose a reason for hiding this comment

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

Why does this need to be async?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

would you prefer to convert the final await to a .then?

or asking why we want to convert the artifact dependencies from promises to the resolved artifacts?

Copy link
Member

Choose a reason for hiding this comment

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

or asking why we want to convert the artifact dependencies from promises to the resolved artifacts?

This. I don't really have a suggestion, just trying to understand this.

Copy link
Collaborator Author

@patrickhulce patrickhulce Feb 5, 2021

Choose a reason for hiding this comment

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

We want to pass the artifact directly to a gatherer that is depending on it so the burden doesn't fall on consumers to await and handle any errors. Resolving any errors ahead of time also means every individual gatherer doesn't have to remap their dependencies' errors. We do a similar thing for audits today, pass them artifacts, not promises of artifacts and remap their artifacts when they fail.

// If artifact was an error, output error result on behalf of audit.
if (artifacts[artifactName] instanceof Error) {
/** @type {Error} */
// @ts-expect-error An artifact *could* be an Error, but caught here, so ignore elsewhere.
const artifactError = artifacts[artifactName];
Sentry.captureException(artifactError, {
tags: {gatherer: artifactName},
level: 'error',
});
log.warn('Runner', `${artifactName} gatherer, required by audit ${audit.meta.id},` +
` encountered an error: ${artifactError.message}`);
// Create a friendlier display error and mark it as expected to avoid duplicates in Sentry
const error = new LHError(LHError.errors.ERRORED_REQUIRED_ARTIFACT,
{artifactName, errorMessage: artifactError.message});

if (!artifact.dependencies) return {};

const dependencyPromises = Object.entries(artifact.dependencies).map(
async ([dependencyName, dependency]) => {
const dependencyArtifact = artifactsById[dependency.id];
if (dependencyArtifact === undefined) throw new Error(`"${dependency.id}" did not run`);

const dependencyPromise = Promise.resolve()
.then(() => dependencyArtifact)
.catch(err =>
Promise.reject(
new Error(`Dependency "${dependency.id}" failed with exception: ${err.message}`)
)
);

return [dependencyName, await dependencyPromise];
}
);

return Object.fromEntries(await Promise.all(dependencyPromises));
}

module.exports = {collectArtifactDependencies};
7 changes: 5 additions & 2 deletions lighthouse-core/fraggle-rock/gather/snapshot-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

const Driver = require('./driver.js');
const Runner = require('../../runner.js');
const {collectArtifactDependencies} = require('./runner-helpers.js');
const {initializeConfig} = require('../config/config.js');
const {getBaseArtifacts} = require('./base-artifacts.js');

Expand All @@ -27,10 +28,12 @@ async function snapshot(options) {
/** @type {Partial<LH.GathererArtifacts>} */
const artifacts = {};

for (const {id, gatherer} of config.artifacts || []) {
for (const artifactDefn of config.artifacts || []) {
const {id, gatherer} = artifactDefn;
const artifactName = /** @type {keyof LH.GathererArtifacts} */ (id);
const dependencies = await collectArtifactDependencies(artifactDefn, artifacts);
const artifact = await Promise.resolve()
.then(() => gatherer.instance.snapshot({gatherMode: 'snapshot', driver}))
.then(() => gatherer.instance.snapshot({gatherMode: 'snapshot', driver, dependencies}))
.catch(err => err);

artifacts[artifactName] = artifact;
Expand Down
27 changes: 17 additions & 10 deletions lighthouse-core/fraggle-rock/gather/timespan-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

const Driver = require('./driver.js');
const Runner = require('../../runner.js');
const {collectArtifactDependencies} = require('./runner-helpers.js');
const {initializeConfig} = require('../config/config.js');
const {getBaseArtifacts} = require('./base-artifacts.js');

Expand All @@ -21,13 +22,17 @@ async function startTimespan(options) {

const requestedUrl = await options.page.url();

/** @type {Record<string, Promise<Error|undefined>>} */
/** @type {Record<string, Promise<void>>} */
const artifactErrors = {};
const dependencies = {};
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved

for (const {id, gatherer} of config.artifacts || []) {
artifactErrors[id] = await Promise.resolve()
.then(() => gatherer.instance.beforeTimespan({gatherMode: 'timespan', driver}))
.catch(err => err);
artifactErrors[id] = Promise.resolve().then(() =>
gatherer.instance.beforeTimespan({gatherMode: 'timespan', driver, dependencies})
);

// Run each beforeTimespan serially, but handle errors in the next pass.
await artifactErrors[id].catch(() => {});
}

return {
Expand All @@ -42,13 +47,15 @@ async function startTimespan(options) {
/** @type {Partial<LH.GathererArtifacts>} */
const artifacts = {};

for (const {id, gatherer} of config.artifacts || []) {
for (const artifactDefn of config.artifacts || []) {
const {id, gatherer} = artifactDefn;
const artifactName = /** @type {keyof LH.GathererArtifacts} */ (id);
const artifact = artifactErrors[id]
? Promise.reject(artifactErrors[id])
: await Promise.resolve()
.then(() => gatherer.instance.afterTimespan({gatherMode: 'timespan', driver}))
.catch(err => err);
const dependencies = await collectArtifactDependencies(artifactDefn, artifacts);
const artifact = await artifactErrors[id]
.then(() =>
gatherer.instance.afterTimespan({gatherMode: 'timespan', driver, dependencies})
)
.catch(err => err);

artifacts[artifactName] = artifact;
}
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/gather/gatherers/seo/tap-targets.js
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@ class TapTargets extends FRGatherer {
* @param {LH.Gatherer.FRTransitionalContext} passContext
* @return {Promise<LH.Artifacts.TapTarget[]>} All visible tap targets with their positions and sizes
*/
afterPass(passContext) {
snapshot(passContext) {
return passContext.driver.evaluate(gatherTapTargets, {
args: [tapTargetsSelector],
useIsolation: true,
Expand Down
Loading