From f6a76586de91fbda2beeabbd141f9cca330b6a37 Mon Sep 17 00:00:00 2001 From: Devon Carew Date: Wed, 27 Nov 2019 00:16:50 +0000 Subject: [PATCH] [analyzer] send additional data to crash reporting Change-Id: I9f2da85e1c283ed4942ba076c98003aef80b7051 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/126203 Reviewed-by: Jaime Wren Reviewed-by: Mike Fairhurst Commit-Queue: Devon Carew --- .../lib/src/analysis_server.dart | 2 +- .../lib/src/domain_completion.dart | 2 +- .../lib/src/server/crash_reporting.dart | 4 +- pkg/telemetry/lib/crash_reporting.dart | 57 +++++++++++++++---- pkg/telemetry/test/crash_reporting_test.dart | 55 ++++++++++++++++-- 5 files changed, 101 insertions(+), 19 deletions(-) diff --git a/pkg/analysis_server/lib/src/analysis_server.dart b/pkg/analysis_server/lib/src/analysis_server.dart index 783c0b445d1..e704c304727 100644 --- a/pkg/analysis_server/lib/src/analysis_server.dart +++ b/pkg/analysis_server/lib/src/analysis_server.dart @@ -356,7 +356,7 @@ class AnalysisServer extends AbstractAnalysisServer { }); }, onError: (exception, stackTrace) { AnalysisEngine.instance.instrumentationService.logException( - FatalException('Failed to handle request: ${request.toJson()}', + FatalException('Failed to handle request: ${request.method}', exception, stackTrace)); }); } diff --git a/pkg/analysis_server/lib/src/domain_completion.dart b/pkg/analysis_server/lib/src/domain_completion.dart index 258e444aa00..8a94e689531 100644 --- a/pkg/analysis_server/lib/src/domain_completion.dart +++ b/pkg/analysis_server/lib/src/domain_completion.dart @@ -258,7 +258,7 @@ class CompletionDomainHandler extends AbstractRequestHandler { }, onError: (exception, stackTrace) { AnalysisEngine.instance.instrumentationService.logException( CaughtException.withMessage( - 'Failed to handle completion domain request: ${request.toJson()}', + 'Failed to handle completion domain request: ${request.method}', exception, stackTrace)); }); diff --git a/pkg/analysis_server/lib/src/server/crash_reporting.dart b/pkg/analysis_server/lib/src/server/crash_reporting.dart index b9566159573..29b88142c29 100644 --- a/pkg/analysis_server/lib/src/server/crash_reporting.dart +++ b/pkg/analysis_server/lib/src/server/crash_reporting.dart @@ -18,7 +18,9 @@ class CrashReportingInstrumentation extends NoopInstrumentationService { // Get the root CaughtException, which matters most for debugging. CaughtException root = exception.rootCaughtException; - reporter.sendReport(root.exception, root.stackTrace).catchError((error) { + reporter + .sendReport(root.exception, root.stackTrace, comment: root.message) + .catchError((error) { // We silently ignore errors sending crash reports (network issues, ...). }); } else { diff --git a/pkg/telemetry/lib/crash_reporting.dart b/pkg/telemetry/lib/crash_reporting.dart index 3b096c6cca8..6e44d8e3b85 100644 --- a/pkg/telemetry/lib/crash_reporting.dart +++ b/pkg/telemetry/lib/crash_reporting.dart @@ -4,8 +4,10 @@ import 'dart:async'; import 'dart:io'; +import 'dart:math' as math; import 'package:http/http.dart' as http; +import 'package:meta/meta.dart'; import 'package:stack_trace/stack_trace.dart'; import 'src/utils.dart'; @@ -42,9 +44,11 @@ class CrashReportSender { final String crashProductId; final EnablementCallback shouldSend; final http.Client _httpClient; + final Stopwatch _processStopwatch = new Stopwatch()..start(); final ThrottlingBucket _throttle = ThrottlingBucket(10, Duration(minutes: 1)); - int _reportsSend = 0; + int _reportsSent = 0; + int _skippedReports = 0; /// Create a new [CrashReportSender]. CrashReportSender( @@ -56,21 +60,36 @@ class CrashReportSender { /// Sends one crash report. /// /// The report is populated from data in [error] and [stackTrace]. - Future sendReport(dynamic error, StackTrace stackTrace) async { + /// + /// Additional context about the crash can optionally be passed in via + /// [comment]. Note that this field should not include PII. + Future sendReport( + dynamic error, + StackTrace stackTrace, { + String comment, + }) async { if (!shouldSend()) { return; } // Check if we've sent too many reports recently. if (!_throttle.removeDrop()) { + _skippedReports++; return; } // Don't send too many total reports to crash reporting. - if (_reportsSend >= _maxReportsToSend) { + if (_reportsSent >= _maxReportsToSend) { return; } + _reportsSent++; + + // Calculate the 'weight' of the this report; we increase the weight of a + // report if we had throttled previous reports. + int weight = math.min(_skippedReports + 1, 10000); + _skippedReports = 0; + try { final String dartVersion = Platform.version.split(' ').first; @@ -82,13 +101,28 @@ class CrashReportSender { ); final http.MultipartRequest req = new http.MultipartRequest('POST', uri); - req.fields['product'] = crashProductId; - req.fields['version'] = dartVersion; - req.fields['osName'] = Platform.operatingSystem; - req.fields['osVersion'] = Platform.operatingSystemVersion; - req.fields['type'] = _dartTypeId; - req.fields['error_runtime_type'] = '${error.runtimeType}'; - req.fields['error_message'] = '$error'; + + Map fields = req.fields; + fields['product'] = crashProductId; + fields['version'] = dartVersion; + fields['osName'] = Platform.operatingSystem; + fields['osVersion'] = Platform.operatingSystemVersion; + fields['type'] = _dartTypeId; + fields['error_runtime_type'] = '${error.runtimeType}'; + fields['error_message'] = '$error'; + + // Optional comments. + if (comment != null) { + fields['comments'] = comment; + } + + // The uptime of the process before it crashed (in milliseconds). + fields['ptime'] = _processStopwatch.elapsedMilliseconds.toString(); + + // Send the amount to weight this report. + if (weight > 1) { + fields['weight'] = weight.toString(); + } final Chain chain = new Chain.forTrace(stackTrace); req.files.add(new http.MultipartFile.fromString( @@ -108,6 +142,9 @@ class CrashReportSender { } } + @visibleForTesting + int get reportsSent => _reportsSent; + /// Closes the client and cleans up any resources associated with it. This /// will close the associated [http.Client]. void dispose() { diff --git a/pkg/telemetry/test/crash_reporting_test.dart b/pkg/telemetry/test/crash_reporting_test.dart index 0954178a8f6..9d4832446f7 100644 --- a/pkg/telemetry/test/crash_reporting_test.dart +++ b/pkg/telemetry/test/crash_reporting_test.dart @@ -11,8 +11,9 @@ import 'package:test/test.dart'; import 'package:usage/usage.dart'; void main() { - group('crash_reporting', () { + group('CrashReportSender', () { MockClient mockClient; + AnalyticsMock analytics; Request request; @@ -21,14 +22,15 @@ void main() { request = r; return new Response('crash-report-001', 200); }); + + analytics = new AnalyticsMock()..enabled = true; }); - test('CrashReportSender', () async { - EnablementCallback shouldSend = () { - return true; - }; + EnablementCallback shouldSend = () { + return true; + }; - AnalyticsMock analytics = new AnalyticsMock()..enabled = true; + test('general', () async { CrashReportSender sender = new CrashReportSender( analytics.trackingId, shouldSend, httpClient: mockClient); @@ -39,5 +41,46 @@ void main() { expect(body, contains('String')); // error.runtimeType expect(body, contains('test-error')); }); + + test('reportsSent', () async { + CrashReportSender sender = new CrashReportSender( + analytics.trackingId, shouldSend, + httpClient: mockClient); + + expect(sender.reportsSent, 0); + + await sender.sendReport('test-error', StackTrace.current); + + expect(sender.reportsSent, 1); + + String body = utf8.decode(request.bodyBytes); + expect(body, contains('String')); + expect(body, contains('test-error')); + }); + + test('contains message', () async { + CrashReportSender sender = new CrashReportSender( + analytics.trackingId, shouldSend, + httpClient: mockClient); + + await sender.sendReport('test-error', StackTrace.current, + comment: 'additional message'); + + String body = utf8.decode(request.bodyBytes); + expect(body, contains('String')); + expect(body, contains('test-error')); + expect(body, contains('additional message')); + }); + + test('has ptime', () async { + CrashReportSender sender = new CrashReportSender( + analytics.trackingId, shouldSend, + httpClient: mockClient); + + await sender.sendReport('test-error', StackTrace.current); + + String body = utf8.decode(request.bodyBytes); + expect(body, contains('name="ptime"')); + }); }); }