From db4c104c8e8a047e189645e30db7d89e64be4460 Mon Sep 17 00:00:00 2001 From: keyonghan <54558023+keyonghan@users.noreply.github.com> Date: Thu, 26 Aug 2021 22:31:02 -0700 Subject: [PATCH] Use task name when uploading metrics to skia perf (#89004) --- dev/bots/pubspec.yaml | 4 +- dev/devicelab/lib/command/upload_results.dart | 4 +- .../lib/framework/metrics_center.dart | 35 +++++++--- dev/devicelab/pubspec.yaml | 4 +- dev/devicelab/test/metrics_center_test.dart | 65 +++++++++++++++++++ 5 files changed, 99 insertions(+), 13 deletions(-) diff --git a/dev/bots/pubspec.yaml b/dev/bots/pubspec.yaml index 1659edb639..d90343788a 100644 --- a/dev/bots/pubspec.yaml +++ b/dev/bots/pubspec.yaml @@ -42,7 +42,7 @@ dependencies: json_annotation: 4.1.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade" logging: 1.0.1 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade" matcher: 0.12.11 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade" - metrics_center: 1.0.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade" + metrics_center: 1.0.1 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade" mime: 1.0.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade" node_preamble: 2.0.1 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade" package_config: 2.0.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade" @@ -72,4 +72,4 @@ dependencies: dev_dependencies: test_api: 0.4.3 -# PUBSPEC CHECKSUM: e827 +# PUBSPEC CHECKSUM: 3028 diff --git a/dev/devicelab/lib/command/upload_results.dart b/dev/devicelab/lib/command/upload_results.dart index df7f06fd7d..6a25ca86af 100644 --- a/dev/devicelab/lib/command/upload_results.dart +++ b/dev/devicelab/lib/command/upload_results.dart @@ -21,6 +21,7 @@ class UploadResultsCommand extends Command { 'checkouts run in detached HEAD state, so the branch must be passed.', ); argParser.addOption('luci-builder', help: '[Flutter infrastructure] Name of the LUCI builder being run on.'); + argParser.addOption('task-name', help: '[Flutter infrastructure] Name of the task being run on.'); argParser.addOption('test-status', help: 'Test status: Succeeded|Failed'); argParser.addOption('commit-time', help: 'Commit time in UNIX timestamp'); } @@ -40,6 +41,7 @@ class UploadResultsCommand extends Command { final String? builderName = argResults!['luci-builder'] as String?; final String? testStatus = argResults!['test-status'] as String?; final String? commitTime = argResults!['commit-time'] as String?; + final String? taskName = argResults!['task-name'] as String?; // Upload metrics to metrics_center from test runner when `commitTime` is specified. This // is mainly for testing purpose. @@ -47,7 +49,7 @@ class UploadResultsCommand extends Command { // TODO(keyong): remove try block to block test when this is validated to work https://github.com/flutter/flutter/issues/88484 try { if (commitTime != null) { - await uploadToMetricsCenter(resultsPath, commitTime); + await uploadToMetricsCenter(resultsPath, commitTime, taskName); print('Successfully uploaded metrics to metrics center'); } } on Exception catch (e, stacktrace) { diff --git a/dev/devicelab/lib/framework/metrics_center.dart b/dev/devicelab/lib/framework/metrics_center.dart index 83bbf5d922..904905a398 100644 --- a/dev/devicelab/lib/framework/metrics_center.dart +++ b/dev/devicelab/lib/framework/metrics_center.dart @@ -70,8 +70,33 @@ List parse(Map resultsJson) { return metricPoints; } +/// Upload metrics to GCS bucket used by Skia Perf. +/// +/// Skia Perf picks up all available files under the folder, and +/// is robust to duplicate entries. +/// +/// Files will be named based on `taskName`, such as +/// `complex_layout_scroll_perf__timeline_summary_values.json`. +/// If no `taskName` is specified, data will be saved to +/// `default_values.json`. +Future upload( + FlutterDestination metricsDestination, + List metricPoints, + int commitTimeSinceEpoch, + String? taskName, +) async { + await metricsDestination.update( + metricPoints, + DateTime.fromMillisecondsSinceEpoch( + commitTimeSinceEpoch, + isUtc: true, + ), + taskName ?? 'default', + ); +} + /// Upload test metrics to metrics center. -Future uploadToMetricsCenter(String? resultsPath, String? commitTime) async { +Future uploadToMetricsCenter(String? resultsPath, String? commitTime, String? taskName) async { int commitTimeSinceEpoch; if (resultsPath == null) { return; @@ -86,11 +111,5 @@ Future uploadToMetricsCenter(String? resultsPath, String? commitTime) asyn resultsJson = json.decode(await resultFile.readAsString()) as Map; final List metricPoints = parse(resultsJson); final FlutterDestination metricsDestination = await connectFlutterDestination(); - await metricsDestination.update( - metricPoints, - DateTime.fromMillisecondsSinceEpoch( - commitTimeSinceEpoch, - isUtc: true, - ), - ); + await upload(metricsDestination, metricPoints, commitTimeSinceEpoch, taskName); } diff --git a/dev/devicelab/pubspec.yaml b/dev/devicelab/pubspec.yaml index 241fed7c30..1bf6ce275d 100644 --- a/dev/devicelab/pubspec.yaml +++ b/dev/devicelab/pubspec.yaml @@ -11,7 +11,7 @@ dependencies: file: 6.1.2 http: 0.13.3 meta: 1.7.0 - metrics_center: 1.0.0 + metrics_center: 1.0.1 path: 1.8.0 platform: 3.0.2 process: 4.2.3 @@ -73,4 +73,4 @@ dev_dependencies: web_socket_channel: 2.1.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade" webkit_inspection_protocol: 1.0.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade" -# PUBSPEC CHECKSUM: e827 +# PUBSPEC CHECKSUM: 3028 diff --git a/dev/devicelab/test/metrics_center_test.dart b/dev/devicelab/test/metrics_center_test.dart index 920cb4c39a..b256d941cc 100644 --- a/dev/devicelab/test/metrics_center_test.dart +++ b/dev/devicelab/test/metrics_center_test.dart @@ -7,6 +7,21 @@ import 'package:metrics_center/metrics_center.dart'; import 'common.dart'; +class FakeFlutterDestination implements FlutterDestination { + /// Overrides the skia perf `update` function, which uploads new data to gcs if there + /// doesn't exist the commit, otherwise updates existing data by appending new ones. + @override + Future update(List points, DateTime commitTime, String taskName) async { + lastUpdatedPoints = points; + time = commitTime; + name = taskName; + } + + List? lastUpdatedPoints; + DateTime? time; + String? name; +} + void main() { group('Parse', () { test('succeeds', () { @@ -42,4 +57,54 @@ void main() { expect(metricPoints.length, 0); }); }); + + group('Update', () { + test('without taskName', () async { + final Map results = { + 'CommitBranch': 'master', + 'CommitSha': 'abc', + 'BuilderName': 'test', + 'ResultData': { + 'average_frame_build_time_millis': 0.4550425531914895, + '90th_percentile_frame_build_time_millis': 0.473, + }, + 'BenchmarkScoreKeys': [ + 'average_frame_build_time_millis', + '90th_percentile_frame_build_time_millis', + ], + }; + final List metricPoints = parse(results); + final FakeFlutterDestination flutterDestination = FakeFlutterDestination(); + String? taskName; + const int commitTimeSinceEpoch = 1629220312; + + await upload(flutterDestination, metricPoints, commitTimeSinceEpoch, taskName); + + expect(flutterDestination.name, 'default'); + }); + + test('with taskName', () async { + final Map results = { + 'CommitBranch': 'master', + 'CommitSha': 'abc', + 'BuilderName': 'test', + 'ResultData': { + 'average_frame_build_time_millis': 0.4550425531914895, + '90th_percentile_frame_build_time_millis': 0.473, + }, + 'BenchmarkScoreKeys': [ + 'average_frame_build_time_millis', + '90th_percentile_frame_build_time_millis', + ], + }; + final List metricPoints = parse(results); + final FakeFlutterDestination flutterDestination = FakeFlutterDestination(); + const String taskName = 'test'; + const int commitTimeSinceEpoch = 1629220312; + + await upload(flutterDestination, metricPoints, commitTimeSinceEpoch, taskName); + + expect(flutterDestination.name, taskName); + }); + }); }