measurement-sets API can incorrectly order points with OS version without commit...
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 20 Jan 2017 06:42:43 +0000 (06:42 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 20 Jan 2017 06:42:43 +0000 (06:42 +0000)
https://bugs.webkit.org/show_bug.cgi?id=167227

Reviewed by Chris Dumez.

Ignore revision_order for the purpose of ordering data points in /api/measurement-sets.

These orderings are used in some UI (e.g A/B testing) to order OS build numbers which do not have a timestamp
associated with each "revision".

The baseline measurements made in our internal dashboard were using these ordering numbers before ordering
results with build time. Because those data points don't have an associated Webkit revisions, all data points
were ordered first by macOS's revision_order, then build time. Because v3 UI completely ignores revision_order
for the purpose of plotting data points, this resulted in some data points being plotted in a wrong order
with some lines going backwards in time.

This patch addresses this discrepancy by stop ordering data points with revision_order in the JSON API.

* public/api/measurement-set.php:
(MeasurementSetFetcher::execute_query): Fixed the bug.
* server-tests/api-measurement-set-tests.js: Added a test.

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@210953 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Websites/perf.webkit.org/ChangeLog
Websites/perf.webkit.org/public/api/measurement-set.php
Websites/perf.webkit.org/server-tests/api-measurement-set-tests.js

index 69394bffdd28ad8d088666fd0a35bb746f4f2d10..429d84aa8a194b43dfcae6f4f68782ba7aed9bba 100644 (file)
@@ -1,3 +1,27 @@
+2017-01-19  Ryosuke Niwa  <rniwa@webkit.org>
+
+        measurement-sets API can incorrectly order points with OS version without commit time
+        https://bugs.webkit.org/show_bug.cgi?id=167227
+
+        Reviewed by Chris Dumez.
+
+        Ignore revision_order for the purpose of ordering data points in /api/measurement-sets.
+
+        These orderings are used in some UI (e.g A/B testing) to order OS build numbers which do not have a timestamp
+        associated with each "revision".
+
+        The baseline measurements made in our internal dashboard were using these ordering numbers before ordering
+        results with build time. Because those data points don't have an associated Webkit revisions, all data points
+        were ordered first by macOS's revision_order, then build time. Because v3 UI completely ignores revision_order
+        for the purpose of plotting data points, this resulted in some data points being plotted in a wrong order
+        with some lines going backwards in time.
+
+        This patch addresses this discrepancy by stop ordering data points with revision_order in the JSON API.
+
+        * public/api/measurement-set.php:
+        (MeasurementSetFetcher::execute_query): Fixed the bug.
+        * server-tests/api-measurement-set-tests.js: Added a test.
+
 2017-01-19  Ryosuke Niwa  <rniwa@webkit.org>
 
         Build fix after r210626. We need to clear Triggerable's static map in each iteration.
index 95bc535626888b434855dc90d29a6e706af5f8ed..6c4d083590f9cf75806da9f57ecdf267c48a160a 100644 (file)
@@ -171,14 +171,14 @@ class MeasurementSetFetcher {
         return $this->db->query('
             SELECT test_runs.*, build_id, build_number, build_builder, build_time,
             array_agg((commit_id, commit_repository, commit_revision, extract(epoch from commit_time at time zone \'utc\') * 1000)) AS revisions,
-            extract(epoch from max(commit_time at time zone \'utc\')) * 1000 AS revision_time, max(commit_order) AS revision_order
+            extract(epoch from max(commit_time at time zone \'utc\')) * 1000 AS revision_time
                 FROM builds
                     LEFT OUTER JOIN build_commits ON commit_build = build_id
                     LEFT OUTER JOIN commits ON build_commit = commit_id, test_runs
                 WHERE run_build = build_id AND run_config = $1 AND NOT EXISTS (SELECT * FROM build_requests WHERE request_build = build_id)
                 GROUP BY build_id, build_builder, build_number, build_time, build_latest_revision, build_slave,
                     run_id, run_config, run_build, run_iteration_count_cache, run_mean_cache, run_sum_cache, run_square_sum_cache, run_marked_outlier
-                ORDER BY revision_time, revision_order, build_time', array($config_id));
+                ORDER BY revision_time, build_time', array($config_id));
     }
 
     static function format_map()
index 030f6c805a126315d5588b8885bd68308ccb910a..9e5a364493f180093c4b45ae8f099d87d290586a 100644 (file)
@@ -372,6 +372,75 @@ describe("/api/measurement-set", function () {
         }).catch(done);
     });
 
+    it("should order results by build time when commit times are missing", function (done) {
+        const remote = TestServer.remoteAPI();
+        let repositoryId;
+        addBuilderForReport(reportWithBuildTime[0]).then(() => {
+            const db = TestServer.database();
+            return Promise.all([
+                db.insert('repositories', {'id': 1, 'name': 'macOS'}),
+                db.insert('commits', {'id': 2, 'repository': 1, 'revision': 'macOS 16A323', 'order': 0}),
+                db.insert('commits', {'id': 3, 'repository': 1, 'revision': 'macOS 16C68', 'order': 1}),
+            ]);
+        }).then(() => {
+            return remote.postJSON('/api/report/', [{
+                "buildNumber": "1001",
+                "buildTime": '2017-01-19 15:28:01',
+                "revisions": {
+                    "macOS": {
+                        "revision": "macOS 16C68",
+                    },
+                },
+                "builderName": "someBuilder",
+                "builderPassword": "somePassword",
+                "platform": "Sierra",
+                "tests": { "Test": {"metrics": {"Time": { "baseline": [1, 2, 3, 4, 5] } } } },
+            }]);
+        }).then(function () {
+            return remote.postJSON('/api/report/', [{
+                "buildNumber": "1002",
+                "buildTime": '2017-01-19 19:46:37',
+                "revisions": {
+                    "macOS": {
+                        "revision": "macOS 16A323",
+                    },
+                },
+                "builderName": "someBuilder",
+                "builderPassword": "somePassword",
+                "platform": "Sierra",
+                "tests": { "Test": {"metrics": {"Time": { "baseline": [5, 6, 7, 8, 9] } } } },
+            }]);
+        }).then(function () {
+            return queryPlatformAndMetricWithRepository('Sierra', 'Time', 'macOS');
+        }).then(function (result) {
+            return remote.getJSONWithStatus(`/api/measurement-set/?platform=${result.platformId}&metric=${result.metricId}`);
+        }).then(function (response) {
+            const currentRows = response['configurations']['baseline'];
+            assert.equal(currentRows.length, 2);
+            assert.deepEqual(format(response['formatMap'], currentRows[0]), {
+               mean: 3,
+               iterationCount: 5,
+               sum: 15,
+               squareSum: 55,
+               markedOutlier: false,
+               revisions: [[3, 1, 'macOS 16C68', 0]],
+               commitTime: +Date.UTC(2017, 0, 19, 15, 28, 1),
+               buildTime: +Date.UTC(2017, 0, 19, 15, 28, 1),
+               buildNumber: '1001' });
+            assert.deepEqual(format(response['formatMap'], currentRows[1]), {
+                mean: 7,
+                iterationCount: 5,
+                sum: 35,
+                squareSum: 255,
+                markedOutlier: false,
+                revisions: [[2, 1, 'macOS 16A323', 0]],
+                commitTime: +Date.UTC(2017, 0, 19, 19, 46, 37),
+                buildTime: +Date.UTC(2017, 0, 19, 19, 46, 37),
+                buildNumber: '1002' });
+            done();
+        }).catch(done);
+    });
+
     function buildNumbers(parsedResult, config)
     {
         return parsedResult['configurations'][config].map(function (row) {
@@ -417,7 +486,6 @@ describe("/api/measurement-set", function () {
         }).catch(done);
     });
 
-
     it("should create cached results", function (done) {
         const remote = TestServer.remoteAPI();
         let cachePrefix;