From d2cdf3d3312b91dfe566e5e9310dfe94820516d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Naz=C4=B1m=20Can=20Alt=C4=B1nova?= Date: Mon, 12 Dec 2022 11:26:26 +0100 Subject: [PATCH] Bug 1802303 - Add performance profile urls to Browsertime regression template (#7588) * Add Firefox Profiler profile urls to browsertime regression template * Rename FilterAlertsWithVideos class to BrowsertimeAlertsExtraData * Add the browsertime alert extra data to 'Copy Summary' button as well * Add tests for the new browsertime profiler url feature --- ... => browsertime_alerts_extra_data_test.js} | 67 ++++++++++++++++--- ...ideos.js => browsertimeAlertsExtraData.js} | 56 +++++++++++++--- ui/perfherder/alerts/StatusDropdown.jsx | 12 ++-- ui/perfherder/perf-helpers/textualSummary.js | 61 +++++++++++++---- 4 files changed, 162 insertions(+), 34 deletions(-) rename tests/ui/models/{filter_alerts_with_videos_test.js => browsertime_alerts_extra_data_test.js} (66%) rename ui/models/{filterAlertsWithVideos.js => browsertimeAlertsExtraData.js} (57%) diff --git a/tests/ui/models/filter_alerts_with_videos_test.js b/tests/ui/models/browsertime_alerts_extra_data_test.js similarity index 66% rename from tests/ui/models/filter_alerts_with_videos_test.js rename to tests/ui/models/browsertime_alerts_extra_data_test.js index 105c99ad8..6718372e7 100644 --- a/tests/ui/models/filter_alerts_with_videos_test.js +++ b/tests/ui/models/browsertime_alerts_extra_data_test.js @@ -1,7 +1,7 @@ import fetchMock from 'fetch-mock'; import { cloneDeep } from 'lodash'; -import FilterAlertsWithVideos from '../../../ui/models/filterAlertsWithVideos'; +import BrowsertimeAlertsExtraData from '../../../ui/models/browsertimeAlertsExtraData'; import testAlertSummaryWithVideos from '../mock/alerts_with_videos/alert_summary_with_browsertime_videos'; import testAlertSummaryWithoutVideos from '../mock/alerts_with_videos/alert_summary_without_browsertime_videos'; import testAlertSummaryNonBrowsertime from '../mock/alerts_with_videos/alert_summary_non_browsertime'; @@ -13,8 +13,9 @@ import currentJoblistWithoutVideoResultsOne from '../mock/alerts_with_videos/cur import currentJoblistWithoutVideoResultsTwo from '../mock/alerts_with_videos/current_joblist_without_video_results_page_2'; import prevJoblistWithoutVideoResultsOne from '../mock/alerts_with_videos/prev_joblist_without_video_results_page_1'; import prevJoblistWithoutVideoResultsTwo from '../mock/alerts_with_videos/prev_joblist_without_video_results_page_2'; +import repos from '../mock/repositories'; -describe('FilterAlertsWithVideos', () => { +describe('BrowsertimeAlertsExtraData', () => { afterEach(() => { fetchMock.reset(); }); @@ -37,11 +38,13 @@ describe('FilterAlertsWithVideos', () => { `/api/jobs/?repo=autoland&push_id=846998&page=2`, prevJoblistWithVideoResultsTwo, ); + // Returns the autoland repo. + fetchMock.mock(`/api/repository/`, repos); }); test('should return alerts with browsertime results links', async () => { const alertSummaryWithVideos = cloneDeep(testAlertSummaryWithVideos); - const alertsWithVideos = new FilterAlertsWithVideos( + const alertsWithVideos = new BrowsertimeAlertsExtraData( alertSummaryWithVideos, [{ id: 13, name: 'browsertime' }], ); @@ -62,6 +65,27 @@ describe('FilterAlertsWithVideos', () => { expect(alert.prev_results_link).toEqual(eprl); }); }); + + test('should return alerts with profiler links', async () => { + const alertSummaryWithVideos = cloneDeep(testAlertSummaryWithVideos); + const alertsExtraData = new BrowsertimeAlertsExtraData( + alertSummaryWithVideos, + [{ id: 13, name: 'browsertime' }], + ); + + await alertsExtraData.enrichAndRetrieveAlerts(); + alertSummaryWithVideos.alerts.forEach((alert) => { + if (!alertsExtraData.shouldHaveVideoLinks(alert)) { + return; + } + const { suite } = alert.series_signature; + const expectedResultsLink = `https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/c6nAPFniThOiaGvDT2DNCw/runs/0/artifacts/public/test_info/profile_${suite}.zip`; + const expectedPrevResultsLink = `https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/LUjQR22tRp6J4wXLHhYk8g/runs/0/artifacts/public/test_info/profile_${suite}.zip`; + + expect(alert.profile_url).toEqual(expectedResultsLink); + expect(alert.prev_profile_url).toEqual(expectedPrevResultsLink); + }); + }); }); describe('File bug url when alerts do not contain browsertime results links', () => { @@ -82,26 +106,31 @@ describe('FilterAlertsWithVideos', () => { `/api/jobs/?repo=autoland&push_id=847398&page=2`, prevJoblistWithoutVideoResultsTwo, ); + // Returns the autoland repo. + fetchMock.mock(`/api/repository/`, repos); }); test('should return alerts without browsertime results links', async () => { const alertSummaryWithoutVideos = cloneDeep( testAlertSummaryWithoutVideos, ); - const alertsWithoutVideos = new FilterAlertsWithVideos( + const alertsWithoutVideos = new BrowsertimeAlertsExtraData( alertSummaryWithoutVideos, [{ id: 13, name: 'browsertime' }], ); - const alerts = await alertsWithoutVideos.enrichAndRetrieveAlerts(); - expect(alerts).toStrictEqual([]); - alertSummaryWithoutVideos.alerts.forEach((alert) => { + function checkForSingleAlert(alert) { if (alertsWithoutVideos.shouldHaveVideoLinks(alert)) { return; } expect(alert.results_link).toBeUndefined(); expect(alert.prev_results_link).toBeUndefined(); - }); + } + const alerts = await alertsWithoutVideos.enrichAndRetrieveAlerts(); + // We still have the alerts array for the profiler links, but they + // shouldn't have results_link and prev_results_link fields. + alerts.forEach(checkForSingleAlert); + alertSummaryWithoutVideos.alerts.forEach(checkForSingleAlert); }); }); @@ -110,7 +139,7 @@ describe('FilterAlertsWithVideos', () => { const alertSummaryNonBrowsertime = cloneDeep( testAlertSummaryNonBrowsertime, ); - const alertsWithoutVideos = new FilterAlertsWithVideos( + const alertsWithoutVideos = new BrowsertimeAlertsExtraData( alertSummaryNonBrowsertime, [{ id: 13, name: 'browsertime' }], ); @@ -125,5 +154,25 @@ describe('FilterAlertsWithVideos', () => { expect(alert.prev_results_link).toBeUndefined(); }); }); + + test('should return alerts without profiler links', async () => { + const alertSummaryNonBrowsertime = cloneDeep( + testAlertSummaryNonBrowsertime, + ); + const alertsWithoutVideos = new BrowsertimeAlertsExtraData( + alertSummaryNonBrowsertime, + [{ id: 13, name: 'browsertime' }], + ); + + const alerts = await alertsWithoutVideos.enrichAndRetrieveAlerts(); + expect(alerts).toStrictEqual([]); + alertSummaryNonBrowsertime.alerts.forEach((alert) => { + if (alertsWithoutVideos.shouldHaveVideoLinks(alert)) { + return; + } + expect(alert.profile_url).toBeUndefined(); + expect(alert.prev_profile_url).toBeUndefined(); + }); + }); }); }); diff --git a/ui/models/filterAlertsWithVideos.js b/ui/models/browsertimeAlertsExtraData.js similarity index 57% rename from ui/models/filterAlertsWithVideos.js rename to ui/models/browsertimeAlertsExtraData.js index e41223872..d2727301f 100644 --- a/ui/models/filterAlertsWithVideos.js +++ b/ui/models/browsertimeAlertsExtraData.js @@ -3,10 +3,12 @@ import { addResultsLink, getFrameworkName, } from '../perfherder/perf-helpers/helpers'; +import { getArtifactsUrl } from '../helpers/url'; import JobModel from './job'; +import RepositoryModel from './repository'; -export default class FilterAlertsWithVideos { +export default class BrowsertimeAlertsExtraData { constructor(alertSummary, frameworks) { this.alertSummary = alertSummary; this.framework = getFrameworkName(frameworks, alertSummary.framework); @@ -14,10 +16,7 @@ export default class FilterAlertsWithVideos { async enrichAndRetrieveAlerts() { let alerts = []; - if ( - this.framework === 'browsertime' && - this.anyAlertWithVideoResults(this.alertSummary) - ) { + if (this.framework === 'browsertime') { alerts = await this.enrichSummaryAlerts( this.alertSummary, this.alertSummary.repository, @@ -35,9 +34,15 @@ export default class FilterAlertsWithVideos { JobModel.getList({ repo, push_id: prevPushId }, { fetchAll: true }), ]); - // add task ids for current rev and previous rev to every relevant alert item - this.enrichWithLinks(alertSummary, jobList); - this.enrichWithLinks(alertSummary, prevJobList); + if (this.anyAlertWithVideoResults(this.alertSummary)) { + // add task ids for current rev and previous rev to every relevant alert item + this.enrichWithLinks(alertSummary, jobList); + this.enrichWithLinks(alertSummary, prevJobList); + } + + const alertsRepo = await this.getAlertsRepo(); + await this.enrichWithProfileLinks(alertSummary, alertsRepo, jobList); + await this.enrichWithProfileLinks(alertSummary, alertsRepo, prevJobList); return alertSummary.alerts; } @@ -82,4 +87,39 @@ export default class FilterAlertsWithVideos { } }); } + + async getAlertsRepo() { + const repos = await RepositoryModel.getList(); + return RepositoryModel.getRepo(this.alertSummary.repository, repos); + } + + async enrichWithProfileLinks(alertSummary, repo, jobList) { + alertSummary.alerts.forEach((alert) => { + const job = jobList.data.find( + (j) => + j.searchStr.includes(alert.series_signature.suite) && + j.searchStr.includes(alert.series_signature.machine_platform) && + j.resultStatus === 'success', + ); + + if (job) { + const { suite } = alert.series_signature; + + const url = getArtifactsUrl({ + taskId: job.task_id, + run: job.retry_id, + rootUrl: repo.tc_root_url, + artifactPath: `public/test_info/profile_${suite}.zip`, + }); + + if (job.push_revision === alertSummary.revision) { + alert.profile_url = url; + } + + if (job.push_revision === alertSummary.prev_push_revision) { + alert.prev_profile_url = url; + } + } + }); + } } diff --git a/ui/perfherder/alerts/StatusDropdown.jsx b/ui/perfherder/alerts/StatusDropdown.jsx index 521cffe56..c06d5130e 100644 --- a/ui/perfherder/alerts/StatusDropdown.jsx +++ b/ui/perfherder/alerts/StatusDropdown.jsx @@ -30,7 +30,7 @@ import { } from '../../helpers/url'; import { summaryStatusMap } from '../perf-helpers/constants'; import DropdownMenuItems from '../../shared/DropdownMenuItems'; -import FilterAlertsWithVideos from '../../models/filterAlertsWithVideos'; +import BrowsertimeAlertsExtraData from '../../models/browsertimeAlertsExtraData'; import AlertModal from './AlertModal'; import FileBugModal from './FileBugModal'; @@ -46,7 +46,7 @@ export default class StatusDropdown extends React.Component { showNotesModal: false, showTagsModal: false, selectedValue: this.props.issueTrackers[0].text, - alertsWithVideos: new FilterAlertsWithVideos( + browsertimeAlertsExtraData: new BrowsertimeAlertsExtraData( this.props.alertSummary, this.props.frameworks, ), @@ -121,7 +121,7 @@ export default class StatusDropdown extends React.Component { filteredAlerts, frameworks, } = this.props; - const { alertsWithVideos } = this.state; + const { browsertimeAlertsExtraData } = this.state; let result = bugTemplate; if (!result) { @@ -144,7 +144,7 @@ export default class StatusDropdown extends React.Component { filteredAlerts, alertSummary, null, - await alertsWithVideos.enrichAndRetrieveAlerts(), + await browsertimeAlertsExtraData.enrichAndRetrieveAlerts(), ); const templateArgs = { bugType: 'defect', @@ -196,13 +196,15 @@ export default class StatusDropdown extends React.Component { } }; - copySummary = () => { + copySummary = async () => { const { filteredAlerts, alertSummary, frameworks } = this.props; + const { browsertimeAlertsExtraData } = this.state; const textualSummary = new TextualSummary( frameworks, filteredAlerts, alertSummary, true, + await browsertimeAlertsExtraData.enrichAndRetrieveAlerts(), ); // can't access the clipboardData on event unless it's done from react's // onCopy, onCut or onPaste props so using this workaround diff --git a/ui/perfherder/perf-helpers/textualSummary.js b/ui/perfherder/perf-helpers/textualSummary.js index b50c8c186..969ff3ea9 100644 --- a/ui/perfherder/perf-helpers/textualSummary.js +++ b/ui/perfherder/perf-helpers/textualSummary.js @@ -13,14 +13,20 @@ export default class TextualSummary { alerts, alertSummary, copySummary = null, - alertsWithVideos = [], + browsertimeAlertsExtraData = [], ) { this.frameworks = frameworks; this.alerts = alerts; this.alertSummary = alertSummary; this.copySummary = copySummary; - this.alertsWithVideos = alertsWithVideos; - this.ellipsesRow = `\n|...|...|...|...|...|`; + this.browsertimeAlertsExtraData = browsertimeAlertsExtraData; + this.hasProfileUrls = this.browsertimeAlertsExtraData.some( + (a) => a.profile_url && a.prev_profile_url, + ); + this.headerRow = `\n|--|--|--|--|--|${this.hasProfileUrls ? '--|' : ''}`; + this.ellipsesRow = `\n|...|...|...|...|...|${ + this.hasProfileUrls ? '...|' : '' + }`; } get markdown() { @@ -83,7 +89,9 @@ export default class TextualSummary { const { suite, test, machine_platform: platform } = alert.series_signature; const extraOptions = alert.series_signature.extra_options.join(' '); - const updatedAlert = this.alertsWithVideos.find((a) => alert.id === a.id); + const updatedAlert = this.browsertimeAlertsExtraData.find( + (a) => alert.id === a.id, + ); const frameworkName = getFrameworkName( this.frameworks, this.alertSummary.framework, @@ -95,14 +103,27 @@ export default class TextualSummary { : suite; const suiteTestName = suite === test ? suiteName : `${suiteName} ${test}`; - if ( + const alertValues = updatedAlert && updatedAlert.results_link && updatedAlert.prev_results_link - ) { - return `| [${amountPct}%](${graphLink}) | ${suiteTestName} | ${platform} | ${extraOptions} | [${prevValue}](${updatedAlert.prev_results_link}) -> [${newValue}](${updatedAlert.results_link}) |`; + ? `[${prevValue}](${updatedAlert.prev_results_link}) -> [${newValue}](${updatedAlert.results_link})` + : `${prevValue} -> ${newValue}`; + + let maybeProfileLinks = ''; + if (this.hasProfileUrls) { + // Add an additional column for the profiler before and after so users can + // find the profiler links easily. Only add this column if at least one alert + // has profile urls. + maybeProfileLinks = + updatedAlert && + updatedAlert.profile_url && + updatedAlert.prev_profile_url + ? `[Before](${updatedAlert.prev_profile_url})/[After](${updatedAlert.profile_url}) |` + : ' |'; } - return `| [${amountPct}%](${graphLink}) | ${suiteTestName} | ${platform} | ${extraOptions} | ${prevValue} -> ${newValue} |`; + + return `| [${amountPct}%](${graphLink}) | ${suiteTestName} | ${platform} | ${extraOptions} | ${alertValues} | ${maybeProfileLinks}`; } formatAlertBulk(alerts) { @@ -117,7 +138,11 @@ export default class TextualSummary { resultStr += '\n'; } const formattedRegressions = this.formatAlertBulk(regressed); - resultStr += `### Regressions:\n\n| **Ratio** | **Test** | **Platform** | **Options** | **Absolute values (old vs new)**| \n|--|--|--|--|--| \n${formattedRegressions}\n`; + // Add a column for the profiler links if at least one alert has them. + const maybeProfileColumn = this.hasProfileUrls + ? ' **Performance Profiles** |' + : ''; + resultStr += `### Regressions:\n\n| **Ratio** | **Test** | **Platform** | **Options** | **Absolute values (old vs new)** | ${maybeProfileColumn} ${this.headerRow} \n${formattedRegressions}\n`; } if (regressed.length > 15) { // add a newline if we displayed the header @@ -136,7 +161,11 @@ export default class TextualSummary { smallestFiveRegressed, ); - resultStr += `### Regressions:\n\n| **Ratio** | **Test** | **Platform** | **Options** | **Absolute values (old vs new)**| \n|--|--|--|--|--| \n${formattedBiggestRegressions}`; + // Add a column for the profiler links if at least one alert has them. + const maybeProfileColumn = this.hasProfileUrls + ? ' **Performance Profiles** |' + : ''; + resultStr += `### Regressions:\n\n| **Ratio** | **Test** | **Platform** | **Options** | **Absolute values (old vs new)** | ${maybeProfileColumn} ${this.headerRow} \n${formattedBiggestRegressions}`; resultStr += this.ellipsesRow; resultStr += `\n${formattedSmallestRegressions}\n`; } @@ -151,7 +180,11 @@ export default class TextualSummary { resultStr += '\n'; } const formattedImprovements = this.formatAlertBulk(improved); - resultStr += `### Improvements:\n\n| **Ratio** | **Test** | **Platform** | **Options** | **Absolute values (old vs new)**| \n|--|--|--|--|--| \n${formattedImprovements}\n`; + // Add a column for the profiler links if at least one alert has them. + const maybeProfileColumn = this.hasProfileUrls + ? ' **Performance Profiles** |' + : ''; + resultStr += `### Improvements:\n\n| **Ratio** | **Test** | **Platform** | **Options** | **Absolute values (old vs new)** | ${maybeProfileColumn} ${this.headerRow} \n${formattedImprovements}\n`; } else if (improved.length > 6) { // Add a newline if we displayed some regressions if (resultStr.length > 0) { @@ -169,7 +202,11 @@ export default class TextualSummary { smallestImprovement, ); - resultStr += `### Improvements:\n\n| **Ratio** | **Test** | **Platform** | **Options** | **Absolute values (old vs new)**| \n|--|--|--|--|--| \n${formattedBiggestImprovements}`; + // Add a column for the profiler links if at least one alert has them. + const maybeProfileColumn = this.hasProfileUrls + ? ' **Performance Profiles** |' + : ''; + resultStr += `### Improvements:\n\n| **Ratio** | **Test** | **Platform** | **Options** | **Absolute values (old vs new)** | ${maybeProfileColumn} ${this.headerRow} \n${formattedBiggestImprovements}`; resultStr += this.ellipsesRow; resultStr += `\n${formattedSmallestImprovement}\n`; }