RUMM-2606 Launch Time Publisher #1030

maxep · 2022-10-20T14:53:35Z

What and why?

Replace the LaunchTimeReader by the LaunchTimePublisher.

How?

The _Datadog_Private expose a __dd_private_AppLaunchHandler interface for providing and listening to Application Launch time information.
The __dd_private_AppLaunchHandler provide a shared instance with atomic properties. Therefor, thread-safety is now ensured by instance lock and the pthread_rwlock_t has been removed.

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

datadog-datadog-prod-us1 · 2022-10-20T15:47:22Z

Datadog Report

Branch report: maxep/RUMM-2606/launch-time-publisher
Commit report: 3be6dc8

✅ dd-sdk-ios 0 Failed, 0 New Flaky, 1955 Passed, 0 Skipped, 17m57.87s Wall Time

maxep · 2022-10-21T12:03:14Z

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

+            // The launchTime can be `nil` if the application is not yet
+            // active (UIApplicationDidBecomeActiveNotification). That is
+            // the case when instrumenting a SwiftUI application that start
+            // a RUM view on `SwiftUI.View.onAppear`.
+            //
+            // In that case, we consider the time between the application
+            // launch and the view start as the application loadint time.
+            loadingTime = viewStartTime.timeIntervalSince(launchDate).toInt64Nanoseconds


This logic was previously enabled here: https://github.com/DataDog/dd-sdk-ios/blob/develop/Sources/_Datadog_Private/ObjcAppLaunchHandler.m#L92

In a SwiftUI application, the first SwiftUI.View/onAppear is called before the UIApplicationDidBecomeActiveNotification, so we consider the startView as the end of the launch sequence. It is best to have this logic explicitly stated in RUM.

ncreated

Nice work 👌! I left some feedback - mostly on ambiguities around launchDate optionality. LTMWDYT @maxep

Datadog/Datadog.xcodeproj/xcshareddata/xcschemes/DatadogIntegrationTests.xcscheme

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

ncreated · 2022-10-24T10:48:51Z

Sources/Datadog/DatadogInternal/Context/LaunchTime.swift

    /// If the `UIApplication.didBecomeActiveNotification` has not yet been received by the
    /// time this value is provided, it will represent the time interval between now and the process start time.
-    /* public */ let launchTime: TimeInterval
+    /* public */ let launchTime: TimeInterval?
+
+    /* public */ let launchDate: Date?


Needs docstrings explaining why launchTime and launchDate are optional.

Does launchDate need to be optional at all? Its obj-c counterpart seems to be not:

NS_ASSUME_NONNULL_BEGIN /// Returns the Application process launch date. @property (atomic, readonly) NSDate* launchDate;

Both interfaces are similar but decorrelated, LaunchTime is publicly exposed by DatadogInternal while the __dd_private_AppLaunchHandler is used internally by the publisher.

DatadogInternal allow nil for the launch date because it needs to defined a .zero value. So we either make DatadogContext/launchTime optional, are we keep DatadogContext/launchTime/launchDate optional 🤔
I think it makes more sense to make DatadogContext/launchTime optional, I will see what's the impact of this changes.

ncreated · 2022-10-24T10:49:24Z

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

+            //
+            // In that case, we consider the time between the application
+            // launch and the view start as the application loadint time.
+            loadingTime = viewStartTime.timeIntervalSince(launchDate).toInt64Nanoseconds
        }


Don't we introduce a regression now? Previously there was no unhandled control flow:

// before: } else { loadingTime = context.launchTime.launchTime.toInt64Nanoseconds }

and now there is:

// now: } else if let launchDate = context.launchTime.launchDate { // ... } // <--- unhandled

Seems that non-optional launchDate could solve it (ref. to above: #1030 (comment)).

Previously, the context.launchTime was 0 when not evaluated. I think it's more correct to have loadingTime == nil instead of 0. WDYT?

Agreed, now it describes much better with the nil 👌.

Sources/_Datadog_Private/ObjcAppLaunchHandler.m

Tests/DatadogTests/Datadog/RUM/RUMMonitor/Scopes/RUMViewScopeTests.swift

Co-Authored-By: Maciek Grzybowski <maciek.grzybowski@datadoghq.com>

ncreated

It looks good 👌🎯. I asked some clarification questions - sorry for being picky 😞🙌, I want to make sure "launch time" logic is obvious and well described to everyone (as it is quite popular topic).

ncreated · 2022-10-26T11:33:02Z

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

+            //
+            // In that case, we consider the time between the application
+            // launch and the view start as the application loadint time.
+            loadingTime = viewStartTime.timeIntervalSince(launchDate).toInt64Nanoseconds
        }


Agreed, now it describes much better with the nil 👌.

Sources/Datadog/DatadogInternal/Context/LaunchTime.swift

ncreated · 2022-10-26T11:48:01Z

Sources/Datadog/DatadogInternal/Context/DatadogContext.swift

+    /// Can be `nil` if the launch could not yet been evaluated.
+    var launchTime: LaunchTime?


So the nil value is quite unexpected and will be there only for a short while when SDK is being warmed up (this is what we mean by "evaluated"), right?

Yes, exactly. In practice, this value will be evaluated when we subscribe to the publisher, it will be done when initialising the core.

Co-authored-by: Maciek Grzybowski <maciek.grzybowski@datadoghq.com>

datadog-datadog-prod-us1 · 2022-11-02T13:02:38Z

Datadog Report

Branch report: maxep/RUMM-2606/launch-time-publisher
Commit report: e747497

✅ dd-sdk-ios 0 Failed, 0 New Flaky, 1955 Passed, 0 Skipped, 18m13.92s Wall Time

maxep self-assigned this Oct 20, 2022

RUMM-2606 Launch Time Publisher

60a8aff

maxep force-pushed the maxep/RUMM-2606/launch-time-publisher branch from 3798560 to 60a8aff Compare October 20, 2022 15:35

RUMM-2606 avoid recursive lock

fbc11e4

maxep force-pushed the maxep/RUMM-2606/launch-time-publisher branch from fb4a0c2 to fbc11e4 Compare October 21, 2022 11:03

maxep marked this pull request as ready for review October 21, 2022 11:03

maxep requested a review from a team as a code owner October 21, 2022 11:03

maxep commented Oct 21, 2022

View reviewed changes

ncreated requested changes Oct 24, 2022

View reviewed changes

maxep force-pushed the maxep/RUMM-2606/launch-time-publisher branch 2 times, most recently from 01f56ef to 1277817 Compare October 24, 2022 13:06

Apply suggestions from code review

f4d4fef

Co-Authored-By: Maciek Grzybowski <maciek.grzybowski@datadoghq.com>

maxep force-pushed the maxep/RUMM-2606/launch-time-publisher branch 2 times, most recently from 806d091 to fd55617 Compare October 24, 2022 13:17

RUMM-2606 Make launchTime optional

3be6dc8

maxep force-pushed the maxep/RUMM-2606/launch-time-publisher branch from fd55617 to 3be6dc8 Compare October 24, 2022 13:38

maxep requested a review from ncreated October 24, 2022 15:59

ncreated approved these changes Oct 26, 2022

View reviewed changes

Update Sources/Datadog/DatadogInternal/Context/LaunchTime.swift

e747497

Co-authored-by: Maciek Grzybowski <maciek.grzybowski@datadoghq.com>

maxep merged commit d620fe0 into feature/v2 Nov 2, 2022

maxep deleted the maxep/RUMM-2606/launch-time-publisher branch November 2, 2022 13:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUMM-2606 Launch Time Publisher #1030

RUMM-2606 Launch Time Publisher #1030

maxep commented Oct 20, 2022 •

edited

Loading

datadog-datadog-prod-us1 bot commented Oct 20, 2022 •

edited

Loading

maxep Oct 21, 2022

ncreated left a comment

ncreated Oct 24, 2022

maxep Oct 24, 2022

ncreated Oct 24, 2022

ncreated Oct 24, 2022

maxep Oct 24, 2022

ncreated Oct 26, 2022

ncreated left a comment

ncreated Oct 26, 2022

ncreated Oct 26, 2022

maxep Nov 2, 2022

datadog-datadog-prod-us1 bot commented Nov 2, 2022 •

edited

Loading

		/// Can be `nil` if the launch could not yet been evaluated.
		var launchTime: LaunchTime?

RUMM-2606 Launch Time Publisher #1030

RUMM-2606 Launch Time Publisher #1030

Conversation

maxep commented Oct 20, 2022 • edited Loading

What and why?

How?

Review checklist

Custom CI job configuration (optional)

datadog-datadog-prod-us1 bot commented Oct 20, 2022 • edited Loading

Datadog Report

Choose a reason for hiding this comment

ncreated 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

Choose a reason for hiding this comment

ncreated 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 Nov 2, 2022 • edited Loading

Datadog Report

maxep commented Oct 20, 2022 •

edited

Loading

datadog-datadog-prod-us1 bot commented Oct 20, 2022 •

edited

Loading

datadog-datadog-prod-us1 bot commented Nov 2, 2022 •

edited

Loading