-
Notifications
You must be signed in to change notification settings - Fork 71
feat: Add reportAppFullyDisplayed to RUM for app launch time to full display (TTFD) #1124
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
base: develop
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
|
|
@@ -187,6 +187,33 @@ void main() { | |
| expect(view1.vitalStepEvents[2].vitalOperationKey, isNull); | ||
| expect(view1.vitalStepEvents[2].vitalFailureReason, 'error'); | ||
|
|
||
| // `reportAppFullyDisplayed` reports TTFD as an app launch vital rather than | ||
| // as a property of the view it was called from, so look for it across the | ||
| // whole session. | ||
| // | ||
| // This is only checked on iOS. The Browser SDK has no equivalent API, and | ||
| // the Android SDK only sends TTFD once it has computed TTID for the startup | ||
| // scenario, which does not happen in this app -- it sends no app launch | ||
| // vitals at all, so there is nothing to assert on there yet. | ||
| if (!kIsWeb && Platform.isIOS) { | ||
| final ttfdVitals = rumLog | ||
| .where((e) => | ||
| e.eventType == 'vital' && | ||
| RumVitalAppLaunchEventDecoder.isAppLaunchVital(e.rumEvent)) | ||
| .map((e) => RumVitalAppLaunchEventDecoder(e.rumEvent)) | ||
| .where((e) => e.appLaunchMetric == 'ttfd') | ||
| .toList(); | ||
|
Comment on lines
+199
to
+205
Member
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'll likely refactor this to have all vitals accessible on the |
||
|
|
||
| // Only the first call to `reportAppFullyDisplayed` is reported. | ||
| expect(ttfdVitals.length, 1); | ||
| // TTFD is measured from the launch of the app, so it should be at least | ||
| // as long as the fake loading the scenario performs before reporting it. | ||
| expect(ttfdVitals[0].duration, | ||
| greaterThanOrEqualTo(const Duration(milliseconds: 50).inNanoseconds)); | ||
| expect(ttfdVitals[0].duration, | ||
| lessThan(const Duration(seconds: 60).inNanoseconds)); | ||
|
Comment on lines
+213
to
+214
Member
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'll see if this is enough for CI or if it flakes 😅. |
||
| } | ||
|
|
||
| // Verify user in all events, except for the first view event | ||
| for (final viewEvent in view1.viewEvents.sublist(1)) { | ||
| verifyUser(viewEvent); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -202,6 +202,14 @@ class DdRumMethodChannel extends DdRumPlatform { | |
| }); | ||
| } | ||
|
|
||
| @override | ||
| Future<void> reportAppFullyDisplayed() { | ||
| return methodChannel.invokeMethod( | ||
| 'reportAppFullyDisplayed', | ||
| <String, Object?>{}, | ||
|
Member
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. nit: We tend to add the type annotations on these maps, especially if they're empty.
Contributor
Author
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. The map is already
Member
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. Sorry, I missed a "not" in there. We tend to not add the type annotations on these maps. It slipped in on |
||
| ); | ||
| } | ||
|
|
||
| @override | ||
| Future<void> addErrorInfo( | ||
| DateTime timestamp, | ||
|
|
||
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.
If that's the case, we may want to hold off on this change. I know we're in the process of looking into TTID for Android. Let me look into the state of it and I'll get back to you.