RUMM-2507 Create an ApplicationLaunch view during session initialization #1160

fuzzybinary · 2023-02-08T18:55:50Z

What and why?

In order to more accurately track application launch and loading times, we are changing the way the ApplicationLaunch view work.

Now, initializing the Datadog SDK with RUM will create the ApplicationLaunch view by default, staring at the process launch time. This view can then track any resources, timing, or other events (including user actions) until the first call to startView.

The application_start event will now always be on the ApplicationLaunch view, but it's timing remains unchanged.

In the event of an active prewarm, the staring time of the ApplicationLaunch view is changed to the time the SDK was initialized, and the loading time of the application_start event is ignored.

How?

One big change to the UnitTests here is that now requesting RUM events from the matchers will discard the ApplicationLaunch view, as most tests assume such a view doesn't exist by default.

Review checklist

Feature or bugfix MUST have appropriate tests (unit, integration)
Make sure each commit and the PR mention the Issue number or JIRA reference
Add CHANGELOG entry for user facing changes

Custom CI job configuration (optional)

Run unit tests
Run integration tests
Run smoke tests

buraizu

Thanks, just made one small suggestion and also noticed a missing link

buraizu · 2023-02-08T23:05:31Z

docs/rum_collection/data_collected.md

+
+The ApplicationLaunch view includes any logs, actions, and resources created before your first call to `startView`. Use the duration of this view to determine time to first view. This view has an action, `application_start`, with a duration equal to the amount of time from process start until the call to `applicationDidBecomeActive`.
+
+In cases where iOS decides to [prewarm your application][4], the ApplicationLaunch view instead starts when the RUM iOS SDK is initialized, and the `application_start` event does not have a duration.


Suggested change

In cases where iOS decides to [prewarm your application][4], the ApplicationLaunch view instead starts when the RUM iOS SDK is initialized, and the `application_start` event does not have a duration.

In cases where iOS [prewarms your application][4], the ApplicationLaunch view instead starts when the RUM iOS SDK is initialized, and the `application_start` event does not have a duration.

Looks like the link for [4] has not beed added yet

Good catch, thanks!

datadog-datadog-prod-us1 · 2023-02-10T13:56:36Z

Datadog Report

Branch report: jward/RUMM-2507-app-launch-view
Commit report: 1ab7b2e

✅ dd-sdk-ios: 0 Failed, 0 New Flaky, 229 Passed, 0 Skipped, 15m 42.79s Wall Time

ncreated · 2023-02-14T10:01:31Z

Sources/Datadog/RUM/RUMMonitor/Scopes/RUMSessionScope.swift

-    private func startApplicationLaunchView(on command: RUMCommand, context: DatadogContext) {
+    internal func startApplicationLaunchView(context: DatadogContext, writer: Writer) {


suggestion/ So far the RUMScopes architecture was only allowing side-effects that happen in response to RUMCommand processing. Everything was implicit and the only internal API on scopes was:

func process(command: RUMCommand, context: DatadogContext, writer: Writer) -> Bool

Here we seem to break this convention and add a precedence: the "start app launch view" routine becomes explicit and can be called from outside, causing session scope to call arbitrary API on view scope. This adds extra complexity to an already complicated RUM architecture - to fully test it now, we'd need to cover all extra usages, like calling startApplicationLaunchView() in already running RUM session.

Could we consider adopting this change without introducing new internal APIs in scopes? At the surface, it looks doable, because RUMSessionScope has isInitialSession flag and RUMViewScope has its isInitialView attribute. It looks that passing necessary timings and != .background state to scopes .init() could be enough.

Alternatively, maybe introducing RUMStartApplicationLaunchView command could make RUM code simpler? Such command could be sent once from RUMMonitor (right after initialising scopes). It shouldn't require introducing much new pieces to RUM scopes as well. Also, because it will be RUMMonitor who emits that command, this won't require such strong surgery in RUM scopes unit tests like we had to do in this PR - the new logic would be additive.

I wonder what you think @maxep @fuzzybinary ?

ncreated · 2023-02-14T10:09:42Z

Tests/DatadogTests/Matchers/RUMEventMatcher.swift

@@ -6,6 +6,7 @@

 import Foundation
 import XCTest
+import Datadog


I think we can't let this import here. The comment below says:

Note: this file is individually referenced by integration tests target, so no dependency on other source files

and IIRC this won't build in UI tests (the UI tests runner doesn't link Datadog).

It seems to be a leftover after some other changes, no? I don't see any Datadog symbols used below.

Yes, it's leftover. I originally did @testable import Datadog which definitely didn't build for integration tests, though this did.

ncreated

It looks okay and I really appreciate the extra care for unit tests and changes in RUMSessionMatcher we do here 👌.

We need to fix at least CI build (in integration tests) to move forward.

On top of that, I left an architectural suggestion. I wonder what you think @fuzzybinary and if it sounds like something that could simplify our implementation.

fuzzybinary · 2023-02-14T13:36:01Z

It's a good point, let's talk this morning and see which approach we like better. I'm leaning toward RUMStartApplicationLaunchView command, as we can have the initial view be not the ApplicationLaunch view.

This leaves the functionality of the `application_start` action intact, but has it always added to it's own ApplicationLaunch view, instead of onto the first view that appears.

Since the RUMEventMatchers are used in integration tests, @testable isn't available for the Datadog module.

Instead of exposing the `startInitialSession` and `sendApplicaitonLaunchEvent` funcitons, we'll instead use a specific command to do both.

louiszawadzki · 2023-02-15T08:39:08Z

Sources/Datadog/RUM/RUMMonitor/Scopes/RUMViewScope.swift

+            if !isInitialView || viewPath != RUMOffViewEventsHandlingRule.Constants.applicationLaunchViewURL {
+                DD.telemetry.error(
+                    id: "application-start-error",
+                    message: "An RUMApplciationStartCommand got sent to something other than the ApplicaitonLaunch view.",


Suggested change

message: "An RUMApplciationStartCommand got sent to something other than the ApplicaitonLaunch view.",

message: "A RUMApplicationStartCommand got sent to something other than the ApplicationLaunch view.",

Co-authored-by: Louis Zawadzki <louiszawadzki@users.noreply.github.com>

maxep

Looks great! I've left a small suggestion.

maxep · 2023-02-15T13:21:08Z

Sources/Datadog/RUM/RUMMonitor/Scopes/RUMViewScope.swift

+                DD.telemetry.error(
+                    id: "application-start-error",
+                    message: "An RUMApplicationStartCommand got sent to something other than the ApplicationLaunch view.",
+                    kind: nil,
+                    stack: nil
+                )


/nit You can use other signature available

Suggested change

DD.telemetry.error(

id: "application-start-error",

message: "An RUMApplicationStartCommand got sent to something other than the ApplicationLaunch view.",

kind: nil,

stack: nil

)

DD.telemetry.error("An RUMApplicationStartCommand got sent to something other than the ApplicationLaunch view.")

The id is inferred using file:line:message.

Maybe we can add the command type as part of the message, I'm afraid something is not super actionable :)

ncreated · 2023-02-15T15:43:03Z

Sources/Datadog/RUM/RUMMonitor/Scopes/RUMApplicationScope.swift

    func process(command: RUMCommand, context: DatadogContext, writer: Writer) -> Bool {
        if sessionScope == nil {
-            startInitialSession(context: context)
+            startInitialSession(on: command, context: context, writer: writer)


It rather seems we're starting initial session (along with the ApplicationLaunch view) on first command received by RUMApplicationScope. It looks different than what we say in PR description (and what we want):

Now, initializing the Datadog SDK with RUM will create the ApplicationLaunch view by default, staring at the process launch time.

No? If that is true, it would make more sense to just send the new command right after application scope init, somewhere here.

Ah, I think I get it - we always do it (start the AL view), it is just that we await for any other command to do it. Makes sense - if the app starts idle and nothing happens, no session will be started 👍.

That was my thought yes. It also made things a little bit cleaner as I could just use the context / writer as part of the command I was already recieving.

ncreated

Looks great! Thanks for looking into this alternative approach @fuzzybinary 💪. I left last question to make sure we got the requirement right - other than this, good to go 🚀.

…nLaunch view instead.

fuzzybinary requested review from a team as code owners February 8, 2023 18:55

buraizu requested changes Feb 8, 2023

View reviewed changes

fuzzybinary force-pushed the jward/RUMM-2507-app-launch-view branch from 2be7c77 to 8d69847 Compare February 9, 2023 15:04

buraizu approved these changes Feb 9, 2023

View reviewed changes

fuzzybinary force-pushed the jward/RUMM-2507-app-launch-view branch 2 times, most recently from 523f160 to 570a9cf Compare February 10, 2023 13:44

ncreated reviewed Feb 14, 2023

View reviewed changes

fuzzybinary added 4 commits February 14, 2023 09:25

RUMM-2507 Create an ApplicationLaunch view during session initialization

0eca578

This leaves the functionality of the `application_start` action intact, but has it always added to it's own ApplicationLaunch view, instead of onto the first view that appears.

RUMM-2507 Add CHANGELOG item

727fd54

Fix build issue

34cfc1e

Since the RUMEventMatchers are used in integration tests, @testable isn't available for the Datadog module.

RUMM-2507 Fix Integration Tests

add1dae

fuzzybinary force-pushed the jward/RUMM-2507-app-launch-view branch from b8a2a54 to af362be Compare February 14, 2023 18:14

RUMM-2507 Switch to using RUMStartApplicationCommand

40e29cf

Instead of exposing the `startInitialSession` and `sendApplicaitonLaunchEvent` funcitons, we'll instead use a specific command to do both.

fuzzybinary force-pushed the jward/RUMM-2507-app-launch-view branch from af362be to 40e29cf Compare February 14, 2023 19:34

louiszawadzki reviewed Feb 15, 2023

View reviewed changes

Update Sources/Datadog/RUM/RUMMonitor/Scopes/RUMViewScope.swift

0fbf63d

Co-authored-by: Louis Zawadzki <louiszawadzki@users.noreply.github.com>

maxep approved these changes Feb 15, 2023

View reviewed changes

Fix test compilation

a75a83c

fuzzybinary force-pushed the jward/RUMM-2507-app-launch-view branch from 1e83b86 to a75a83c Compare February 15, 2023 15:12

Minor language / signature change.

f1fd808

ncreated reviewed Feb 15, 2023

View reviewed changes

ncreated approved these changes Feb 15, 2023

View reviewed changes

fuzzybinary added 3 commits February 15, 2023 12:39

RUMM-2507 Fix tests from launch timing change

125b1f6

RUMM-2507 Fix application start time logic for SwiftUI

1ab7b2e

RUMM-2507 Fix crash tests to look for applicaiton start on Applicatio…

dc406a4

…nLaunch view instead.

fuzzybinary merged commit c1e845e into develop Feb 21, 2023

fuzzybinary deleted the jward/RUMM-2507-app-launch-view branch February 21, 2023 20:03

ncreated mentioned this pull request Feb 23, 2023

Dogfood recent changes #1174

Merged

6 tasks

maxep mentioned this pull request Mar 1, 2023

Update feature/v2 with develop #1183

Merged

6 tasks

maciejburda mentioned this pull request Mar 1, 2023

Release 1.16.0 #1186

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUMM-2507 Create an ApplicationLaunch view during session initialization #1160

RUMM-2507 Create an ApplicationLaunch view during session initialization #1160

fuzzybinary commented Feb 8, 2023

buraizu left a comment

buraizu Feb 8, 2023

buraizu Feb 8, 2023

fuzzybinary Feb 9, 2023

datadog-datadog-prod-us1 bot commented Feb 10, 2023 •

edited

Loading

ncreated Feb 14, 2023

ncreated Feb 14, 2023

fuzzybinary Feb 14, 2023

ncreated left a comment

fuzzybinary commented Feb 14, 2023

louiszawadzki Feb 15, 2023 •

edited

Loading

maxep left a comment

maxep Feb 15, 2023

maxep Feb 15, 2023

ncreated Feb 15, 2023

ncreated Feb 15, 2023

fuzzybinary Feb 15, 2023

ncreated left a comment


		The ApplicationLaunch view includes any logs, actions, and resources created before your first call to `startView`. Use the duration of this view to determine time to first view. This view has an action, `application_start`, with a duration equal to the amount of time from process start until the call to `applicationDidBecomeActive`.

		In cases where iOS decides to [prewarm your application][4], the ApplicationLaunch view instead starts when the RUM iOS SDK is initialized, and the `application_start` event does not have a duration.

	In cases where iOS decides to [prewarm your application][4], the ApplicationLaunch view instead starts when the RUM iOS SDK is initialized, and the `application_start` event does not have a duration.
	In cases where iOS [prewarms your application][4], the ApplicationLaunch view instead starts when the RUM iOS SDK is initialized, and the `application_start` event does not have a duration.

		private func startApplicationLaunchView(on command: RUMCommand, context: DatadogContext) {
		internal func startApplicationLaunchView(context: DatadogContext, writer: Writer) {

	message: "An RUMApplciationStartCommand got sent to something other than the ApplicaitonLaunch view.",
	message: "A RUMApplicationStartCommand got sent to something other than the ApplicationLaunch view.",

RUMM-2507 Create an ApplicationLaunch view during session initialization #1160

RUMM-2507 Create an ApplicationLaunch view during session initialization #1160

Conversation

fuzzybinary commented Feb 8, 2023

What and why?

How?

Review checklist

Custom CI job configuration (optional)

buraizu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datadog-datadog-prod-us1 bot commented Feb 10, 2023 • edited Loading

Datadog Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncreated left a comment

Choose a reason for hiding this comment

fuzzybinary commented Feb 14, 2023

louiszawadzki Feb 15, 2023 • edited Loading

Choose a reason for hiding this comment

maxep left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncreated left a comment

Choose a reason for hiding this comment

datadog-datadog-prod-us1 bot commented Feb 10, 2023 •

edited

Loading

louiszawadzki Feb 15, 2023 •

edited

Loading