From 7a278ae47c8939eeaa203770a4e980db44c1082d Mon Sep 17 00:00:00 2001 From: Elias Yishak <42216813+eliasyishak@users.noreply.github.com> Date: Fri, 10 Nov 2023 15:19:50 -0500 Subject: [PATCH] `CommandResultEvent` migrated (#138165) Related to tracker issue: - https://github.com/flutter/flutter/issues/128251 This event was only called from one file (`flutter_command.dart`). With the previous implementation, we actually sent 2 events, one for the result of the `commandPath` and another containing the `maxRss` value from `ProcessInfo`. I have consolidated this down to just one event and used a function to safely get the `maxRss` value, or return null when if there was an error getting that integer value --- .../lib/src/reporting/events.dart | 18 +++++---- .../lib/src/reporting/unified_analytics.dart | 12 ++++++ .../lib/src/runner/flutter_command.dart | 19 +++++++++- .../runner/flutter_command_test.dart | 37 +++++++++++++++++++ 4 files changed, 76 insertions(+), 10 deletions(-) diff --git a/packages/flutter_tools/lib/src/reporting/events.dart b/packages/flutter_tools/lib/src/reporting/events.dart index ec3933ed5d..bffce94fb1 100644 --- a/packages/flutter_tools/lib/src/reporting/events.dart +++ b/packages/flutter_tools/lib/src/reporting/events.dart @@ -189,8 +189,14 @@ class BuildEvent extends UsageEvent { /// An event that reports the result of a top-level command. class CommandResultEvent extends UsageEvent { - CommandResultEvent(super.commandPath, super.result) - : super(flutterUsage: globals.flutterUsage); + CommandResultEvent( + super.commandPath, + super.result, + int? maxRss, + ) : _maxRss = maxRss, + super(flutterUsage: globals.flutterUsage); + + final int? _maxRss; @override void send() { @@ -204,17 +210,13 @@ class CommandResultEvent extends UsageEvent { // A separate event for the memory highwater mark. This is a separate event // so that we can get the command result even if trying to grab maxRss // throws an exception. - try { - final int maxRss = globals.processInfo.maxRss; + if (_maxRss != null) { flutterUsage.sendEvent( 'tool-command-max-rss', category, label: parameter, - value: maxRss, + value: _maxRss, ); - } on Exception catch (error) { - // If grabbing the maxRss fails for some reason, just don't send an event. - globals.printTrace('Querying maxRss failed with error: $error'); } } } diff --git a/packages/flutter_tools/lib/src/reporting/unified_analytics.dart b/packages/flutter_tools/lib/src/reporting/unified_analytics.dart index 7e6fcd3c8c..bc88c620c3 100644 --- a/packages/flutter_tools/lib/src/reporting/unified_analytics.dart +++ b/packages/flutter_tools/lib/src/reporting/unified_analytics.dart @@ -4,6 +4,8 @@ import 'package:unified_analytics/unified_analytics.dart'; +import '../base/io.dart'; +import '../globals.dart' as globals; import '../version.dart'; /// This function is called from within the context runner to perform @@ -52,3 +54,13 @@ Analytics getAnalytics({ clientIde: clientIde, ); } + +/// Function to safely grab the max rss from [ProcessInfo]. +int? getMaxRss(ProcessInfo processInfo) { + try { + return globals.processInfo.maxRss; + } on Exception catch (error) { + globals.printTrace('Querying maxRss failed with error: $error'); + } + return null; +} diff --git a/packages/flutter_tools/lib/src/runner/flutter_command.dart b/packages/flutter_tools/lib/src/runner/flutter_command.dart index 56dd642afb..ced54f2a85 100644 --- a/packages/flutter_tools/lib/src/runner/flutter_command.dart +++ b/packages/flutter_tools/lib/src/runner/flutter_command.dart @@ -13,6 +13,7 @@ import '../application_package.dart'; import '../base/common.dart'; import '../base/context.dart'; import '../base/io.dart' as io; +import '../base/io.dart'; import '../base/os.dart'; import '../base/user_messages.dart'; import '../base/utils.dart'; @@ -30,6 +31,7 @@ import '../globals.dart' as globals; import '../preview_device.dart'; import '../project.dart'; import '../reporting/reporting.dart'; +import '../reporting/unified_analytics.dart'; import '../web/compile.dart'; import 'flutter_command_runner.dart'; import 'target_devices.dart'; @@ -214,6 +216,8 @@ abstract class FlutterCommand extends Command { bool get deprecated => false; + ProcessInfo get processInfo => globals.processInfo; + /// When the command runs and this is true, trigger an async process to /// discover devices from discoverers that support wireless devices for an /// extended amount of time and refresh the device cache with the results. @@ -1376,7 +1380,12 @@ abstract class FlutterCommand extends Command { final DateTime endTime = globals.systemClock.now(); globals.printTrace(userMessages.flutterElapsedTime(name, getElapsedAsMilliseconds(endTime.difference(startTime)))); if (commandPath != null) { - _sendPostUsage(commandPath, commandResult, startTime, endTime); + _sendPostUsage( + commandPath, + commandResult, + startTime, + endTime, + ); } if (_usesFatalWarnings) { globals.logger.checkForFatalLogs(); @@ -1596,7 +1605,13 @@ abstract class FlutterCommand extends Command { DateTime endTime, ) { // Send command result. - CommandResultEvent(commandPath, commandResult.toString()).send(); + final int? maxRss = getMaxRss(processInfo); + CommandResultEvent(commandPath, commandResult.toString(), maxRss).send(); + analytics.send(Event.flutterCommandResult( + commandPath: commandPath, + result: commandResult.toString(), + maxRss: maxRss, + )); // Send timing. final List labels = [ diff --git a/packages/flutter_tools/test/general.shard/runner/flutter_command_test.dart b/packages/flutter_tools/test/general.shard/runner/flutter_command_test.dart index 7a18c3170d..10d2b4a34a 100644 --- a/packages/flutter_tools/test/general.shard/runner/flutter_command_test.dart +++ b/packages/flutter_tools/test/general.shard/runner/flutter_command_test.dart @@ -27,10 +27,12 @@ import 'package:flutter_tools/src/project.dart'; import 'package:flutter_tools/src/reporting/reporting.dart'; import 'package:flutter_tools/src/runner/flutter_command.dart'; import 'package:test/fake.dart'; +import 'package:unified_analytics/unified_analytics.dart'; import '../../src/common.dart'; import '../../src/context.dart'; import '../../src/fake_devices.dart'; +import '../../src/fakes.dart'; import '../../src/test_flutter_command_runner.dart'; import 'utils.dart'; @@ -38,6 +40,7 @@ void main() { group('Flutter Command', () { late FakeCache cache; late TestUsage usage; + late FakeAnalytics fakeAnalytics; late FakeClock clock; late FakeProcessInfo processInfo; late MemoryFileSystem fileSystem; @@ -64,6 +67,10 @@ void main() { logger = BufferLogger.test(); processManager = FakeProcessManager.empty(); preRunValidator = PreRunValidator(fileSystem: fileSystem); + fakeAnalytics = getInitializedFakeAnalyticsInstance( + fs: fileSystem, + fakeFlutterVersion: FakeFlutterVersion(), + ); }); tearDown(() { @@ -194,6 +201,7 @@ void main() { ProcessManager: () => processManager, SystemClock: () => clock, Usage: () => usage, + Analytics: () => fakeAnalytics, }); } @@ -221,6 +229,13 @@ void main() { value: 10, ), ]); + expect(fakeAnalytics.sentEvents, [ + Event.flutterCommandResult( + commandPath: 'dummy', + result: 'success', + maxRss: 10 + ), + ]); }); testUsingCommandContext('reports command that results in warning', () async { @@ -247,6 +262,13 @@ void main() { value: 10, ), ]); + expect(fakeAnalytics.sentEvents, [ + Event.flutterCommandResult( + commandPath: 'dummy', + result: 'warning', + maxRss: 10 + ), + ]); }); testUsingCommandContext('reports command that results in error', () async { @@ -275,6 +297,13 @@ void main() { value: 10, ), ]); + expect(fakeAnalytics.sentEvents, [ + Event.flutterCommandResult( + commandPath: 'dummy', + result: 'fail', + maxRss: 10 + ), + ]); }); test('FlutterCommandResult.success()', () async { @@ -381,6 +410,13 @@ void main() { value: 10, ), ]); + expect(fakeAnalytics.sentEvents, [ + Event.flutterCommandResult( + commandPath: 'dummy', + result: 'killed', + maxRss: 10 + ), + ]); }, overrides: { FileSystem: () => fileSystem, ProcessManager: () => processManager, @@ -391,6 +427,7 @@ void main() { ), SystemClock: () => clock, Usage: () => usage, + Analytics: () => fakeAnalytics, }); testUsingContext('command release lock on kill signal', () async {