From 3b6bd2ac8ab96bb7b25e693de4ef56c29f2d4230 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Naz=C4=B1m=20Can=20Alt=C4=B1nova?= Date: Wed, 4 Jan 2023 10:16:21 +0100 Subject: [PATCH 1/5] Do not throw an error when browsertime provides null timestamps incorrectly --- src/profile-logic/process-profile.js | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/profile-logic/process-profile.js b/src/profile-logic/process-profile.js index 14541240fa..505600fe71 100644 --- a/src/profile-logic/process-profile.js +++ b/src/profile-logic/process-profile.js @@ -1778,6 +1778,12 @@ export function processVisualMetrics( continue; } + if (metric.every((m) => m.timestamp === null)) { + // Skip it if we don't have any timestamps. + // This is due to https://github.com/sitespeedio/browsertime/issues/1746. + continue; + } + const startTime = navigationStartTime ?? metric[0].timestamp; const endTime = metric[metric.length - 1].timestamp; @@ -1798,6 +1804,11 @@ export function processVisualMetrics( const changeMarkerName = `${metricName} Change`; for (const { timestamp, percent } of metric) { + if (timestamp === null) { + // Skip it if we don't have a timestamp. + // This might be due to https://github.com/sitespeedio/browsertime/issues/1746. + continue; + } const payload = { type: 'VisualMetricProgress', // 'percentage' type expects a value between 0 and 1. From f047982c90a05918bdf894e3092f195efb0ee5c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Naz=C4=B1m=20Can=20Alt=C4=B1nova?= Date: Wed, 4 Jan 2023 10:33:52 +0100 Subject: [PATCH 2/5] Add a test for null metric timestamps --- src/test/unit/process-profile.test.js | 62 +++++++++++++++++++++++++-- 1 file changed, 59 insertions(+), 3 deletions(-) diff --git a/src/test/unit/process-profile.test.js b/src/test/unit/process-profile.test.js index 1ffe854ded..84a637a17b 100644 --- a/src/test/unit/process-profile.test.js +++ b/src/test/unit/process-profile.test.js @@ -664,8 +664,14 @@ describe('profile meta processing', function () { }); describe('visualMetrics processing', function () { - function checkVisualMetricsForThread(thread: Thread) { - const metrics = [ + function checkVisualMetricsForThread( + thread: Thread, + expectedMetricMarkers?: Array<{| + name: string, + changeMarkerLength: number, + |}> + ) { + const metrics = expectedMetricMarkers ?? [ { name: 'Visual', changeMarkerLength: 7 }, { name: 'ContentfulSpeedIndex', changeMarkerLength: 6 }, { name: 'PerceptualSpeedIndex', changeMarkerLength: 6 }, @@ -679,7 +685,12 @@ describe('visualMetrics processing', function () { const metricProgressMarker = thread.markers.name.find( (name) => name === metricProgressMarkerIndex ); - expect(metricProgressMarker).toBeTruthy(); + + if (changeMarkerLength > 0) { + expect(metricProgressMarker).toBeTruthy(); + } else { + expect(metricProgressMarker).toBeFalsy(); + } // Check the visual metric change markers. const metricChangeMarkerIndex = thread.stringTable.indexForString( @@ -736,4 +747,49 @@ describe('visualMetrics processing', function () { checkVisualMetricsForThread(tabProcessMainThread); }); + + it('does not add markers to the parent process if metric timestamps are null', function () { + const geckoProfile = createGeckoProfile(); + const visualMetrics = getVisualMetrics(); + + // Make all the VisualProgress timestamps null on purpose. + // Flow doesn't like null because it thinks that the timestamp can't be null. + // But this is a bug on browsertime. + visualMetrics.VisualProgress = visualMetrics.VisualProgress.map( + (progress) => ({ ...progress, timestamp: (null: any) }) + ); + // Make only one ContentfulSpeedIndexProgress timestamp null on purpose. + // Flow doesn't like null because it thinks that the timestamp can't be null. + // But this is a bug on browsertime. + ensureExists(visualMetrics.ContentfulSpeedIndexProgress)[0].timestamp = + (null: any); + + // Add the visual metrics to the profile. + geckoProfile.meta.visualMetrics = visualMetrics; + // Make sure that the visual metrics are not changed. + expect(visualMetrics.VisualProgress).toHaveLength(7); + expect(visualMetrics.ContentfulSpeedIndexProgress).toHaveLength(6); + expect(visualMetrics.PerceptualSpeedIndexProgress).toHaveLength(6); + + // Processing the profile. + const processedProfile = processGeckoProfile(geckoProfile); + const parentProcessMainThread = processedProfile.threads.find( + (thread) => + thread.name === 'GeckoMain' && thread.processType === 'default' + ); + + if (!parentProcessMainThread) { + throw new Error('Could not find the parent process main thread.'); + } + + checkVisualMetricsForThread(parentProcessMainThread, [ + // Instead of 7, we should have 0 markers for Visual because we made all + // the timestamps null. + { name: 'Visual', changeMarkerLength: 0 }, + // Instead of 6, we should have 5 markers for ContentfulSpeedIndex. + { name: 'ContentfulSpeedIndex', changeMarkerLength: 5 }, + // We didn't change the PerceptualSpeedIndexProgress, so we should have 6. + { name: 'PerceptualSpeedIndex', changeMarkerLength: 6 }, + ]); + }); }); From 4ce0d46b942b06997cdbb0bbe79e1e2316cdda6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Naz=C4=B1m=20Can=20Alt=C4=B1nova?= Date: Wed, 4 Jan 2023 15:42:07 +0100 Subject: [PATCH 3/5] Make the visual progress graph timestamp nullable because of the browsertime bug --- .../timeline/TrackVisualProgressGraph.js | 14 ++++++++------ src/test/unit/process-profile.test.js | 8 ++------ src/types/profile.js | 3 ++- 3 files changed, 12 insertions(+), 13 deletions(-) diff --git a/src/components/timeline/TrackVisualProgressGraph.js b/src/components/timeline/TrackVisualProgressGraph.js index decbb0422c..41f6cde3d0 100644 --- a/src/components/timeline/TrackVisualProgressGraph.js +++ b/src/components/timeline/TrackVisualProgressGraph.js @@ -102,7 +102,7 @@ class TrackVisualProgressCanvas extends React.PureComponent { // Create a path for the top of the chart. This is the line that will have // a stroke applied to it. x = - (deviceWidth * (progressGraphData[i].timestamp - rangeStart)) / + (deviceWidth * ((progressGraphData[i].timestamp ?? 0) - rangeStart)) / rangeLength; // Add on half the stroke's line width so that it won't be cut off the edge // of the graph. @@ -133,7 +133,7 @@ class TrackVisualProgressCanvas extends React.PureComponent { // of the canvas. ctx.lineTo(x + interval, deviceHeight); ctx.lineTo( - (deviceWidth * (progressGraphData[0].timestamp - rangeStart)) / + (deviceWidth * ((progressGraphData[0].timestamp ?? 0) - rangeStart)) / rangeLength + interval, deviceHeight @@ -221,15 +221,16 @@ class TrackVisualProgressGraphImpl extends React.PureComponent { const rangeLength = rangeEnd - rangeStart; const timeAtMouse = rangeStart + ((mouseX - left) / width) * rangeLength; if ( - timeAtMouse < progressGraphData[0].timestamp || + timeAtMouse < (progressGraphData[0].timestamp ?? 0) || timeAtMouse > - progressGraphData[progressGraphData.length - 1].timestamp + interval + (progressGraphData[progressGraphData.length - 1].timestamp ?? 0) + + interval ) { // We are outside the range of the samples, do not display hover information. this.setState({ hoveredVisualProgress: null }); } else { const graphTimestamps = progressGraphData.map( - ({ timestamp }) => timestamp + ({ timestamp }) => timestamp ?? 0 ); let hoveredVisualProgress = bisectionRight(graphTimestamps, timeAtMouse); if (hoveredVisualProgress === progressGraphData.length) { @@ -277,7 +278,8 @@ class TrackVisualProgressGraphImpl extends React.PureComponent { } = this.props; const rangeLength = rangeEnd - rangeStart; const left = - (width * (progressGraphData[graphDataIndex].timestamp - rangeStart)) / + (width * + ((progressGraphData[graphDataIndex].timestamp ?? 0) - rangeStart)) / rangeLength; const unitSampleCount = progressGraphData[graphDataIndex].percent / 100; diff --git a/src/test/unit/process-profile.test.js b/src/test/unit/process-profile.test.js index 84a637a17b..5b76f6f63f 100644 --- a/src/test/unit/process-profile.test.js +++ b/src/test/unit/process-profile.test.js @@ -753,16 +753,12 @@ describe('visualMetrics processing', function () { const visualMetrics = getVisualMetrics(); // Make all the VisualProgress timestamps null on purpose. - // Flow doesn't like null because it thinks that the timestamp can't be null. - // But this is a bug on browsertime. visualMetrics.VisualProgress = visualMetrics.VisualProgress.map( - (progress) => ({ ...progress, timestamp: (null: any) }) + (progress) => ({ ...progress, timestamp: null }) ); // Make only one ContentfulSpeedIndexProgress timestamp null on purpose. - // Flow doesn't like null because it thinks that the timestamp can't be null. - // But this is a bug on browsertime. ensureExists(visualMetrics.ContentfulSpeedIndexProgress)[0].timestamp = - (null: any); + null; // Add the visual metrics to the profile. geckoProfile.meta.visualMetrics = visualMetrics; diff --git a/src/types/profile.js b/src/types/profile.js index 4638e1acc0..e57a2d92be 100644 --- a/src/types/profile.js +++ b/src/types/profile.js @@ -681,7 +681,8 @@ export type ProgressGraphData = {| // A percentage that describes the visual completeness of the webpage, ranging from 0% - 100% percent: number, // The time in milliseconds which the sample was taken. - timestamp: Milliseconds, + // This can be null due to https://github.com/sitespeedio/browsertime/issues/1746. + timestamp: Milliseconds | null, |}; /** From 7b58cc003cf431d28b8f9c53165d6f53763b218a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Naz=C4=B1m=20Can=20Alt=C4=B1nova?= Date: Thu, 5 Jan 2023 11:43:20 +0100 Subject: [PATCH 4/5] Refactor the browsertime marker tests --- src/test/unit/process-profile.test.js | 55 ++++++++++++++++++++------- 1 file changed, 41 insertions(+), 14 deletions(-) diff --git a/src/test/unit/process-profile.test.js b/src/test/unit/process-profile.test.js index 5b76f6f63f..d95b7169cc 100644 --- a/src/test/unit/process-profile.test.js +++ b/src/test/unit/process-profile.test.js @@ -666,18 +666,13 @@ describe('profile meta processing', function () { describe('visualMetrics processing', function () { function checkVisualMetricsForThread( thread: Thread, - expectedMetricMarkers?: Array<{| + metrics: Array<{| name: string, + hasProgressMarker: boolean, changeMarkerLength: number, |}> ) { - const metrics = expectedMetricMarkers ?? [ - { name: 'Visual', changeMarkerLength: 7 }, - { name: 'ContentfulSpeedIndex', changeMarkerLength: 6 }, - { name: 'PerceptualSpeedIndex', changeMarkerLength: 6 }, - ]; - - for (const { name, changeMarkerLength } of metrics) { + for (const { name, hasProgressMarker, changeMarkerLength } of metrics) { // Check the visual metric progress markers. const metricProgressMarkerIndex = thread.stringTable.indexForString( `${name} Progress` @@ -686,7 +681,7 @@ describe('visualMetrics processing', function () { (name) => name === metricProgressMarkerIndex ); - if (changeMarkerLength > 0) { + if (hasProgressMarker) { expect(metricProgressMarker).toBeTruthy(); } else { expect(metricProgressMarker).toBeFalsy(); @@ -723,7 +718,19 @@ describe('visualMetrics processing', function () { throw new Error('Could not find the parent process main thread.'); } - checkVisualMetricsForThread(parentProcessMainThread); + checkVisualMetricsForThread(parentProcessMainThread, [ + { name: 'Visual', hasProgressMarker: true, changeMarkerLength: 7 }, + { + name: 'ContentfulSpeedIndex', + hasProgressMarker: true, + changeMarkerLength: 6, + }, + { + name: 'PerceptualSpeedIndex', + hasProgressMarker: true, + changeMarkerLength: 6, + }, + ]); }); it('adds markers to the tab process', function () { @@ -745,7 +752,19 @@ describe('visualMetrics processing', function () { throw new Error('Could not find the tab process main thread.'); } - checkVisualMetricsForThread(tabProcessMainThread); + checkVisualMetricsForThread(tabProcessMainThread, [ + { name: 'Visual', hasProgressMarker: true, changeMarkerLength: 7 }, + { + name: 'ContentfulSpeedIndex', + hasProgressMarker: true, + changeMarkerLength: 6, + }, + { + name: 'PerceptualSpeedIndex', + hasProgressMarker: true, + changeMarkerLength: 6, + }, + ]); }); it('does not add markers to the parent process if metric timestamps are null', function () { @@ -781,11 +800,19 @@ describe('visualMetrics processing', function () { checkVisualMetricsForThread(parentProcessMainThread, [ // Instead of 7, we should have 0 markers for Visual because we made all // the timestamps null. - { name: 'Visual', changeMarkerLength: 0 }, + { name: 'Visual', hasProgressMarker: false, changeMarkerLength: 0 }, // Instead of 6, we should have 5 markers for ContentfulSpeedIndex. - { name: 'ContentfulSpeedIndex', changeMarkerLength: 5 }, + { + name: 'ContentfulSpeedIndex', + hasProgressMarker: true, + changeMarkerLength: 5, + }, // We didn't change the PerceptualSpeedIndexProgress, so we should have 6. - { name: 'PerceptualSpeedIndex', changeMarkerLength: 6 }, + { + name: 'PerceptualSpeedIndex', + hasProgressMarker: true, + changeMarkerLength: 6, + }, ]); }); }); From 7fb305d8211afe7df6b583f85128a452275d36d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Naz=C4=B1m=20Can=20Alt=C4=B1nova?= Date: Thu, 5 Jan 2023 12:01:24 +0100 Subject: [PATCH 5/5] Make the null timestmap checks more generic for the metric markers --- src/profile-logic/process-profile.js | 49 +++++++++++++++------------- 1 file changed, 27 insertions(+), 22 deletions(-) diff --git a/src/profile-logic/process-profile.js b/src/profile-logic/process-profile.js index 505600fe71..238deb726f 100644 --- a/src/profile-logic/process-profile.js +++ b/src/profile-logic/process-profile.js @@ -28,6 +28,7 @@ import { isArtTraceFormat, convertArtTraceProfile } from './import/art-trace'; import { PROCESSED_PROFILE_VERSION, INTERVAL, + INTERVAL_END, INSTANT, } from '../app-logic/constants'; import { @@ -87,6 +88,7 @@ import type { PageList, ThreadIndex, BrowsertimeMarkerPayload, + MarkerPhase, } from 'firefox-profiler/types'; type RegExpResult = null | string[]; @@ -1736,18 +1738,30 @@ export function processVisualMetrics( (cat) => cat.name === 'Test' ); - function addMetricMarker( + function maybeAddMetricMarker( thread: Thread, name: string, - startTime: number, - endTime?: number, + phase: MarkerPhase, + startTime: number | null, + endTime: number | null, payload?: BrowsertimeMarkerPayload ) { + if ( + // All phases except INTERVAL_END should have a start time. + (phase !== INTERVAL_END && startTime === null) || + // Only INTERVAL and INTERVAL_END should have an end time. + ((phase === INTERVAL || phase === INTERVAL_END) && endTime === null) + ) { + // Do not add if some timestamps we expect are missing. + // This should ideally never happen but timestamps could be null due to + // browsertime bug here: https://github.com/sitespeedio/browsertime/issues/1746. + return; + } // Add the marker to the given thread. thread.markers.name.push(thread.stringTable.indexForString(name)); thread.markers.startTime.push(startTime); - thread.markers.endTime.push(endTime ? endTime : 0); - thread.markers.phase.push(endTime ? INTERVAL : INSTANT); + thread.markers.endTime.push(endTime); + thread.markers.phase.push(phase); thread.markers.category.push(testingCategoryIdx); thread.markers.data.push(payload ?? null); thread.markers.length++; @@ -1778,20 +1792,14 @@ export function processVisualMetrics( continue; } - if (metric.every((m) => m.timestamp === null)) { - // Skip it if we don't have any timestamps. - // This is due to https://github.com/sitespeedio/browsertime/issues/1746. - continue; - } - const startTime = navigationStartTime ?? metric[0].timestamp; const endTime = metric[metric.length - 1].timestamp; // Add the progress marker to the parent process main thread. const markerName = `${metricName} Progress`; - addMetricMarker(mainThread, markerName, startTime, endTime); + maybeAddMetricMarker(mainThread, markerName, INTERVAL, startTime, endTime); // Add the progress marker to the tab process main thread. - addMetricMarker(tabThread, markerName, startTime, endTime); + maybeAddMetricMarker(tabThread, markerName, INTERVAL, startTime, endTime); // Add progress markers for every visual progress change for more fine grained information. const progressMarkerSchema = { @@ -1804,11 +1812,6 @@ export function processVisualMetrics( const changeMarkerName = `${metricName} Change`; for (const { timestamp, percent } of metric) { - if (timestamp === null) { - // Skip it if we don't have a timestamp. - // This might be due to https://github.com/sitespeedio/browsertime/issues/1746. - continue; - } const payload = { type: 'VisualMetricProgress', // 'percentage' type expects a value between 0 and 1. @@ -1816,19 +1819,21 @@ export function processVisualMetrics( }; // Add it to the parent process main thread. - addMetricMarker( + maybeAddMetricMarker( mainThread, changeMarkerName, + INSTANT, timestamp, - undefined, // endTime + null, // endTime payload ); // Add it to the tab process main thread. - addMetricMarker( + maybeAddMetricMarker( tabThread, changeMarkerName, + INSTANT, timestamp, - undefined, // endTime + null, // endTime payload ); }