Skip to content

Checking for devtools config file for opt out #227

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.

Sign up for GitHub

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 3 commits into from
Jan 29, 2024
Merged

Checking for devtools config file for opt out #227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
49ff3d1
Checking for devtools config file for opt out
eliasyishak Jan 24, 2024
926dfd9
Update version
eliasyishak Jan 25, 2024
c5cc219
Correct mislabeled tests
eliasyishak Jan 26, 2024
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
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 5.8.1

- Check devtools config file for legacy opt out status

## 5.8.0

- Fix template string for consent message
Expand Down
2 changes: 1 addition & 1 deletion pkgs/unified_analytics/lib/src/constants.dart
View file
Open in desktop
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.0';
const String kPackageVersion = '5.8.1';

/// The minimum length for a session.
const int kSessionDurationMinutes = 30;
Expand Down
36 changes: 36 additions & 0 deletions pkgs/unified_analytics/lib/src/utils.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,13 +137,17 @@ Directory? getHomeDirectory(FileSystem fs) {
/// Dart: `$HOME/.dart/dartdev.json`
///
/// Flutter: `$HOME/.flutter`
///
/// Devtools: `$HOME/.flutter-devtools/.devtools`
Copy link
Contributor

Choose a reason for hiding this comment

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

So here "legacy" doesn't necessarily refer to the analytics instance it's going to, but an implementation of analytics that predates package:unified_analytics, right?

And therefore, we will need to maintain this code in perpetuity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Couldn't it mean both? Currently devtools has their own implementation for analytics while it also sends to a different GA instance.. is that what you mean?

And yes, we would maintain this. It only runs the first time package:unified_analytics is run on a developers workstation though

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
//
Expand Down Expand Up @@ -202,6 +206,38 @@ bool legacyOptOut({
}
}

// 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<String, Object?>;
if (devtoolsObj.containsKey('analyticsEnabled') &&
devtoolsObj['analyticsEnabled'] == 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;
Copy link
Contributor

@kenzieschmoll kenzieschmoll Jan 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

i see this is the same logic for other tools with legacy opt in / opt out files, but why assume they are opted out if we can't read the file? Seems like instead we should assume that they haven't answered the question as to whether they want to opt in or not, and then we can ask them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a fair question, I think we wanted to approach this with a more conservative approach. I was imagining a developer who thought they were opted out in the past, somehow ended up with a corrupted file, and now gets prompted again... they may dismiss it thinking it was bug and now we're collecting their data

I'm not opposed to swapping out this logic, just wanted to highlight that edge case

Copy link
Contributor

Choose a reason for hiding this comment

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

I would think that if a user opted out once, they'd opt out again when prompted. The user set I'd be worried about dropping are the users who have opted yes, and then somehow their file gets corrected, and now we opt them out instead of asking them again, which would show up as a drop in our usage that could be due to the logic here.

I would guess the corrupted file case is pretty rare though, so maybe this isn't as big of a risk.

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 would guess the corrupted file case is pretty rare though, so maybe this isn't as big of a risk.

I have a separate PR that is out that will allow us to track how many times we run into these catch blocks so that should help answer if this happens often in the wild

}
}

return false;
}

Expand Down
2 changes: 1 addition & 1 deletion pkgs/unified_analytics/pubspec.yaml
View file
Open in desktop
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.0
version: 5.8.1
repository: https://github.com/dart-lang/tools/tree/main/pkgs/unified_analytics

environment:
Expand Down
116 changes: 113 additions & 3 deletions pkgs/unified_analytics/test/legacy_analytics_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ void main() {
});

test('Honor legacy flutter analytics opt out', () {
// Create the file for the dart legacy opt out
// Create the file for the flutter legacy opt out
final flutterLegacyConfigFile =
home.childDirectory('.dart').childFile('dartdev.json');
flutterLegacyConfigFile.createSync(recursive: true);
Expand Down Expand Up @@ -134,7 +134,7 @@ void main() {
});

test('Telemetry enabled if legacy flutter analytics is enabled', () {
// Create the file for the dart legacy opt out
// Create the file for the flutter legacy opt out
final flutterLegacyConfigFile =
home.childDirectory('.dart').childFile('dartdev.json');
flutterLegacyConfigFile.createSync(recursive: true);
Expand Down Expand Up @@ -165,6 +165,78 @@ void main() {
expect(analytics.telemetryEnabled, true);
});

test('Honor legacy devtools analytics opt out', () {
// Create the file for the devtools legacy opt out
final devtoolsLegacyConfigFile =
home.childDirectory('.flutter-devtools').childFile('.devtools');
devtoolsLegacyConfigFile.createSync(recursive: true);
devtoolsLegacyConfigFile.writeAsStringSync('''
{
"analyticsEnabled": false,
"isFirstRun": false,
"lastReleaseNotesVersion": "2.31.0",
"2023-Q4": {
"surveyActionTaken": false,
"surveyShownCount": 0
}
}
''');

// The main analytics instance, other instances can be spawned within tests
// to test how to instances running together work
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,
);

expect(analytics.telemetryEnabled, false);
});

test('Telemetry enabled if legacy devtools analytics is enabled', () {
// Create the file for the devtools legacy opt out
final devtoolsLegacyConfigFile =
home.childDirectory('.flutter-devtools').childFile('.devtools');
devtoolsLegacyConfigFile.createSync(recursive: true);
devtoolsLegacyConfigFile.writeAsStringSync('''
{
"analyticsEnabled": true,
"isFirstRun": false,
"lastReleaseNotesVersion": "2.31.0",
"2023-Q4": {
"surveyActionTaken": false,
"surveyShownCount": 0
}
}
''');

// The main analytics instance, other instances can be spawned within tests
// to test how to instances running together work
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,
);

expect(analytics.telemetryEnabled, true);
});

test('Telemetry disabled if dart config file corrupted', () {
// Create the file for the dart legacy opt out with text that
// is not valid JSON
Expand Down Expand Up @@ -199,8 +271,46 @@ NOT VALID JSON
expect(analytics.telemetryEnabled, false);
});

test('Telemetry disabled if devtools config file corrupted', () {
// Create the file for the devtools legacy opt out with text that
// is not valid JSON
final devtoolsLegacyConfigFile =
home.childDirectory('.flutter-devtools').childFile('.devtools');
devtoolsLegacyConfigFile.createSync(recursive: true);
devtoolsLegacyConfigFile.writeAsStringSync('''
NOT VALID JSON
{
"analyticsEnabled": true,
"isFirstRun": false,
"lastReleaseNotesVersion": "2.31.0",
"2023-Q4": {
"surveyActionTaken": false,
"surveyShownCount": 0
}
}
''');

// The main analytics instance, other instances can be spawned within tests
// to test how to instances running together work
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,
);

expect(analytics.telemetryEnabled, false);
});

test('Telemetry disabled if flutter config file corrupted', () {
// Create the file for the dart legacy opt out with text that
// Create the file for the flutter legacy opt out with text that
// is not valid JSON
final fluttterLegacyConfigFile =
home.childDirectory('.dart').childFile('dartdev.json');
Expand Down