Skip to content

New event added for sending analytics within package on errors #229

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions pkgs/unified_analytics/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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`
Expand Down
27 changes: 17 additions & 10 deletions pkgs/unified_analytics/lib/src/analytics.dart
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ abstract class Analytics {
final homeDirectory = getHomeDirectory(fs);
if (homeDirectory == null ||
!checkDirectoryForWritePermissions(homeDirectory)) {
return NoOpAnalytics();
return const NoOpAnalytics();
}

// Resolve the OS using dart:io
Expand Down Expand Up @@ -187,7 +187,7 @@ abstract class Analytics {
int toolsMessageVersion = kToolsMessageVersion,
String toolsMessage = kToolsMessage,
}) =>
AnalyticsImpl(
FakeAnalytics(
tool: tool,
homeDirectory: homeDirectory,
flutterChannel: flutterChannel,
Expand All @@ -203,7 +203,6 @@ abstract class Analytics {
initializedSurveys: [],
),
gaClient: gaClient ?? const FakeGAClient(),
enableAsserts: true,
clientIde: clientIde,
enabledFeatures: enabledFeatures,
);
Expand Down Expand Up @@ -418,7 +417,11 @@ class AnalyticsImpl implements Analytics {
// 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,
analyticsInstance: this,
);
userProperty = UserProperty(
session: _sessionHandler,
flutterChannel: flutterChannel,
Expand All @@ -436,7 +439,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,
analyticsInstance: this,
);
}

@override
Expand Down Expand Up @@ -712,10 +719,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
Expand Down Expand Up @@ -767,9 +776,7 @@ class NoOpAnalytics implements Analytics {
final Map<String, Map<String, Object?>> userPropertyMap =
const <String, Map<String, Object?>>{};

factory NoOpAnalytics() => const NoOpAnalytics._();

const NoOpAnalytics._();
const NoOpAnalytics();

@override
String get clientId => staticClientId;
Expand Down
2 changes: 1 addition & 1 deletion pkgs/unified_analytics/lib/src/constants.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
4 changes: 4 additions & 0 deletions pkgs/unified_analytics/lib/src/enums.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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 occur within the package for logging',
),
exception(
label: 'exception',
description: 'General errors to log',
Expand Down
22 changes: 22 additions & 0 deletions pkgs/unified_analytics/lib/src/event.dart
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,28 @@ 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. Instead they should 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.
///
Expand Down
65 changes: 42 additions & 23 deletions pkgs/unified_analytics/lib/src/log_handler.dart
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import 'package:clock/clock.dart';
import 'package:file/file.dart';
import 'package:path/path.dart' as p;

import '../unified_analytics.dart';
import 'constants.dart';
import 'initializer.dart';

Expand Down Expand Up @@ -81,22 +82,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.
///
Expand Down Expand Up @@ -149,6 +134,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
Expand All @@ -160,17 +161,23 @@ class LogHandler {
final FileSystem fs;
final Directory homeDirectory;
final File logFile;
final Analytics _analyticsInstance;

/// Ensure we are only sending once per invocation for this class.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why only send once?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the log handlers case, we are checking if each log record is malformed. So in the worst case scenario, we could have 2,500 malformed log records. This check would it make it so that we don't receive tons of errors logs from just one client

Code from log handler below where send these events

    final records = logFile
        .readAsLinesSync()
        .map((String e) {
          try {
            return LogItem.fromRecord(jsonDecode(e) as Map<String, Object?>);
          } on FormatException catch (err) {
            if (!_errorEventSent) {
              _sendFunction(Event.analyticsException(
                workflow: 'LogFileStats.logFileStats',
                error: err.runtimeType.toString(),
                description: 'message: ${err.message}\nsource: ${err.source}',
              ));
              _errorEventSent = true;
            }
            return null;
            // ignore: avoid_catching_errors
          } on TypeError catch (err) {
            if (!_errorEventSent) {
              _sendFunction(Event.analyticsException(
                workflow: 'LogFileStats.logFileStats',
                error: err.runtimeType.toString(),
              ));
              _errorEventSent = true;
            }
            return null;
          }
        })
        .whereType<LogItem>()
        .toList();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is an "invocation" in this instance? For an analysis server session that lasts 8 hours, would we only ever send one error event, or is the LogHandler not that long-lived?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that is a good point, as it stands right now, it would be per dart process. In that case, i can rework the logic for _errorEventSent so that it gets reset every x minutes. The main purpose of this was so that one client wasn't reporting hundreds of the same error event

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than using timestamps, can we rather group together operations that we expect could error multiple times, and track a single local bool errorAlreadySent for those operations?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be even better to create an abstraction for handling errors, something like ErrorHandler that can be passed to each class created by Analytics.

This class can keep track of what events we have sent for errors and block any events we have already sent. That should centralize our error handling to one class

var _errorEventSent = false;

/// 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 Analytics analyticsInstance,
}) : logFile = fs.file(p.join(
homeDirectory.path,
kDartToolDirectoryName,
kLogFileName,
));
)),
_analyticsInstance = analyticsInstance;

/// Get stats from the persisted log file.
///
Expand All @@ -184,15 +191,27 @@ 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<String, Object?>);
} on FormatException {
} on FormatException catch (err) {
if (!_errorEventSent) {
_analyticsInstance.send(Event.analyticsException(
workflow: 'LogFileStats.logFileStats',
error: err.runtimeType.toString(),
description: 'message: ${err.message}\nsource: ${err.source}',
));
_errorEventSent = true;
}
return null;
// ignore: avoid_catching_errors
} on TypeError {
} on TypeError catch (err) {
if (!_errorEventSent) {
_analyticsInstance.send(Event.analyticsException(
workflow: 'LogFileStats.logFileStats',
error: err.runtimeType.toString(),
));
_errorEventSent = true;
}
return null;
}
})
Expand Down
35 changes: 30 additions & 5 deletions pkgs/unified_analytics/lib/src/session.dart
Original file line number Diff line number Diff line change
Expand Up @@ -3,28 +3,35 @@
// 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 'analytics.dart';
import 'constants.dart';
import 'event.dart';
import 'initializer.dart';

class Session {
final Directory homeDirectory;
final FileSystem fs;
final File sessionFile;
final Analytics _analyticsInstance;

late int _sessionId;
late int _lastPing;

/// Ensure we are only sending once per invocation for this class.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, why only once?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same idea for the session handler here except we aren't dealing with the loop from the map method. With the session handler, if we have a problem with the _refreshSessionData method, then it will end up sending an error event every time we call that method, which is every time we send an event.

So if a user had 100 events sent while an instance of Analytics is alive, then we will also have 100 error events

var _errorEventSent = false;

Session({
required this.homeDirectory,
required this.fs,
}) : sessionFile = fs.file(p.join(
homeDirectory.path, kDartToolDirectoryName, kSessionFileName)) {
required Analytics analyticsInstance,
}) : sessionFile = fs.file(p.join(
homeDirectory.path, kDartToolDirectoryName, kSessionFileName)),
_analyticsInstance = analyticsInstance {
_refreshSessionData();
}

Expand Down Expand Up @@ -87,13 +94,31 @@ class Session {

try {
parseContents();
} on FormatException {
} on FormatException catch (err) {
Initializer.createSessionFile(sessionFile: sessionFile);

if (!_errorEventSent) {
_analyticsInstance.send(Event.analyticsException(
workflow: 'Session._refreshSessionData',
error: err.runtimeType.toString(),
description: 'message: ${err.message}\nsource: ${err.source}',
));
_errorEventSent = true;
}

parseContents();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this just throw again and not be caught?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this was naively assuming that the error we encounter while parsing the json was user related and not coming from the package, creating a fix for this now

} on PathNotFoundException {
} on FileSystemException catch (err) {
Initializer.createSessionFile(sessionFile: sessionFile);

if (!_errorEventSent) {
_analyticsInstance.send(Event.analyticsException(
workflow: 'Session._refreshSessionData',
error: err.runtimeType.toString(),
description: err.osError?.toString(),
));
_errorEventSent = true;
}

parseContents();
}
}
Expand Down
Loading