Skip to content
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

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

Merged
Show file tree
Hide file tree
Changes from 8 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
51 changes: 40 additions & 11 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 @@ -225,6 +225,15 @@ abstract class Analytics {
/// out of the configuration file.
Map<String, ToolInfo> get parsedTools;

/// Use this list to check for events that have been emitted when
/// invoking the send method.
///
/// This list will only get populated when running the [Analytics.test]
/// constructor, [Analytics.development], or an instance of [FakeAnalytics].
/// It will otherwise remain empty when used in production.
@visibleForTesting
List<Event> get sentEvents;

/// Boolean that lets the client know if they should display the message.
bool get shouldShowMessage;

Expand Down Expand Up @@ -347,6 +356,10 @@ class AnalyticsImpl implements Analytics {
/// from the [GAClient].
final _futures = <Future<Response>>[];

/// Stores the events that have been sent while the instance exists.
/// Events will only be stored in this list if
final List<Event> _sentEvents = [];

AnalyticsImpl({
required this.tool,
required Directory homeDirectory,
Expand Down Expand Up @@ -418,7 +431,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 +453,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 @@ -475,6 +496,9 @@ class AnalyticsImpl implements Analytics {
@override
Map<String, ToolInfo> get parsedTools => _configHandler.parsedTools;

@override
List<Event> get sentEvents => _sentEvents;

@override
bool get shouldShowMessage => _showMessage;

Expand Down Expand Up @@ -596,7 +620,15 @@ class AnalyticsImpl implements Analytics {
userProperty: userProperty,
);

if (_enableAsserts) checkBody(body);
// This will be set to true by default if running the development or
// test constructors
//
// It will also be enabled by default for the
// [FakeAnalytics] instance
if (_enableAsserts) {
checkBody(body);
_sentEvents.add(event);
}

_logHandler.save(data: body);

Expand Down Expand Up @@ -696,10 +728,6 @@ class AnalyticsImpl implements Analytics {
/// workflow. Invoking the [send] method on this instance will not make any
/// network requests to Google Analytics.
class FakeAnalytics extends AnalyticsImpl {
/// Use this list to check for events that have been emitted when
/// invoking the send method
final List<Event> sentEvents = [];

/// Class to use when you want to see which events were sent
FakeAnalytics({
required super.tool,
Expand Down Expand Up @@ -767,13 +795,14 @@ 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;

@override
List<Event> get sentEvents => [];

@override
void clientShowedMessage() {}

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
Member

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
Member

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
Member

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
Member

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
Member

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