From 30d3866ab9efd2c71154f4f58acd5d70914e9a33 Mon Sep 17 00:00:00 2001 From: keyonghan <54558023+keyonghan@users.noreply.github.com> Date: Wed, 27 Oct 2021 15:33:06 -0700 Subject: [PATCH] Add extra benchmark metrics to test name in addition to builder name (#92530) --- .../lib/framework/metrics_center.dart | 21 ++++++++-- dev/devicelab/test/metrics_center_test.dart | 39 ++++++++++++++----- 2 files changed, 47 insertions(+), 13 deletions(-) diff --git a/dev/devicelab/lib/framework/metrics_center.dart b/dev/devicelab/lib/framework/metrics_center.dart index 03a9965780..6a587749ec 100644 --- a/dev/devicelab/lib/framework/metrics_center.dart +++ b/dev/devicelab/lib/framework/metrics_center.dart @@ -54,7 +54,7 @@ Future connectFlutterDestination() async { /// "host_type": "linux", /// "host_version": "debian-10.11" /// } -List parse(Map resultsJson, Map benchmarkTags) { +List parse(Map resultsJson, Map benchmarkTags, String taskName) { print('Results to upload to skia perf: $resultsJson'); print('Benchmark tags to upload to skia perf: $benchmarkTags'); final List scoreKeys = @@ -82,6 +82,18 @@ List parse(Map resultsJson, Map b tags, ), ); + + // Add an extra entry under test name. This way we have duplicate metrics + // under both builder name and test name. Once we have enough data and update + // existing alerts to point to test name, we can deprecate builder name ones. + // https://github.com/flutter/flutter/issues/74522#issuecomment-942575581 + tags[kNameKey] = taskName; + metricPoints.add( + MetricPoint( + (resultData[scoreKey] as num).toDouble(), + tags, + ), + ); } return metricPoints; } @@ -99,7 +111,7 @@ Future upload( FlutterDestination metricsDestination, List metricPoints, int commitTimeSinceEpoch, - String? taskName, + String taskName, ) async { await metricsDestination.update( metricPoints, @@ -107,7 +119,7 @@ Future upload( commitTimeSinceEpoch, isUtc: true, ), - taskName ?? 'default', + taskName, ); } @@ -127,11 +139,12 @@ Future uploadToSkiaPerf(String? resultsPath, String? commitTime, String? t } else { commitTimeSinceEpoch = DateTime.now().millisecondsSinceEpoch; } + taskName = taskName ?? 'default'; final Map benchmarkTagsMap = jsonDecode(benchmarkTags ?? '{}') as Map; final File resultFile = File(resultsPath); Map resultsJson = {}; resultsJson = json.decode(await resultFile.readAsString()) as Map; - final List metricPoints = parse(resultsJson, benchmarkTagsMap); + final List metricPoints = parse(resultsJson, benchmarkTagsMap, taskName); final FlutterDestination metricsDestination = await connectFlutterDestination(); await upload(metricsDestination, metricPoints, commitTimeSinceEpoch, taskName); } diff --git a/dev/devicelab/test/metrics_center_test.dart b/dev/devicelab/test/metrics_center_test.dart index f9bebee5fc..54928c2158 100644 --- a/dev/devicelab/test/metrics_center_test.dart +++ b/dev/devicelab/test/metrics_center_test.dart @@ -24,6 +24,27 @@ class FakeFlutterDestination implements FlutterDestination { void main() { group('Parse', () { + test('duplicate entries for both builder name and test name', () { + final Map results = { + 'CommitBranch': 'master', + 'CommitSha': 'abc', + 'BuilderName': 'Linux test', + 'ResultData': { + 'average_frame_build_time_millis': 0.4550425531914895, + }, + 'BenchmarkScoreKeys': [ + 'average_frame_build_time_millis', + ], + }; + final List metricPoints = parse(results, {}, 'test'); + + expect(metricPoints.length, 2); + expect(metricPoints[0].value, equals(0.4550425531914895)); + expect(metricPoints[0].tags[kNameKey], 'Linux test'); + expect(metricPoints[1].value, equals(0.4550425531914895)); + expect(metricPoints[1].tags[kNameKey], 'test'); + }); + test('without additional benchmark tags', () { final Map results = { 'CommitBranch': 'master', @@ -38,10 +59,10 @@ void main() { '90th_percentile_frame_build_time_millis', ], }; - final List metricPoints = parse(results, {}); + final List metricPoints = parse(results, {}, 'task abc'); expect(metricPoints[0].value, equals(0.4550425531914895)); - expect(metricPoints[1].value, equals(0.473)); + expect(metricPoints[2].value, equals(0.473)); }); test('with additional benchmark tags', () { @@ -65,12 +86,12 @@ void main() { 'host_type': 'linux', 'host_version': 'debian-10.11' }; - final List metricPoints = parse(results, tags); + final List metricPoints = parse(results, tags, 'task abc'); expect(metricPoints[0].value, equals(0.4550425531914895)); expect(metricPoints[0].tags.keys.contains('arch'), isTrue); - expect(metricPoints[1].value, equals(0.473)); - expect(metricPoints[1].tags.keys.contains('device_type'), isTrue); + expect(metricPoints[2].value, equals(0.473)); + expect(metricPoints[2].tags.keys.contains('device_type'), isTrue); }); test('succeeds - null ResultData', () { @@ -81,7 +102,7 @@ void main() { 'ResultData': null, 'BenchmarkScoreKeys': null, }; - final List metricPoints = parse(results, {}); + final List metricPoints = parse(results, {}, 'tetask abcst'); expect(metricPoints.length, 0); }); @@ -102,9 +123,9 @@ void main() { '90th_percentile_frame_build_time_millis', ], }; - final List metricPoints = parse(results, {}); + final List metricPoints = parse(results, {}, 'task abc'); final FakeFlutterDestination flutterDestination = FakeFlutterDestination(); - String? taskName; + const String taskName = 'default'; const int commitTimeSinceEpoch = 1629220312; await upload(flutterDestination, metricPoints, commitTimeSinceEpoch, taskName); @@ -126,7 +147,7 @@ void main() { '90th_percentile_frame_build_time_millis', ], }; - final List metricPoints = parse(results, {}); + final List metricPoints = parse(results, {}, 'task abc'); final FakeFlutterDestination flutterDestination = FakeFlutterDestination(); const String taskName = 'test'; const int commitTimeSinceEpoch = 1629220312;