From f55d5360de9fb1300d33d42cf4feb7cd874705e0 Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Wed, 4 Jun 2025 15:40:35 -0700 Subject: [PATCH] core(trace-elements): remove sentry debugging for invalid impactedNodes --- core/audits/insights/cls-culprits-insight.js | 2 +- core/audits/layout-shifts.js | 2 +- core/gather/gatherers/trace-elements.js | 50 ++++--------------- core/runner.js | 6 +-- .../gather/gatherers/trace-elements-test.js | 9 ---- 5 files changed, 14 insertions(+), 55 deletions(-) diff --git a/core/audits/insights/cls-culprits-insight.js b/core/audits/insights/cls-culprits-insight.js index 993c562ada39..0ea0849f53a1 100644 --- a/core/audits/insights/cls-culprits-insight.js +++ b/core/audits/insights/cls-culprits-insight.js @@ -114,7 +114,7 @@ class CLSCulpritsInsight extends Audit { /** @type {LH.Audit.Details.Table['items']} */ const items = events.map(event => { const biggestImpactNodeId = TraceElements.getBiggestImpactNodeForShiftEvent( - event.args.data.impacted_nodes || [], impactByNodeId, event); + event.args.data.impacted_nodes || [], impactByNodeId); return { node: makeNodeItemForNodeId(artifacts.TraceElements, biggestImpactNodeId), score: event.args.data?.weighted_score_delta, diff --git a/core/audits/layout-shifts.js b/core/audits/layout-shifts.js index 22817026a1e1..a5c591b61490 100644 --- a/core/audits/layout-shifts.js +++ b/core/audits/layout-shifts.js @@ -89,7 +89,7 @@ class LayoutShifts extends Audit { .slice(0, MAX_LAYOUT_SHIFTS); for (const event of topLayoutShiftEvents) { const biggestImpactNodeId = TraceElements.getBiggestImpactNodeForShiftEvent( - event.args.data.impacted_nodes || [], impactByNodeId, event); + event.args.data.impacted_nodes || [], impactByNodeId); const biggestImpactElement = traceElements.find(t => t.nodeId === biggestImpactNodeId); // Turn root causes into sub-items. diff --git a/core/gather/gatherers/trace-elements.js b/core/gather/gatherers/trace-elements.js index 463a3e3be66b..8d41e97c0185 100644 --- a/core/gather/gatherers/trace-elements.js +++ b/core/gather/gatherers/trace-elements.js @@ -152,49 +152,19 @@ class TraceElements extends BaseGatherer { * * @param {LH.Artifacts.TraceImpactedNode[]} impactedNodes * @param {Map} impactByNodeId - * @param {import('../../lib/trace-engine.js').SaneSyntheticLayoutShift} event Only for debugging * @return {number|undefined} */ - static getBiggestImpactNodeForShiftEvent(impactedNodes, impactByNodeId, event) { - try { - let biggestImpactNodeId; - let biggestImpactNodeScore = Number.NEGATIVE_INFINITY; - for (const node of impactedNodes) { - const impactScore = impactByNodeId.get(node.node_id); - if (impactScore !== undefined && impactScore > biggestImpactNodeScore) { - biggestImpactNodeId = node.node_id; - biggestImpactNodeScore = impactScore; - } + static getBiggestImpactNodeForShiftEvent(impactedNodes, impactByNodeId) { + let biggestImpactNodeId; + let biggestImpactNodeScore = Number.NEGATIVE_INFINITY; + for (const node of impactedNodes) { + const impactScore = impactByNodeId.get(node.node_id); + if (impactScore !== undefined && impactScore > biggestImpactNodeScore) { + biggestImpactNodeId = node.node_id; + biggestImpactNodeScore = impactScore; } - return biggestImpactNodeId; - } catch (err) { - // See https://github.com/GoogleChrome/lighthouse/issues/15870 - // `impactedNodes` should always be an array here, but it can randomly be something else for - // currently unknown reasons. This exception handling will help us identify what - // `impactedNodes` really is and also prevent the error from being fatal. - - // It's possible `impactedNodes` is not JSON serializable, so let's add more supplemental - // fields just in case. - const impactedNodesType = typeof impactedNodes; - const impactedNodesClassName = impactedNodes?.constructor?.name; - - let impactedNodesJson; - let eventJson; - try { - impactedNodesJson = JSON.parse(JSON.stringify(impactedNodes)); - eventJson = JSON.parse(JSON.stringify(event)); - } catch {} - - Sentry.captureException(err, { - extra: { - impactedNodes: impactedNodesJson, - event: eventJson, - impactedNodesType, - impactedNodesClassName, - }, - }); - return; } + return biggestImpactNodeId; } /** @@ -222,7 +192,7 @@ class TraceElements extends BaseGatherer { const nodeIds = []; const impactedNodes = event.args.data.impacted_nodes || []; const biggestImpactedNodeId = - this.getBiggestImpactNodeForShiftEvent(impactedNodes, impactByNodeId, event); + this.getBiggestImpactNodeForShiftEvent(impactedNodes, impactByNodeId); if (biggestImpactedNodeId !== undefined) { nodeIds.push(biggestImpactedNodeId); } diff --git a/core/runner.js b/core/runner.js index 4b9217289ad9..f5a55b707e2e 100644 --- a/core/runner.js +++ b/core/runner.js @@ -312,10 +312,8 @@ class Runner { if (!isEqual(normalizedGatherSettings[k], normalizedAuditSettings[k])) { throw new Error( `Cannot change settings between gathering and auditing… -Difference found at: \`${k}\` - ${normalizedGatherSettings[k]} -vs - ${normalizedAuditSettings[k]}`); +Difference found at: \`${k}\`: ${JSON.stringify(normalizedGatherSettings[k], null, 2)} +vs: ${JSON.stringify(normalizedAuditSettings[k], null, 2)}`); } } diff --git a/core/test/gather/gatherers/trace-elements-test.js b/core/test/gather/gatherers/trace-elements-test.js index 1252814be8bf..07bad462f16d 100644 --- a/core/test/gather/gatherers/trace-elements-test.js +++ b/core/test/gather/gatherers/trace-elements-test.js @@ -72,15 +72,6 @@ function makeLCPTraceEvent(nodeId) { }; } -describe('Trace Elements gatherer - GetTopLayoutShifts', () => { - describe('getBiggestImpactForShiftEvent', () => { - it('is non fatal if impactedNodes is not iterable', () => { - const result = TraceElementsGatherer.getBiggestImpactNodeForShiftEvent(1, new Map()); - expect(result).toBeUndefined(); - }); - }); -}); - describe('Trace Elements gatherer - Animated Elements', () => { it('gets animated node ids with non-composited animations', async () => { const traceEvents = [