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

delete old log file if it exceeds kMaxLogFileSize of 25MiB #277

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 @@
## 6.1.2

- Avoid opening large telemetry log files to prevent out of memory errors.

## 6.1.1

- Fixed bug where calling `Analytics.send` could result in a `FileSystemException` when unable to write to a log file.
Expand Down
7 changes: 6 additions & 1 deletion pkgs/unified_analytics/lib/src/constants.dart
Original file line number Diff line number Diff line change
Expand Up @@ -78,11 +78,16 @@ const String kGoogleAnalyticsMeasurementId = 'G-04BXPVBCWJ';
/// How many data records to store in the log file.
const int kLogFileLength = 2500;

/// The maximum allowed size of the telemetry log file.
///
/// 25 MiB.
const int kMaxLogFileSize = 25 * (1 << 20);

/// Filename for the log file to persist sent events on user's machine.
const String kLogFileName = 'dart-flutter-telemetry.log';

/// The current version of the package, should be in line with pubspec version.
const String kPackageVersion = '6.1.1';
const String kPackageVersion = '6.1.2';

/// The minimum length for a session.
const int kSessionDurationMinutes = 30;
Expand Down
14 changes: 11 additions & 3 deletions pkgs/unified_analytics/lib/src/log_handler.dart
Original file line number Diff line number Diff line change
Expand Up @@ -272,10 +272,18 @@ class LogHandler {
/// This will keep the max number of records limited to equal to
/// or less than [kLogFileLength] records.
void save({required Map<String, Object?> data}) {
var records = logFile.readAsLinesSync();
final content = '${jsonEncode(data)}\n';

try {
final stat = logFile.statSync();
List<String> records;
if (stat.size > kMaxLogFileSize) {
logFile.deleteSync();
logFile.createSync();
records = [];
} else {
records = logFile.readAsLinesSync();
}
christopherfujino marked this conversation as resolved.
Show resolved Hide resolved
final content = '${jsonEncode(data)}\n';

// When the record count is less than the max, add as normal;
// else drop the oldest records until equal to max
if (records.length < kLogFileLength) {
Expand Down
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: 6.1.1
version: 6.1.2
Copy link
Member

Choose a reason for hiding this comment

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

As an FYI, for the publishing automation we have set up for this repo, you generally either want to have the pubspec version end in -wip - to indicate that there are intentionally unpublished changes in the repo - or rev to the next approipriate semver version, land the PR, and then use the link in the publishing comment to publish that version.

Docs for this are at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

Copy link
Member Author

Choose a reason for hiding this comment

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

or rev to the next approipriate semver version, land the PR, and then use the link in the publishing comment to publish that version.

Ahh, I missed the last step (link in the comment to publish). Thanks.

repository: https://github.com/dart-lang/tools/tree/main/pkgs/unified_analytics

environment:
Expand Down
92 changes: 92 additions & 0 deletions pkgs/unified_analytics/test/log_handler_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,12 @@
// 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:file/file.dart';
import 'package:file/memory.dart';
import 'package:path/path.dart' as p;
import 'package:test/fake.dart';
import 'package:test/test.dart';

import 'package:unified_analytics/src/constants.dart';
Expand Down Expand Up @@ -212,6 +215,47 @@ void main() {
logHandler.save(data: {});
});

test('deletes log file larger than kMaxLogFileSize', () async {
var deletedLargeLogFile = false;
var wroteDataToLogFile = false;
const data = <String, Object?>{};
final logFile = _FakeFile('log.txt')
.._deleteSyncImpl = (() => deletedLargeLogFile = true)
.._createSyncImpl = () {}
.._statSyncImpl = (() => _FakeFileStat(kMaxLogFileSize + 1))
.._writeAsStringSync = (contents, {mode = FileMode.append}) {
expect(contents.trim(), data.toString());
expect(mode, FileMode.writeOnlyAppend);
wroteDataToLogFile = true;
};
final logHandler = LogHandler(logFile: logFile);

logHandler.save(data: data);
expect(deletedLargeLogFile, isTrue);
expect(wroteDataToLogFile, isTrue);
});

test('does not delete log file if smaller than kMaxLogFileSize', () async {
var wroteDataToLogFile = false;
const data = <String, Object?>{};
final logFile = _FakeFile('log.txt')
.._deleteSyncImpl =
(() => fail('called logFile.deleteSync() when file was less than '
'kMaxLogFileSize'))
.._createSyncImpl = () {}
.._readAsLinesSyncImpl = (() => ['three', 'previous', 'lines'])
.._statSyncImpl = (() => _FakeFileStat(kMaxLogFileSize - 1))
.._writeAsStringSync = (contents, {mode = FileMode.append}) {
expect(contents.trim(), data.toString());
expect(mode, FileMode.writeOnlyAppend);
wroteDataToLogFile = true;
};
final logHandler = LogHandler(logFile: logFile);

logHandler.save(data: data);
expect(wroteDataToLogFile, isTrue);
});

test('Catching cast errors for each log record silently', () async {
// Write a json array to the log file which will cause
// a cast error when parsing each line
Expand Down Expand Up @@ -290,3 +334,51 @@ void main() {
expect(newString, testString);
});
}

class _FakeFileStat extends Fake implements FileStat {
_FakeFileStat(this.size);

@override
final int size;
}

class _FakeFile extends Fake implements File {
_FakeFile(this.path);
Comment on lines +345 to +346
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, why use mutable fields for method implementations over optional constructor parameters? Do you find this more readable?


List<String> Function()? _readAsLinesSyncImpl;

@override
List<String> readAsLinesSync({Encoding encoding = utf8}) =>
_readAsLinesSyncImpl!();

@override
final String path;

FileStat Function()? _statSyncImpl;

@override
FileStat statSync() => _statSyncImpl!();

void Function()? _deleteSyncImpl;

@override
void deleteSync({bool recursive = false}) => _deleteSyncImpl!();

void Function()? _createSyncImpl;

@override
void createSync({bool recursive = false, bool exclusive = false}) {
return _createSyncImpl!();
}

void Function(String contents, {FileMode mode})? _writeAsStringSync;

@override
void writeAsStringSync(
String contents, {
FileMode mode = FileMode.write,
Encoding encoding = utf8,
bool flush = false,
}) =>
_writeAsStringSync!(contents, mode: mode);
}