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
2022-12-12 11:26:26 +01:00 · 2022-12-12 11:26:26 +01:00 · d2cdf3d331
--- a/tests/ui/models/browsertime_alerts_extra_data_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();
+      });
+    });
  });
 });
--- a/ui/models/browsertimeAlertsExtraData.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;
+        }
+      }
+    });
+  }
 }
--- 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
--- 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`;
    }