From 8af348e12c049efe815cefc256d9bf729d8f78fc Mon Sep 17 00:00:00 2001 From: GIancarlo Buenaflor Date: Fri, 11 Oct 2024 16:27:42 +0200 Subject: [PATCH 1/4] fix --- .../time_to_full_display_tracker.dart | 7 +- .../test/sentry_navigator_observer_test.dart | 76 +++++++++++++++++++ 2 files changed, 82 insertions(+), 1 deletion(-) diff --git a/flutter/lib/src/navigation/time_to_full_display_tracker.dart b/flutter/lib/src/navigation/time_to_full_display_tracker.dart index 6c9130eebf..87a6d43bf9 100644 --- a/flutter/lib/src/navigation/time_to_full_display_tracker.dart +++ b/flutter/lib/src/navigation/time_to_full_display_tracker.dart @@ -83,7 +83,12 @@ class TimeToFullDisplayTracker { return; } - _setTTFDMeasurement(startTimestamp, endTimestamp); + final status = + timestamp != null ? SpanStatus.ok() : SpanStatus.deadlineExceeded(); + // Should only add measurements if the span is successful + if (status == SpanStatus.ok()) { + _setTTFDMeasurement(startTimestamp, endTimestamp); + } await ttfdSpan.finish( status: timestamp != null ? SpanStatus.ok() : SpanStatus.deadlineExceeded(), diff --git a/flutter/test/sentry_navigator_observer_test.dart b/flutter/test/sentry_navigator_observer_test.dart index c43ccfece1..3d33544bf8 100644 --- a/flutter/test/sentry_navigator_observer_test.dart +++ b/flutter/test/sentry_navigator_observer_test.dart @@ -380,6 +380,82 @@ void main() { .called(1); }); + test('cancelled TTID and TTFD spans do not add measurements', () async { + final initialRoute = route(RouteSettings(name: 'Initial Route')); + final newRoute = route(RouteSettings(name: 'New Route')); + + final hub = _MockHub(); + final transaction = getMockSentryTracer(finished: false) as SentryTracer; + + final mockChildTTID = MockSentrySpan(); + final mockChildTTFD = MockSentrySpan(); + + when(transaction.children).thenReturn([ + mockChildTTID, + mockChildTTFD, + ]); + + when(transaction.measurements).thenReturn({}); + + when(mockChildTTID.finished).thenReturn(false); + when(mockChildTTID.context).thenReturn(SentrySpanContext( + operation: SentrySpanOperations.uiTimeToInitialDisplay)); + when(mockChildTTID.status).thenReturn(SpanStatus.cancelled()); + + when(mockChildTTFD.finished).thenReturn(false); + when(mockChildTTFD.context).thenReturn(SentrySpanContext( + operation: SentrySpanOperations.uiTimeToFullDisplay)); + when(mockChildTTFD.status).thenReturn(SpanStatus.cancelled()); + + when(transaction.context) + .thenReturn(SentrySpanContext(operation: 'navigation')); + when(transaction.status).thenReturn(null); + when(transaction.finished).thenReturn(false); + + when(transaction.startChild( + 'ui.load.initial_display', + description: anyNamed('description'), + startTimestamp: anyNamed('startTimestamp'), + )).thenReturn(MockSentrySpan()); + + when(hub.getSpan()).thenReturn(transaction); + when(hub.startTransactionWithContext( + any, + startTimestamp: anyNamed('startTimestamp'), + waitForChildren: true, + autoFinishAfter: anyNamed('autoFinishAfter'), + trimEnd: true, + onFinish: anyNamed('onFinish'), + )).thenReturn(transaction); + + final sut = + fixture.getSut(hub: hub, autoFinishAfter: Duration(seconds: 5)); + + // Simulate pushing the initial route + sut.didPush(initialRoute, null); + + // Simulate navigating to a new route before TTID and TTFD spans finish + sut.didPush(newRoute, initialRoute); + + // Allow async operations to complete + await Future.delayed(const Duration(milliseconds: 100)); + + // Verify that the TTID and TTFD spans are finished with a cancelled status + verify(mockChildTTID.finish( + endTimestamp: anyNamed('endTimestamp'), + status: SpanStatus.cancelled())) + .called(1); + verify(mockChildTTFD.finish( + endTimestamp: anyNamed('endTimestamp'), + status: SpanStatus.cancelled())) + .called(1); + + // Verify that the measurements are not added to the transaction + final measurements = transaction.measurements; + expect(measurements.containsKey('time_to_initial_display'), isFalse); + expect(measurements.containsKey('time_to_full_display'), isFalse); + }); + test( 'unfinished children will be finished with deadline_exceeded on didPush', () async { From c9c557c0c81da4e00df60baccea3bddbcdf9db34 Mon Sep 17 00:00:00 2001 From: GIancarlo Buenaflor Date: Fri, 11 Oct 2024 16:46:17 +0200 Subject: [PATCH 2/4] update --- .../time_to_full_display_tracker.dart | 32 ++++++++++++------- .../time_to_full_display_tracker_test.dart | 2 ++ 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/flutter/lib/src/navigation/time_to_full_display_tracker.dart b/flutter/lib/src/navigation/time_to_full_display_tracker.dart index 87a6d43bf9..52cd27f4b7 100644 --- a/flutter/lib/src/navigation/time_to_full_display_tracker.dart +++ b/flutter/lib/src/navigation/time_to_full_display_tracker.dart @@ -71,7 +71,7 @@ class TimeToFullDisplayTracker { final endTimestamp = timestamp ?? _endTimestampProvider(); if (ttfdSpan == null || - ttfdSpan.finished == true || + ttfdSpan.finished || startTimestamp == null || endTimestamp == null) { options.logger( @@ -83,19 +83,29 @@ class TimeToFullDisplayTracker { return; } + // If a timestamp is provided, the operation was successful; otherwise, it timed out final status = timestamp != null ? SpanStatus.ok() : SpanStatus.deadlineExceeded(); - // Should only add measurements if the span is successful - if (status == SpanStatus.ok()) { - _setTTFDMeasurement(startTimestamp, endTimestamp); + try { + // Should only add measurements if the span is successful + if (status == SpanStatus.ok()) { + _setTTFDMeasurement(startTimestamp, endTimestamp); + } + await ttfdSpan.finish( + status: status, + endTimestamp: endTimestamp, + ); + } catch (e, stackTrace) { + options.logger( + SentryLevel.error, + 'Failed to finish TTFD span', + exception: e, + stackTrace: stackTrace, + ); + } finally { + _completedTTFDTracking.complete(); + clear(); } - await ttfdSpan.finish( - status: - timestamp != null ? SpanStatus.ok() : SpanStatus.deadlineExceeded(), - endTimestamp: endTimestamp, - ); - _completedTTFDTracking.complete(); - clear(); } void _setTTFDMeasurement(DateTime startTimestamp, DateTime endTimestamp) { diff --git a/flutter/test/navigation/time_to_full_display_tracker_test.dart b/flutter/test/navigation/time_to_full_display_tracker_test.dart index a2adf62aeb..2dc62ecd79 100644 --- a/flutter/test/navigation/time_to_full_display_tracker_test.dart +++ b/flutter/test/navigation/time_to_full_display_tracker_test.dart @@ -44,6 +44,7 @@ void main() { final differenceInSeconds = actualEndTimestamp.difference(expectedEndTimestamp).inSeconds.abs(); expect(differenceInSeconds, lessThanOrEqualTo(1)); + expect(transaction.measurements, isNotEmpty); }); test( @@ -66,6 +67,7 @@ void main() { expect(ttfdSpan.status, equals(SpanStatus.deadlineExceeded())); expect(ttfdSpan.context.description, equals('Current route full display')); expect(ttfdSpan.origin, equals(SentryTraceOrigins.manualUiTimeToDisplay)); + expect(transaction.measurements, isEmpty); }); test('finishing ttfd twice does not throw', () async { From b84171d9859f9db38f2f3d65bc7066684fcdadd9 Mon Sep 17 00:00:00 2001 From: GIancarlo Buenaflor Date: Fri, 11 Oct 2024 16:46:36 +0200 Subject: [PATCH 3/4] update --- .../test/sentry_navigator_observer_test.dart | 76 ------------------- 1 file changed, 76 deletions(-) diff --git a/flutter/test/sentry_navigator_observer_test.dart b/flutter/test/sentry_navigator_observer_test.dart index 3d33544bf8..c43ccfece1 100644 --- a/flutter/test/sentry_navigator_observer_test.dart +++ b/flutter/test/sentry_navigator_observer_test.dart @@ -380,82 +380,6 @@ void main() { .called(1); }); - test('cancelled TTID and TTFD spans do not add measurements', () async { - final initialRoute = route(RouteSettings(name: 'Initial Route')); - final newRoute = route(RouteSettings(name: 'New Route')); - - final hub = _MockHub(); - final transaction = getMockSentryTracer(finished: false) as SentryTracer; - - final mockChildTTID = MockSentrySpan(); - final mockChildTTFD = MockSentrySpan(); - - when(transaction.children).thenReturn([ - mockChildTTID, - mockChildTTFD, - ]); - - when(transaction.measurements).thenReturn({}); - - when(mockChildTTID.finished).thenReturn(false); - when(mockChildTTID.context).thenReturn(SentrySpanContext( - operation: SentrySpanOperations.uiTimeToInitialDisplay)); - when(mockChildTTID.status).thenReturn(SpanStatus.cancelled()); - - when(mockChildTTFD.finished).thenReturn(false); - when(mockChildTTFD.context).thenReturn(SentrySpanContext( - operation: SentrySpanOperations.uiTimeToFullDisplay)); - when(mockChildTTFD.status).thenReturn(SpanStatus.cancelled()); - - when(transaction.context) - .thenReturn(SentrySpanContext(operation: 'navigation')); - when(transaction.status).thenReturn(null); - when(transaction.finished).thenReturn(false); - - when(transaction.startChild( - 'ui.load.initial_display', - description: anyNamed('description'), - startTimestamp: anyNamed('startTimestamp'), - )).thenReturn(MockSentrySpan()); - - when(hub.getSpan()).thenReturn(transaction); - when(hub.startTransactionWithContext( - any, - startTimestamp: anyNamed('startTimestamp'), - waitForChildren: true, - autoFinishAfter: anyNamed('autoFinishAfter'), - trimEnd: true, - onFinish: anyNamed('onFinish'), - )).thenReturn(transaction); - - final sut = - fixture.getSut(hub: hub, autoFinishAfter: Duration(seconds: 5)); - - // Simulate pushing the initial route - sut.didPush(initialRoute, null); - - // Simulate navigating to a new route before TTID and TTFD spans finish - sut.didPush(newRoute, initialRoute); - - // Allow async operations to complete - await Future.delayed(const Duration(milliseconds: 100)); - - // Verify that the TTID and TTFD spans are finished with a cancelled status - verify(mockChildTTID.finish( - endTimestamp: anyNamed('endTimestamp'), - status: SpanStatus.cancelled())) - .called(1); - verify(mockChildTTFD.finish( - endTimestamp: anyNamed('endTimestamp'), - status: SpanStatus.cancelled())) - .called(1); - - // Verify that the measurements are not added to the transaction - final measurements = transaction.measurements; - expect(measurements.containsKey('time_to_initial_display'), isFalse); - expect(measurements.containsKey('time_to_full_display'), isFalse); - }); - test( 'unfinished children will be finished with deadline_exceeded on didPush', () async { From ed28c796da248cf29080bd6da24d1135f6c8fc87 Mon Sep 17 00:00:00 2001 From: GIancarlo Buenaflor Date: Fri, 11 Oct 2024 16:49:17 +0200 Subject: [PATCH 4/4] update --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7aabdc80c6..5b5cf74b2f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -66,6 +66,7 @@ Navigator.push( - Start missing TTFD for root screen transaction ([#2332](https://github.com/getsentry/sentry-dart/pull/2332)) - Accessing invalid json fields from `fetchNativeAppStart` should return null ([#2340](https://github.com/getsentry/sentry-dart/pull/2340)) - Error when calling `SentryFlutter.reportFullyDisplayed()` twice ([#2339](https://github.com/getsentry/sentry-dart/pull/2339)) +- TTFD measurements should only be added for successful TTFD spans ([#2348](https://github.com/getsentry/sentry-dart/pull/2348)) ### Deprecate