diff --git a/CHANGELOG.md b/CHANGELOG.md index 2997308558..ec1f4776f1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,22 @@ ``` - Linux native error & obfuscation support ([#2431](https://github.com/getsentry/sentry-dart/pull/2431)) - Improve Device context on plain Dart and Flutter desktop apps ([#2441](https://github.com/getsentry/sentry-dart/pull/2441)) +- Add debounce to capturing screenshots ([#2368](https://github.com/getsentry/sentry-dart/pull/2368)) + - Per default, screenshots are debounced for 2 seconds. + - If you need more granular screenshots, you can opt out of debouncing: + ```dart + await SentryFlutter.init((options) { + options.beforeCaptureScreenshot = (event, hint, debounce) { + if (debounce) { + return true; // Capture screenshot even if the SDK wants to debounce it. + } else { + // check event and hint + ... + } + }; + }); + ``` + - Replace deprecated `BeforeScreenshotCallback` with new `BeforeCaptureCallback`. ### Dependencies diff --git a/flutter/lib/src/event_processor/screenshot_event_processor.dart b/flutter/lib/src/event_processor/screenshot_event_processor.dart index 724d74e0d5..ff6a4c0bc6 100644 --- a/flutter/lib/src/event_processor/screenshot_event_processor.dart +++ b/flutter/lib/src/event_processor/screenshot_event_processor.dart @@ -9,14 +9,16 @@ import '../screenshot/recorder.dart'; import '../screenshot/recorder_config.dart'; import 'package:flutter/widgets.dart' as widget; +import '../utils/debouncer.dart'; + class ScreenshotEventProcessor implements EventProcessor { final SentryFlutterOptions _options; late final ScreenshotRecorder _recorder; + late final Debouncer _debouncer; ScreenshotEventProcessor(this._options) { final targetResolution = _options.screenshotQuality.targetResolution(); - _recorder = ScreenshotRecorder( ScreenshotRecorderConfig( width: targetResolution, @@ -25,6 +27,11 @@ class ScreenshotEventProcessor implements EventProcessor { _options, isReplayRecorder: false, ); + _debouncer = Debouncer( + // ignore: invalid_use_of_internal_member + _options.clock, + waitTimeMs: 2000, + ); } @override @@ -43,29 +50,51 @@ class ScreenshotEventProcessor implements EventProcessor { return event; // No need to attach screenshot of feedback form. } + // skip capturing in case of debouncing (=too many frequent capture requests) + // the BeforeCaptureCallback may overrules the debouncing decision + final shouldDebounce = _debouncer.shouldDebounce(); + + // ignore: deprecated_member_use_from_same_package final beforeScreenshot = _options.beforeScreenshot; - if (beforeScreenshot != null) { - try { - final result = beforeScreenshot(event, hint: hint); - bool takeScreenshot; + final beforeCapture = _options.beforeCaptureScreenshot; + + try { + FutureOr? result; + + if (beforeCapture != null) { + result = beforeCapture(event, hint, shouldDebounce); + } else if (beforeScreenshot != null) { + result = beforeScreenshot(event, hint: hint); + } + + bool takeScreenshot = true; + + if (result != null) { if (result is Future) { takeScreenshot = await result; } else { takeScreenshot = result; } - if (!takeScreenshot) { - return event; - } - } catch (exception, stackTrace) { + } else if (shouldDebounce) { _options.logger( - SentryLevel.error, - 'The beforeScreenshot callback threw an exception', - exception: exception, - stackTrace: stackTrace, + SentryLevel.debug, + 'Skipping screenshot capture due to debouncing (too many captures within ${_debouncer.waitTimeMs}ms)', ); - if (_options.automatedTestMode) { - rethrow; - } + takeScreenshot = false; + } + + if (!takeScreenshot) { + return event; + } + } catch (exception, stackTrace) { + _options.logger( + SentryLevel.error, + 'The beforeCapture/beforeScreenshot callback threw an exception', + exception: exception, + stackTrace: stackTrace, + ); + if (_options.automatedTestMode) { + rethrow; } } @@ -88,8 +117,7 @@ class ScreenshotEventProcessor implements EventProcessor { return event; } - Uint8List? screenshotData = await createScreenshot(); - + final screenshotData = await createScreenshot(); if (screenshotData != null) { hint.screenshot = SentryAttachment.fromScreenshotData(screenshotData); } diff --git a/flutter/lib/src/integrations/screenshot_integration.dart b/flutter/lib/src/integrations/screenshot_integration.dart index b1d9db3dd3..10cf60228a 100644 --- a/flutter/lib/src/integrations/screenshot_integration.dart +++ b/flutter/lib/src/integrations/screenshot_integration.dart @@ -3,7 +3,7 @@ import '../event_processor/screenshot_event_processor.dart'; import '../sentry_flutter_options.dart'; /// Adds [ScreenshotEventProcessor] to options event processors if -/// [SentryFlutterOptions.screenshot.attach] is true +/// [SentryFlutterOptions.attachScreenshot] is true class ScreenshotIntegration implements Integration { SentryFlutterOptions? _options; ScreenshotEventProcessor? _screenshotEventProcessor; diff --git a/flutter/lib/src/sentry_flutter_options.dart b/flutter/lib/src/sentry_flutter_options.dart index 6459eb55bc..5d95eb4817 100644 --- a/flutter/lib/src/sentry_flutter_options.dart +++ b/flutter/lib/src/sentry_flutter_options.dart @@ -196,10 +196,14 @@ class SentryFlutterOptions extends SentryOptions { /// See https://docs.sentry.io/platforms/flutter/troubleshooting/#screenshot-integration-background-crash bool attachScreenshotOnlyWhenResumed = false; + @Deprecated( + 'Will be removed in a future version. Use [beforeCaptureScreenshot] instead') + BeforeScreenshotCallback? beforeScreenshot; + /// Sets a callback which is executed before capturing screenshots. Only /// relevant if `attachScreenshot` is set to true. When false is returned /// from the function, no screenshot will be attached. - BeforeScreenshotCallback? beforeScreenshot; + BeforeCaptureCallback? beforeCaptureScreenshot; /// Enable or disable automatic breadcrumbs for User interactions Using [Listener] /// @@ -417,9 +421,34 @@ class _SentryFlutterExperimentalOptions { _privacy ?? SentryPrivacyOptions(); } -/// Callback being executed in [ScreenshotEventProcessor], deciding if a -/// screenshot should be recorded and attached. -typedef BeforeScreenshotCallback = FutureOr Function( - SentryEvent event, { - Hint? hint, -}); +@Deprecated( + 'Will be removed in a future version. Use [BeforeCaptureCallback] instead') +typedef BeforeScreenshotCallback = FutureOr Function(SentryEvent event, + {Hint? hint}); + +/// A callback which can be used to suppress capturing of screenshots. +/// It's called in [ScreenshotEventProcessor] if screenshots are enabled. +/// This gives more fine-grained control over when capturing should be performed, +/// e.g., only capture screenshots for fatal events or override any debouncing for important events. +/// +/// Since capturing can be resource-intensive, the debounce parameter should be respected if possible. +/// +/// Example: +/// ```dart +/// if (debounce) { +/// return false; +/// } else { +/// // check event and hint +/// } +/// ``` +/// +/// [event] is the event to be checked. +/// [hint] provides additional hints. +/// [debounce] indicates if capturing is marked for being debounced. +/// +/// Returns `true` if capturing should be performed, otherwise `false`. +typedef BeforeCaptureCallback = FutureOr Function( + SentryEvent event, + Hint hint, + bool debounce, +); diff --git a/flutter/lib/src/utils/debouncer.dart b/flutter/lib/src/utils/debouncer.dart index b714b41b4e..d3027d8849 100644 --- a/flutter/lib/src/utils/debouncer.dart +++ b/flutter/lib/src/utils/debouncer.dart @@ -1,20 +1,24 @@ -import 'dart:async'; -import 'package:flutter/foundation.dart'; import 'package:meta/meta.dart'; +import 'package:sentry/sentry.dart'; @internal class Debouncer { - final int milliseconds; - Timer? _timer; + final ClockProvider clockProvider; + final int waitTimeMs; + DateTime? _lastExecutionTime; - Debouncer({required this.milliseconds}); + Debouncer(this.clockProvider, {this.waitTimeMs = 2000}); - void run(VoidCallback action) { - _timer?.cancel(); - _timer = Timer(Duration(milliseconds: milliseconds), action); - } + bool shouldDebounce() { + final currentTime = clockProvider(); + final lastExecutionTime = _lastExecutionTime; + _lastExecutionTime = currentTime; + + if (lastExecutionTime != null && + currentTime.difference(lastExecutionTime).inMilliseconds < waitTimeMs) { + return true; + } - void dispose() { - _timer?.cancel(); + return false; } } diff --git a/flutter/lib/src/utils/timer_debouncer.dart b/flutter/lib/src/utils/timer_debouncer.dart new file mode 100644 index 0000000000..bd953ef0a8 --- /dev/null +++ b/flutter/lib/src/utils/timer_debouncer.dart @@ -0,0 +1,20 @@ +import 'dart:async'; +import 'package:flutter/foundation.dart'; +import 'package:meta/meta.dart'; + +@internal +class TimerDebouncer { + final int milliseconds; + Timer? _timer; + + TimerDebouncer({required this.milliseconds}); + + void run(VoidCallback action) { + _timer?.cancel(); + _timer = Timer(Duration(milliseconds: milliseconds), action); + } + + void dispose() { + _timer?.cancel(); + } +} diff --git a/flutter/lib/src/widgets_binding_observer.dart b/flutter/lib/src/widgets_binding_observer.dart index 7c0db1842f..525027f328 100644 --- a/flutter/lib/src/widgets_binding_observer.dart +++ b/flutter/lib/src/widgets_binding_observer.dart @@ -3,8 +3,8 @@ import 'dart:ui'; import 'package:flutter/foundation.dart'; import 'package:flutter/material.dart'; +import 'utils/timer_debouncer.dart'; import '../sentry_flutter.dart'; -import 'utils/debouncer.dart'; /// This is a `WidgetsBindingObserver` which can observe some events of a /// Flutter application. @@ -25,7 +25,8 @@ class SentryWidgetsBindingObserver with WidgetsBindingObserver { required SentryFlutterOptions options, }) : _hub = hub ?? HubAdapter(), _options = options, - _screenSizeStreamController = StreamController(sync: true) { + _screenSizeStreamController = StreamController(sync: true), + _didChangeMetricsDebouncer = TimerDebouncer(milliseconds: 100) { if (_options.enableWindowMetricBreadcrumbs) { _screenSizeStreamController.stream .map( @@ -47,12 +48,11 @@ class SentryWidgetsBindingObserver with WidgetsBindingObserver { final Hub _hub; final SentryFlutterOptions _options; + final TimerDebouncer _didChangeMetricsDebouncer; // ignore: deprecated_member_use final StreamController _screenSizeStreamController; - final _didChangeMetricsDebouncer = Debouncer(milliseconds: 100); - /// This method records lifecycle events. /// It tries to mimic the behavior of ActivityBreadcrumbsIntegration of Sentry /// Android for lifecycle events. @@ -91,7 +91,6 @@ class SentryWidgetsBindingObserver with WidgetsBindingObserver { if (!_options.enableWindowMetricBreadcrumbs) { return; } - _didChangeMetricsDebouncer.run(() { // ignore: deprecated_member_use final window = _options.bindingUtils.instance?.window; diff --git a/flutter/test/event_processor/screenshot_event_processor_test.dart b/flutter/test/event_processor/screenshot_event_processor_test.dart index 635d2a5770..6250a6557d 100644 --- a/flutter/test/event_processor/screenshot_event_processor_test.dart +++ b/flutter/test/event_processor/screenshot_event_processor_test.dart @@ -129,6 +129,7 @@ void main() { group('beforeScreenshot', () { testWidgets('does add screenshot if beforeScreenshot returns true', (tester) async { + // ignore: deprecated_member_use_from_same_package fixture.options.beforeScreenshot = (SentryEvent event, {Hint? hint}) { return true; }; @@ -138,6 +139,7 @@ void main() { testWidgets('does add screenshot if async beforeScreenshot returns true', (tester) async { + // ignore: deprecated_member_use_from_same_package fixture.options.beforeScreenshot = (SentryEvent event, {Hint? hint}) async { await Future.delayed(Duration(milliseconds: 1)); @@ -149,6 +151,7 @@ void main() { testWidgets('does not add screenshot if beforeScreenshot returns false', (tester) async { + // ignore: deprecated_member_use_from_same_package fixture.options.beforeScreenshot = (SentryEvent event, {Hint? hint}) { return false; }; @@ -159,6 +162,7 @@ void main() { testWidgets( 'does not add screenshot if async beforeScreenshot returns false', (tester) async { + // ignore: deprecated_member_use_from_same_package fixture.options.beforeScreenshot = (SentryEvent event, {Hint? hint}) async { await Future.delayed(Duration(milliseconds: 1)); @@ -171,6 +175,7 @@ void main() { testWidgets('does add screenshot if beforeScreenshot throws', (tester) async { fixture.options.automatedTestMode = false; + // ignore: deprecated_member_use_from_same_package fixture.options.beforeScreenshot = (SentryEvent event, {Hint? hint}) { throw Error(); }; @@ -181,6 +186,7 @@ void main() { testWidgets('does add screenshot if async beforeScreenshot throws', (tester) async { fixture.options.automatedTestMode = false; + // ignore: deprecated_member_use_from_same_package fixture.options.beforeScreenshot = (SentryEvent event, {Hint? hint}) async { await Future.delayed(Duration(milliseconds: 1)); @@ -195,6 +201,7 @@ void main() { SentryEvent? beforeScreenshotEvent; Hint? beforeScreenshotHint; + // ignore: deprecated_member_use_from_same_package fixture.options.beforeScreenshot = (SentryEvent event, {Hint? hint}) { beforeScreenshotEvent = event; beforeScreenshotHint = hint; @@ -208,6 +215,208 @@ void main() { expect(beforeScreenshotHint, hint); }); }); + + group('beforeCaptureScreenshot', () { + testWidgets('does add screenshot if beforeCapture returns true', + (tester) async { + fixture.options.beforeCaptureScreenshot = + (SentryEvent event, Hint hint, bool shouldDebounce) { + return true; + }; + await _addScreenshotAttachment(tester, FlutterRenderer.canvasKit, + added: true, isWeb: false); + }); + + testWidgets('does add screenshot if async beforeCapture returns true', + (tester) async { + fixture.options.beforeCaptureScreenshot = + (SentryEvent event, Hint hint, bool shouldDebounce) async { + await Future.delayed(Duration(milliseconds: 1)); + return true; + }; + await _addScreenshotAttachment(tester, FlutterRenderer.canvasKit, + added: true, isWeb: false); + }); + + testWidgets('does not add screenshot if beforeCapture returns false', + (tester) async { + fixture.options.beforeCaptureScreenshot = + (SentryEvent event, Hint hint, bool shouldDebounce) { + return false; + }; + await _addScreenshotAttachment(tester, FlutterRenderer.canvasKit, + added: false, isWeb: false); + }); + + testWidgets('does not add screenshot if async beforeCapture returns false', + (tester) async { + fixture.options.beforeCaptureScreenshot = + (SentryEvent event, Hint hint, bool shouldDebounce) async { + await Future.delayed(Duration(milliseconds: 1)); + return false; + }; + await _addScreenshotAttachment(tester, FlutterRenderer.canvasKit, + added: false, isWeb: false); + }); + + testWidgets('does add screenshot if beforeCapture throws', (tester) async { + fixture.options.automatedTestMode = false; + fixture.options.beforeCaptureScreenshot = + (SentryEvent event, Hint hint, bool shouldDebounce) { + throw Error(); + }; + await _addScreenshotAttachment(tester, FlutterRenderer.canvasKit, + added: true, isWeb: false); + }); + + testWidgets('does add screenshot if async beforeCapture throws', + (tester) async { + fixture.options.automatedTestMode = false; + fixture.options.beforeCaptureScreenshot = + (SentryEvent event, Hint hint, bool shouldDebounce) async { + await Future.delayed(Duration(milliseconds: 1)); + throw Error(); + }; + await _addScreenshotAttachment(tester, FlutterRenderer.canvasKit, + added: true, isWeb: false); + }); + + testWidgets('does add screenshot event if shouldDebounce true', + (tester) async { + await tester.runAsync(() async { + var shouldDebounceValues = []; + + fixture.options.beforeCaptureScreenshot = + (SentryEvent event, Hint hint, bool shouldDebounce) { + shouldDebounceValues.add(shouldDebounce); + return true; + }; + + final sut = fixture.getSut(FlutterRenderer.canvasKit, false); + + await tester.pumpWidget( + SentryScreenshotWidget( + child: Text( + 'Catching Pokémon is a snap!', + textDirection: TextDirection.ltr, + ), + ), + ); + + final event = SentryEvent(throwable: Exception()); + final hintOne = Hint(); + final hintTwo = Hint(); + + await sut.apply(event, hintOne); + await sut.apply(event, hintTwo); + + expect(hintOne.screenshot, isNotNull); + expect(hintTwo.screenshot, isNotNull); + + expect(shouldDebounceValues[0], false); + expect(shouldDebounceValues[1], true); + }); + }); + + testWidgets('passes event & hint to beforeCapture callback', + (tester) async { + SentryEvent? beforeScreenshotEvent; + Hint? beforeScreenshotHint; + + fixture.options.beforeCaptureScreenshot = + (SentryEvent event, Hint hint, bool shouldDebounce) { + beforeScreenshotEvent = event; + beforeScreenshotHint = hint; + return true; + }; + + await _addScreenshotAttachment(tester, FlutterRenderer.canvasKit, + added: true, isWeb: false); + + expect(beforeScreenshotEvent, event); + expect(beforeScreenshotHint, hint); + }); + }); + + group("debounce", () { + testWidgets("limits added screenshots within debounce timeframe", + (tester) async { + // Run with real async https://stackoverflow.com/a/54021863 + await tester.runAsync(() async { + var firstCall = true; + // ignore: invalid_use_of_internal_member + fixture.options.clock = () { + if (firstCall) { + firstCall = false; + return DateTime.fromMillisecondsSinceEpoch(0); + } else { + return DateTime.fromMillisecondsSinceEpoch(2000 - 1); + } + }; + + final sut = fixture.getSut(FlutterRenderer.canvasKit, false); + + await tester.pumpWidget(SentryScreenshotWidget( + child: Text('Catching Pokémon is a snap!', + textDirection: TextDirection.ltr))); + + final throwable = Exception(); + + final firstEvent = SentryEvent(throwable: throwable); + final firstHint = Hint(); + + final secondEvent = SentryEvent(throwable: throwable); + final secondHint = Hint(); + + await sut.apply(firstEvent, firstHint); + await sut.apply(secondEvent, secondHint); + + expect(firstHint.screenshot, isNotNull); + expect(secondHint.screenshot, isNull); + }); + }); + + testWidgets("adds screenshots after debounce timeframe", (tester) async { + // Run with real async https://stackoverflow.com/a/54021863 + await tester.runAsync(() async { + var firstCall = true; + // ignore: invalid_use_of_internal_member + fixture.options.clock = () { + if (firstCall) { + firstCall = false; + return DateTime.fromMillisecondsSinceEpoch(0); + } else { + return DateTime.fromMillisecondsSinceEpoch(2001); + } + }; + + final sut = fixture.getSut(FlutterRenderer.canvasKit, false); + + await tester.pumpWidget( + SentryScreenshotWidget( + child: Text( + 'Catching Pokémon is a snap!', + textDirection: TextDirection.ltr, + ), + ), + ); + + final throwable = Exception(); + + final firstEvent = SentryEvent(throwable: throwable); + final firstHint = Hint(); + + final secondEvent = SentryEvent(throwable: throwable); + final secondHint = Hint(); + + await sut.apply(firstEvent, firstHint); + await sut.apply(secondEvent, secondHint); + + expect(firstHint.screenshot, isNotNull); + expect(secondHint.screenshot, isNotNull); + }); + }); + }); } class Fixture { diff --git a/flutter/test/utils/debouncer_test.dart b/flutter/test/utils/debouncer_test.dart new file mode 100644 index 0000000000..b78987d360 --- /dev/null +++ b/flutter/test/utils/debouncer_test.dart @@ -0,0 +1,53 @@ +import 'package:flutter_test/flutter_test.dart'; +import 'package:sentry_flutter/src/utils/debouncer.dart'; + +void main() { + late Fixture fixture; + + setUp(() { + fixture = Fixture(); + }); + + test('Debouncer should not debounce on the first check', () { + final sut = fixture.getSut(); + expect(sut.shouldDebounce(), isFalse); + }); + + test('Debouncer should not debounce if wait time is 0', () { + final sut = fixture.getSut(waitTimeMs: 0); + expect(sut.shouldDebounce(), isFalse); + expect(sut.shouldDebounce(), isFalse); + expect(sut.shouldDebounce(), isFalse); + }); + + test('Debouncer should signal debounce if the second invocation is too early', + () { + fixture.currentTimeMs = 1000; + final sut = fixture.getSut(waitTimeMs: 3000); + expect(sut.shouldDebounce(), isFalse); + + fixture.currentTimeMs = 3999; + expect(sut.shouldDebounce(), isTrue); + }); + + test( + 'Debouncer should not signal debounce if the second invocation is late enough', + () { + fixture.currentTimeMs = 1000; + final sut = fixture.getSut(waitTimeMs: 3000); + expect(sut.shouldDebounce(), isFalse); + + fixture.currentTimeMs = 4000; + expect(sut.shouldDebounce(), isFalse); + }); +} + +class Fixture { + int currentTimeMs = 0; + + DateTime mockClock() => DateTime.fromMillisecondsSinceEpoch(currentTimeMs); + + Debouncer getSut({int waitTimeMs = 3000}) { + return Debouncer(mockClock, waitTimeMs: waitTimeMs); + } +}