From 8323b217373091d258e124ea80e50b4ad52c919d Mon Sep 17 00:00:00 2001 From: Elias Yishak <42216813+eliasyishak@users.noreply.github.com> Date: Tue, 13 Feb 2024 15:45:24 -0500 Subject: [PATCH] New event added for sending analytics within package on errors (#229) * Added new event + refactoring sentEvent on impl * Fix tests + limiting one error event for logFileStats * Make `Analytics` required for `LogHandler` * Make error sent a field in class * Events added for error handling in session handler * Remove unnecessary `io` import * Refactoring `legacyOptOut` to use loop * Only expose `sentEvents` on the `FakeAnalytics` instance * Bump version * Misc * Convert to wip * Pass send method instead of `Analytics` + nits * `ErrorHandler` class created + used in session * Use `ErrorHandler` with `LogHandler` * Check telemetry status in `Session` * Tests added for the survey handler * Fix error * Tests added for log handler exceptions * Use set for sent error messages * Test added to check for 2 unique error events --- pkgs/unified_analytics/CHANGELOG.md | 5 + pkgs/unified_analytics/lib/src/analytics.dart | 37 +- pkgs/unified_analytics/lib/src/constants.dart | 2 +- pkgs/unified_analytics/lib/src/enums.dart | 4 + .../lib/src/error_handler.dart | 32 ++ pkgs/unified_analytics/lib/src/event.dart | 26 +- .../lib/src/log_handler.dart | 201 +++++----- pkgs/unified_analytics/lib/src/session.dart | 33 +- pkgs/unified_analytics/lib/src/utils.dart | 162 ++++----- pkgs/unified_analytics/pubspec.yaml | 2 +- .../test/error_handler_test.dart | 343 ++++++++++++++++++ pkgs/unified_analytics/test/event_test.dart | 19 +- .../test/log_handler_test.dart | 42 ++- .../test/no_op_analytics_test.dart | 10 +- .../test/unified_analytics_test.dart | 47 ++- 15 files changed, 734 insertions(+), 231 deletions(-) create mode 100644 pkgs/unified_analytics/lib/src/error_handler.dart create mode 100644 pkgs/unified_analytics/test/error_handler_test.dart diff --git a/pkgs/unified_analytics/CHANGELOG.md b/pkgs/unified_analytics/CHANGELOG.md index 25ed04f8c..6e6e6b9ee 100644 --- a/pkgs/unified_analytics/CHANGELOG.md +++ b/pkgs/unified_analytics/CHANGELOG.md @@ -1,3 +1,8 @@ +## 5.8.2-wip + +- Added new event `Event.analyticsException` to track internal errors for this package +- Redirecting the `Analytics.test` factory to return an instance of `FakeAnalytics` + ## 5.8.1 - Refactor logic for `okToSend` and `shouldShowMessage` diff --git a/pkgs/unified_analytics/lib/src/analytics.dart b/pkgs/unified_analytics/lib/src/analytics.dart index 90fefb5ef..a0d32280c 100644 --- a/pkgs/unified_analytics/lib/src/analytics.dart +++ b/pkgs/unified_analytics/lib/src/analytics.dart @@ -16,6 +16,7 @@ import 'asserts.dart'; import 'config_handler.dart'; import 'constants.dart'; import 'enums.dart'; +import 'error_handler.dart'; import 'event.dart'; import 'ga_client.dart'; import 'initializer.dart'; @@ -25,6 +26,9 @@ import 'survey_handler.dart'; import 'user_property.dart'; import 'utils.dart'; +/// For passing the [Analytics.send] method to classes created by [Analytics]. +typedef SendFunction = void Function(Event event); + abstract class Analytics { /// The default factory constructor that will return an implementation /// of the [Analytics] abstract class using the [LocalFileSystem]. @@ -60,7 +64,7 @@ abstract class Analytics { final homeDirectory = getHomeDirectory(fs); if (homeDirectory == null || !checkDirectoryForWritePermissions(homeDirectory)) { - return NoOpAnalytics(); + return const NoOpAnalytics(); } // Resolve the OS using dart:io @@ -187,7 +191,7 @@ abstract class Analytics { int toolsMessageVersion = kToolsMessageVersion, String toolsMessage = kToolsMessage, }) => - AnalyticsImpl( + FakeAnalytics( tool: tool, homeDirectory: homeDirectory, flutterChannel: flutterChannel, @@ -203,7 +207,6 @@ abstract class Analytics { initializedSurveys: [], ), gaClient: gaClient ?? const FakeGAClient(), - enableAsserts: true, clientIde: clientIde, enabledFeatures: enabledFeatures, ); @@ -325,6 +328,7 @@ class AnalyticsImpl implements Analytics { late final UserProperty userProperty; late final LogHandler _logHandler; late final Session _sessionHandler; + late final ErrorHandler _errorHandler; final int toolsMessageVersion; /// Tells the client if they need to show a message to the @@ -414,11 +418,20 @@ class AnalyticsImpl implements Analytics { p.join(homeDirectory.path, kDartToolDirectoryName, kClientIdFileName)); _clientId = _clientIdFile.readAsStringSync(); + // Initialization for the error handling class that will prevent duplicate + // [Event.analyticsException] events from being sent to GA4 + _errorHandler = ErrorHandler(sendFunction: send); + // Initialize the user property class that will be attached to // each event that is sent to Google Analytics -- it will be responsible // for getting the session id or rolling the session if the duration // exceeds [kSessionDurationMinutes] - _sessionHandler = Session(homeDirectory: homeDirectory, fs: fs); + _sessionHandler = Session( + homeDirectory: homeDirectory, + fs: fs, + errorHandler: _errorHandler, + telemetryEnabled: telemetryEnabled, + ); userProperty = UserProperty( session: _sessionHandler, flutterChannel: flutterChannel, @@ -436,7 +449,11 @@ class AnalyticsImpl implements Analytics { ); // Initialize the log handler to persist events that are being sent - _logHandler = LogHandler(fs: fs, homeDirectory: homeDirectory); + _logHandler = LogHandler( + fs: fs, + homeDirectory: homeDirectory, + errorHandler: _errorHandler, + ); } @override @@ -712,10 +729,12 @@ class FakeAnalytics extends AnalyticsImpl { super.flutterVersion, super.clientIde, super.enabledFeatures, + int? toolsMessageVersion, + GAClient? gaClient, }) : super( - gaClient: const FakeGAClient(), + gaClient: gaClient ?? const FakeGAClient(), enableAsserts: true, - toolsMessageVersion: kToolsMessageVersion, + toolsMessageVersion: toolsMessageVersion ?? kToolsMessageVersion, ); @override @@ -767,9 +786,7 @@ class NoOpAnalytics implements Analytics { final Map> userPropertyMap = const >{}; - factory NoOpAnalytics() => const NoOpAnalytics._(); - - const NoOpAnalytics._(); + const NoOpAnalytics(); @override String get clientId => staticClientId; diff --git a/pkgs/unified_analytics/lib/src/constants.dart b/pkgs/unified_analytics/lib/src/constants.dart index 48606f032..77c8892d6 100644 --- a/pkgs/unified_analytics/lib/src/constants.dart +++ b/pkgs/unified_analytics/lib/src/constants.dart @@ -82,7 +82,7 @@ const int kLogFileLength = 2500; const String kLogFileName = 'dart-flutter-telemetry.log'; /// The current version of the package, should be in line with pubspec version. -const String kPackageVersion = '5.8.1'; +const String kPackageVersion = '5.8.2-wip'; /// The minimum length for a session. const int kSessionDurationMinutes = 30; diff --git a/pkgs/unified_analytics/lib/src/enums.dart b/pkgs/unified_analytics/lib/src/enums.dart index 90acc3a12..1bceab116 100644 --- a/pkgs/unified_analytics/lib/src/enums.dart +++ b/pkgs/unified_analytics/lib/src/enums.dart @@ -22,6 +22,10 @@ enum DashEvent { label: 'analytics_collection_enabled', description: 'The opt-in status for analytics collection', ), + analyticsException( + label: 'analytics_exception', + description: 'Errors that are encountered within package:unified_analytics', + ), exception( label: 'exception', description: 'General errors to log', diff --git a/pkgs/unified_analytics/lib/src/error_handler.dart b/pkgs/unified_analytics/lib/src/error_handler.dart new file mode 100644 index 000000000..5937db93a --- /dev/null +++ b/pkgs/unified_analytics/lib/src/error_handler.dart @@ -0,0 +1,32 @@ +// Copyright (c) 2023, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +import 'analytics.dart'; +import 'enums.dart'; +import 'event.dart'; + +class ErrorHandler { + /// Stores each of the events that have been sent to GA4 so that the + /// same error doesn't get sent twice. + final Set _sentErrorEvents = {}; + final SendFunction _sendFunction; + + /// Handles any errors encountered within package:unified_analytics. + ErrorHandler({required SendFunction sendFunction}) + : _sendFunction = sendFunction; + + /// Sends the encountered error [Event.analyticsException] to GA4 backend. + /// + /// This method will not send the event to GA4 if it has already been + /// sent before during the current process. + void log(Event event) { + if (event.eventName != DashEvent.analyticsException || + _sentErrorEvents.contains(event)) { + return; + } + + _sendFunction(event); + _sentErrorEvents.add(event); + } +} diff --git a/pkgs/unified_analytics/lib/src/event.dart b/pkgs/unified_analytics/lib/src/event.dart index de288ce74..5ad7d13ce 100644 --- a/pkgs/unified_analytics/lib/src/event.dart +++ b/pkgs/unified_analytics/lib/src/event.dart @@ -19,6 +19,30 @@ final class Event { : eventName = DashEvent.analyticsCollectionEnabled, eventData = {'status': status}; + /// Event that is emitted when an error occurs within + /// `package:unified_analytics`, tools that are using this package + /// should not use this event constructor. + /// + /// Tools using this package should instead use the more generic + /// [Event.exception] constructor. + /// + /// [workflow] - refers to what process caused the error, such as + /// "LogHandler.logFileStats". + /// + /// [error] - the name of the error, such as "FormatException". + /// + /// [description] - the description of the error being caught. + Event.analyticsException({ + required String workflow, + required String error, + String? description, + }) : eventName = DashEvent.analyticsException, + eventData = { + 'workflow': workflow, + 'error': error, + if (description != null) 'description': description, + }; + /// This is for various workflows within the flutter tool related /// to iOS and macOS workflows. /// @@ -685,7 +709,7 @@ final class Event { }; @override - int get hashCode => eventData.hashCode; + int get hashCode => Object.hash(eventName, jsonEncode(eventData)); @override bool operator ==(Object other) => diff --git a/pkgs/unified_analytics/lib/src/log_handler.dart b/pkgs/unified_analytics/lib/src/log_handler.dart index 4f5dbf3be..c406cce94 100644 --- a/pkgs/unified_analytics/lib/src/log_handler.dart +++ b/pkgs/unified_analytics/lib/src/log_handler.dart @@ -9,6 +9,8 @@ import 'package:file/file.dart'; import 'package:path/path.dart' as p; import 'constants.dart'; +import 'error_handler.dart'; +import 'event.dart'; import 'initializer.dart'; /// Data class that will be returned when analyzing the @@ -81,22 +83,6 @@ class LogFileStats { required this.eventCount, }); - @override - String toString() { - final encoder = const JsonEncoder.withIndent(' '); - return encoder.convert({ - 'startDateTime': startDateTime.toString(), - 'minsFromStartDateTime': minsFromStartDateTime, - 'endDateTime': endDateTime.toString(), - 'minsFromEndDateTime': minsFromEndDateTime, - 'sessionCount': sessionCount, - 'recordCount': recordCount, - 'eventCount': eventCount, - 'toolCount': toolCount, - 'flutterChannelCount': flutterChannelCount, - }); - } - /// Pass in a string label for one of the instance variables /// and return the integer value of that label. /// @@ -149,6 +135,22 @@ class LogFileStats { return null; } + + @override + String toString() { + final encoder = const JsonEncoder.withIndent(' '); + return encoder.convert({ + 'startDateTime': startDateTime.toString(), + 'minsFromStartDateTime': minsFromStartDateTime, + 'endDateTime': endDateTime.toString(), + 'minsFromEndDateTime': minsFromEndDateTime, + 'sessionCount': sessionCount, + 'recordCount': recordCount, + 'eventCount': eventCount, + 'toolCount': toolCount, + 'flutterChannelCount': flutterChannelCount, + }); + } } /// This class is responsible for writing to a log @@ -160,17 +162,20 @@ class LogHandler { final FileSystem fs; final Directory homeDirectory; final File logFile; + final ErrorHandler _errorHandler; /// A log handler constructor that will delegate saving /// logs and retrieving stats from the persisted log. LogHandler({ required this.fs, required this.homeDirectory, - }) : logFile = fs.file(p.join( + required ErrorHandler errorHandler, + }) : logFile = fs.file(p.join( homeDirectory.path, kDartToolDirectoryName, kLogFileName, - )); + )), + _errorHandler = errorHandler; /// Get stats from the persisted log file. /// @@ -184,15 +189,23 @@ class LogHandler { final records = logFile .readAsLinesSync() .map((String e) { - // TODO: eliasyishak, once https://github.com/dart-lang/tools/issues/167 - // has landed ensure we are sending an event for each error - // with helpful messages try { return LogItem.fromRecord(jsonDecode(e) as Map); - } on FormatException { + } on FormatException catch (err) { + _errorHandler.log(Event.analyticsException( + workflow: 'LogFileStats.logFileStats', + error: err.runtimeType.toString(), + description: 'message: ${err.message}\nsource: ${err.source}', + )); + return null; // ignore: avoid_catching_errors - } on TypeError { + } on TypeError catch (err) { + _errorHandler.log(Event.analyticsException( + workflow: 'LogFileStats.logFileStats', + error: err.runtimeType.toString(), + )); + return null; } }) @@ -377,82 +390,72 @@ class LogItem { return null; } - // Using a try/except here to parse out the fields if possible, - // if not, it will quietly return null and won't get processed - // downstream - try { - // Parse out values from the top level key = 'events' and return - // a map for the one event in the value - final eventProp = - (record['events']! as List).first as Map; - final eventName = eventProp['name'] as String; - - // Parse the data out of the `user_properties` value - final userProps = record['user_properties'] as Map; - - // Parse out the values from the top level key = 'user_properties` - final sessionId = - (userProps['session_id']! as Map)['value'] as int?; - final flutterChannel = (userProps['flutter_channel']! - as Map)['value'] as String?; - final host = - (userProps['host']! as Map)['value'] as String?; - final flutterVersion = (userProps['flutter_version']! - as Map)['value'] as String?; - final dartVersion = (userProps['dart_version']! - as Map)['value'] as String?; - final tool = - (userProps['tool']! as Map)['value'] as String?; - final localTimeString = (userProps['local_time']! - as Map)['value'] as String?; - final hostOsVersion = (userProps['host_os_version']! - as Map)['value'] as String?; - final locale = - (userProps['locale']! as Map)['value'] as String?; - final clientIde = (userProps['client_ide']! - as Map)['value'] as String?; - - // If any of the above values are null, return null since that - // indicates the record is malformed; note that `flutter_version`, - // `flutter_channel`, and `client_ide` are nullable fields in the log file - final values = [ - // Values associated with the top level key = 'events' - eventName, - - // Values associated with the top level key = 'events' - sessionId, - host, - dartVersion, - tool, - localTimeString, - hostOsVersion, - locale, - ]; - for (var value in values) { - if (value == null) return null; - } - - // Parse the local time from the string extracted - final localTime = DateTime.parse(localTimeString!).toLocal(); - - return LogItem( - eventName: eventName, - sessionId: sessionId!, - flutterChannel: flutterChannel, - host: host!, - flutterVersion: flutterVersion, - dartVersion: dartVersion!, - tool: tool!, - localTime: localTime, - hostOsVersion: hostOsVersion!, - locale: locale!, - clientIde: clientIde, - ); - // ignore: avoid_catching_errors - } on TypeError { - return null; - } on FormatException { - return null; + // Parse out values from the top level key = 'events' and return + // a map for the one event in the value + final eventProp = + (record['events']! as List).first as Map; + final eventName = eventProp['name'] as String; + + // Parse the data out of the `user_properties` value + final userProps = record['user_properties'] as Map; + + // Parse out the values from the top level key = 'user_properties` + final sessionId = + (userProps['session_id']! as Map)['value'] as int?; + final flutterChannel = (userProps['flutter_channel']! + as Map)['value'] as String?; + final host = + (userProps['host']! as Map)['value'] as String?; + final flutterVersion = (userProps['flutter_version']! + as Map)['value'] as String?; + final dartVersion = (userProps['dart_version']! + as Map)['value'] as String?; + final tool = + (userProps['tool']! as Map)['value'] as String?; + final localTimeString = + (userProps['local_time']! as Map)['value'] as String?; + final hostOsVersion = (userProps['host_os_version']! + as Map)['value'] as String?; + final locale = + (userProps['locale']! as Map)['value'] as String?; + final clientIde = + (userProps['client_ide']! as Map)['value'] as String?; + + // If any of the above values are null, return null since that + // indicates the record is malformed; note that `flutter_version`, + // `flutter_channel`, and `client_ide` are nullable fields in the log file + final values = [ + // Values associated with the top level key = 'events' + eventName, + + // Values associated with the top level key = 'events' + sessionId, + host, + dartVersion, + tool, + localTimeString, + hostOsVersion, + locale, + ]; + for (var value in values) { + if (value == null) return null; } + + // Parse the local time from the string extracted + final localTime = DateTime.parse(localTimeString!).toLocal(); + + return LogItem( + eventName: eventName, + sessionId: sessionId!, + flutterChannel: flutterChannel, + host: host!, + flutterVersion: flutterVersion, + dartVersion: dartVersion!, + tool: tool!, + localTime: localTime, + hostOsVersion: hostOsVersion!, + locale: locale!, + clientIde: clientIde, + ); } } diff --git a/pkgs/unified_analytics/lib/src/session.dart b/pkgs/unified_analytics/lib/src/session.dart index f5dc66808..77640d68a 100644 --- a/pkgs/unified_analytics/lib/src/session.dart +++ b/pkgs/unified_analytics/lib/src/session.dart @@ -3,19 +3,21 @@ // BSD-style license that can be found in the LICENSE file. import 'dart:convert'; -import 'dart:io'; import 'package:clock/clock.dart'; import 'package:file/file.dart'; import 'package:path/path.dart' as p; import 'constants.dart'; +import 'error_handler.dart'; +import 'event.dart'; import 'initializer.dart'; class Session { final Directory homeDirectory; final FileSystem fs; final File sessionFile; + final ErrorHandler _errorHandler; late int _sessionId; late int _lastPing; @@ -23,9 +25,16 @@ class Session { Session({ required this.homeDirectory, required this.fs, - }) : sessionFile = fs.file(p.join( - homeDirectory.path, kDartToolDirectoryName, kSessionFileName)) { - _refreshSessionData(); + required ErrorHandler errorHandler, + required bool telemetryEnabled, + }) : sessionFile = fs.file(p.join( + homeDirectory.path, kDartToolDirectoryName, kSessionFileName)), + _errorHandler = errorHandler { + // We must check if telemetry is enabled to refresh the session data + // because the refresh method will write to the session file and for + // users that have opted out, we have to leave the session file empty + // per the privacy document + if (telemetryEnabled) _refreshSessionData(); } /// This will use the data parsed from the @@ -87,13 +96,25 @@ class Session { try { parseContents(); - } on FormatException { + } on FormatException catch (err) { Initializer.createSessionFile(sessionFile: sessionFile); + _errorHandler.log(Event.analyticsException( + workflow: 'Session._refreshSessionData', + error: err.runtimeType.toString(), + description: 'message: ${err.message}\nsource: ${err.source}', + )); + parseContents(); - } on PathNotFoundException { + } on FileSystemException catch (err) { Initializer.createSessionFile(sessionFile: sessionFile); + _errorHandler.log(Event.analyticsException( + workflow: 'Session._refreshSessionData', + error: err.runtimeType.toString(), + description: err.osError?.toString(), + )); + parseContents(); } } diff --git a/pkgs/unified_analytics/lib/src/utils.dart b/pkgs/unified_analytics/lib/src/utils.dart index 455eb1e1f..3044a50ad 100644 --- a/pkgs/unified_analytics/lib/src/utils.dart +++ b/pkgs/unified_analytics/lib/src/utils.dart @@ -129,112 +129,86 @@ Directory? getHomeDirectory(FileSystem fs) { return fs.directory(home); } -/// Returns `true` if user has opted out of legacy analytics in Dart or Flutter. +/// Returns `true` if user has opted out of legacy analytics in +/// Dart or Flutter. /// /// Checks legacy opt-out status for the Flutter /// and Dart in the following locations. /// /// Dart: `$HOME/.dart/dartdev.json` +/// ``` +/// { +/// "firstRun": false, +/// "enabled": false, <-- THIS USER HAS OPTED OUT +/// "disclosureShown": true, +/// "clientId": "52710e60-7c70-4335-b3a4-9d922630f12a" +/// } +/// ``` /// /// Flutter: `$HOME/.flutter` +/// ``` +/// { +/// "firstRun": false, +/// "clientId": "4c3a3d1e-e545-47e7-b4f8-10129f6ab169", +/// "enabled": false <-- THIS USER HAS OPTED OUT +/// } +/// ``` /// /// Devtools: `$HOME/.flutter-devtools/.devtools` -bool legacyOptOut({ - required FileSystem fs, - required Directory home, -}) { - final dartLegacyConfigFile = - fs.file(p.join(home.path, '.dart', 'dartdev.json')); - final flutterLegacyConfigFile = fs.file(p.join(home.path, '.flutter')); - final devtoolsLegacyConfigFile = - fs.file(p.join(home.path, '.flutter-devtools', '.devtools')); - - // Example of what the file looks like for dart - // - // { - // "firstRun": false, - // "enabled": false, <-- THIS USER HAS OPTED OUT - // "disclosureShown": true, - // "clientId": "52710e60-7c70-4335-b3a4-9d922630f12a" - // } - if (dartLegacyConfigFile.existsSync()) { - try { - // Read in the json object into a Map and check for - // the enabled key being set to false; this means the user - // has opted out of analytics for dart - final dartObj = jsonDecode(dartLegacyConfigFile.readAsStringSync()) - as Map; - if (dartObj.containsKey('enabled') && dartObj['enabled'] == false) { - return true; - } - } on FormatException { - // In the case of an error when parsing the json file, return true - // which will result in the user being opted out of unified_analytics - // - // A corrupted file could mean they opted out previously but for some - // reason, the file was written incorrectly - return true; - } on FileSystemException { - return true; - } - } - - // Example of what the file looks like for flutter - // - // { - // "firstRun": false, - // "clientId": "4c3a3d1e-e545-47e7-b4f8-10129f6ab169", - // "enabled": false <-- THIS USER HAS OPTED OUT - // } - if (flutterLegacyConfigFile.existsSync()) { - try { - // Same process as above for dart - final flutterObj = jsonDecode(dartLegacyConfigFile.readAsStringSync()) - as Map; - if (flutterObj.containsKey('enabled') && flutterObj['enabled'] == false) { +/// ``` +/// { +/// "analyticsEnabled": false, <-- THIS USER HAS OPTED OUT +/// "isFirstRun": false, +/// "lastReleaseNotesVersion": "2.31.0", +/// "2023-Q4": { +/// "surveyActionTaken": false, +/// "surveyShownCount": 0 +/// } +/// } +/// ``` +bool legacyOptOut({required FileSystem fs, required Directory home}) { + // List of Maps for each of the config file, `key` refers to the + // key in the json file that indicates if the user has been opted + // out or not + final legacyConfigFiles = [ + { + 'tool': DashTool.dartTool, + 'file': fs.file(p.join(home.path, '.dart', 'dartdev.json')), + 'key': 'enabled', + }, + { + 'tool': DashTool.flutterTool, + 'file': fs.file(p.join(home.path, '.flutter')), + 'key': 'enabled', + }, + { + 'tool': DashTool.devtools, + 'file': fs.file(p.join(home.path, '.flutter-devtools', '.devtools')), + 'key': 'analyticsEnabled', + }, + ]; + for (final legacyConfigObj in legacyConfigFiles) { + final legacyFile = legacyConfigObj['file']! as File; + final lookupKey = legacyConfigObj['key']! as String; + + if (legacyFile.existsSync()) { + try { + final legacyFileObj = + jsonDecode(legacyFile.readAsStringSync()) as Map; + if (legacyFileObj.containsKey(lookupKey) && + legacyFileObj[lookupKey] == false) { + return true; + } + } on FormatException { + // In the case of an error when parsing the json file, return true + // which will result in the user being opted out of unified_analytics + // + // A corrupted file could mean they opted out previously but for some + // reason, the file was written incorrectly return true; - } - } on FormatException { - // In the case of an error when parsing the json file, return true - // which will result in the user being opted out of unified_analytics - // - // A corrupted file could mean they opted out previously but for some - // reason, the file was written incorrectly - return true; - } on FileSystemException { - return true; - } - } - - // Example of what the file looks like for devtools - // - // { - // "analyticsEnabled": false, <-- THIS USER HAS OPTED OUT - // "isFirstRun": false, - // "lastReleaseNotesVersion": "2.31.0", - // "2023-Q4": { - // "surveyActionTaken": false, - // "surveyShownCount": 0 - // } - // } - if (devtoolsLegacyConfigFile.existsSync()) { - try { - final devtoolsObj = - jsonDecode(devtoolsLegacyConfigFile.readAsStringSync()) - as Map; - if (devtoolsObj.containsKey('analyticsEnabled') && - devtoolsObj['analyticsEnabled'] == false) { + } on FileSystemException { return true; } - } on FormatException { - // In the case of an error when parsing the json file, return true - // which will result in the user being opted out of unified_analytics - // - // A corrupted file could mean they opted out previously but for some - // reason, the file was written incorrectly - return true; - } on FileSystemException { - return true; } } diff --git a/pkgs/unified_analytics/pubspec.yaml b/pkgs/unified_analytics/pubspec.yaml index ef8808f6d..b090399e4 100644 --- a/pkgs/unified_analytics/pubspec.yaml +++ b/pkgs/unified_analytics/pubspec.yaml @@ -4,7 +4,7 @@ description: >- to Google Analytics. # When updating this, keep the version consistent with the changelog and the # value in lib/src/constants.dart. -version: 5.8.1 +version: 5.8.2-wip repository: https://github.com/dart-lang/tools/tree/main/pkgs/unified_analytics environment: diff --git a/pkgs/unified_analytics/test/error_handler_test.dart b/pkgs/unified_analytics/test/error_handler_test.dart new file mode 100644 index 000000000..c7156e2aa --- /dev/null +++ b/pkgs/unified_analytics/test/error_handler_test.dart @@ -0,0 +1,343 @@ +// Copyright (c) 2023, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +import 'dart:io' as io; + +import 'package:file/file.dart'; +import 'package:file/memory.dart'; +import 'package:test/test.dart'; + +import 'package:unified_analytics/src/constants.dart'; +import 'package:unified_analytics/src/enums.dart'; +import 'package:unified_analytics/unified_analytics.dart'; + +void main() { + late FileSystem fs; + late Directory home; + late FakeAnalytics initializationAnalytics; + late FakeAnalytics analytics; + late File sessionFile; + late File logFile; + + const homeDirName = 'home'; + const initialTool = DashTool.flutterTool; + const measurementId = 'measurementId'; + const apiSecret = 'apiSecret'; + const toolsMessageVersion = 1; + const toolsMessage = 'toolsMessage'; + const flutterChannel = 'flutterChannel'; + const flutterVersion = 'flutterVersion'; + const dartVersion = 'dartVersion'; + const platform = DevicePlatform.macos; + const clientIde = 'VSCode'; + + final testEvent = Event.codeSizeAnalysis(platform: 'platform'); + + setUp(() { + // Setup the filesystem with the home directory + final fsStyle = + io.Platform.isWindows ? FileSystemStyle.windows : FileSystemStyle.posix; + fs = MemoryFileSystem.test(style: fsStyle); + home = fs.directory(homeDirName); + + // This is the first analytics instance that will be used to demonstrate + // that events will not be sent with the first run of analytics + initializationAnalytics = Analytics.test( + tool: initialTool, + homeDirectory: home, + measurementId: measurementId, + apiSecret: apiSecret, + flutterChannel: flutterChannel, + toolsMessageVersion: toolsMessageVersion, + toolsMessage: toolsMessage, + flutterVersion: flutterVersion, + dartVersion: dartVersion, + fs: fs, + platform: platform, + ) as FakeAnalytics; + expect(initializationAnalytics.shouldShowMessage, true); + initializationAnalytics.clientShowedMessage(); + expect(initializationAnalytics.shouldShowMessage, false); + + // The main analytics instance, other instances can be spawned within tests + // to test how to instances running together work + // + // This instance should have the same parameters as the one above for + // [initializationAnalytics] + analytics = Analytics.test( + tool: initialTool, + homeDirectory: home, + measurementId: measurementId, + apiSecret: apiSecret, + flutterChannel: flutterChannel, + toolsMessageVersion: toolsMessageVersion, + toolsMessage: toolsMessage, + flutterVersion: flutterVersion, + dartVersion: dartVersion, + fs: fs, + platform: platform, + clientIde: clientIde, + ) as FakeAnalytics; + analytics.clientShowedMessage(); + + // The files that should have been generated that will be used for tests + sessionFile = + home.childDirectory(kDartToolDirectoryName).childFile(kSessionFileName); + logFile = + home.childDirectory(kDartToolDirectoryName).childFile(kLogFileName); + }); + + group('Session handler:', () { + test('no error when opted out already and opting in', () async { + // When we opt out from an analytics instance, we clear the contents of + // session file, as required by the privacy document. When creating a + // second instance of [Analytics], it should not detect that the file is + // empty and recreate it, it should remain opted out and no error event + // should have been sent + await analytics.setTelemetry(false); + expect(analytics.telemetryEnabled, false); + expect(sessionFile.readAsStringSync(), isEmpty); + + final secondAnalytics = Analytics.test( + tool: initialTool, + homeDirectory: home, + measurementId: measurementId, + apiSecret: apiSecret, + flutterChannel: flutterChannel, + toolsMessageVersion: toolsMessageVersion, + toolsMessage: toolsMessage, + flutterVersion: flutterVersion, + dartVersion: dartVersion, + fs: fs, + platform: platform, + clientIde: clientIde, + ) as FakeAnalytics; + expect(sessionFile.readAsStringSync(), isEmpty); + expect(secondAnalytics.telemetryEnabled, false); + + expect( + analytics.sentEvents.where( + (element) => element.eventName == DashEvent.analyticsException), + isEmpty, + ); + expect( + secondAnalytics.sentEvents.where( + (element) => element.eventName == DashEvent.analyticsException), + isEmpty, + ); + + await secondAnalytics.setTelemetry(true); + expect(sessionFile.readAsStringSync(), isNotEmpty, + reason: 'Toggling telemetry should bring back the session data'); + }); + test('only sends one event for FormatException', () { + // Begin with the session file empty, it should recreate the file + // and send an error event + sessionFile.writeAsStringSync(''); + expect(sessionFile.readAsStringSync(), isEmpty); + analytics.send(testEvent); + expect(sessionFile.readAsStringSync(), isNotEmpty); + + expect( + analytics.sentEvents.where( + (element) => element.eventName == DashEvent.analyticsException), + hasLength(1)); + + // Making the file empty again and sending an event should not send + // an additional event + sessionFile.writeAsStringSync(''); + expect(sessionFile.readAsStringSync(), isEmpty); + analytics.send(testEvent); + expect(sessionFile.readAsStringSync(), isNotEmpty); + + expect( + analytics.sentEvents.where( + (element) => element.eventName == DashEvent.analyticsException), + hasLength(1), + reason: 'We should not have added a new error event'); + }); + + test('only sends one event for FileSystemException', () { + // Deleting the session file should cause the file system exception and + // sending a new event should log the error the first time and recreate + // the file. If we delete the file again and attempt to send an event, + // the session file should get recreated without sending a second error. + sessionFile.deleteSync(); + expect(sessionFile.existsSync(), isFalse); + analytics.send(testEvent); + + expect( + analytics.sentEvents.where( + (element) => element.eventName == DashEvent.analyticsException), + hasLength(1), + ); + expect(sessionFile.existsSync(), isTrue); + expect(sessionFile.readAsStringSync(), isNotEmpty); + + // Remove the file again and send an event + sessionFile.deleteSync(); + expect(sessionFile.existsSync(), isFalse); + analytics.send(testEvent); + + expect( + analytics.sentEvents.where( + (element) => element.eventName == DashEvent.analyticsException), + hasLength(1), + reason: 'Only the first error event should exist', + ); + expect(sessionFile.existsSync(), isTrue); + expect(sessionFile.readAsStringSync(), isNotEmpty); + }); + + test('sends two unique errors', () { + // Begin with the session file empty, it should recreate the file + // and send an error event + sessionFile.writeAsStringSync(''); + expect(sessionFile.readAsStringSync(), isEmpty); + analytics.send(testEvent); + expect(sessionFile.readAsStringSync(), isNotEmpty); + + expect( + analytics.sentEvents.where( + (element) => element.eventName == DashEvent.analyticsException), + hasLength(1)); + + // Deleting the file now before sending an additional event should + // cause a different test error + sessionFile.deleteSync(); + expect(sessionFile.existsSync(), isFalse); + + analytics.send(testEvent); + expect(sessionFile.readAsStringSync(), isNotEmpty); + expect( + analytics.sentEvents.where( + (element) => element.eventName == DashEvent.analyticsException), + hasLength(2)); + expect(analytics.sentEvents, hasLength(4)); + + sessionFile.deleteSync(); + expect(sessionFile.existsSync(), isFalse); + + analytics.send(testEvent); + expect(sessionFile.readAsStringSync(), isNotEmpty); + expect( + analytics.sentEvents.where( + (element) => element.eventName == DashEvent.analyticsException), + hasLength(2)); + }); + }); + + group('Log handler:', () { + test('only sends one event for FormatException', () { + expect(logFile.existsSync(), isTrue); + + // Write invalid lines to the log file to have a FormatException + // thrown when trying to parse the log file + logFile.writeAsStringSync(''' +{{} +{{} +'''); + + // Send one event so that the logFileStats method returns a valid value + analytics.send(testEvent); + expect(analytics.sentEvents, hasLength(1)); + expect(logFile.readAsLinesSync(), hasLength(3)); + expect( + analytics.sentEvents.where( + (element) => element.eventName == DashEvent.analyticsException), + isEmpty, + ); + + // This call below will cause a FormatException while parsing the log file + final logFileStats = analytics.logFileStats(); + expect(logFileStats, isNotNull); + expect(logFileStats!.recordCount, 1, + reason: 'The error event is not counted'); + expect( + analytics.sentEvents.where( + (element) => element.eventName == DashEvent.analyticsException), + hasLength(1), + ); + expect(logFile.readAsLinesSync(), hasLength(4)); + }); + + test('only sends one event for TypeError', () { + expect(logFile.existsSync(), isTrue); + // Write valid json but have one of the types wrong for + // the keys so that we throw a TypeError while casting the values + // + // In the json below, we have made the session id value a string when + // it should be an integer + logFile.writeAsStringSync(''' +{"client_id":"fcd6c0d5-6582-4c36-b09e-3ecedee9145c","events":[{"name":"command_usage_values","params":{"workflow":"doctor","commandHasTerminal":true}}],"user_properties":{"session_id":{"value":"this should be a string"},"flutter_channel":{"value":"master"},"host":{"value":"macOS"},"flutter_version":{"value":"3.20.0-2.0.pre.9"},"dart_version":{"value":"3.4.0 (build 3.4.0-99.0.dev)"},"analytics_pkg_version":{"value":"5.8.1"},"tool":{"value":"flutter-tool"},"local_time":{"value":"2024-02-07 15:46:19.920784 -0500"},"host_os_version":{"value":"Version 14.3 (Build 23D56)"},"locale":{"value":"en"},"client_ide":{"value":null},"enabled_features":{"value":"enable-native-assets"}}} +'''); + expect(logFile.readAsLinesSync(), hasLength(1)); + + // Send the test event so that the LogFileStats object is not null + analytics.send(testEvent); + + final logFileStats = analytics.logFileStats(); + expect( + analytics.sentEvents.where( + (element) => element.eventName == DashEvent.analyticsException), + hasLength(1), + ); + expect(logFileStats, isNotNull); + expect(logFileStats!.recordCount, 1); + }); + + test('sends two unique errors', () { + expect(logFile.existsSync(), isTrue); + + // Write invalid lines to the log file to have a FormatException + // thrown when trying to parse the log file + logFile.writeAsStringSync(''' +{{} +{{} +'''); + + // Send one event so that the logFileStats method returns a valid value + analytics.send(testEvent); + expect(analytics.sentEvents, hasLength(1)); + expect(logFile.readAsLinesSync(), hasLength(3)); + expect( + analytics.sentEvents.where( + (element) => element.eventName == DashEvent.analyticsException), + isEmpty, + ); + + // This will cause the first error + analytics.logFileStats(); + expect( + analytics.sentEvents.where( + (element) => element.eventName == DashEvent.analyticsException), + hasLength(1), + ); + + // Overwrite the contents of the log file now to include something that + // will cause a TypeError by changing the expected value for session id + // from integer to a string + logFile.writeAsStringSync(''' +{"client_id":"fcd6c0d5-6582-4c36-b09e-3ecedee9145c","events":[{"name":"command_usage_values","params":{"workflow":"doctor","commandHasTerminal":true}}],"user_properties":{"session_id":{"value":"this should be a string"},"flutter_channel":{"value":"master"},"host":{"value":"macOS"},"flutter_version":{"value":"3.20.0-2.0.pre.9"},"dart_version":{"value":"3.4.0 (build 3.4.0-99.0.dev)"},"analytics_pkg_version":{"value":"5.8.1"},"tool":{"value":"flutter-tool"},"local_time":{"value":"2024-02-07 15:46:19.920784 -0500"},"host_os_version":{"value":"Version 14.3 (Build 23D56)"},"locale":{"value":"en"},"client_ide":{"value":null},"enabled_features":{"value":"enable-native-assets"}}} +'''); + expect(logFile.readAsLinesSync(), hasLength(1)); + + // This will cause the second error + analytics.logFileStats(); + expect( + analytics.sentEvents.where( + (element) => element.eventName == DashEvent.analyticsException), + hasLength(2), + ); + + // Attempting to cause the same error won't send another error event + analytics.logFileStats(); + expect( + analytics.sentEvents.where( + (element) => element.eventName == DashEvent.analyticsException), + hasLength(2), + ); + }); + }); +} diff --git a/pkgs/unified_analytics/test/event_test.dart b/pkgs/unified_analytics/test/event_test.dart index dd3f37fa1..4bd3464ba 100644 --- a/pkgs/unified_analytics/test/event_test.dart +++ b/pkgs/unified_analytics/test/event_test.dart @@ -536,6 +536,23 @@ void main() { expect(constructedEvent.eventData.length, 27); }); + test('Event.analyticsException constructed', () { + Event generateEvent() => Event.analyticsException( + workflow: 'workflow', + error: 'error', + description: 'description', + ); + + final constructedEvent = generateEvent(); + + expect(generateEvent, returnsNormally); + expect(constructedEvent.eventName, DashEvent.analyticsException); + expect(constructedEvent.eventData['workflow'], 'workflow'); + expect(constructedEvent.eventData['error'], 'error'); + expect(constructedEvent.eventData['description'], 'description'); + expect(constructedEvent.eventData.length, 3); + }); + test('Confirm all constructors were checked', () { var constructorCount = 0; for (var declaration in reflectClass(Event).declarations.keys) { @@ -544,7 +561,7 @@ void main() { // Change this integer below if your PR either adds or removes // an Event constructor - final eventsAccountedForInTests = 25; + final eventsAccountedForInTests = 26; expect(eventsAccountedForInTests, constructorCount, reason: 'If you added or removed an event constructor, ' 'ensure you have updated ' diff --git a/pkgs/unified_analytics/test/log_handler_test.dart b/pkgs/unified_analytics/test/log_handler_test.dart index d7715573a..a1f494a6e 100644 --- a/pkgs/unified_analytics/test/log_handler_test.dart +++ b/pkgs/unified_analytics/test/log_handler_test.dart @@ -13,7 +13,7 @@ import 'package:unified_analytics/src/utils.dart'; import 'package:unified_analytics/unified_analytics.dart'; void main() { - late Analytics analytics; + late FakeAnalytics analytics; late Directory homeDirectory; late FileSystem fs; late File logFile; @@ -51,7 +51,7 @@ void main() { dartVersion: 'dartVersion', fs: fs, platform: DevicePlatform.macos, - ); + ) as FakeAnalytics; }); test('Ensure that log file is created', () { @@ -78,11 +78,20 @@ void main() { // Write invalid json for the only log record logFile.writeAsStringSync('{{\n'); - final logFileStats = analytics.logFileStats(); expect(logFile.readAsLinesSync().length, 1); + final logFileStats = analytics.logFileStats(); expect(logFileStats, isNull, reason: 'Null should be returned since only ' 'one record is in there and it is malformed'); + expect( + analytics.sentEvents, + contains( + Event.analyticsException( + workflow: 'LogFileStats.logFileStats', + error: 'FormatException', + description: 'message: Unexpected character\nsource: {{', + ), + )); }); test('The first record is malformed, but rest are valid', () async { @@ -94,9 +103,9 @@ void main() { for (var i = 0; i < countOfEventsToSend; i++) { analytics.send(testEvent); } + expect(logFile.readAsLinesSync().length, countOfEventsToSend + 1); final logFileStats = analytics.logFileStats(); - expect(logFile.readAsLinesSync().length, countOfEventsToSend + 1); expect(logFileStats, isNotNull); expect(logFileStats!.recordCount, countOfEventsToSend); }); @@ -113,10 +122,15 @@ void main() { for (var i = 0; i < countOfEventsToSend; i++) { analytics.send(testEvent); } - final logFileStats = analytics.logFileStats(); expect(logFile.readAsLinesSync().length, countOfEventsToSend + countOfMalformedRecords); + final logFileStats = analytics.logFileStats(); + expect(logFile.readAsLinesSync().length, + countOfEventsToSend + countOfMalformedRecords + 1, + reason: + 'There should have been on error event sent when getting stats'); + expect(logFileStats, isNotNull); expect(logFileStats!.recordCount, countOfEventsToSend); }); @@ -146,16 +160,23 @@ void main() { // Write invalid json for the only log record logFile.writeAsStringSync('{{\n'); - // Send the max number of events minus one so that we have + // Send the max number of events minus two so that we have // one malformed record on top of the logs and the rest // are valid log records - for (var i = 0; i < kLogFileLength - 1; i++) { + // + // We need to account for the event that is sent when + // calling [logFileStats()] fails and sends an instance + // of [Event.analyticsException] + final recordsToSendInitially = kLogFileLength - 2; + for (var i = 0; i < recordsToSendInitially; i++) { analytics.send(testEvent); } final logFileStats = analytics.logFileStats(); + expect(analytics.sentEvents.last.eventName, DashEvent.analyticsException, + reason: 'Calling for the stats should have caused an error'); expect(logFile.readAsLinesSync().length, kLogFileLength); expect(logFileStats, isNotNull); - expect(logFileStats!.recordCount, kLogFileLength - 1, + expect(logFileStats!.recordCount, recordsToSendInitially, reason: 'The first record should be malformed'); expect(logFile.readAsLinesSync()[0].trim(), '{{'); @@ -163,6 +184,7 @@ void main() { analytics.send(testEvent); final secondLogFileStats = analytics.logFileStats(); + expect(analytics.sentEvents.last, testEvent); expect(secondLogFileStats, isNotNull); expect(secondLogFileStats!.recordCount, kLogFileLength); expect(logFile.readAsLinesSync()[0].trim(), isNot('{{')); @@ -184,7 +206,9 @@ void main() { final secondLogFileStats = analytics.logFileStats(); expect(secondLogFileStats, isNotNull); - expect(secondLogFileStats!.recordCount, countOfEventsToSend); + expect(secondLogFileStats!.recordCount, countOfEventsToSend + 1, + reason: 'Plus one for the error event that is sent ' + 'from the first logFileStats call'); }); test( diff --git a/pkgs/unified_analytics/test/no_op_analytics_test.dart b/pkgs/unified_analytics/test/no_op_analytics_test.dart index 4fb6baa37..66c1ef6c8 100644 --- a/pkgs/unified_analytics/test/no_op_analytics_test.dart +++ b/pkgs/unified_analytics/test/no_op_analytics_test.dart @@ -13,7 +13,7 @@ void main() { final testEvent = Event.hotReloadTime(timeMs: 50); test('NoOpAnalytics.telemetryEnabled is always false', () async { - final analytics = NoOpAnalytics(); + final analytics = const NoOpAnalytics(); expect(analytics.telemetryEnabled, isFalse); await analytics.setTelemetry(true); @@ -21,7 +21,7 @@ void main() { }); test('NoOpAnalytics.shouldShowMessage is always false', () async { - final analytics = NoOpAnalytics(); + final analytics = const NoOpAnalytics(); expect(analytics.shouldShowMessage, isFalse); analytics.clientShowedMessage(); @@ -29,7 +29,7 @@ void main() { }); test('NoOpAnalytics.sendEvent() always returns null', () async { - final analytics = NoOpAnalytics(); + final analytics = const NoOpAnalytics(); await analytics.setTelemetry(true); analytics.clientShowedMessage(); @@ -40,7 +40,7 @@ void main() { }); test('NoOpAnalytics.logFileStats() always returns null', () async { - final analytics = NoOpAnalytics(); + final analytics = const NoOpAnalytics(); expect(analytics.logFileStats(), isNull); @@ -64,7 +64,7 @@ void main() { }); test('Fetching the client id', () { - final analytics = NoOpAnalytics(); + final analytics = const NoOpAnalytics(); expect(analytics.clientId, 'xxxx-xxxx'); }); } diff --git a/pkgs/unified_analytics/test/unified_analytics_test.dart b/pkgs/unified_analytics/test/unified_analytics_test.dart index 22e088596..2b794b671 100644 --- a/pkgs/unified_analytics/test/unified_analytics_test.dart +++ b/pkgs/unified_analytics/test/unified_analytics_test.dart @@ -14,6 +14,7 @@ import 'package:file/memory.dart'; import 'package:test/test.dart'; import 'package:unified_analytics/src/constants.dart'; import 'package:unified_analytics/src/enums.dart'; +import 'package:unified_analytics/src/error_handler.dart'; import 'package:unified_analytics/src/session.dart'; import 'package:unified_analytics/src/user_property.dart'; import 'package:unified_analytics/src/utils.dart'; @@ -25,7 +26,7 @@ void main() { late Directory home; late Directory dartToolDirectory; late Analytics initializationAnalytics; - late Analytics analytics; + late FakeAnalytics analytics; late File clientIdFile; late File sessionFile; late File configFile; @@ -96,10 +97,10 @@ void main() { fs: fs, platform: platform, clientIde: clientIde, - ); + ) as FakeAnalytics; analytics.clientShowedMessage(); - // The 3 files that should have been generated + // The 5 files that should have been generated clientIdFile = home .childDirectory(kDartToolDirectoryName) .childFile(kClientIdFileName); @@ -116,7 +117,12 @@ void main() { // Create the user property object that is also // created within analytics for testing userProperty = UserProperty( - session: Session(homeDirectory: home, fs: fs), + session: Session( + homeDirectory: home, + fs: fs, + errorHandler: ErrorHandler(sendFunction: analytics.send), + telemetryEnabled: analytics.telemetryEnabled, + ), flutterChannel: flutterChannel, host: platform.label, flutterVersion: flutterVersion, @@ -166,6 +172,39 @@ void main() { userProperty.preparePayload(); expect(sessionFile.readAsStringSync(), '{"session_id":$timestamp,"last_ping":$timestamp}'); + + // Attempting to fetch the session id when malformed should also + // send an error event while parsing + final lastEvent = analytics.sentEvents.last; + expect(lastEvent, isNotNull); + expect(lastEvent.eventName, DashEvent.analyticsException); + expect(lastEvent.eventData['workflow']!, 'Session._refreshSessionData'); + expect(lastEvent.eventData['error']!, 'FormatException'); + }); + }); + + test('Resetting session file when file is removed', () { + // Purposefully write delete the file + sessionFile.deleteSync(); + + // Define the initial time to start + final start = DateTime(1995, 3, 3, 12, 0); + + // Set the clock to the start value defined above + withClock(Clock.fixed(start), () { + final timestamp = clock.now().millisecondsSinceEpoch.toString(); + expect(sessionFile.existsSync(), false); + userProperty.preparePayload(); + expect(sessionFile.readAsStringSync(), + '{"session_id":$timestamp,"last_ping":$timestamp}'); + + // Attempting to fetch the session id when malformed should also + // send an error event while parsing + final lastEvent = analytics.sentEvents.last; + expect(lastEvent, isNotNull); + expect(lastEvent.eventName, DashEvent.analyticsException); + expect(lastEvent.eventData['workflow']!, 'Session._refreshSessionData'); + expect(lastEvent.eventData['error']!, 'FileSystemException'); }); });