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.

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

Jump to bottom

Swap crash reporting with unified analytics #147296

Closed
wants to merge 3 commits into from
Closed

Swap crash reporting with unified analytics #147296

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
402e0d2
Use unified_analytics for crash reporting
eliasyishak Apr 23, 2024
20d2707
Remove `Usage` from crash reporting
eliasyishak Apr 23, 2024
5d1c906
Merge branch 'master' into swap-crash-reporting-with-unified-analytics
andrewkolos May 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/flutter_tools/lib/runner.dart
View file
Open in desktop
Expand Up @@ -201,10 +201,10 @@ Future<int> _handleToolError(
globals.analytics.send(Event.exception(exception: error.runtimeType.toString()));
await asyncGuard(() async {
final CrashReportSender crashReportSender = CrashReportSender(
usage: globals.flutterUsage,
platform: globals.platform,
logger: globals.logger,
operatingSystemUtils: globals.os,
analytics: globals.analytics,
);
await crashReportSender.sendReport(
error: error,
Expand Down
14 changes: 7 additions & 7 deletions packages/flutter_tools/lib/src/reporting/crash_reporting.dart
View file
Open in desktop
Expand Up @@ -6,6 +6,7 @@ import 'dart:async';

import 'package:file/file.dart';
import 'package:http/http.dart' as http;
import 'package:unified_analytics/unified_analytics.dart';

import '../base/file_system.dart';
import '../base/io.dart';
Expand All @@ -15,7 +16,6 @@ import '../base/platform.dart';
import '../doctor.dart';
import '../project.dart';
import 'github_template.dart';
import 'reporting.dart';

/// Tells crash backend that the error is from the Flutter CLI.
const String _kProductId = 'Flutter_Tools';
Expand Down Expand Up @@ -105,21 +105,21 @@ class CrashReporter {
class CrashReportSender {
CrashReportSender({
http.Client? client,
required Usage usage,
required Platform platform,
required Logger logger,
required OperatingSystemUtils operatingSystemUtils,
required Analytics analytics,
}) : _client = client ?? http.Client(),
_usage = usage,
_platform = platform,
_logger = logger,
_operatingSystemUtils = operatingSystemUtils;
_operatingSystemUtils = operatingSystemUtils,
_analytics = analytics;

final http.Client _client;
final Usage _usage;
final Platform _platform;
final Logger _logger;
final OperatingSystemUtils _operatingSystemUtils;
final Analytics _analytics;

bool _crashReportSent = false;

Expand Down Expand Up @@ -154,7 +154,7 @@ class CrashReportSender {
final String flutterVersion = getFlutterVersion();

// We don't need to report exceptions happening on user branches
if (_usage.suppressAnalytics || RegExp(r'^\[user-branch\]\/').hasMatch(flutterVersion)) {
if (!_analytics.okToSend || RegExp(r'^\[user-branch\]\/').hasMatch(flutterVersion)) {
return;
}

Expand All @@ -168,7 +168,7 @@ class CrashReportSender {
);

final http.MultipartRequest req = http.MultipartRequest('POST', uri);
req.fields['uuid'] = _usage.clientId;
req.fields['uuid'] = _analytics.clientId;
req.fields['product'] = _kProductId;
req.fields['version'] = flutterVersion;
req.fields['osName'] = _platform.operatingSystem;
Expand Down
34 changes: 19 additions & 15 deletions packages/flutter_tools/test/general.shard/crash_reporting_test.dart
View file
Open in desktop
Expand Up @@ -13,26 +13,30 @@ import 'package:flutter_tools/src/base/platform.dart';
import 'package:flutter_tools/src/doctor.dart';
import 'package:flutter_tools/src/project.dart';
import 'package:flutter_tools/src/reporting/crash_reporting.dart';
import 'package:flutter_tools/src/reporting/reporting.dart';
import 'package:http/http.dart';
import 'package:http/testing.dart';
import 'package:test/fake.dart';
import 'package:unified_analytics/unified_analytics.dart';

import '../src/common.dart';
import '../src/fake_process_manager.dart';
import '../src/fakes.dart';

void main() {
late BufferLogger logger;
late FileSystem fs;
late TestUsage testUsage;
late MemoryFileSystem fs;
late Platform platform;
late OperatingSystemUtils operatingSystemUtils;
late StackTrace stackTrace;
late FakeAnalytics fakeAnalytics;

setUp(() async {
logger = BufferLogger.test();
fs = MemoryFileSystem.test();
testUsage = TestUsage();
fakeAnalytics = getInitializedFakeAnalyticsInstance(
Copy link
Member

Choose a reason for hiding this comment

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

What about instead we just use our own implementation of this class, such as:

class FakeAnalytics extends Fake implements Analytics {
  const FakeAnalytics({this.okToSend = true});
  
  @override
  bool okToSend;
  
  @override
  void suppressTelemetry() => okToSend = false;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't done a deep dive on getInitializedFakeAnalyticsInstance, but I am inclined to agree with this suggestion. Speaking generally, I think it's good for a test to fake as little as needed so that it's more clear as to what that test depends upon.

fs: fs,
fakeFlutterVersion: FakeFlutterVersion(),
);

platform = FakePlatform(environment: <String, String>{});
operatingSystemUtils = OperatingSystemUtils(
Expand Down Expand Up @@ -63,7 +67,7 @@ void main() {
'version': 'test-version',
},
));
expect(crashInfo.fields?['uuid'], testUsage.clientId);
expect(crashInfo.fields?['uuid'], fakeAnalytics.clientId);
expect(crashInfo.fields?['product'], 'Flutter_Tools');
expect(crashInfo.fields?['version'], 'test-version');
expect(crashInfo.fields?['osName'], 'linux');
Expand Down Expand Up @@ -104,14 +108,14 @@ void main() {
});

testWithoutContext('suppress analytics', () async {
testUsage.suppressAnalytics = true;
fakeAnalytics.suppressTelemetry();

final CrashReportSender crashReportSender = CrashReportSender(
client: CrashingCrashReportSender(const SocketException('no internets')),
usage: testUsage,
platform: platform,
logger: logger,
operatingSystemUtils: operatingSystemUtils,
analytics: fakeAnalytics,
);

await crashReportSender.sendReport(
Expand All @@ -126,18 +130,18 @@ void main() {

group('allow analytics', () {
setUp(() async {
testUsage.suppressAnalytics = false;
await fakeAnalytics.setTelemetry(true);
});

testWithoutContext('should send crash reports', () async {
final RequestInfo requestInfo = RequestInfo();

final CrashReportSender crashReportSender = CrashReportSender(
client: MockCrashReportSender(requestInfo),
usage: testUsage,
platform: platform,
logger: logger,
operatingSystemUtils: operatingSystemUtils,
analytics: fakeAnalytics,
);

await crashReportSender.sendReport(
Expand All @@ -153,10 +157,10 @@ void main() {
testWithoutContext('should print an explanatory message when there is a SocketException', () async {
final CrashReportSender crashReportSender = CrashReportSender(
client: CrashingCrashReportSender(const SocketException('no internets')),
usage: testUsage,
platform: platform,
logger: logger,
operatingSystemUtils: operatingSystemUtils,
analytics: fakeAnalytics,
);

await crashReportSender.sendReport(
Expand All @@ -172,10 +176,10 @@ void main() {
testWithoutContext('should print an explanatory message when there is an HttpException', () async {
final CrashReportSender crashReportSender = CrashReportSender(
client: CrashingCrashReportSender(const HttpException('no internets')),
usage: testUsage,
platform: platform,
logger: logger,
operatingSystemUtils: operatingSystemUtils,
analytics: fakeAnalytics,
);

await crashReportSender.sendReport(
Expand All @@ -191,10 +195,10 @@ void main() {
testWithoutContext('should print an explanatory message when there is a ClientException', () async {
final CrashReportSender crashReportSender = CrashReportSender(
client: CrashingCrashReportSender(const HttpException('no internets')),
usage: testUsage,
platform: platform,
logger: logger,
operatingSystemUtils: operatingSystemUtils,
analytics: fakeAnalytics,
);

await crashReportSender.sendReport(
Expand All @@ -212,10 +216,10 @@ void main() {

final CrashReportSender crashReportSender = CrashReportSender(
client: MockCrashReportSender(requestInfo),
usage: testUsage,
platform: platform,
logger: logger,
operatingSystemUtils: operatingSystemUtils,
analytics: fakeAnalytics,
);

await crashReportSender.sendReport(
Expand Down Expand Up @@ -266,10 +270,10 @@ void main() {

final CrashReportSender crashReportSender = CrashReportSender(
client: mockClient,
usage: testUsage,
platform: platform,
logger: logger,
operatingSystemUtils: operatingSystemUtils,
analytics: fakeAnalytics,
);

await crashReportSender.sendReport(
Expand Down Expand Up @@ -303,10 +307,10 @@ void main() {

final CrashReportSender crashReportSender = CrashReportSender(
client: mockClient,
usage: testUsage,
platform: environmentPlatform,
logger: logger,
operatingSystemUtils: operatingSystemUtils,
analytics: fakeAnalytics,
);

await crashReportSender.sendReport(
Expand Down