Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix TTID timing issue #2326

Merged
merged 6 commits into from
Oct 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
- Only start frame tracking if we receive valid display refresh data ([#2307](https://github.com/getsentry/sentry-dart/pull/2307))
- Rounding error used on frames.total and reject frame measurements if frames.total is less than frames.slow or frames.frozen ([#2308](https://github.com/getsentry/sentry-dart/pull/2308))
- iOS replay integration when only `onErrorSampleRate` is specified ([#2306](https://github.com/getsentry/sentry-dart/pull/2306))
- Fix TTID timing issue ([#2326](https://github.com/getsentry/sentry-dart/pull/2326))

### Deprecate

Expand Down
24 changes: 15 additions & 9 deletions flutter/lib/src/navigation/sentry_navigator_observer.dart
Original file line number Diff line number Diff line change
Expand Up @@ -159,8 +159,10 @@ class SentryNavigatorObserver extends RouteObserver<PageRoute<dynamic>> {

// Clearing the display tracker here is safe since didPush happens before the Widget is built
_timeToDisplayTracker?.clear();
_finishTimeToDisplayTracking();
_startTimeToDisplayTracking(route);

DateTime timestamp = _hub.options.clock();
_finishTimeToDisplayTracking(endTimestamp: timestamp);
_startTimeToDisplayTracking(route, timestamp);
denrase marked this conversation as resolved.
Show resolved Hide resolved
}

@override
Expand Down Expand Up @@ -200,7 +202,8 @@ class SentryNavigatorObserver extends RouteObserver<PageRoute<dynamic>> {
to: previousRoute?.settings,
);

_finishTimeToDisplayTracking(clearAfter: true);
final timestamp = _hub.options.clock();
_finishTimeToDisplayTracking(endTimestamp: timestamp, clearAfter: true);
}

void _addBreadcrumb({
Expand Down Expand Up @@ -295,7 +298,8 @@ class SentryNavigatorObserver extends RouteObserver<PageRoute<dynamic>> {
await _native?.beginNativeFrames();
}

Future<void> _finishTimeToDisplayTracking({bool clearAfter = false}) async {
Future<void> _finishTimeToDisplayTracking(
{required DateTime endTimestamp, bool clearAfter = false}) async {
final transaction = _transaction;
_transaction = null;
try {
Expand All @@ -317,7 +321,10 @@ class SentryNavigatorObserver extends RouteObserver<PageRoute<dynamic>> {
final isTTFDSpan =
child.context.operation == SentrySpanOperations.uiTimeToFullDisplay;
if (!child.finished && (isTTIDSpan || isTTFDSpan)) {
await child.finish(status: SpanStatus.deadlineExceeded());
await child.finish(
endTimestamp: endTimestamp,
status: SpanStatus.deadlineExceeded(),
);
}
}
} catch (exception, stacktrace) {
Expand All @@ -331,23 +338,22 @@ class SentryNavigatorObserver extends RouteObserver<PageRoute<dynamic>> {
rethrow;
}
} finally {
await transaction?.finish();
await transaction?.finish(endTimestamp: endTimestamp);
if (clearAfter) {
_clear();
}
}
}

Future<void> _startTimeToDisplayTracking(Route<dynamic>? route) async {
Future<void> _startTimeToDisplayTracking(
Route<dynamic>? route, DateTime startTimestamp) async {
try {
final routeName = _getRouteName(route) ?? _currentRouteName;
if (!_enableAutoTransactions || routeName == null) {
return;
}

bool isAppStart = routeName == '/';
DateTime startTimestamp = _hub.options.clock();

await _startTransaction(route, startTimestamp);

final transaction = _transaction;
Expand Down
18 changes: 15 additions & 3 deletions flutter/lib/src/navigation/time_to_initial_display_tracker.dart
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ class TimeToInitialDisplayTracker {
bool _isManual = false;
Completer<DateTime?>? _trackingCompleter;
DateTime? _endTimestamp;
DateTime? _completeTrackingTimeStamp;

final Duration _determineEndtimeTimeout = Duration(seconds: 5);

/// This endTimestamp is needed in the [TimeToFullDisplayTracker] class
Expand Down Expand Up @@ -87,6 +89,13 @@ class TimeToInitialDisplayTracker {

// If we already know it's manual we can return the future immediately
if (_isManual) {
final completeTrackingTimeStamp = _completeTrackingTimeStamp;
if (completeTrackingTimeStamp != null) {
// If complete was called before we could call start, complete it here.
_endTimestamp = completeTrackingTimeStamp;
_trackingCompleter?.complete(completeTrackingTimeStamp);
_completeTrackingTimeStamp = null;
}
return future;
}

Expand All @@ -106,10 +115,13 @@ class TimeToInitialDisplayTracker {
}

void completeTracking() {
final timestamp = DateTime.now();

if (_trackingCompleter != null && !_trackingCompleter!.isCompleted) {
final endTimestamp = DateTime.now();
_endTimestamp = endTimestamp;
_trackingCompleter?.complete(endTimestamp);
_endTimestamp = timestamp;
_trackingCompleter?.complete(timestamp);
} else {
_completeTrackingTimeStamp = timestamp;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,37 @@ void main() {
.difference(ttidSpan.startTimestamp)
.inMilliseconds);
});

test('starting after completing still finished correctly', () async {
await Future.delayed(fixture.finishFrameDuration, () {
sut.markAsManual();
sut.completeTracking();
});

final transaction = fixture.getTransaction() as SentryTracer;
await sut.trackRegularRoute(transaction, fixture.startTimestamp);

final children = transaction.children;
expect(children, hasLength(1));

final ttidSpan = children.first;
expect(ttidSpan.context.operation,
SentrySpanOperations.uiTimeToInitialDisplay);
expect(ttidSpan.finished, isTrue);
expect(ttidSpan.context.description, 'Regular route initial display');
expect(ttidSpan.origin, SentryTraceOrigins.manualUiTimeToDisplay);
final ttidMeasurement =
transaction.measurements['time_to_initial_display'];
expect(ttidMeasurement, isNotNull);
expect(ttidMeasurement?.unit, DurationSentryMeasurementUnit.milliSecond);
expect(ttidMeasurement?.value,
greaterThanOrEqualTo(fixture.finishFrameDuration.inMilliseconds));
expect(
ttidMeasurement?.value,
ttidSpan.endTimestamp!
.difference(ttidSpan.startTimestamp)
.inMilliseconds);
});
});

group('determineEndtime', () {
Expand Down
25 changes: 18 additions & 7 deletions flutter/test/sentry_navigator_observer_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,8 @@ void main() {
expect(scope.span, null);
});

verify(span.finish()).called(2);
verify(span.finish(endTimestamp: captureAnyNamed('endTimestamp')))
.called(2);
});

test('didPop finishes transaction', () async {
Expand All @@ -344,7 +345,8 @@ void main() {
expect(scope.span, null);
});

verify(span.finish()).called(1);
verify(span.finish(endTimestamp: captureAnyNamed('endTimestamp')))
.called(1);
});

test('multiple didPop only finish transaction once', () async {
Expand Down Expand Up @@ -373,7 +375,8 @@ void main() {
expect(scope.span, null);
});

verify(span.finish()).called(1);
verify(span.finish(endTimestamp: captureAnyNamed('endTimestamp')))
.called(1);
});

test(
Expand Down Expand Up @@ -413,9 +416,13 @@ void main() {

await Future<void>.delayed(const Duration(milliseconds: 100));

verify(mockChildA.finish(status: SpanStatus.deadlineExceeded()))
verify(mockChildA.finish(
endTimestamp: captureAnyNamed('endTimestamp'),
status: SpanStatus.deadlineExceeded()))
.called(1);
verify(mockChildB.finish(status: SpanStatus.deadlineExceeded()))
verify(mockChildB.finish(
endTimestamp: captureAnyNamed('endTimestamp'),
status: SpanStatus.deadlineExceeded()))
.called(1);
});

Expand Down Expand Up @@ -456,9 +463,13 @@ void main() {

await Future<void>.delayed(const Duration(milliseconds: 100));

verify(mockChildA.finish(status: SpanStatus.deadlineExceeded()))
verify(mockChildA.finish(
endTimestamp: captureAnyNamed('endTimestamp'),
status: SpanStatus.deadlineExceeded()))
.called(1);
verify(mockChildB.finish(status: SpanStatus.deadlineExceeded()))
verify(mockChildB.finish(
endTimestamp: captureAnyNamed('endTimestamp'),
status: SpanStatus.deadlineExceeded()))
.called(1);
});

Expand Down
Loading