From 77cce7cf8c0f7bf8630f5b9348250f31c104ed53 Mon Sep 17 00:00:00 2001 From: Denis Andrasec Date: Tue, 22 Oct 2024 11:30:35 +0200 Subject: [PATCH 01/21] Add debounce to `ScreenshotWidget` --- .../screenshot_event_processor.dart | 21 ++++++++ .../integrations/screenshot_integration.dart | 1 + .../screenshot_event_processor_test.dart | 54 +++++++++++++++++++ 3 files changed, 76 insertions(+) diff --git a/flutter/lib/src/event_processor/screenshot_event_processor.dart b/flutter/lib/src/event_processor/screenshot_event_processor.dart index 2b9c80dc05..355db1a3ae 100644 --- a/flutter/lib/src/event_processor/screenshot_event_processor.dart +++ b/flutter/lib/src/event_processor/screenshot_event_processor.dart @@ -19,6 +19,14 @@ class ScreenshotEventProcessor implements EventProcessor { bool get _hasSentryScreenshotWidget => sentryScreenshotWidgetGlobalKey.currentContext != null; + final _debounceDuration = Duration(milliseconds: 100); + + /// Only apply this event processor every [debounceDuration] duration. + DateTime? _lastApplyCall; + + /// The debounce duration for applying this event processor. + Duration get debounceDuration => _debounceDuration; + @override Future apply(SentryEvent event, Hint hint) async { if (event is SentryTransaction) { @@ -30,6 +38,19 @@ class ScreenshotEventProcessor implements EventProcessor { _hasSentryScreenshotWidget) { return event; } + + final now = DateTime.now(); + final difference = _lastApplyCall?.difference(DateTime.now()).abs(); + _lastApplyCall = now; + + if (difference != null && difference < debounceDuration) { + _options.logger( + SentryLevel.warning, + 'Skipping screenshot due to too many calls within a short time frame.', + ); + return event; // Debounce + } + final beforeScreenshot = _options.beforeScreenshot; if (beforeScreenshot != null) { try { diff --git a/flutter/lib/src/integrations/screenshot_integration.dart b/flutter/lib/src/integrations/screenshot_integration.dart index 10cf60228a..39eb89419d 100644 --- a/flutter/lib/src/integrations/screenshot_integration.dart +++ b/flutter/lib/src/integrations/screenshot_integration.dart @@ -1,6 +1,7 @@ import 'package:sentry/sentry.dart'; import '../event_processor/screenshot_event_processor.dart'; import '../sentry_flutter_options.dart'; +import '../utils/debouncer.dart'; /// Adds [ScreenshotEventProcessor] to options event processors if /// [SentryFlutterOptions.attachScreenshot] is true diff --git a/flutter/test/event_processor/screenshot_event_processor_test.dart b/flutter/test/event_processor/screenshot_event_processor_test.dart index f2ce37396b..0afb73a203 100644 --- a/flutter/test/event_processor/screenshot_event_processor_test.dart +++ b/flutter/test/event_processor/screenshot_event_processor_test.dart @@ -178,6 +178,60 @@ void main() { 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 { + 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 { + 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 Future.delayed(sut.debounceDuration); + await sut.apply(secondEvent, secondHint); + + expect(firstHint.screenshot, isNotNull); + expect(secondHint.screenshot, isNotNull); + }); + }); + }); } class Fixture { From f540ecbbc6ea519a547cceff35613f492b78ef7e Mon Sep 17 00:00:00 2001 From: Denis Andrasec Date: Tue, 22 Oct 2024 11:51:37 +0200 Subject: [PATCH 02/21] add changelog entry --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 38f93e41b2..bf1e18df15 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +### Enhancements + +- Add debounce to `ScreenshotWidget` ([#2368](https://github.com/getsentry/sentry-dart/pull/2368)) + ## 8.10.0-beta.2 ### Fixes From e5f628ad5813f9efe50100b0469490b6e2873f5e Mon Sep 17 00:00:00 2001 From: Denis Andrasec Date: Tue, 22 Oct 2024 12:56:21 +0200 Subject: [PATCH 03/21] provide clock in tests --- .../event_processor/screenshot_event_processor.dart | 3 ++- .../screenshot_event_processor_test.dart | 13 ++++++++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/flutter/lib/src/event_processor/screenshot_event_processor.dart b/flutter/lib/src/event_processor/screenshot_event_processor.dart index 355db1a3ae..205b889144 100644 --- a/flutter/lib/src/event_processor/screenshot_event_processor.dart +++ b/flutter/lib/src/event_processor/screenshot_event_processor.dart @@ -39,7 +39,8 @@ class ScreenshotEventProcessor implements EventProcessor { return event; } - final now = DateTime.now(); + // ignore: invalid_use_of_internal_member + final now = _options.clock(); final difference = _lastApplyCall?.difference(DateTime.now()).abs(); _lastApplyCall = now; diff --git a/flutter/test/event_processor/screenshot_event_processor_test.dart b/flutter/test/event_processor/screenshot_event_processor_test.dart index 0afb73a203..9399c64ef8 100644 --- a/flutter/test/event_processor/screenshot_event_processor_test.dart +++ b/flutter/test/event_processor/screenshot_event_processor_test.dart @@ -201,7 +201,13 @@ void main() { await sut.apply(firstEvent, firstHint); await sut.apply(secondEvent, secondHint); + // ignore: invalid_use_of_internal_member + fixture.options.clock = () => DateTime.fromMillisecondsSinceEpoch(0); expect(firstHint.screenshot, isNotNull); + + // ignore: invalid_use_of_internal_member + fixture.options.clock = () => DateTime.fromMillisecondsSinceEpoch( + sut.debounceDuration.inMilliseconds - 1); expect(secondHint.screenshot, isNull); }); }); @@ -223,8 +229,13 @@ void main() { final secondEvent = SentryEvent(throwable: throwable); final secondHint = Hint(); + // ignore: invalid_use_of_internal_member + fixture.options.clock = () => DateTime.fromMillisecondsSinceEpoch(0); await sut.apply(firstEvent, firstHint); - await Future.delayed(sut.debounceDuration); + + // ignore: invalid_use_of_internal_member + fixture.options.clock = () => DateTime.fromMillisecondsSinceEpoch( + sut.debounceDuration.inMilliseconds); await sut.apply(secondEvent, secondHint); expect(firstHint.screenshot, isNotNull); From 235de7b69953540366b29f3279eccce0ee243460 Mon Sep 17 00:00:00 2001 From: Denis Andrasec Date: Tue, 22 Oct 2024 13:37:02 +0200 Subject: [PATCH 04/21] remove unused import --- flutter/lib/src/integrations/screenshot_integration.dart | 1 - 1 file changed, 1 deletion(-) diff --git a/flutter/lib/src/integrations/screenshot_integration.dart b/flutter/lib/src/integrations/screenshot_integration.dart index 39eb89419d..10cf60228a 100644 --- a/flutter/lib/src/integrations/screenshot_integration.dart +++ b/flutter/lib/src/integrations/screenshot_integration.dart @@ -1,7 +1,6 @@ import 'package:sentry/sentry.dart'; import '../event_processor/screenshot_event_processor.dart'; import '../sentry_flutter_options.dart'; -import '../utils/debouncer.dart'; /// Adds [ScreenshotEventProcessor] to options event processors if /// [SentryFlutterOptions.attachScreenshot] is true From 09c7f9195de2e7b969f04ee9c0e778d5a262d2fd Mon Sep 17 00:00:00 2001 From: Denis Andrasec Date: Tue, 22 Oct 2024 13:42:22 +0200 Subject: [PATCH 05/21] update clock at correct line --- .../event_processor/screenshot_event_processor_test.dart | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/flutter/test/event_processor/screenshot_event_processor_test.dart b/flutter/test/event_processor/screenshot_event_processor_test.dart index 9399c64ef8..5d7f2b2551 100644 --- a/flutter/test/event_processor/screenshot_event_processor_test.dart +++ b/flutter/test/event_processor/screenshot_event_processor_test.dart @@ -198,16 +198,16 @@ void main() { final secondEvent = SentryEvent(throwable: throwable); final secondHint = Hint(); - await sut.apply(firstEvent, firstHint); - await sut.apply(secondEvent, secondHint); - // ignore: invalid_use_of_internal_member fixture.options.clock = () => DateTime.fromMillisecondsSinceEpoch(0); - expect(firstHint.screenshot, isNotNull); + await sut.apply(firstEvent, firstHint); // ignore: invalid_use_of_internal_member fixture.options.clock = () => DateTime.fromMillisecondsSinceEpoch( sut.debounceDuration.inMilliseconds - 1); + await sut.apply(secondEvent, secondHint); + + expect(firstHint.screenshot, isNotNull); expect(secondHint.screenshot, isNull); }); }); From ba0b8e775021034667ce2f715644bd0efdcd8db8 Mon Sep 17 00:00:00 2001 From: Denis Andrasec Date: Tue, 22 Oct 2024 13:49:23 +0200 Subject: [PATCH 06/21] update cl --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f2455283f5..0b54f8c117 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,8 +4,8 @@ ### Enhancements -- Add debounce to `ScreenshotWidget` ([#2368](https://github.com/getsentry/sentry-dart/pull/2368)) - Cache parsed DSN ([#2365](https://github.com/getsentry/sentry-dart/pull/2365)) +- Add debounce to `ScreenshotWidget` ([#2368](https://github.com/getsentry/sentry-dart/pull/2368)) ## 8.10.0-beta.2 From 3d489d9dae7f8f15d52b9e542f78acb86d1858db Mon Sep 17 00:00:00 2001 From: Denis Andrasec Date: Tue, 22 Oct 2024 14:19:18 +0200 Subject: [PATCH 07/21] use now from clock --- flutter/lib/src/event_processor/screenshot_event_processor.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flutter/lib/src/event_processor/screenshot_event_processor.dart b/flutter/lib/src/event_processor/screenshot_event_processor.dart index 205b889144..b2969355f4 100644 --- a/flutter/lib/src/event_processor/screenshot_event_processor.dart +++ b/flutter/lib/src/event_processor/screenshot_event_processor.dart @@ -41,7 +41,7 @@ class ScreenshotEventProcessor implements EventProcessor { // ignore: invalid_use_of_internal_member final now = _options.clock(); - final difference = _lastApplyCall?.difference(DateTime.now()).abs(); + final difference = _lastApplyCall?.difference(now).abs(); _lastApplyCall = now; if (difference != null && difference < debounceDuration) { From 3a9d843ba5efa1bcd0a6ddb1fb64cfd6e49cff67 Mon Sep 17 00:00:00 2001 From: Denis Andrasec Date: Tue, 12 Nov 2024 10:26:54 +0100 Subject: [PATCH 08/21] refactor debouncer to be similar to sentry-java impl --- flutter/lib/src/utils/debouncer.dart | 31 ++++++---- flutter/lib/src/widgets_binding_observer.dart | 17 ++++-- flutter/test/utils/debouncer_test.dart | 60 +++++++++++++++++++ 3 files changed, 92 insertions(+), 16 deletions(-) create mode 100644 flutter/test/utils/debouncer_test.dart diff --git a/flutter/lib/src/utils/debouncer.dart b/flutter/lib/src/utils/debouncer.dart index b714b41b4e..04266571b5 100644 --- a/flutter/lib/src/utils/debouncer.dart +++ b/flutter/lib/src/utils/debouncer.dart @@ -1,20 +1,31 @@ -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 clock; + final int waitTimeMs; + final bool debounceOnFirstTry; + DateTime? _lastExecutionTime; - Debouncer({required this.milliseconds}); + Debouncer(this.clock, + {this.waitTimeMs = 2000, this.debounceOnFirstTry = false}); - void run(VoidCallback action) { - _timer?.cancel(); - _timer = Timer(Duration(milliseconds: milliseconds), action); - } + bool shouldDebounce() { + final currentTime = clock(); + final lastExecutionTime = _lastExecutionTime; + _lastExecutionTime = currentTime; + + if (lastExecutionTime == null && debounceOnFirstTry) { + return true; + } + + if (lastExecutionTime != null && + currentTime.difference(lastExecutionTime).inMilliseconds < waitTimeMs) { + return true; + } - void dispose() { - _timer?.cancel(); + return false; } } diff --git a/flutter/lib/src/widgets_binding_observer.dart b/flutter/lib/src/widgets_binding_observer.dart index 7c0db1842f..23243b7224 100644 --- a/flutter/lib/src/widgets_binding_observer.dart +++ b/flutter/lib/src/widgets_binding_observer.dart @@ -25,7 +25,13 @@ class SentryWidgetsBindingObserver with WidgetsBindingObserver { required SentryFlutterOptions options, }) : _hub = hub ?? HubAdapter(), _options = options, - _screenSizeStreamController = StreamController(sync: true) { + _screenSizeStreamController = StreamController(sync: true), + // ignore: invalid_use_of_internal_member + _didChangeMetricsDebouncer = Debouncer( + options.clock, + waitTimeMs: 100, + debounceOnFirstTry: true, + ) { if (_options.enableWindowMetricBreadcrumbs) { _screenSizeStreamController.stream .map( @@ -47,12 +53,11 @@ class SentryWidgetsBindingObserver with WidgetsBindingObserver { final Hub _hub; final SentryFlutterOptions _options; + final Debouncer _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. @@ -92,11 +97,11 @@ class SentryWidgetsBindingObserver with WidgetsBindingObserver { return; } - _didChangeMetricsDebouncer.run(() { - // ignore: deprecated_member_use + final shouldDebounce = _didChangeMetricsDebouncer.shouldDebounce(); + if (!shouldDebounce) { final window = _options.bindingUtils.instance?.window; _screenSizeStreamController.add(window); - }); + } } void _onScreenSizeChanged(Map data) { diff --git a/flutter/test/utils/debouncer_test.dart b/flutter/test/utils/debouncer_test.dart new file mode 100644 index 0000000000..33d4ad11ee --- /dev/null +++ b/flutter/test/utils/debouncer_test.dart @@ -0,0 +1,60 @@ +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 per default', () { + final debouncer = fixture.getDebouncer(); + expect(debouncer.shouldDebounce(), isFalse); + }); + + test('Debouncer should debounce on the first check', () { + final debouncer = fixture.getDebouncer(debounceOnFirstTry: true); + expect(debouncer.shouldDebounce(), isTrue); + }); + + test('Debouncer should not debounce if wait time is 0', () { + final debouncer = fixture.getDebouncer(waitTimeMs: 0); + expect(debouncer.shouldDebounce(), isFalse); + expect(debouncer.shouldDebounce(), isFalse); + expect(debouncer.shouldDebounce(), isFalse); + }); + + test('Debouncer should signal debounce if the second invocation is too early', + () { + fixture.currentTimeMs = 1000; + final debouncer = fixture.getDebouncer(waitTimeMs: 3000); + expect(debouncer.shouldDebounce(), isFalse); + + fixture.currentTimeMs = 3999; + expect(debouncer.shouldDebounce(), isTrue); + }); + + test( + 'Debouncer should not signal debounce if the second invocation is late enough', + () { + fixture.currentTimeMs = 1000; + final debouncer = fixture.getDebouncer(waitTimeMs: 3000); + expect(debouncer.shouldDebounce(), isFalse); + + fixture.currentTimeMs = 4000; + expect(debouncer.shouldDebounce(), isFalse); + }); +} + +class Fixture { + int currentTimeMs = 0; + + DateTime mockClock() => DateTime.fromMillisecondsSinceEpoch(currentTimeMs); + + Debouncer getDebouncer( + {int waitTimeMs = 3000, bool debounceOnFirstTry = false}) { + return Debouncer(mockClock, + waitTimeMs: waitTimeMs, debounceOnFirstTry: debounceOnFirstTry); + } +} From e71009853856b0ef401cf0f7ddf8ca619e802f08 Mon Sep 17 00:00:00 2001 From: Denis Andrasec Date: Tue, 12 Nov 2024 10:42:46 +0100 Subject: [PATCH 09/21] use debouncer in screenshot_event_processor --- .../screenshot_event_processor.dart | 35 ++++++++----------- flutter/lib/src/utils/debouncer.dart | 6 ++-- flutter/lib/src/widgets_binding_observer.dart | 2 +- .../screenshot_event_processor_test.dart | 34 +++++++++++------- 4 files changed, 40 insertions(+), 37 deletions(-) diff --git a/flutter/lib/src/event_processor/screenshot_event_processor.dart b/flutter/lib/src/event_processor/screenshot_event_processor.dart index c5cbc734b2..f36019d2e3 100644 --- a/flutter/lib/src/event_processor/screenshot_event_processor.dart +++ b/flutter/lib/src/event_processor/screenshot_event_processor.dart @@ -11,18 +11,19 @@ import '../sentry_flutter_options.dart'; import '../renderer/renderer.dart'; import 'package:flutter/widgets.dart' as widget; +import '../utils/debouncer.dart'; + class ScreenshotEventProcessor implements EventProcessor { final SentryFlutterOptions _options; - - ScreenshotEventProcessor(this._options); - - final _debounceDuration = Duration(milliseconds: 100); - - /// Only apply this event processor every [debounceDuration] duration. - DateTime? _lastApplyCall; - - /// The debounce duration for applying this event processor. - Duration get debounceDuration => _debounceDuration; + final Debouncer _debouncer; + + ScreenshotEventProcessor(this._options) + : _debouncer = Debouncer( + // ignore: invalid_use_of_internal_member + _options.clock, + waitTimeMs: 2000, + debounceOnFirstTry: false, + ); @override Future apply(SentryEvent event, Hint hint) async { @@ -40,17 +41,9 @@ class ScreenshotEventProcessor implements EventProcessor { return event; // No need to attach screenshot of feedback form. } - // ignore: invalid_use_of_internal_member - final now = _options.clock(); - final difference = _lastApplyCall?.difference(now).abs(); - _lastApplyCall = now; - - if (difference != null && difference < debounceDuration) { - _options.logger( - SentryLevel.warning, - 'Skipping screenshot due to too many calls within a short time frame.', - ); - return event; // Debounce + final shouldDebounce = _debouncer.shouldDebounce(); + if (shouldDebounce) { + return event; } final beforeScreenshot = _options.beforeScreenshot; diff --git a/flutter/lib/src/utils/debouncer.dart b/flutter/lib/src/utils/debouncer.dart index 04266571b5..4716e7a888 100644 --- a/flutter/lib/src/utils/debouncer.dart +++ b/flutter/lib/src/utils/debouncer.dart @@ -4,16 +4,16 @@ import 'package:sentry/sentry.dart'; @internal class Debouncer { - final ClockProvider clock; + final ClockProvider clockProvider; final int waitTimeMs; final bool debounceOnFirstTry; DateTime? _lastExecutionTime; - Debouncer(this.clock, + Debouncer(this.clockProvider, {this.waitTimeMs = 2000, this.debounceOnFirstTry = false}); bool shouldDebounce() { - final currentTime = clock(); + final currentTime = clockProvider(); final lastExecutionTime = _lastExecutionTime; _lastExecutionTime = currentTime; diff --git a/flutter/lib/src/widgets_binding_observer.dart b/flutter/lib/src/widgets_binding_observer.dart index 23243b7224..660f20421c 100644 --- a/flutter/lib/src/widgets_binding_observer.dart +++ b/flutter/lib/src/widgets_binding_observer.dart @@ -26,8 +26,8 @@ class SentryWidgetsBindingObserver with WidgetsBindingObserver { }) : _hub = hub ?? HubAdapter(), _options = options, _screenSizeStreamController = StreamController(sync: true), - // ignore: invalid_use_of_internal_member _didChangeMetricsDebouncer = Debouncer( + // ignore: invalid_use_of_internal_member options.clock, waitTimeMs: 100, debounceOnFirstTry: true, diff --git a/flutter/test/event_processor/screenshot_event_processor_test.dart b/flutter/test/event_processor/screenshot_event_processor_test.dart index a78178a659..de6e01e0b8 100644 --- a/flutter/test/event_processor/screenshot_event_processor_test.dart +++ b/flutter/test/event_processor/screenshot_event_processor_test.dart @@ -214,6 +214,17 @@ void main() { (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( @@ -228,13 +239,7 @@ void main() { final secondEvent = SentryEvent(throwable: throwable); final secondHint = Hint(); - // ignore: invalid_use_of_internal_member - fixture.options.clock = () => DateTime.fromMillisecondsSinceEpoch(0); await sut.apply(firstEvent, firstHint); - - // ignore: invalid_use_of_internal_member - fixture.options.clock = () => DateTime.fromMillisecondsSinceEpoch( - sut.debounceDuration.inMilliseconds - 1); await sut.apply(secondEvent, secondHint); expect(firstHint.screenshot, isNotNull); @@ -245,6 +250,17 @@ void main() { 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( @@ -259,13 +275,7 @@ void main() { final secondEvent = SentryEvent(throwable: throwable); final secondHint = Hint(); - // ignore: invalid_use_of_internal_member - fixture.options.clock = () => DateTime.fromMillisecondsSinceEpoch(0); await sut.apply(firstEvent, firstHint); - - // ignore: invalid_use_of_internal_member - fixture.options.clock = () => DateTime.fromMillisecondsSinceEpoch( - sut.debounceDuration.inMilliseconds); await sut.apply(secondEvent, secondHint); expect(firstHint.screenshot, isNotNull); From 161f18264fa6c055b3bedb1fb93a3d4961ac7b1a Mon Sep 17 00:00:00 2001 From: Denis Andrasec Date: Tue, 12 Nov 2024 11:12:34 +0100 Subject: [PATCH 10/21] Change beforeScreenshot to beforeCapture callback --- .../screenshot_event_processor.dart | 10 +-- flutter/lib/src/sentry_flutter_options.dart | 34 ++++++++-- .../screenshot_event_processor_test.dart | 66 ++++++++++++++++--- 3 files changed, 88 insertions(+), 22 deletions(-) diff --git a/flutter/lib/src/event_processor/screenshot_event_processor.dart b/flutter/lib/src/event_processor/screenshot_event_processor.dart index f36019d2e3..38fbc57435 100644 --- a/flutter/lib/src/event_processor/screenshot_event_processor.dart +++ b/flutter/lib/src/event_processor/screenshot_event_processor.dart @@ -41,15 +41,13 @@ 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(); - if (shouldDebounce) { - return event; - } - final beforeScreenshot = _options.beforeScreenshot; if (beforeScreenshot != null) { try { - final result = beforeScreenshot(event, hint: hint); + final result = beforeScreenshot(event, hint, shouldDebounce); bool takeScreenshot; if (result is Future) { takeScreenshot = await result; @@ -70,6 +68,8 @@ class ScreenshotEventProcessor implements EventProcessor { rethrow; } } + } else if (shouldDebounce) { + return event; } final renderer = _options.rendererWrapper.getRenderer(); diff --git a/flutter/lib/src/sentry_flutter_options.dart b/flutter/lib/src/sentry_flutter_options.dart index 23aefdd849..590a3feca0 100644 --- a/flutter/lib/src/sentry_flutter_options.dart +++ b/flutter/lib/src/sentry_flutter_options.dart @@ -198,7 +198,7 @@ class SentryFlutterOptions extends SentryOptions { /// 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? beforeScreenshot; /// Enable or disable automatic breadcrumbs for User interactions Using [Listener] /// @@ -387,9 +387,29 @@ class _SentryFlutterExperimentalOptions { final replay = SentryReplayOptions(); } -/// Callback being executed in [ScreenshotEventProcessor], deciding if a -/// screenshot should be recorded and attached. -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/test/event_processor/screenshot_event_processor_test.dart b/flutter/test/event_processor/screenshot_event_processor_test.dart index de6e01e0b8..7b7b0f8c2a 100644 --- a/flutter/test/event_processor/screenshot_event_processor_test.dart +++ b/flutter/test/event_processor/screenshot_event_processor_test.dart @@ -129,7 +129,8 @@ void main() { group('beforeScreenshot', () { testWidgets('does add screenshot if beforeScreenshot returns true', (tester) async { - fixture.options.beforeScreenshot = (SentryEvent event, {Hint? hint}) { + fixture.options.beforeScreenshot = + (SentryEvent event, Hint hint, bool shouldDebounce) { return true; }; await _addScreenshotAttachment(tester, FlutterRenderer.canvasKit, @@ -139,7 +140,7 @@ void main() { testWidgets('does add screenshot if async beforeScreenshot returns true', (tester) async { fixture.options.beforeScreenshot = - (SentryEvent event, {Hint? hint}) async { + (SentryEvent event, Hint hint, bool shouldDebounce) async { await Future.delayed(Duration(milliseconds: 1)); return true; }; @@ -149,7 +150,8 @@ void main() { testWidgets('does not add screenshot if beforeScreenshot returns false', (tester) async { - fixture.options.beforeScreenshot = (SentryEvent event, {Hint? hint}) { + fixture.options.beforeScreenshot = + (SentryEvent event, Hint hint, bool shouldDebounce) { return false; }; await _addScreenshotAttachment(tester, FlutterRenderer.canvasKit, @@ -160,7 +162,7 @@ void main() { 'does not add screenshot if async beforeScreenshot returns false', (tester) async { fixture.options.beforeScreenshot = - (SentryEvent event, {Hint? hint}) async { + (SentryEvent event, Hint hint, bool shouldDebounce) async { await Future.delayed(Duration(milliseconds: 1)); return false; }; @@ -171,7 +173,8 @@ void main() { testWidgets('does add screenshot if beforeScreenshot throws', (tester) async { fixture.options.automatedTestMode = false; - fixture.options.beforeScreenshot = (SentryEvent event, {Hint? hint}) { + fixture.options.beforeScreenshot = + (SentryEvent event, Hint hint, bool shouldDebounce) { throw Error(); }; await _addScreenshotAttachment(tester, FlutterRenderer.canvasKit, @@ -182,7 +185,7 @@ void main() { (tester) async { fixture.options.automatedTestMode = false; fixture.options.beforeScreenshot = - (SentryEvent event, {Hint? hint}) async { + (SentryEvent event, Hint hint, bool shouldDebounce) async { await Future.delayed(Duration(milliseconds: 1)); throw Error(); }; @@ -190,12 +193,50 @@ void main() { added: true, isWeb: false); }); + testWidgets('does add screenshot event if shouldDebounce true', + (tester) async { + await tester.runAsync(() async { + var shouldDebounceValues = []; + + fixture.options.beforeScreenshot = + (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 beforeScreenshot callback', (tester) async { SentryEvent? beforeScreenshotEvent; Hint? beforeScreenshotHint; - fixture.options.beforeScreenshot = (SentryEvent event, {Hint? hint}) { + fixture.options.beforeScreenshot = + (SentryEvent event, Hint hint, bool shouldDebounce) { beforeScreenshotEvent = event; beforeScreenshotHint = hint; return true; @@ -263,9 +304,14 @@ void main() { final sut = fixture.getSut(FlutterRenderer.canvasKit, false); - await tester.pumpWidget(SentryScreenshotWidget( - child: Text('Catching Pokémon is a snap!', - textDirection: TextDirection.ltr))); + await tester.pumpWidget( + SentryScreenshotWidget( + child: Text( + 'Catching Pokémon is a snap!', + textDirection: TextDirection.ltr, + ), + ), + ); final throwable = Exception(); From d6b74f21e7548ed0ad8ac9502b3d20b6f3900c71 Mon Sep 17 00:00:00 2001 From: Denis Andrasec Date: Tue, 12 Nov 2024 11:15:16 +0100 Subject: [PATCH 11/21] update changelog entry --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2367c9c673..23d28d6ceb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,8 @@ - SDK creates a synthetic trace using `StackTrace.current` - Internal SDK frames are removed to reduce noise - Original stack traces (when provided) are left unchanged +- Add debounce to `ScreenshotWidget` ([#2368](https://github.com/getsentry/sentry-dart/pull/2368)) + - This is a breaking change. Replace `BeforeScreenshotCallback` with new `BeforeCaptureCallback`. ### Features @@ -114,7 +116,6 @@ - Avoid sending too many empty client reports when Http Transport is used ([#2380](https://github.com/getsentry/sentry-dart/pull/2380)) - Cache parsed DSN ([#2365](https://github.com/getsentry/sentry-dart/pull/2365)) -- Add debounce to `ScreenshotWidget` ([#2368](https://github.com/getsentry/sentry-dart/pull/2368)) - Handle backpressure earlier in pipeline ([#2371](https://github.com/getsentry/sentry-dart/pull/2371)) - Drops max un-awaited parallel tasks earlier, so event processors & callbacks are not executed for them. - Change by setting `SentryOptions.maxQueueSize`. Default is 30. From 570b1aaa9a9035ee7b7f88092e79fac74dcf36e6 Mon Sep 17 00:00:00 2001 From: Denis Andrasec Date: Tue, 12 Nov 2024 11:17:24 +0100 Subject: [PATCH 12/21] format --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 23d28d6ceb..ad8046e3a8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,7 @@ - Original stack traces (when provided) are left unchanged - Add debounce to `ScreenshotWidget` ([#2368](https://github.com/getsentry/sentry-dart/pull/2368)) - This is a breaking change. Replace `BeforeScreenshotCallback` with new `BeforeCaptureCallback`. - + ### Features - Improve frame tracking accuracy ([#2372](https://github.com/getsentry/sentry-dart/pull/2372)) From fbea7d715faaecc6d97a2828d131dd27a11ce207 Mon Sep 17 00:00:00 2001 From: Denis Andrasec Date: Tue, 12 Nov 2024 11:20:21 +0100 Subject: [PATCH 13/21] fix analyze issue --- flutter/lib/src/utils/debouncer.dart | 1 - 1 file changed, 1 deletion(-) diff --git a/flutter/lib/src/utils/debouncer.dart b/flutter/lib/src/utils/debouncer.dart index 4716e7a888..a2a4a64f99 100644 --- a/flutter/lib/src/utils/debouncer.dart +++ b/flutter/lib/src/utils/debouncer.dart @@ -1,4 +1,3 @@ -import 'package:flutter/foundation.dart'; import 'package:meta/meta.dart'; import 'package:sentry/sentry.dart'; From ad5a9cec4920c71f5a40e0c686ec8081b766d669 Mon Sep 17 00:00:00 2001 From: Denis Andrasec Date: Wed, 13 Nov 2024 11:47:03 +0100 Subject: [PATCH 14/21] feedback --- .../screenshot_event_processor.dart | 4 +++ flutter/test/utils/debouncer_test.dart | 31 +++++++++---------- 2 files changed, 19 insertions(+), 16 deletions(-) diff --git a/flutter/lib/src/event_processor/screenshot_event_processor.dart b/flutter/lib/src/event_processor/screenshot_event_processor.dart index 38fbc57435..3097140fed 100644 --- a/flutter/lib/src/event_processor/screenshot_event_processor.dart +++ b/flutter/lib/src/event_processor/screenshot_event_processor.dart @@ -69,6 +69,10 @@ class ScreenshotEventProcessor implements EventProcessor { } } } else if (shouldDebounce) { + _options.logger( + SentryLevel.debug, + 'Skipping screenshot capture due to debouncing (too many captures within ${_debouncer.waitTimeMs}ms)', + ); return event; } diff --git a/flutter/test/utils/debouncer_test.dart b/flutter/test/utils/debouncer_test.dart index 33d4ad11ee..c63d749b4f 100644 --- a/flutter/test/utils/debouncer_test.dart +++ b/flutter/test/utils/debouncer_test.dart @@ -9,41 +9,41 @@ void main() { }); test('Debouncer should not debounce on the first check per default', () { - final debouncer = fixture.getDebouncer(); - expect(debouncer.shouldDebounce(), isFalse); + final sut = fixture.getSut(); + expect(sut.shouldDebounce(), isFalse); }); test('Debouncer should debounce on the first check', () { - final debouncer = fixture.getDebouncer(debounceOnFirstTry: true); - expect(debouncer.shouldDebounce(), isTrue); + final sut = fixture.getSut(debounceOnFirstTry: true); + expect(sut.shouldDebounce(), isTrue); }); test('Debouncer should not debounce if wait time is 0', () { - final debouncer = fixture.getDebouncer(waitTimeMs: 0); - expect(debouncer.shouldDebounce(), isFalse); - expect(debouncer.shouldDebounce(), isFalse); - expect(debouncer.shouldDebounce(), isFalse); + 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 debouncer = fixture.getDebouncer(waitTimeMs: 3000); - expect(debouncer.shouldDebounce(), isFalse); + final sut = fixture.getSut(waitTimeMs: 3000); + expect(sut.shouldDebounce(), isFalse); fixture.currentTimeMs = 3999; - expect(debouncer.shouldDebounce(), isTrue); + expect(sut.shouldDebounce(), isTrue); }); test( 'Debouncer should not signal debounce if the second invocation is late enough', () { fixture.currentTimeMs = 1000; - final debouncer = fixture.getDebouncer(waitTimeMs: 3000); - expect(debouncer.shouldDebounce(), isFalse); + final sut = fixture.getSut(waitTimeMs: 3000); + expect(sut.shouldDebounce(), isFalse); fixture.currentTimeMs = 4000; - expect(debouncer.shouldDebounce(), isFalse); + expect(sut.shouldDebounce(), isFalse); }); } @@ -52,8 +52,7 @@ class Fixture { DateTime mockClock() => DateTime.fromMillisecondsSinceEpoch(currentTimeMs); - Debouncer getDebouncer( - {int waitTimeMs = 3000, bool debounceOnFirstTry = false}) { + Debouncer getSut({int waitTimeMs = 3000, bool debounceOnFirstTry = false}) { return Debouncer(mockClock, waitTimeMs: waitTimeMs, debounceOnFirstTry: debounceOnFirstTry); } From 57087b191b0969695943e30c3e46b1a2287a2fe0 Mon Sep 17 00:00:00 2001 From: Denis Andrasec Date: Wed, 13 Nov 2024 11:58:06 +0100 Subject: [PATCH 15/21] recert timer impl for widgets observer --- .../screenshot_event_processor.dart | 1 - flutter/lib/src/utils/debouncer.dart | 8 +------- flutter/lib/src/utils/timer_debouncer.dart | 20 +++++++++++++++++++ flutter/lib/src/widgets_binding_observer.dart | 17 +++++----------- flutter/test/utils/debouncer_test.dart | 12 +++-------- 5 files changed, 29 insertions(+), 29 deletions(-) create mode 100644 flutter/lib/src/utils/timer_debouncer.dart diff --git a/flutter/lib/src/event_processor/screenshot_event_processor.dart b/flutter/lib/src/event_processor/screenshot_event_processor.dart index 3097140fed..045ab0c0e9 100644 --- a/flutter/lib/src/event_processor/screenshot_event_processor.dart +++ b/flutter/lib/src/event_processor/screenshot_event_processor.dart @@ -22,7 +22,6 @@ class ScreenshotEventProcessor implements EventProcessor { // ignore: invalid_use_of_internal_member _options.clock, waitTimeMs: 2000, - debounceOnFirstTry: false, ); @override diff --git a/flutter/lib/src/utils/debouncer.dart b/flutter/lib/src/utils/debouncer.dart index a2a4a64f99..d3027d8849 100644 --- a/flutter/lib/src/utils/debouncer.dart +++ b/flutter/lib/src/utils/debouncer.dart @@ -5,21 +5,15 @@ import 'package:sentry/sentry.dart'; class Debouncer { final ClockProvider clockProvider; final int waitTimeMs; - final bool debounceOnFirstTry; DateTime? _lastExecutionTime; - Debouncer(this.clockProvider, - {this.waitTimeMs = 2000, this.debounceOnFirstTry = false}); + Debouncer(this.clockProvider, {this.waitTimeMs = 2000}); bool shouldDebounce() { final currentTime = clockProvider(); final lastExecutionTime = _lastExecutionTime; _lastExecutionTime = currentTime; - if (lastExecutionTime == null && debounceOnFirstTry) { - return true; - } - if (lastExecutionTime != null && currentTime.difference(lastExecutionTime).inMilliseconds < waitTimeMs) { return true; 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 660f20421c..5dcfd2661e 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. @@ -26,12 +26,7 @@ class SentryWidgetsBindingObserver with WidgetsBindingObserver { }) : _hub = hub ?? HubAdapter(), _options = options, _screenSizeStreamController = StreamController(sync: true), - _didChangeMetricsDebouncer = Debouncer( - // ignore: invalid_use_of_internal_member - options.clock, - waitTimeMs: 100, - debounceOnFirstTry: true, - ) { + _didChangeMetricsDebouncer = TimerDebouncer(milliseconds: 100) { if (_options.enableWindowMetricBreadcrumbs) { _screenSizeStreamController.stream .map( @@ -53,7 +48,7 @@ class SentryWidgetsBindingObserver with WidgetsBindingObserver { final Hub _hub; final SentryFlutterOptions _options; - final Debouncer _didChangeMetricsDebouncer; + final TimerDebouncer _didChangeMetricsDebouncer; // ignore: deprecated_member_use final StreamController _screenSizeStreamController; @@ -96,12 +91,10 @@ class SentryWidgetsBindingObserver with WidgetsBindingObserver { if (!_options.enableWindowMetricBreadcrumbs) { return; } - - final shouldDebounce = _didChangeMetricsDebouncer.shouldDebounce(); - if (!shouldDebounce) { + _didChangeMetricsDebouncer.run(() { final window = _options.bindingUtils.instance?.window; _screenSizeStreamController.add(window); - } + }); } void _onScreenSizeChanged(Map data) { diff --git a/flutter/test/utils/debouncer_test.dart b/flutter/test/utils/debouncer_test.dart index c63d749b4f..b78987d360 100644 --- a/flutter/test/utils/debouncer_test.dart +++ b/flutter/test/utils/debouncer_test.dart @@ -8,16 +8,11 @@ void main() { fixture = Fixture(); }); - test('Debouncer should not debounce on the first check per default', () { + test('Debouncer should not debounce on the first check', () { final sut = fixture.getSut(); expect(sut.shouldDebounce(), isFalse); }); - test('Debouncer should debounce on the first check', () { - final sut = fixture.getSut(debounceOnFirstTry: true); - expect(sut.shouldDebounce(), isTrue); - }); - test('Debouncer should not debounce if wait time is 0', () { final sut = fixture.getSut(waitTimeMs: 0); expect(sut.shouldDebounce(), isFalse); @@ -52,8 +47,7 @@ class Fixture { DateTime mockClock() => DateTime.fromMillisecondsSinceEpoch(currentTimeMs); - Debouncer getSut({int waitTimeMs = 3000, bool debounceOnFirstTry = false}) { - return Debouncer(mockClock, - waitTimeMs: waitTimeMs, debounceOnFirstTry: debounceOnFirstTry); + Debouncer getSut({int waitTimeMs = 3000}) { + return Debouncer(mockClock, waitTimeMs: waitTimeMs); } } From 1f5f85c3321e33df99c7468d1d729a044bd5345f Mon Sep 17 00:00:00 2001 From: Denis Andrasec Date: Wed, 13 Nov 2024 12:14:10 +0100 Subject: [PATCH 16/21] keep before screenshot callback --- .../screenshot_event_processor.dart | 53 +++++---- flutter/lib/src/sentry_flutter_options.dart | 11 +- .../screenshot_event_processor_test.dart | 103 ++++++++++++++++-- 3 files changed, 134 insertions(+), 33 deletions(-) diff --git a/flutter/lib/src/event_processor/screenshot_event_processor.dart b/flutter/lib/src/event_processor/screenshot_event_processor.dart index 045ab0c0e9..ac5a57a6ce 100644 --- a/flutter/lib/src/event_processor/screenshot_event_processor.dart +++ b/flutter/lib/src/event_processor/screenshot_event_processor.dart @@ -44,35 +44,46 @@ class ScreenshotEventProcessor implements EventProcessor { // the BeforeCaptureCallback may overrules the debouncing decision final shouldDebounce = _debouncer.shouldDebounce(); final beforeScreenshot = _options.beforeScreenshot; - if (beforeScreenshot != null) { - try { - final result = beforeScreenshot(event, hint, shouldDebounce); - bool takeScreenshot; + final beforeCapture = _options.beforeCapture; + + try { + FutureOr? result; + + if (beforeCapture != null) { + result = beforeCapture(event, hint, shouldDebounce); + } else if (beforeScreenshot != null) { + result = beforeScreenshot(event, hint: hint); + } else if (shouldDebounce) { + _options.logger( + SentryLevel.debug, + 'Skipping screenshot capture due to debouncing (too many captures within ${_debouncer.waitTimeMs}ms)', + ); + return event; + } + + bool takeScreenshot = true; + + if (result != null) { if (result is Future) { takeScreenshot = await result; } else { takeScreenshot = result; } - if (!takeScreenshot) { - return event; - } - } catch (exception, stackTrace) { - _options.logger( - SentryLevel.error, - 'The beforeScreenshot callback threw an exception', - exception: exception, - stackTrace: stackTrace, - ); - if (_options.automatedTestMode) { - rethrow; - } } - } else if (shouldDebounce) { + + if (!takeScreenshot) { + return event; + } + } catch (exception, stackTrace) { _options.logger( - SentryLevel.debug, - 'Skipping screenshot capture due to debouncing (too many captures within ${_debouncer.waitTimeMs}ms)', + SentryLevel.error, + 'The beforeScreenshot callback threw an exception', + exception: exception, + stackTrace: stackTrace, ); - return event; + if (_options.automatedTestMode) { + rethrow; + } } final renderer = _options.rendererWrapper.getRenderer(); diff --git a/flutter/lib/src/sentry_flutter_options.dart b/flutter/lib/src/sentry_flutter_options.dart index 590a3feca0..cc307bff73 100644 --- a/flutter/lib/src/sentry_flutter_options.dart +++ b/flutter/lib/src/sentry_flutter_options.dart @@ -195,10 +195,14 @@ class SentryFlutterOptions extends SentryOptions { /// Only attach a screenshot when the app is resumed. bool attachScreenshotOnlyWhenResumed = false; + @Deprecated( + 'Will be removed in a future version. Use [beforeCapture] 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. - BeforeCaptureCallback? beforeScreenshot; + BeforeCaptureCallback? beforeCapture; /// Enable or disable automatic breadcrumbs for User interactions Using [Listener] /// @@ -387,6 +391,11 @@ class _SentryFlutterExperimentalOptions { final replay = SentryReplayOptions(); } +@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, diff --git a/flutter/test/event_processor/screenshot_event_processor_test.dart b/flutter/test/event_processor/screenshot_event_processor_test.dart index 7b7b0f8c2a..7028a02bca 100644 --- a/flutter/test/event_processor/screenshot_event_processor_test.dart +++ b/flutter/test/event_processor/screenshot_event_processor_test.dart @@ -129,8 +129,7 @@ void main() { group('beforeScreenshot', () { testWidgets('does add screenshot if beforeScreenshot returns true', (tester) async { - fixture.options.beforeScreenshot = - (SentryEvent event, Hint hint, bool shouldDebounce) { + fixture.options.beforeScreenshot = (SentryEvent event, {Hint? hint}) { return true; }; await _addScreenshotAttachment(tester, FlutterRenderer.canvasKit, @@ -140,7 +139,7 @@ void main() { testWidgets('does add screenshot if async beforeScreenshot returns true', (tester) async { fixture.options.beforeScreenshot = - (SentryEvent event, Hint hint, bool shouldDebounce) async { + (SentryEvent event, {Hint? hint}) async { await Future.delayed(Duration(milliseconds: 1)); return true; }; @@ -150,8 +149,7 @@ void main() { testWidgets('does not add screenshot if beforeScreenshot returns false', (tester) async { - fixture.options.beforeScreenshot = - (SentryEvent event, Hint hint, bool shouldDebounce) { + fixture.options.beforeScreenshot = (SentryEvent event, {Hint? hint}) { return false; }; await _addScreenshotAttachment(tester, FlutterRenderer.canvasKit, @@ -162,7 +160,7 @@ void main() { 'does not add screenshot if async beforeScreenshot returns false', (tester) async { fixture.options.beforeScreenshot = - (SentryEvent event, Hint hint, bool shouldDebounce) async { + (SentryEvent event, {Hint? hint}) async { await Future.delayed(Duration(milliseconds: 1)); return false; }; @@ -173,8 +171,7 @@ void main() { testWidgets('does add screenshot if beforeScreenshot throws', (tester) async { fixture.options.automatedTestMode = false; - fixture.options.beforeScreenshot = - (SentryEvent event, Hint hint, bool shouldDebounce) { + fixture.options.beforeScreenshot = (SentryEvent event, {Hint? hint}) { throw Error(); }; await _addScreenshotAttachment(tester, FlutterRenderer.canvasKit, @@ -185,6 +182,90 @@ void main() { (tester) async { fixture.options.automatedTestMode = false; fixture.options.beforeScreenshot = + (SentryEvent event, {Hint? hint}) async { + await Future.delayed(Duration(milliseconds: 1)); + throw Error(); + }; + await _addScreenshotAttachment(tester, FlutterRenderer.canvasKit, + added: true, isWeb: false); + }); + + testWidgets('passes event & hint to beforeScreenshot callback', + (tester) async { + SentryEvent? beforeScreenshotEvent; + Hint? beforeScreenshotHint; + + fixture.options.beforeScreenshot = (SentryEvent event, {Hint? hint}) { + beforeScreenshotEvent = event; + beforeScreenshotHint = hint; + return true; + }; + + await _addScreenshotAttachment(tester, FlutterRenderer.canvasKit, + added: true, isWeb: false); + + expect(beforeScreenshotEvent, event); + expect(beforeScreenshotHint, hint); + }); + }); + + group('beforeCapture', () { + testWidgets('does add screenshot if beforeCapture returns true', + (tester) async { + fixture.options.beforeCapture = + (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.beforeCapture = + (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.beforeCapture = + (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.beforeCapture = + (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.beforeCapture = + (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.beforeCapture = (SentryEvent event, Hint hint, bool shouldDebounce) async { await Future.delayed(Duration(milliseconds: 1)); throw Error(); @@ -198,7 +279,7 @@ void main() { await tester.runAsync(() async { var shouldDebounceValues = []; - fixture.options.beforeScreenshot = + fixture.options.beforeCapture = (SentryEvent event, Hint hint, bool shouldDebounce) { shouldDebounceValues.add(shouldDebounce); return true; @@ -230,12 +311,12 @@ void main() { }); }); - testWidgets('passes event & hint to beforeScreenshot callback', + testWidgets('passes event & hint to beforeCapture callback', (tester) async { SentryEvent? beforeScreenshotEvent; Hint? beforeScreenshotHint; - fixture.options.beforeScreenshot = + fixture.options.beforeCapture = (SentryEvent event, Hint hint, bool shouldDebounce) { beforeScreenshotEvent = event; beforeScreenshotHint = hint; From cfafff0550b21406f178e3ebe20e24856f5d8ae6 Mon Sep 17 00:00:00 2001 From: Denis Andrasec Date: Wed, 13 Nov 2024 12:15:00 +0100 Subject: [PATCH 17/21] update cl --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ad8046e3a8..2257e68df9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ - Internal SDK frames are removed to reduce noise - Original stack traces (when provided) are left unchanged - Add debounce to `ScreenshotWidget` ([#2368](https://github.com/getsentry/sentry-dart/pull/2368)) - - This is a breaking change. Replace `BeforeScreenshotCallback` with new `BeforeCaptureCallback`. + - Replace deprecated `BeforeScreenshotCallback` with new `BeforeCaptureCallback`. ### Features From eea29fcb763409fc2f97043d60e02ebe4af2f074 Mon Sep 17 00:00:00 2001 From: Denis Andrasec Date: Wed, 13 Nov 2024 12:16:33 +0100 Subject: [PATCH 18/21] update error msg --- flutter/lib/src/event_processor/screenshot_event_processor.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flutter/lib/src/event_processor/screenshot_event_processor.dart b/flutter/lib/src/event_processor/screenshot_event_processor.dart index ac5a57a6ce..da7e5fddb9 100644 --- a/flutter/lib/src/event_processor/screenshot_event_processor.dart +++ b/flutter/lib/src/event_processor/screenshot_event_processor.dart @@ -77,7 +77,7 @@ class ScreenshotEventProcessor implements EventProcessor { } catch (exception, stackTrace) { _options.logger( SentryLevel.error, - 'The beforeScreenshot callback threw an exception', + 'The beforeCapture/beforeScreenshot callback threw an exception', exception: exception, stackTrace: stackTrace, ); From 006839e7300ea2c439c2c9cb5888927b15d946a0 Mon Sep 17 00:00:00 2001 From: Denis Andrasec Date: Wed, 13 Nov 2024 12:18:01 +0100 Subject: [PATCH 19/21] refactor --- .../event_processor/screenshot_event_processor.dart | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/flutter/lib/src/event_processor/screenshot_event_processor.dart b/flutter/lib/src/event_processor/screenshot_event_processor.dart index da7e5fddb9..2711d36fca 100644 --- a/flutter/lib/src/event_processor/screenshot_event_processor.dart +++ b/flutter/lib/src/event_processor/screenshot_event_processor.dart @@ -53,12 +53,6 @@ class ScreenshotEventProcessor implements EventProcessor { result = beforeCapture(event, hint, shouldDebounce); } else if (beforeScreenshot != null) { result = beforeScreenshot(event, hint: hint); - } else if (shouldDebounce) { - _options.logger( - SentryLevel.debug, - 'Skipping screenshot capture due to debouncing (too many captures within ${_debouncer.waitTimeMs}ms)', - ); - return event; } bool takeScreenshot = true; @@ -69,6 +63,12 @@ class ScreenshotEventProcessor implements EventProcessor { } else { takeScreenshot = result; } + } else if (shouldDebounce) { + _options.logger( + SentryLevel.debug, + 'Skipping screenshot capture due to debouncing (too many captures within ${_debouncer.waitTimeMs}ms)', + ); + takeScreenshot = false; } if (!takeScreenshot) { From 2e43ad3f4f2285a2d8e05342ba18df65150d811a Mon Sep 17 00:00:00 2001 From: Denis Andrasec Date: Wed, 13 Nov 2024 12:25:00 +0100 Subject: [PATCH 20/21] add warning --- flutter/lib/src/event_processor/screenshot_event_processor.dart | 2 ++ 1 file changed, 2 insertions(+) diff --git a/flutter/lib/src/event_processor/screenshot_event_processor.dart b/flutter/lib/src/event_processor/screenshot_event_processor.dart index 2711d36fca..7b97071ed9 100644 --- a/flutter/lib/src/event_processor/screenshot_event_processor.dart +++ b/flutter/lib/src/event_processor/screenshot_event_processor.dart @@ -43,6 +43,8 @@ class ScreenshotEventProcessor implements EventProcessor { // 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; final beforeCapture = _options.beforeCapture; From e96bb8da03ed175ff22a28293107131f8ec3e49a Mon Sep 17 00:00:00 2001 From: Denis Andrasec Date: Wed, 13 Nov 2024 12:27:19 +0100 Subject: [PATCH 21/21] add ignores for analyzer --- flutter/lib/src/widgets_binding_observer.dart | 1 + .../event_processor/screenshot_event_processor_test.dart | 7 +++++++ 2 files changed, 8 insertions(+) diff --git a/flutter/lib/src/widgets_binding_observer.dart b/flutter/lib/src/widgets_binding_observer.dart index 5dcfd2661e..525027f328 100644 --- a/flutter/lib/src/widgets_binding_observer.dart +++ b/flutter/lib/src/widgets_binding_observer.dart @@ -92,6 +92,7 @@ class SentryWidgetsBindingObserver with WidgetsBindingObserver { return; } _didChangeMetricsDebouncer.run(() { + // ignore: deprecated_member_use final window = _options.bindingUtils.instance?.window; _screenSizeStreamController.add(window); }); diff --git a/flutter/test/event_processor/screenshot_event_processor_test.dart b/flutter/test/event_processor/screenshot_event_processor_test.dart index 7028a02bca..c4a5075266 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;