Bug 180126 – Add a test freshness page.

• Component: New Bugs
• Status: RESOLVED FIXED

Add a test health page.

Created attachment 327826 [details]
Patch

Comment on attachment 327826 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=327826&action=review

> Websites/perf.webkit.org/public/include/manifest-generator.php:48
> +            'warningHourBaseline' => config('warningHourBaseline'),
> +            'ratingCurveBase' => config('ratingCurveBase'),

The sigmoid like function is 1 / (1 + ratingCurveBase ^ (x - warningHourBaseline)), I'm open to have more descriptive names for those 2 variables.

[Alexey Proskuryakov]

Comment on attachment 327826 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=327826&action=review

> Websites/perf.webkit.org/ChangeLog:8
> +        Added a tet health page which shows freshness of a test.

s/tet/test/

> Websites/perf.webkit.org/public/v3/components/test-health-cell.js:42
> +        return `/v3/#/charts?since=${Date.now() - 7 * 24 * 3600 * 1000}&paneList=((${this._platform.id()}-${this._metric.id()}))`;

Does performance dashboard code have a centralized place where URLs are built? Doing it in every view controller seems less than ideal.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:18
> +        for(const config of configs) {

Missing space after "for".

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:19
> +            const platforms = Array.prototype.concat(...config.platformGroups.map((platformGroup) => platformGroup.platforms));

Another way to write this:

const platforms = config.platformGroups.reduce((result, group) => result.concat(group.platforms), []);

Not sure which one is easier to read.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:28
> +            const metrics = Array.prototype.concat(...config.metricGroups.map((metricGroup) =>
> +                Array.prototype.concat(...metricGroup.subgroups.map((subgroup) => subgroup.metrics))));

Similar to the above, but this chain definitely appears tricky to read.

Given that you have to use an explicit iteration just below anyway, does the map/concat chain actually improve the code?

[Ryosuke Niwa]

Comment on attachment 327826 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=327826&action=review

> Websites/perf.webkit.org/public/v3/components/test-health-cell.js:19
> +            const now = new Date();

Use Date.now() instead.

>> Websites/perf.webkit.org/public/v3/components/test-health-cell.js:42
>> +        return `/v3/#/charts?since=${Date.now() - 7 * 24 * 3600 * 1000}&paneList=((${this._platform.id()}-${this._metric.id()}))`;
> 
> Does performance dashboard code have a centralized place where URLs are built? Doing it in every view controller seems less than ideal.

See PageRouter and how it's used. We should never build a URL string like this. r-.

> Websites/perf.webkit.org/public/v3/components/test-health-cell.js:52
> +        return (new Date() - this._timeForLastDataPoint) / 1000 / 3600;

Ditto. Also, it's not okay to keep calling Date.now() in different places.
This page can be open for multiple hours or even days.
The value of new Date/Date.now() would change over time.'
r- for that reason as well.

Comment on attachment 327826 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=327826&action=review

>>> Websites/perf.webkit.org/public/v3/components/test-health-cell.js:42
>>> +        return `/v3/#/charts?since=${Date.now() - 7 * 24 * 3600 * 1000}&paneList=((${this._platform.id()}-${this._metric.id()}))`;
>> 
>> Does performance dashboard code have a centralized place where URLs are built? Doing it in every view controller seems less than ideal.
> 
> See PageRouter and how it's used. We should never build a URL string like this. r-.

Are you suggesting make PageRouter.url and dependent functions static functions and call PageRouter.url here with `charts` and {since: ..., paneList ...} as arguments?
Or I dup the logic implemented in PageRouter.url?

Created attachment 328163 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 328163 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=328163&action=review

> Websites/perf.webkit.org/ChangeLog:8
> +        Added a tet health page which shows freshness of a test.

Typo: tet -> test.
Also, this page isn't showing the freshness of tests, it's showing the freshness of test results.

> Websites/perf.webkit.org/ChangeLog:9
> +        Shows the health status of tests which are specified in summary pages.

It's not that summary page specifies which tests' health to reported.
It's that we're reporting the health status of the tests shown in the summary page; or put in another way,
the health status page reports on the same set of tests as the one shown in the summary page.

> Websites/perf.webkit.org/ChangeLog:13
> +        (TestHealthCell): A cell of the health status table, color will transit from green to red base on the time of last data point for a test.

This is a really long line. Wrap the line at some point?

> Websites/perf.webkit.org/ChangeLog:33
> +        * server-tests/api-manifest-tests.js: Added 'warningHourBaseline' and 'ratingCurveCoefficient' so that we can config the parameter of sigmoid funciton.

Wrap this line?

> Websites/perf.webkit.org/public/v3/components/test-health-cell.js:1
> +class TestHealthCell extends ComponentBase {

Since this is not a table cell, we should call it TestHealthStatus or something instead.

> Websites/perf.webkit.org/public/v3/components/test-health-cell.js:13
> +        this._checkTimeInterval = 2 * 24 * 3600 * 1000;

It seems like this shouldn't be hard-coded here.

> Websites/perf.webkit.org/public/v3/components/test-health-cell.js:23
> +            this._measurementSet.fetchBetween(this._measurementSetFetchTime - this._checkTimeInterval, this._measurementSetFetchTime).then(() => {

render() function shouldn't be the one doing fetching.
render() function in general shouldn't have that kind of side effect.
r- because this needs to be moved to elsewhere.

I think it's a cleaner design for the constructor of this class to take a measurement set,
and then it had a separate function like update() which does fetching.
We can then call it in loadConfig of TestHealthPage.

> Websites/perf.webkit.org/public/v3/components/test-health-cell.js:29
> +                }
> +                else

Nit: } and else should be on the same line.

> Websites/perf.webkit.org/public/v3/components/test-health-cell.js:35
> +        }
> +        else {

Ditto.

> Websites/perf.webkit.org/public/v3/components/test-health-cell.js:37
> +            const rating = 1 / (1 + Math.pow(this._ratingCurveCoefficient, this.hoursSinceLastDataPoint() - this._warningHourBaseline));
> +            const hue = Math.round(120 * rating);

What is this function about? Please explain in the change log.
Also, I don't think it makes sense to make ratingCurveCoefficient configurable.
It's just to be simply hard-coded since it's not something the user would be keep adjusting over time.

> Websites/perf.webkit.org/public/v3/components/test-health-cell.js:65
> +        for (const key in state)
> +            params.push(key + '=' + this._serializeHashQueryValue(state[key]));
> +        const query = params.length ? ('?' + params.join('&')) : '';
> +        return `/v3/#/charts${query}`;

This is still all wrong. See DashboardPage's line 101.
You just copy & paste that code here, not duplicate the logic to construct a magic string.
r- because of this.

> Websites/perf.webkit.org/public/v3/components/test-health-cell.js:70
> +        return `${this._noDataWithinInterval ? 'More than ' : ''}${this.hoursSinceLastDataPoint().toFixed(2)} hours since last data point on platform: "${this._platform.name()}" test: "${this._metric.test().fullName()}"`;

Why don't we generalize BuildRequest's waitingTime and use it here instead?
That'd make the time interval label look much nicer.

> Websites/perf.webkit.org/public/v3/components/test-health-cell.js:80
> +        return `<div id='cell'></div>`

Nit: Needs a semicolon at the end.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:3
> +    constructor(configs, warningHourBaseline, ratingCurveCoefficient)

summaryPages isn't a list of configurations.
I think it's better to call this explicitly summaryPageConfiguration instead.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:9
> +        this.loadConfig(configs);

We should call this after having set this.constructTableLazily.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:13
> +    loadConfig(configs) {

Nit: { should be on the new line.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:15
> +        const platformWithOrder = [];
> +        const metricWithOrder = [];

What's the point of having separate array?
You can just use spread operator at where you're calling a map as in:
[...platformSet].map((platformId) => Platform.findById(platformId)).

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:20
> +            const platforms = [].concat(...config.platformGroups.map((platformGroup) => platformGroup.platforms));

These aren't platforms but rather platform IDs.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:25
> +                platformSet.add(platform);
> +                platformWithOrder.push(platform);

You should resolve platform IDs to platform right here.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:29
> +            const metrics = [].concat(...config.metricGroups.map((metricGroup) =>
> +                [].concat(...metricGroup.subgroups.map((subgroup) => subgroup.metrics))));

Ditto. These are metric IDs.
Having two nested concat like this is really hard to read.
Why don't we extract the lambda to a separate line with a descriptive name like metricsInGroup.
By the way, it's probably better to resolve metric ID to Metric objects right here instead.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:38
> +            for (const platform of Object.keys(excludedConfigs)) {

Just do: for (const platformID in excludedConfigs) instead.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:46
> +        this._platforms = platformWithOrder.map(Platform.findById.bind(Platform));
> +        this._metrics = metricWithOrder.map(Metric.findById.bind(Metric));

This is not a great way to resolve platform & metric IDs.
FWIW, some metric & platform IDs may not exist.
If anything it should be resolved when we're adding things to the set instead.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:52
> +        this.renderReplace(this.content().querySelector('.test-health'),

Use id="test-health" instead in the markup so that we can do this.content('test-health').
querySelector is the old way. I'm trying to get rid of its use.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:60
> +        const tableHeadElements = [element('th',  {class: 'table-corner'},'Platform \\ Test')];

Nit: Need a space before ,.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:63
> +            tableHeadElements.push(element('th', {class: 'diagonal-header'}, element('div', element('span', metric.test().path()[0].name()))));

Why are we not calling metric.test.label() instead?

Not that for LabeledObject, label() is the human readable name and name() is some machine readable name
even though Test object currently aliases label() to name().
Nevertheless, we should be calling label() since what we want here is the human readable name.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:66
> +            const row = [element('th', platform.name())];

Ditto: platform.label().

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:69
> +            metrics.forEach((metric) => {
> +                row.push(this.constructTableCell(platform, metric, excludedConfigurations));
> +            });

More canonical way to write this in ES6 would be:
const row = [element('th', platform.name()), ...metrics.map((metric) => this.constructTableCell(platform, metric, excludedConfigurations))]

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:81
> +            return element('td', {class: 'blank-cell'}, element('div'));

Instead of manually inserting a div just to show a grey box,
generate that synthetically in CSS using either ::before or ::after.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:84
> +            return element('td', {class: 'blank-cell'}, element('div'));

Ditto.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:121
> +            .test-health th.diagonal-header > div {
> +                transform:
> +                  translate(1rem, 4rem)
> +                  rotate(315deg);

Nit: typically, the entire transform is writing in a single line.

[Ryosuke Niwa]

(In reply to dewei_zhu from comment #5)
> Comment on attachment 327826 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=327826&action=review
> 
> >>> Websites/perf.webkit.org/public/v3/components/test-health-cell.js:42
> >>> +        return `/v3/#/charts?since=${Date.now() - 7 * 24 * 3600 * 1000}&paneList=((${this._platform.id()}-${this._metric.id()}))`;
> >> 
> >> Does performance dashboard code have a centralized place where URLs are built? Doing it in every view controller seems less than ideal.
> > 
> > See PageRouter and how it's used. We should never build a URL string like this. r-.
> 
> Are you suggesting make PageRouter.url and dependent functions static
> functions and call PageRouter.url here with `charts` and {since: ...,
> paneList ...} as arguments?
> Or I dup the logic implemented in PageRouter.url?

See line 101 of dashboard-pages.js.

Created attachment 328751 [details]
Patch

[EWS Watchlist]

Comment on attachment 328751 [details]
Patch

Attachment 328751 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5537195

New failing tests:
http/tests/inspector/network/resource-response-service-worker.html

[EWS Watchlist]

Created attachment 328771 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Ryosuke Niwa]

Comment on attachment 328751 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=328751&action=review

This patch looks better but I think we need one more iteration of polishing before we can land.

> Websites/perf.webkit.org/public/include/manifest-generator.php:47
> +            'warningHourBaseline' => config('warningHourBaseline'),

I don't think this variable name makes much sense. What does "baseline" mean in this context?

> Websites/perf.webkit.org/public/v3/components/test-health-status.js:1
> +class TestHealthStatus extends ComponentBase {

I would call this FreshnessIndicator or so.
There is nothing about this component that's specific to test other than the fact it takes platform & metric just to create a label.
We're better off creating that label in the page label and just passing in a label for the configuration here.

> Websites/perf.webkit.org/public/v3/components/test-health-status.js:20
> +        if (!this._timeForLastDataPoint)
> +            this.renderReplace(this.content('container'), new SpinnerIcon);

Just add an early exit here instead of having else.

> Websites/perf.webkit.org/public/v3/components/test-health-status.js:36
> +        return `${this._noCurrentDataPoint ? 'More than ' : ''}${BuildRequest.formatTimeInterval((this._measurementSetFetchTime - this._timeForLastDataPoint) / 1000)}` +
> +            `since last data point on platform: "${this._platform.name()}" test: "${this._metric.test().fullName()}"`;

These two lines are really hard to read.
Store the results of each JS expression into a separate local variable so that the whole string is easier to read?
If you've made the above suggestion, then you can do `~ since the last data point for {$this._configLabel}`
where this._configLabel is `"${this._metric.test().fullName()}" for "${this._platform.name()}"`,
which is constructed in the page component. There is no need to say test or platform.
Names such as Speedometer and macOS would make it obvious what we're talking about.

> Websites/perf.webkit.org/public/v3/models/build-request.js:87
> +    static formatTimeInterval(intervalInSeconds) {

Could you add some unit tests for this function?
There are many tests for waitingTime in unit-tests/build-request-tests.js ready.
So just add a few to make sure formatTimeInterval doest the same thing.

> Websites/perf.webkit.org/public/v3/models/build-request.js:125
> +        const diffInSeconds = (referenceTime - this.createdAt()) / 1000;

Instead of forcing each caller to divide the number with 1000, just pass in time in milliseconds.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:2
> +class TestHealthPage extends PageWithHeading {

I'd call this TestFreshnessPage instead because "health" can mean many things.
e.g. current test results being 50% worse than baseline might be "unhealthy" but that's not what this page tells you.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:5
> +        super('Test-Health', null);

Don't capitalize the component/page name. It should all be in lower case.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:7
> +        this._checkTimeInterval = this._warningHourBaseline * 2 * 3600 * 1000;

I'd call this variable _timeDuration instead.
Time interval seems to imply more of a range (i.e. start & end) than the duration.
e.g. https://en.wikipedia.org/wiki/Interval_(mathematics)

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:9
> +        this._healthStatusForPlatformAndMetric = null;

This map doesn't store health status at all. It just stores data points in a given platform & metric.
lastDataPointByConfiguration?

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:15
> +    _loadConfig(summaryPageConfiguration) {

Nit: { should be on a new line.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:23
> +            const platformIds = [].concat(...config.platformGroups.map((platformGroup) => platformGroup.platforms));
> +            for (const platformId of platformIds) {

Instead of flattening arrays upfront like this, you can just have a nested for loops that go over platformGroups and platforms.
That'd make the code a lot clearer.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:36
> +            const metricIds = [].concat(...[].concat(...metricsInGroup));
> +            for (const metricId of metricIds) {

Ditto. Two nested concat like this is impossible to reason.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:62
> +    _loadDataPointsForTests() {

Nit: _*fetch*DataPointsForTests, { should be on a new line.
Also, I'd call this _fetchTimeSeries or _fetchTestResults instead.
Data points is very specific kind of data returned by timeSeries.data() in our codebase.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:67
> +            const healthStatusByMetric = new Map;

This map doesn't store health status. It stores the time for the last data point & url.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:77
> +                    let noCurrentDataPoint = false;

There is no need to have this separate boolean stored.
We can use timeForLastDataPoint === null as !noCurrentDataPoint.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:86
> +                        metric.id(), this._measurementSetFetchTime - this._checkTimeInterval));

Please store the difference as a local variable, e.g. startTime instead of duplicating the subtraction everywhere.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:87
> +                    healthStatusByMetric.set(metric, [timeForLastDataPoint, noCurrentDataPoint, url]);

Use dictionary instead of an array for clarity.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:108
> +            tableHeadElements.push(element('th', {class: 'diagonal-header'}, element('div', element('span', metric.test().path()[0].label()))));

Why are you creating div & span?
In general, creating wrapper elements like these is an anti-pattern.
We're better off using SVG and alike if you're trying to create a rectangle, etc... with it.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:112
> +                const [timeForLastDataPoint, noCurrentDataPoint, url] = healthStatusForPlatformAndMetric.get(platform).get(metric) || [null, null, null];

There is virtually no point in storing the URL earlier and using it here.
Here, we have platform, metric, and the startTime can be computed from the instance variables.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:121
> +    static _isValidPlatformMetricCombination(platform, metric, excludedConfigurations) {

Nit: { should be on a new line.
What's the point of making this function static?
I think we're better off making it an instance method so that you don't have to pass in excludedConfigurations.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:127
> +    _constructTableCell(platform, metric, excludedConfigurations, timeForLastDataPoint, noCurrentDataPoint, url) {

Nit: { should be on a new line.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:133
> +        return element('td', {class: 'status-cell'}, new TestHealthStatus(platform, metric, this._warningHourBaseline, this._measurementSetFetchTime, timeForLastDataPoint, noCurrentDataPoint, url));

Instead of re-creating the entire table each time, we should simply construct the table once,
and then for each status component, we should update its value in a separate loop.
In subsequent calls to render(), we would only execute that second loop.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:169
> +            #test-health th.diagonal-header > div > span {

We shouldn't have a span just so that can have a border line this. That's an anti-pattern.

[Ryosuke Niwa]

Comment on attachment 328751 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=328751&action=review

> Websites/perf.webkit.org/public/v3/components/test-health-status.js:23
> +            const rating = 1 / (1 + Math.pow(1.2, hoursSinceLastDataPoint - this._warningHourBaseline));

This is a logistic function: https://en.wikipedia.org/wiki/Logistic_function
It's probably better if we used the standard form: 1 / (1 + Math.exp(- Math.log(1.2) * (x - x_0)))

Created attachment 329214 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 329214 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=329214&action=review

> Websites/perf.webkit.org/ChangeLog:10
> +        Using an S-shape logistic function to evaluate the freshness of the data points.

Nit: S-shaped but I don't think we need to say that since logistic function is by definition S-shaped.

> Websites/perf.webkit.org/public/include/manifest-generator.php:47
> +            'acceptableLastDataPointDurationInHour' => config('acceptableLastDataPointDurationInHour'),

This is a very long name. How about something like testAgeToleranceInHours or testFreshnessToleranceInHours,
or even expectedTestFreshness.

> Websites/perf.webkit.org/public/v3/components/freshness-indicator.js:25
> +        this._lastDataPointDuration = lastDataPointDuration;
> +        this._summary = summary;
> +        this._acceptableLastDataPointDurationInHour = acceptableLastDataPointDurationInHour;
> +        this._url = url;
> +        this.enqueueToRender();

You should do this work in update, and then render() should lazily update the DOM.

> Websites/perf.webkit.org/public/v3/components/freshness-indicator.js:32
> +        this.renderReplace(this.content('container'), icon);

You should do this renderReplace in _createIcon. Should be renamed to _renderIndicator or something.

> Websites/perf.webkit.org/public/v3/components/freshness-indicator.js:41
> +        const hoursSinceLastDataPoint = this._lastDataPointDuration / 3600 ;

Nit: Trailing space before ;.

> Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:7
> +        this._acceptableLastDataPointDurationInHour = acceptableLastDataPointDurationInHour || 24;
> +        this._timeDurationInMillionSeconds = this._acceptableLastDataPointDurationInHour * 2 * 3600 * 1000;

We should store all these values in milliseconds always.

> Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:34
> +                    const platform = Platform.findById(platformId);
> +                    if (!platform)
> +                        continue;
> +                    this._platforms.push(platform);

Why don't we just add them all to the set and sort them later?
In fact, Set preserves the order in JS.

> Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:84
> +                measurementSet.fetchBetween(startTime, this._measurementSetFetchTime).then(() => {

It's a bit odd that you're keep running this code without setting fetchBetwen's noCache option to false.

> Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:102
> +        this.renderReplace(this.content('test-health'), this._constructTableLazily.evaluate(this._platforms, this._metrics));

You wanna do the replace call in _constructTable because otherwise, it would run appendChild each time.

> Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:115
> +                indicator.update(timeDurationInMillionSeconds / 1000, this._acceptableLastDataPointDurationInHour, summary, url);

You should just make update take milliseconds instead since that's the canonical time unit in JS.

> Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:127
> +            tableHeadElements.push(element('th', {class: 'diagonal-header'}, element('div', metric.test().path()[0].label())));

You need to call metric.test().fullName() as discussed in person.
Otherwise, you won't be able to distinguish subtests under a single top-level test like Dromaeo.

> Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:143
> +            && this._excludedConfigurations[platform.id()].includes(+metric.id()))

Instead of converting metric.id() to a number, do: some((x) => x == metric.id()).

[Ryosuke Niwa]

The patch was landed in https://trac.webkit.org/changeset/225898.

[Radar WebKit Bug Importer]

<rdar://problem/36066362>