Skip to content

Commit 200cceb

Browse files
buenaflorbuenaflor
authored
Fix navigator observer behaviour on didPush root (#3111)
* Update * Update * Update * Update CHANGELOG * Update test * Update sentry_navigator_observer.dart * Update * Update * Fix analyze
1 parent eca355d commit 200cceb

File tree

4 files changed

+90
-46
lines changed

4 files changed

+90
-46
lines changed

‎CHANGELOG.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,10 @@
55
### Fixes
66

77
- Debug meta not loaded for split debug info only builds ([#3104](https://github.com/getsentry/sentry-dart/pull/3104))
8-
- TTID/TTFD root transactions ([#3099](https://github.com/getsentry/sentry-dart/pull/3099))
8+
- TTID/TTFD root transactions ([#3099](https://github.com/getsentry/sentry-dart/pull/3099), [#3111](https://github.com/getsentry/sentry-dart/pull/3111))
99
- Web, Linux and Windows now create a UI transaction for the root page
1010
- iOS, Android now correctly create idle transactions
11+
- Fixes behaviour of traceId generation and TTFD for app start
1112

1213
### Dependencies
1314

‎flutter/lib/src/navigation/sentry_navigator_observer.dart‎

Lines changed: 26 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,6 @@ class SentryNavigatorObserver extends RouteObserver<PageRoute<dynamic>> {
149149
return;
150150
}
151151

152-
_startNewTraceIfEnabled();
153152
_setCurrentRouteName(route);
154153
_setCurrentRouteNameAsTransaction(route);
155154

@@ -161,14 +160,15 @@ class SentryNavigatorObserver extends RouteObserver<PageRoute<dynamic>> {
161160

162161
_addWebSessions(from: previousRoute, to: route);
163162

164-
// Clearing the display tracker here is safe since didPush happens before the Widget is built
165-
_timeToDisplayTracker?.clear();
166-
167-
DateTime timestamp = _hub.options.clock();
168-
_finishTransaction(endTimestamp: timestamp);
163+
final routeName = _getRouteName(route) ?? _currentRouteName;
164+
if (routeName != null && routeName != '/') {
165+
// Don't generate a new trace on initial app start / root
166+
// During SentryFlutter.init a traceId is already created
167+
_startNewTraceIfEnabled();
169168

170-
final transactionContext = _createTransactionContext(route);
171-
_startTransaction(route, timestamp, transactionContext);
169+
// App start TTID/TTFD is taken care of by app start integrations
170+
_instrumentTimeToDisplayOnPush(routeName, route.settings.arguments);
171+
}
172172
}
173173

174174
@override
@@ -224,6 +224,21 @@ class SentryNavigatorObserver extends RouteObserver<PageRoute<dynamic>> {
224224
}
225225
}
226226

227+
void _instrumentTimeToDisplayOnPush(String routeName, Object? arguments) {
228+
if (!_enableAutoTransactions) {
229+
return;
230+
}
231+
232+
// Clearing the display tracker here is safe since didPush happens before the Widget is built
233+
_timeToDisplayTracker?.clear();
234+
235+
DateTime timestamp = _hub.options.clock();
236+
_finishTransaction(endTimestamp: timestamp);
237+
238+
final transactionContext = _createTransactionContext(routeName);
239+
_startTransaction(timestamp, transactionContext, arguments);
240+
}
241+
227242
void _addWebSessions({Route<dynamic>? from, Route<dynamic>? to}) async {
228243
final fromName = from != null ? _getRouteName(from) : null;
229244
final toName = to != null ? _getRouteName(to) : null;
@@ -266,11 +281,7 @@ class SentryNavigatorObserver extends RouteObserver<PageRoute<dynamic>> {
266281
}
267282
}
268283

269-
SentryTransactionContext? _createTransactionContext(Route<dynamic>? route) {
270-
final routeName = _getRouteName(route) ?? _currentRouteName;
271-
if (routeName == null) {
272-
return null;
273-
}
284+
SentryTransactionContext _createTransactionContext(String routeName) {
274285
return SentryTransactionContext(
275286
routeName,
276287
SentrySpanOperations.uiLoad,
@@ -280,21 +291,10 @@ class SentryNavigatorObserver extends RouteObserver<PageRoute<dynamic>> {
280291
}
281292

282293
Future<void> _startTransaction(
283-
Route<dynamic>? route,
284294
DateTime startTimestamp,
285-
SentryTransactionContext? transactionContext,
295+
SentryTransactionContext transactionContext,
296+
Object? arguments,
286297
) async {
287-
final routeName = _getRouteName(route) ?? _currentRouteName;
288-
final arguments = route?.settings.arguments;
289-
290-
// Skip root - app start integrations create TTID/TTFD for root
291-
final isRoot = routeName == '/';
292-
if (!_enableAutoTransactions || routeName == null || isRoot) {
293-
return;
294-
}
295-
if (transactionContext == null) {
296-
return;
297-
}
298298
_timeToDisplayTracker?.transactionId = transactionContext.spanId;
299299

300300
final transaction = _hub.startTransactionWithContext(

‎flutter/test/navigation/sentry_navigator_observer_test.dart‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,39 @@ void main() {
4141
});
4242

4343
group('$SentryNavigatorObserver', () {
44+
test('didPush on root does not start a transaction', () async {
45+
const name = '/';
46+
final currentRoute = route(RouteSettings(name: name));
47+
48+
const op = 'ui.load';
49+
final hub = _MockHub();
50+
final span = getMockSentryTracer(name: name);
51+
when(span.context).thenReturn(SentrySpanContext(operation: op));
52+
_whenAnyStart(hub, span);
53+
when(span.finished).thenReturn(false);
54+
when(span.status).thenReturn(SpanStatus.ok());
55+
when(span.startChild('ui.load.initial_display',
56+
description: anyNamed('description'),
57+
startTimestamp: anyNamed('startTimestamp')))
58+
.thenReturn(NoOpSentrySpan());
59+
60+
final sut = fixture.getSut(
61+
hub: hub,
62+
autoFinishAfter: Duration(seconds: 5),
63+
);
64+
65+
sut.didPush(currentRoute, null);
66+
67+
verifyNever(hub.startTransactionWithContext(
68+
any,
69+
startTimestamp: anyNamed('startTimestamp'),
70+
waitForChildren: anyNamed('waitForChildren'),
71+
autoFinishAfter: anyNamed('autoFinishAfter'),
72+
trimEnd: anyNamed('trimEnd'),
73+
onFinish: anyNamed('onFinish'),
74+
));
75+
});
76+
4477
test('didPush starts transaction', () async {
4578
const name = 'Current Route';
4679
final currentRoute = route(RouteSettings(name: name));

‎flutter/test/navigation/sentry_navigator_observer_traces_test.dart‎

Lines changed: 29 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -17,37 +17,47 @@ void main() {
1717

1818
group('SentryNavigatorObserver', () {
1919
group('when starting traces on navigation is enabled (default)', () {
20+
test('didPush should not start a new trace on root', () {
21+
final to = _route(RouteSettings(name: '/'));
22+
final beforeTraceId = fixture.hub.scope.propagationContext.traceId;
23+
24+
fixture.getSut().didPush(to, null);
25+
26+
final afterTraceId = fixture.hub.scope.propagationContext.traceId;
27+
expect(afterTraceId, equals(beforeTraceId));
28+
});
29+
2030
test('didPush should start a new trace', () {
2131
final from = _route(RouteSettings(name: 'From Route'));
2232
final to = _route(RouteSettings(name: 'To Route'));
23-
final before = fixture.hub.scope.propagationContext.traceId;
33+
final beforeTraceId = fixture.hub.scope.propagationContext.traceId;
2434

2535
fixture.getSut().didPush(to, from);
2636

27-
final after = fixture.hub.scope.propagationContext.traceId;
28-
expect(after, isNot(before));
37+
final afterTraceId = fixture.hub.scope.propagationContext.traceId;
38+
expect(afterTraceId, isNot(beforeTraceId));
2939
});
3040

3141
test('didPop should start a new trace', () {
3242
final from = _route(RouteSettings(name: 'From Route'));
3343
final to = _route(RouteSettings(name: 'To Route'));
34-
final before = fixture.hub.scope.propagationContext.traceId;
44+
final beforeTraceId = fixture.hub.scope.propagationContext.traceId;
3545

3646
fixture.getSut().didPop(to, from);
3747

38-
final after = fixture.hub.scope.propagationContext.traceId;
39-
expect(after, isNot(before));
48+
final afterTraceId = fixture.hub.scope.propagationContext.traceId;
49+
expect(afterTraceId, isNot(beforeTraceId));
4050
});
4151

4252
test('didReplace should start a new trace', () {
4353
final from = _route(RouteSettings(name: 'From Route'));
4454
final to = _route(RouteSettings(name: 'To Route'));
45-
final before = fixture.hub.scope.propagationContext.traceId;
55+
final beforeTraceId = fixture.hub.scope.propagationContext.traceId;
4656

4757
fixture.getSut().didReplace(newRoute: to, oldRoute: from);
4858

49-
final after = fixture.hub.scope.propagationContext.traceId;
50-
expect(after, isNot(before));
59+
final afterTraceId = fixture.hub.scope.propagationContext.traceId;
60+
expect(afterTraceId, isNot(beforeTraceId));
5161
});
5262

5363
group('execution order', () {
@@ -68,7 +78,7 @@ void main() {
6878
}
6979

7080
test(
71-
'didPush should call generateNewTrace before starting the transaction',
81+
'didPush should call generateNewTrace beforeTraceId starting the transaction',
7282
() {
7383
final from = _route(RouteSettings(name: 'From Route'));
7484
final to = _route(RouteSettings(name: 'To Route'));
@@ -98,36 +108,36 @@ void main() {
98108
test('didPush should not start a new trace', () {
99109
final from = _route(RouteSettings(name: 'From Route'));
100110
final to = _route(RouteSettings(name: 'To Route'));
101-
final before = fixture.hub.scope.propagationContext.traceId;
111+
final beforeTraceId = fixture.hub.scope.propagationContext.traceId;
102112

103113
fixture.getSut(enableNewTraceOnNavigation: false).didPush(to, from);
104114

105-
final after = fixture.hub.scope.propagationContext.traceId;
106-
expect(after, equals(before));
115+
final afterTraceId = fixture.hub.scope.propagationContext.traceId;
116+
expect(afterTraceId, equals(beforeTraceId));
107117
});
108118

109119
test('didPop should not start a new trace', () {
110120
final from = _route(RouteSettings(name: 'From Route'));
111121
final to = _route(RouteSettings(name: 'To Route'));
112-
final before = fixture.hub.scope.propagationContext.traceId;
122+
final beforeTraceId = fixture.hub.scope.propagationContext.traceId;
113123

114124
fixture.getSut(enableNewTraceOnNavigation: false).didPop(to, from);
115125

116-
final after = fixture.hub.scope.propagationContext.traceId;
117-
expect(after, equals(before));
126+
final afterTraceId = fixture.hub.scope.propagationContext.traceId;
127+
expect(afterTraceId, equals(beforeTraceId));
118128
});
119129

120130
test('didReplace should not start a new trace', () {
121131
final from = _route(RouteSettings(name: 'From Route'));
122132
final to = _route(RouteSettings(name: 'To Route'));
123-
final before = fixture.hub.scope.propagationContext.traceId;
133+
final beforeTraceId = fixture.hub.scope.propagationContext.traceId;
124134

125135
fixture
126136
.getSut(enableNewTraceOnNavigation: false)
127137
.didReplace(newRoute: to, oldRoute: from);
128138

129-
final after = fixture.hub.scope.propagationContext.traceId;
130-
expect(after, equals(before));
139+
final afterTraceId = fixture.hub.scope.propagationContext.traceId;
140+
expect(afterTraceId, equals(beforeTraceId));
131141
});
132142
});
133143
});

0 commit comments

Comments
 (0)