From e44284713d5ef0ec79e405736fff81e3f3b8227a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Denis=20Andra=C5=A1ec?= Date: Thu, 2 Jan 2025 12:38:46 +0100 Subject: [PATCH] Add `beforeCapture` for View Hierarchy (#2523) --- CHANGELOG.md | 5 + .../screenshot_event_processor.dart | 8 +- flutter/lib/src/sentry_flutter_options.dart | 8 +- flutter/lib/src/utils/debouncer.dart | 7 +- .../view_hierarchy_event_processor.dart | 65 +++++- flutter/test/utils/debouncer_test.dart | 2 +- .../view_hierarchy_event_processor_test.dart | 216 +++++++++++++++++- 7 files changed, 289 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 58239ff20a..e36342380a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,11 @@ ## Unreleased +### Features + +- Add `beforeCapture` for View Hierarchy ([#2523](https://github.com/getsentry/sentry-dart/pull/2523)) + - View hierarchy calls are now debounced for 2 seconds. + ### Enhancements - Replay: improve performance of screenshot data to native recorder ([#2530](https://github.com/getsentry/sentry-dart/pull/2530)) diff --git a/flutter/lib/src/event_processor/screenshot_event_processor.dart b/flutter/lib/src/event_processor/screenshot_event_processor.dart index 97922bae1a..6f8618bf28 100644 --- a/flutter/lib/src/event_processor/screenshot_event_processor.dart +++ b/flutter/lib/src/event_processor/screenshot_event_processor.dart @@ -29,7 +29,7 @@ class ScreenshotEventProcessor implements EventProcessor { _debouncer = Debouncer( // ignore: invalid_use_of_internal_member _options.clock, - waitTimeMs: 2000, + waitTime: Duration(milliseconds: 2000), ); } @@ -50,7 +50,7 @@ class ScreenshotEventProcessor implements EventProcessor { } // skip capturing in case of debouncing (=too many frequent capture requests) - // the BeforeCaptureCallback may overrules the debouncing decision + // the BeforeCaptureCallback may overrule the debouncing decision final shouldDebounce = _debouncer.shouldDebounce(); // ignore: deprecated_member_use_from_same_package @@ -77,7 +77,7 @@ class ScreenshotEventProcessor implements EventProcessor { } else if (shouldDebounce) { _options.logger( SentryLevel.debug, - 'Skipping screenshot capture due to debouncing (too many captures within ${_debouncer.waitTimeMs}ms)', + 'Skipping screenshot capture due to debouncing (too many captures within ${_debouncer.waitTime.inMilliseconds}ms)', ); takeScreenshot = false; } @@ -88,7 +88,7 @@ class ScreenshotEventProcessor implements EventProcessor { } catch (exception, stackTrace) { _options.logger( SentryLevel.error, - 'The beforeCapture/beforeScreenshot callback threw an exception', + 'The beforeCaptureScreenshot/beforeScreenshot callback threw an exception', exception: exception, stackTrace: stackTrace, ); diff --git a/flutter/lib/src/sentry_flutter_options.dart b/flutter/lib/src/sentry_flutter_options.dart index 5d95eb4817..56c20161b9 100644 --- a/flutter/lib/src/sentry_flutter_options.dart +++ b/flutter/lib/src/sentry_flutter_options.dart @@ -241,11 +241,17 @@ class SentryFlutterOptions extends SentryOptions { /// Enables the View Hierarchy feature. /// - /// Renders an ASCII represention of the entire view hierarchy of the + /// Renders an ASCII representation of the entire view hierarchy of the /// application when an error happens and includes it as an attachment. @meta.experimental bool attachViewHierarchy = false; + /// Sets a callback which is executed before capturing view hierarchy. Only + /// relevant if `attachViewHierarchy` is set to true. When false is returned + /// from the function, no view hierarchy will be attached. + @meta.experimental + BeforeCaptureCallback? beforeCaptureViewHierarchy; + /// Enables collection of view hierarchy element identifiers. /// /// Identifiers are extracted from widget keys. diff --git a/flutter/lib/src/utils/debouncer.dart b/flutter/lib/src/utils/debouncer.dart index d3027d8849..0f66e0529c 100644 --- a/flutter/lib/src/utils/debouncer.dart +++ b/flutter/lib/src/utils/debouncer.dart @@ -4,10 +4,11 @@ import 'package:sentry/sentry.dart'; @internal class Debouncer { final ClockProvider clockProvider; - final int waitTimeMs; + final Duration waitTime; DateTime? _lastExecutionTime; - Debouncer(this.clockProvider, {this.waitTimeMs = 2000}); + Debouncer(this.clockProvider, + {this.waitTime = const Duration(milliseconds: 2000)}); bool shouldDebounce() { final currentTime = clockProvider(); @@ -15,7 +16,7 @@ class Debouncer { _lastExecutionTime = currentTime; if (lastExecutionTime != null && - currentTime.difference(lastExecutionTime).inMilliseconds < waitTimeMs) { + currentTime.difference(lastExecutionTime) < waitTime) { return true; } diff --git a/flutter/lib/src/view_hierarchy/view_hierarchy_event_processor.dart b/flutter/lib/src/view_hierarchy/view_hierarchy_event_processor.dart index cf9ae008ec..f595b3d456 100644 --- a/flutter/lib/src/view_hierarchy/view_hierarchy_event_processor.dart +++ b/flutter/lib/src/view_hierarchy/view_hierarchy_event_processor.dart @@ -1,16 +1,26 @@ +import 'dart:async'; + import '../../sentry_flutter.dart'; +import '../utils/debouncer.dart'; import 'sentry_tree_walker.dart'; /// A [EventProcessor] that renders an ASCII representation of the entire view /// hierarchy of the application when an error happens and includes it as an /// attachment to the [Hint]. class SentryViewHierarchyEventProcessor implements EventProcessor { - SentryViewHierarchyEventProcessor(this._options); - final SentryFlutterOptions _options; + late final Debouncer _debouncer; + + SentryViewHierarchyEventProcessor(this._options) { + _debouncer = Debouncer( + // ignore: invalid_use_of_internal_member + _options.clock, + waitTime: Duration(milliseconds: 2000), + ); + } @override - SentryEvent? apply(SentryEvent event, Hint hint) { + Future apply(SentryEvent event, Hint hint) async { if (event is SentryTransaction) { return event; } @@ -23,15 +33,56 @@ class SentryViewHierarchyEventProcessor implements EventProcessor { if (instance == null) { return event; } - final sentryViewHierarchy = walkWidgetTree(instance, _options); + // skip capturing in case of debouncing (=too many frequent capture requests) + // the BeforeCaptureCallback may overrule the debouncing decision + final shouldDebounce = _debouncer.shouldDebounce(); + + try { + final beforeCapture = _options.beforeCaptureViewHierarchy; + FutureOr? result; + + if (beforeCapture != null) { + result = beforeCapture(event, hint, shouldDebounce); + } + + bool captureViewHierarchy = true; + + if (result != null) { + if (result is Future) { + captureViewHierarchy = await result; + } else { + captureViewHierarchy = result; + } + } else if (shouldDebounce) { + _options.logger( + SentryLevel.debug, + 'Skipping view hierarchy capture due to debouncing (too many captures within ${_debouncer.waitTime.inMilliseconds}ms)', + ); + captureViewHierarchy = false; + } + + if (!captureViewHierarchy) { + return event; + } + } catch (exception, stackTrace) { + _options.logger( + SentryLevel.error, + 'The beforeCaptureViewHierarchy callback threw an exception', + exception: exception, + stackTrace: stackTrace, + ); + if (_options.automatedTestMode) { + rethrow; + } + } + + final sentryViewHierarchy = walkWidgetTree(instance, _options); if (sentryViewHierarchy == null) { return event; } - - final viewHierarchy = + hint.viewHierarchy = SentryAttachment.fromViewHierarchy(sentryViewHierarchy); - hint.viewHierarchy = viewHierarchy; return event; } } diff --git a/flutter/test/utils/debouncer_test.dart b/flutter/test/utils/debouncer_test.dart index b78987d360..3b5e827355 100644 --- a/flutter/test/utils/debouncer_test.dart +++ b/flutter/test/utils/debouncer_test.dart @@ -48,6 +48,6 @@ class Fixture { DateTime mockClock() => DateTime.fromMillisecondsSinceEpoch(currentTimeMs); Debouncer getSut({int waitTimeMs = 3000}) { - return Debouncer(mockClock, waitTimeMs: waitTimeMs); + return Debouncer(mockClock, waitTime: Duration(milliseconds: waitTimeMs)); } } diff --git a/flutter/test/view_hierarchy/view_hierarchy_event_processor_test.dart b/flutter/test/view_hierarchy/view_hierarchy_event_processor_test.dart index 9ba8a694ff..29454707e1 100644 --- a/flutter/test/view_hierarchy/view_hierarchy_event_processor_test.dart +++ b/flutter/test/view_hierarchy/view_hierarchy_event_processor_test.dart @@ -4,6 +4,7 @@ import 'package:flutter/material.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:sentry/sentry.dart'; import 'package:sentry_flutter/src/binding_wrapper.dart'; +import 'package:sentry_flutter/src/sentry_flutter_options.dart'; import 'package:sentry_flutter/src/view_hierarchy/view_hierarchy_event_processor.dart'; import '../mocks.dart'; @@ -29,7 +30,7 @@ void main() { exceptions: [SentryException(type: 'type', value: 'value')]); final hint = Hint(); - sut.apply(event, hint); + await sut.apply(event, hint); expect(hint.viewHierarchy, isNotNull); }); @@ -45,7 +46,7 @@ void main() { final event = SentryEvent(throwable: StateError('error')); final hint = Hint(); - sut.apply(event, hint); + await sut.apply(event, hint); expect(hint.viewHierarchy, isNotNull); }); @@ -61,7 +62,7 @@ void main() { final event = SentryEvent(); final hint = Hint(); - sut.apply(event, hint); + await sut.apply(event, hint); expect(hint.viewHierarchy, isNull); }); @@ -77,7 +78,7 @@ void main() { final event = SentryEvent(); final hint = Hint(); - sut.apply(event, hint); + await sut.apply(event, hint); expect(hint.viewHierarchy, isNull); }); @@ -95,7 +96,7 @@ void main() { exceptions: [SentryException(type: 'type', value: 'value')]); final hint = Hint(); - sut.apply(event, hint); + await sut.apply(event, hint); expect(hint.viewHierarchy, isNotNull); final bytes = await hint.viewHierarchy!.bytes; @@ -103,6 +104,207 @@ void main() { expect(jsonString, isNot(contains('identifier'))); }); }); + + group('beforeCaptureViewHierarchy', () { + late SentryEvent event; + late Hint hint; + + Future _addViewHierarchyAttachment( + WidgetTester tester, { + required bool added, + }) async { + // Run with real async https://stackoverflow.com/a/54021863 + await tester.runAsync(() async { + final sut = fixture.getSut(instance); + + await tester.pumpWidget(MyApp()); + + final throwable = Exception(); + event = SentryEvent(throwable: throwable); + hint = Hint(); + await sut.apply(event, hint); + + expect(hint.viewHierarchy != null, added); + }); + } + + testWidgets('does add view hierarchy if beforeCapture returns true', + (tester) async { + fixture.options.beforeCaptureViewHierarchy = + (SentryEvent event, Hint hint, bool shouldDebounce) { + return true; + }; + await _addViewHierarchyAttachment(tester, added: true); + }); + + testWidgets('does add view hierarchy if async beforeCapture returns true', + (tester) async { + fixture.options.beforeCaptureViewHierarchy = + (SentryEvent event, Hint hint, bool shouldDebounce) async { + await Future.delayed(Duration(milliseconds: 1)); + return true; + }; + await _addViewHierarchyAttachment(tester, added: true); + }); + + testWidgets('does not add view hierarchy if beforeCapture returns false', + (tester) async { + fixture.options.beforeCaptureViewHierarchy = + (SentryEvent event, Hint hint, bool shouldDebounce) { + return false; + }; + await _addViewHierarchyAttachment(tester, added: false); + }); + + testWidgets( + 'does not add view hierarchy if async beforeCapture returns false', + (tester) async { + fixture.options.beforeCaptureViewHierarchy = + (SentryEvent event, Hint hint, bool shouldDebounce) async { + await Future.delayed(Duration(milliseconds: 1)); + return false; + }; + await _addViewHierarchyAttachment(tester, added: false); + }); + + testWidgets('does add view hierarchy if beforeCapture throws', + (tester) async { + fixture.options.automatedTestMode = false; + fixture.options.beforeCaptureViewHierarchy = + (SentryEvent event, Hint hint, bool shouldDebounce) { + throw Error(); + }; + await _addViewHierarchyAttachment(tester, added: true); + }); + + testWidgets('does add view hierarchy if async beforeCapture throws', + (tester) async { + fixture.options.automatedTestMode = false; + fixture.options.beforeCaptureViewHierarchy = + (SentryEvent event, Hint hint, bool shouldDebounce) async { + await Future.delayed(Duration(milliseconds: 1)); + throw Error(); + }; + await _addViewHierarchyAttachment(tester, added: true); + }); + + testWidgets('does add view hierarchy event if shouldDebounce true', + (tester) async { + await tester.runAsync(() async { + var shouldDebounceValues = []; + + fixture.options.beforeCaptureViewHierarchy = + (SentryEvent event, Hint hint, bool shouldDebounce) { + shouldDebounceValues.add(shouldDebounce); + return true; + }; + + final sut = fixture.getSut(instance); + await tester.pumpWidget(MyApp()); + + final event = SentryEvent(throwable: Exception()); + final hintOne = Hint(); + final hintTwo = Hint(); + + await sut.apply(event, hintOne); + await sut.apply(event, hintTwo); + + expect(hintOne.viewHierarchy, isNotNull); + expect(hintTwo.viewHierarchy, isNotNull); + + expect(shouldDebounceValues[0], false); + expect(shouldDebounceValues[1], true); + }); + }); + + testWidgets('passes event & hint to beforeCapture callback', + (tester) async { + SentryEvent? beforeScreenshotEvent; + Hint? beforeScreenshotHint; + + fixture.options.beforeCaptureViewHierarchy = + (SentryEvent event, Hint hint, bool shouldDebounce) { + beforeScreenshotEvent = event; + beforeScreenshotHint = hint; + return true; + }; + + await _addViewHierarchyAttachment(tester, added: true); + + expect(beforeScreenshotEvent, event); + expect(beforeScreenshotHint, hint); + }); + }); + + group("debounce", () { + testWidgets("limits added view hierarchy 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(instance); + await tester.pumpWidget(MyApp()); + + 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.viewHierarchy, isNotNull); + expect(secondHint.viewHierarchy, isNull); + }); + }); + + testWidgets("adds view hierarchy 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(instance); + await tester.pumpWidget(MyApp()); + + 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.viewHierarchy, isNotNull); + expect(secondHint.viewHierarchy, isNotNull); + }); + }); + }); }); } @@ -123,9 +325,11 @@ class TestBindingWrapper implements BindingWrapper { } class Fixture { + SentryFlutterOptions options = defaultTestOptions(); + SentryViewHierarchyEventProcessor getSut(WidgetsBinding instance, {bool reportViewHierarchyIdentifiers = true}) { - final options = defaultTestOptions() + options ..bindingUtils = TestBindingWrapper(instance) ..reportViewHierarchyIdentifiers = reportViewHierarchyIdentifiers; return SentryViewHierarchyEventProcessor(options);