-
-
Notifications
You must be signed in to change notification settings - Fork 235
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
feat: ttfd #1920
feat: ttfd #1920
Changes from all commits
0b21267
6f6e71c
6ed7700
2f8a47d
50f298b
3477e6f
dc80015
ed3d4ae
81346de
9d09211
cf5af40
c653eb1
487e55f
845e6a8
96c8766
1d9e71f
e3c227f
cea12c5
9af3455
71fd7ec
7a977fb
d7a6a83
cde560f
f66206d
87d2755
4dedf37
5dd824e
bf2ad2f
4d33aab
248cb8f
35ec31c
e1fde58
4efd9f9
579a3c5
f2ba992
7c64a3f
d90a7ed
b2767aa
4c6301a
eaa6d8d
6ca06dd
c73de7b
caa9897
438c8a2
c4def3a
bbb60ec
f9baa71
238995e
133e166
75a5eee
e50d7c5
cc4398d
c0f41d8
2dafed4
7434ca1
e0298fe
b799120
897b4f5
7e8478c
f1a2740
a530fc3
b99510a
532fcc4
969b578
0b12c80
5ff7c0f
67c6e8f
d32bdea
320f92b
a823366
1cd1025
8dbe4ba
dbc4320
9c6e2b1
1aa2343
b15eccf
b122a49
a7bb055
f9a34d7
62e4e54
aeaebfa
5552732
524935c
6e86523
5fa126e
d209276
66b310b
2d16c67
a0505c1
b203653
a353de2
e775c09
7456eef
7420ca4
d1ff7f6
9fdc8e2
1b23622
dcbc1e6
b19f95c
73022e4
10b0b93
3085394
8548050
a269ec8
671ed7d
c6a8b27
bc18524
e422ae4
d4f3d88
cb95c14
a809130
788eed7
335528f
b4ce9ef
a210151
d32a735
0fd5c7f
dfd36b6
6ae7df1
7ee807a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,9 @@ import '../../sentry_flutter.dart'; | |
import '../event_processor/flutter_enricher_event_processor.dart'; | ||
import '../native/sentry_native.dart'; | ||
|
||
// ignore: implementation_imports | ||
import 'package:sentry/src/sentry_tracer.dart'; | ||
|
||
/// This key must be used so that the web interface displays the events nicely | ||
/// See https://develop.sentry.dev/sdk/event-payloads/breadcrumbs/ | ||
const _navigationKey = 'navigation'; | ||
|
@@ -82,11 +85,23 @@ class SentryNavigatorObserver extends RouteObserver<PageRoute<dynamic>> { | |
_setRouteNameAsTransaction = setRouteNameAsTransaction, | ||
_routeNameExtractor = routeNameExtractor, | ||
_additionalInfoProvider = additionalInfoProvider, | ||
_native = SentryFlutter.native, | ||
_timeToDisplayTracker = timeToDisplayTracker ?? TimeToDisplayTracker() { | ||
_native = SentryFlutter.native { | ||
if (enableAutoTransactions) { | ||
_hub.options.sdk.addIntegration('UINavigationTracing'); | ||
} | ||
_timeToDisplayTracker = | ||
timeToDisplayTracker ?? _initializeTimeToDisplayTracker(); | ||
} | ||
|
||
/// Initializes the TimeToDisplayTracker with the option to enable time to full display tracing. | ||
TimeToDisplayTracker _initializeTimeToDisplayTracker() { | ||
bool enableTimeToFullDisplayTracing = false; | ||
final options = _hub.options; | ||
if (options is SentryFlutterOptions) { | ||
enableTimeToFullDisplayTracing = options.enableTimeToFullDisplayTracing; | ||
} | ||
return TimeToDisplayTracker( | ||
enableTimeToFullDisplayTracing: enableTimeToFullDisplayTracing); | ||
} | ||
|
||
final Hub _hub; | ||
|
@@ -96,7 +111,11 @@ class SentryNavigatorObserver extends RouteObserver<PageRoute<dynamic>> { | |
final RouteNameExtractor? _routeNameExtractor; | ||
final AdditionalInfoExtractor? _additionalInfoProvider; | ||
final SentryNative? _native; | ||
final TimeToDisplayTracker? _timeToDisplayTracker; | ||
static TimeToDisplayTracker? _timeToDisplayTracker; | ||
|
||
@internal | ||
static TimeToDisplayTracker? get timeToDisplayTracker => | ||
_timeToDisplayTracker; | ||
|
||
ISentrySpan? _transaction; | ||
|
||
|
@@ -105,7 +124,7 @@ class SentryNavigatorObserver extends RouteObserver<PageRoute<dynamic>> { | |
@internal | ||
static String? get currentRouteName => _currentRouteName; | ||
|
||
Completer<void>? _completedDisplayTracking; | ||
Completer<void>? _completedDisplayTracking = Completer(); | ||
|
||
// Since didPush does not have a future, we can keep track of when the display tracking has finished | ||
@visibleForTesting | ||
|
@@ -124,6 +143,8 @@ class SentryNavigatorObserver extends RouteObserver<PageRoute<dynamic>> { | |
to: route.settings, | ||
); | ||
|
||
// Clearing the display tracker here is safe since didPush happens before the Widget is built | ||
_timeToDisplayTracker?.clear(); | ||
_finishTimeToDisplayTracking(); | ||
_startTimeToDisplayTracking(route); | ||
} | ||
|
@@ -155,7 +176,7 @@ class SentryNavigatorObserver extends RouteObserver<PageRoute<dynamic>> { | |
to: previousRoute?.settings, | ||
); | ||
|
||
_finishTimeToDisplayTracking(); | ||
_finishTimeToDisplayTracking(clearAfter: true); | ||
} | ||
|
||
void _addBreadcrumb({ | ||
|
@@ -253,56 +274,96 @@ class SentryNavigatorObserver extends RouteObserver<PageRoute<dynamic>> { | |
await _native?.beginNativeFramesCollection(); | ||
} | ||
|
||
Future<void> _finishTimeToDisplayTracking() async { | ||
_timeToDisplayTracker?.clear(); | ||
|
||
Future<void> _finishTimeToDisplayTracking({bool clearAfter = false}) async { | ||
final transaction = _transaction; | ||
_transaction = null; | ||
if (transaction == null || transaction.finished) { | ||
return; | ||
try { | ||
_hub.configureScope((scope) { | ||
if (scope.span == transaction) { | ||
scope.span = null; | ||
} | ||
}); | ||
|
||
if (transaction == null || transaction.finished) { | ||
return; | ||
stefanosiano marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
// Cancel unfinished TTID/TTFD spans, e.g this might happen if the user navigates | ||
// away from the current route before TTFD or TTID is finished. | ||
for (final child in (transaction as SentryTracer).children) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we can't just finish all spans of the transaction, as we would finish the spans started by the user, too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
👍
I think that's fine, probably is better than forcefully finishing it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what if we remove it from the scope and still finish it without awaiting? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
yep, that's the idea |
||
final isTTIDSpan = child.context.operation == | ||
SentrySpanOperations.uiTimeToInitialDisplay; | ||
final isTTFDSpan = | ||
child.context.operation == SentrySpanOperations.uiTimeToFullDisplay; | ||
if (!child.finished && (isTTIDSpan || isTTFDSpan)) { | ||
await child.finish(status: SpanStatus.deadlineExceeded()); | ||
} | ||
} | ||
} catch (exception, stacktrace) { | ||
_hub.options.logger( | ||
SentryLevel.error, | ||
'Error while finishing time to display tracking', | ||
exception: exception, | ||
stackTrace: stacktrace, | ||
); | ||
} finally { | ||
await transaction?.finish(); | ||
if (clearAfter) { | ||
_clear(); | ||
} | ||
} | ||
transaction.status ??= SpanStatus.ok(); | ||
await transaction.finish(); | ||
} | ||
|
||
Future<void> _startTimeToDisplayTracking(Route<dynamic>? route) async { | ||
if (!_enableAutoTransactions) { | ||
return; | ||
} | ||
try { | ||
final routeName = _getRouteName(route) ?? _currentRouteName; | ||
if (!_enableAutoTransactions || routeName == null) { | ||
return; | ||
} | ||
|
||
_completedDisplayTracking = Completer<void>(); | ||
String? routeName = _currentRouteName; | ||
if (routeName == null) return; | ||
bool isAppStart = routeName == '/'; | ||
DateTime startTimestamp = _hub.options.clock(); | ||
DateTime? endTimestamp; | ||
|
||
DateTime startTimestamp = _hub.options.clock(); | ||
DateTime? endTimestamp; | ||
if (isAppStart) { | ||
final appStartInfo = await NativeAppStartIntegration.getAppStartInfo(); | ||
if (appStartInfo == null) return; | ||
|
||
if (routeName == '/') { | ||
final appStartInfo = await NativeAppStartIntegration.getAppStartInfo(); | ||
if (appStartInfo == null) { | ||
return; | ||
startTimestamp = appStartInfo.start; | ||
endTimestamp = appStartInfo.end; | ||
} | ||
|
||
startTimestamp = appStartInfo.start; | ||
endTimestamp = appStartInfo.end; | ||
} | ||
await _startTransaction(route, startTimestamp); | ||
|
||
await _startTransaction(route, startTimestamp); | ||
final transaction = _transaction; | ||
if (transaction == null) { | ||
return; | ||
} | ||
final transaction = _transaction; | ||
if (transaction == null) { | ||
return; | ||
} | ||
|
||
if (routeName == '/' && endTimestamp != null) { | ||
await _timeToDisplayTracker?.trackAppStartTTD(transaction, | ||
startTimestamp: startTimestamp, endTimestamp: endTimestamp); | ||
} else { | ||
await _timeToDisplayTracker?.trackRegularRouteTTD(transaction, | ||
startTimestamp: startTimestamp); | ||
if (isAppStart && endTimestamp != null) { | ||
await _timeToDisplayTracker?.trackAppStartTTD(transaction, | ||
startTimestamp: startTimestamp, endTimestamp: endTimestamp); | ||
} else { | ||
await _timeToDisplayTracker?.trackRegularRouteTTD(transaction, | ||
startTimestamp: startTimestamp); | ||
} | ||
} catch (exception, stacktrace) { | ||
_hub.options.logger( | ||
SentryLevel.error, | ||
'Error while tracking time to display', | ||
exception: exception, | ||
stackTrace: stacktrace, | ||
); | ||
} finally { | ||
_clear(); | ||
} | ||
} | ||
|
||
// Mark the tracking as completed and clear any temporary state. | ||
_completedDisplayTracking?.complete(); | ||
void _clear() { | ||
if (_completedDisplayTracking?.isCompleted == false) { | ||
_completedDisplayTracking?.complete(); | ||
} | ||
_completedDisplayTracking = Completer(); | ||
_timeToDisplayTracker?.clear(); | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,34 +1,55 @@ | ||
// ignore_for_file: invalid_use_of_internal_member | ||
|
||
import 'dart:async'; | ||
|
||
import 'package:meta/meta.dart'; | ||
|
||
import '../../sentry_flutter.dart'; | ||
import 'time_to_full_display_tracker.dart'; | ||
import 'time_to_initial_display_tracker.dart'; | ||
|
||
@internal | ||
class TimeToDisplayTracker { | ||
final TimeToInitialDisplayTracker _ttidTracker; | ||
final TimeToFullDisplayTracker? _ttfdTracker; | ||
final bool enableTimeToFullDisplayTracing; | ||
|
||
TimeToDisplayTracker({ | ||
TimeToInitialDisplayTracker? ttidTracker, | ||
}) : _ttidTracker = ttidTracker ?? TimeToInitialDisplayTracker(); | ||
TimeToFullDisplayTracker? ttfdTracker, | ||
required this.enableTimeToFullDisplayTracing, | ||
}) : _ttidTracker = ttidTracker ?? TimeToInitialDisplayTracker(), | ||
_ttfdTracker = enableTimeToFullDisplayTracing | ||
? ttfdTracker ?? TimeToFullDisplayTracker() | ||
: null; | ||
|
||
Future<void> trackAppStartTTD(ISentrySpan transaction, | ||
{required DateTime startTimestamp, | ||
required DateTime endTimestamp}) async { | ||
// We start and immediately finish the spans since we cannot mutate the history of spans. | ||
await _ttidTracker.trackAppStart(transaction, | ||
startTimestamp: startTimestamp, endTimestamp: endTimestamp); | ||
await _trackTTFDIfEnabled(transaction, startTimestamp); | ||
} | ||
|
||
Future<void> trackRegularRouteTTD(ISentrySpan transaction, | ||
{required DateTime startTimestamp}) async { | ||
await _ttidTracker.trackRegularRoute(transaction, startTimestamp); | ||
await _trackTTFDIfEnabled(transaction, startTimestamp); | ||
} | ||
|
||
Future<void> _trackTTFDIfEnabled( | ||
ISentrySpan transaction, DateTime startTimestamp) async { | ||
if (enableTimeToFullDisplayTracing) { | ||
await _ttfdTracker?.track(transaction, startTimestamp); | ||
} | ||
} | ||
|
||
@internal | ||
Future<void> reportFullyDisplayed() async { | ||
return _ttfdTracker?.reportFullyDisplayed(); | ||
} | ||
|
||
void clear() { | ||
_ttidTracker.clear(); | ||
_ttfdTracker?.clear(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is TTID not released, yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
craft messed up the merging. I merged the ttid pr during the release of 7.17.0 so craft merged the changelog together in a wrong way when the release finished - but it's not included in the release changelog so all good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can release it together with ttfd on 7.18.0 this week