Skip to content

Limit writes to session file #243

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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions pkgs/unified_analytics/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 5.8.6-wip

- Refactored session handler class to use the last modified timestamp as the last ping value

## 5.8.5

- Fix late initialization error for `Analytics.userProperty` [bug](https://github.com/dart-lang/tools/issues/238)
Expand Down
4 changes: 2 additions & 2 deletions pkgs/unified_analytics/lib/src/analytics.dart
Original file line number Diff line number Diff line change
Expand Up @@ -454,8 +454,8 @@ class AnalyticsImpl implements Analytics {
errorHandler: _errorHandler,
);

// Initialize the session handler with the session_id and last_ping
// variables by parsing the json file
// Initialize the session handler with the session_id
// by parsing the json file
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As a side note, I think we should refactor the error handling class so that it doesn't depend on session data AND remove the need for the log handler

This is because:

  1. Having session information for error reports is not necessary, this is just for maintainers of this package so we know what errors are coming up in the wild
  2. We can also remove the need for the log handler because there is no value in locally persisting the error event on the user's computer (this log file is used for the survey handler feature, we are not using error events for targeting users)

_sessionHandler.initialize(telemetryEnabled);
}

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.5';
const String kPackageVersion = '5.8.6-wip';

/// The minimum length for a session.
const int kSessionDurationMinutes = 30;
Expand Down
24 changes: 12 additions & 12 deletions pkgs/unified_analytics/lib/src/initializer.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
// 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:convert';

import 'package:clock/clock.dart';
import 'package:file/file.dart';
import 'package:path/path.dart' as p;
Expand Down Expand Up @@ -75,17 +73,19 @@ class Initializer {
logFile.createSync(recursive: true);
}

/// Creates the session json file which will contain
/// the current session id along with the timestamp for
/// the last ping which will be used to increment the session
/// if current timestamp is greater than the session window.
static void createSessionFile({required File sessionFile}) {
final now = clock.now();
/// Creates the session file which will contain
/// the current session id which is the current timestamp.
///
/// [sessionIdOverride] can be provided as an override, otherwise it
/// will use the current timestamp from [Clock.now].
static void createSessionFile({
required File sessionFile,
DateTime? sessionIdOverride,
}) {
final now = sessionIdOverride ?? clock.now();
sessionFile.createSync(recursive: true);
sessionFile.writeAsStringSync(jsonEncode(<String, int>{
'session_id': now.millisecondsSinceEpoch,
'last_ping': now.millisecondsSinceEpoch,
}));
sessionFile
.writeAsStringSync('{"session_id": ${now.millisecondsSinceEpoch}}');
}

/// This will check that there is a client ID populated in
Expand Down
55 changes: 25 additions & 30 deletions pkgs/unified_analytics/lib/src/session.dart
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,7 @@ class Session {
final File sessionFile;
final ErrorHandler _errorHandler;

late int _sessionId;
late int _lastPing;
int? _sessionId;

Session({
required this.homeDirectory,
Expand All @@ -31,7 +30,7 @@ class Session {
_errorHandler = errorHandler;

/// This will use the data parsed from the
/// session json file in the dart-tool directory
/// session file in the dart-tool directory
/// to get the session id if the last ping was within
/// [kSessionDurationMinutes].
///
Expand All @@ -40,30 +39,27 @@ class Session {
///
/// Note, the file will always be updated when calling this method
/// because the last ping variable will always need to be persisted.
int getSessionId() {
int? getSessionId() {
_refreshSessionData();
final now = clock.now();

// Convert the epoch time from the last ping into datetime and check if we
// are within the kSessionDurationMinutes.
final lastPingDateTime = DateTime.fromMillisecondsSinceEpoch(_lastPing);
final lastPingDateTime = sessionFile.lastModifiedSync();
if (now.difference(lastPingDateTime).inMinutes > kSessionDurationMinutes) {
// In this case, we will need to change both the session id
// and the last ping value
// Update the session file with the latest session id
_sessionId = now.millisecondsSinceEpoch;
sessionFile.writeAsStringSync('{"session_id": $_sessionId}');
} else {
// Update the last modified timestamp with the current timestamp so that
// we can use it for the next _lastPing calculation
sessionFile.setLastModifiedSync(now);
}

// Update the last ping to reflect session activity continuing
_lastPing = now.millisecondsSinceEpoch;

// Rewrite the session object back to the file to persist
// for future events
sessionFile.writeAsStringSync(toJson());

return _sessionId;
}

/// Preps the [Session] class with the data found in the session json file.
/// Preps the [Session] class with the data found in the session file.
///
/// We must check if telemetry is enabled to refresh the session data
/// because the refresh method will write to the session file and for
Expand All @@ -73,15 +69,9 @@ class Session {
if (telemetryEnabled) _refreshSessionData();
}

/// Return a json formatted representation of the class.
String toJson() => jsonEncode(<String, int>{
'session_id': _sessionId,
'last_ping': _lastPing,
});

/// This will go to the session file within the dart-tool
/// directory and fetch the latest data from the json to update
/// the class's variables. If the json file is malformed, a new
/// directory and fetch the latest data from the session file to update
/// the class's variables. If the session file is malformed, a new
/// session file will be recreated.
///
/// This allows the session data in this class to always be up
Expand All @@ -94,13 +84,18 @@ class Session {
final sessionObj =
jsonDecode(sessionFileContents) as Map<String, Object?>;
_sessionId = sessionObj['session_id'] as int;
_lastPing = sessionObj['last_ping'] as int;
}

try {
// Failing to parse the contents will result in the current timestamp
// being used as the session id and will get used to recreate the file
parseContents();
} on FormatException catch (err) {
Initializer.createSessionFile(sessionFile: sessionFile);
final now = clock.now();
Initializer.createSessionFile(
sessionFile: sessionFile,
sessionIdOverride: now,
);

_errorHandler.log(Event.analyticsException(
workflow: 'Session._refreshSessionData',
Expand All @@ -109,11 +104,13 @@ class Session {
));

// Fallback to setting the session id as the current time
final now = clock.now();
_sessionId = now.millisecondsSinceEpoch;
_lastPing = now.millisecondsSinceEpoch;
} on FileSystemException catch (err) {
Initializer.createSessionFile(sessionFile: sessionFile);
final now = clock.now();
Initializer.createSessionFile(
sessionFile: sessionFile,
sessionIdOverride: now,
);

_errorHandler.log(Event.analyticsException(
workflow: 'Session._refreshSessionData',
Expand All @@ -122,9 +119,7 @@ class Session {
));

// Fallback to setting the session id as the current time
final now = clock.now();
_sessionId = now.millisecondsSinceEpoch;
_lastPing = now.millisecondsSinceEpoch;
}
}
}
2 changes: 1 addition & 1 deletion pkgs/unified_analytics/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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.5
version: 5.8.6-wip
repository: https://github.com/dart-lang/tools/tree/main/pkgs/unified_analytics

environment:
Expand Down
50 changes: 16 additions & 34 deletions pkgs/unified_analytics/test/unified_analytics_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ void main() {

test('Resetting session file when data is malformed', () {
// Purposefully write content to the session file that
// can't be decoded as json
// can't be decoded as an integer
sessionFile.writeAsStringSync('contents');

// Define the initial time to start
Expand All @@ -143,8 +143,7 @@ void main() {
final timestamp = clock.now().millisecondsSinceEpoch.toString();
expect(sessionFile.readAsStringSync(), 'contents');
analytics.userProperty.preparePayload();
expect(sessionFile.readAsStringSync(),
'{"session_id":$timestamp,"last_ping":$timestamp}');
expect(sessionFile.readAsStringSync(), '{"session_id": $timestamp}');

// Attempting to fetch the session id when malformed should also
// send an error event while parsing
Expand All @@ -158,9 +157,9 @@ void main() {

test('Handles malformed session file on startup', () {
// Ensure that we are able to send an error message on startup if
// we encounter an error while parsing the contents of the json file
// we encounter an error while parsing the contents of the session file
// for session data
sessionFile.writeAsStringSync('contents');
sessionFile.writeAsStringSync('not a valid session id');

analytics = Analytics.test(
tool: initialTool,
Expand All @@ -185,7 +184,7 @@ void main() {
expect(errorEvent!.eventData['workflow'], 'Session._refreshSessionData');
expect(errorEvent.eventData['error'], 'FormatException');
expect(errorEvent.eventData['description'],
'message: Unexpected character\nsource: contents');
'message: Unexpected character\nsource: not a valid session id');
});

test('Resetting session file when file is removed', () {
Expand All @@ -200,8 +199,7 @@ void main() {
final timestamp = clock.now().millisecondsSinceEpoch.toString();
expect(sessionFile.existsSync(), false);
analytics.userProperty.preparePayload();
expect(sessionFile.readAsStringSync(),
'{"session_id":$timestamp,"last_ping":$timestamp}');
expect(sessionFile.readAsStringSync(), '{"session_id": $timestamp}');

// Attempting to fetch the session id when malformed should also
// send an error event while parsing
Expand Down Expand Up @@ -701,14 +699,10 @@ ${initialTool.label}=$dateStamp,$toolsMessageVersion
);
secondAnalytics.clientShowedMessage();

// Read the contents of the session file
final sessionFileContents = sessionFile.readAsStringSync();
final sessionObj =
jsonDecode(sessionFileContents) as Map<String, Object?>;

expect(secondAnalytics.userPropertyMap['session_id']?['value'],
start.millisecondsSinceEpoch);
expect(sessionObj['last_ping'], start.millisecondsSinceEpoch);
expect(sessionFile.lastModifiedSync().millisecondsSinceEpoch,
start.millisecondsSinceEpoch);
});

// Add time to the start time that is less than the duration
Expand Down Expand Up @@ -739,17 +733,13 @@ ${initialTool.label}=$dateStamp,$toolsMessageVersion
// no events will be sent
thirdAnalytics.send(testEvent);

// Read the contents of the session file
final sessionFileContents = sessionFile.readAsStringSync();
final sessionObj =
jsonDecode(sessionFileContents) as Map<String, Object?>;

expect(thirdAnalytics.userPropertyMap['session_id']?['value'],
start.millisecondsSinceEpoch,
reason: 'The session id should not have changed since it was made '
'within the duration');
expect(sessionObj['last_ping'], end.millisecondsSinceEpoch,
reason: 'The last_ping value should have been updated');
expect(sessionFile.lastModifiedSync().millisecondsSinceEpoch,
end.millisecondsSinceEpoch,
reason: 'The last modified value should have been updated');
});
});

Expand Down Expand Up @@ -782,14 +772,10 @@ ${initialTool.label}=$dateStamp,$toolsMessageVersion
);
secondAnalytics.clientShowedMessage();

// Read the contents of the session file
final sessionFileContents = sessionFile.readAsStringSync();
final sessionObj =
jsonDecode(sessionFileContents) as Map<String, Object?>;

expect(secondAnalytics.userPropertyMap['session_id']?['value'],
start.millisecondsSinceEpoch);
expect(sessionObj['last_ping'], start.millisecondsSinceEpoch);
expect(sessionFile.lastModifiedSync().millisecondsSinceEpoch,
start.millisecondsSinceEpoch);

secondAnalytics.send(testEvent);
});
Expand Down Expand Up @@ -822,17 +808,13 @@ ${initialTool.label}=$dateStamp,$toolsMessageVersion
// no events will be sent
thirdAnalytics.send(testEvent);

// Read the contents of the session file
final sessionFileContents = sessionFile.readAsStringSync();
final sessionObj =
jsonDecode(sessionFileContents) as Map<String, Object?>;

expect(thirdAnalytics.userPropertyMap['session_id']?['value'],
end.millisecondsSinceEpoch,
reason: 'The session id should have changed since it was made '
'outside the duration');
expect(sessionObj['last_ping'], end.millisecondsSinceEpoch,
reason: 'The last_ping value should have been updated');
expect(sessionFile.lastModifiedSync().millisecondsSinceEpoch,
end.millisecondsSinceEpoch,
reason: 'The last modified value should have been updated');
});
});

Expand Down