Allow users to manually end a RUM session #1219

fuzzybinary · 2023-03-21T21:21:33Z

What and why?

This adds a method to manually end a RUM session with stopRumSession. This immediately stops the active view and sets the current session to inactive. A new session starts automatically when a user action is sent (startView or addUserAction).

How?

First, may RUM events now have a new property: session.isActive. This can be null, and is only currently set in RUMViewEvents. The state of this member is pulled from the parent session context.

Second, I've modified RUMApplicationScope to support holding multiple RUMSessionScopes in a similar way to how Sessions can handle multiple Views until all of their events have been processed.

When a RUMStopSession event is received, the Application Scope forwards it to the Session, which marks itself as inactive, then forwards the event to the all views, which mark themselves as inactive and send a ViewUpdate event which captures the session now being inactive.

Provided views have finished processing all of their user actions and resources, the session is removed from available sessions on the ApplicationScope. Otherwise, it sicks around (marked as inactive) until all events are complete.

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

This includes most of the work for the SessionScope for stopping a session, but does not include any ApplicationScope work for restarting sessions or ViewScope work for sending session status to intake.

Adds `is_active` to Session information. Default it to true for the time being.

This requires ApplicationScope keep multiple sessions, and discard them as they're completed.

fuzzybinary · 2023-03-22T19:29:29Z

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

+
+        if command is RUMStopSessionCommand {
+            // Reach in and grab the last active view
+            lastActiveView = activeSession?.viewScopes.first(where: { $0.isActiveView } )


I'm interested in thoughts on this approach. This feels like it's pulling in some extra dependencies in the ViewScope, but I couldn't find a good way to make activating the previous SessionScope's view work, even with the existing refresh logic.

maciejburda

Great Effort!

Left a couple of minor suggestions 🙌

maciejburda · 2023-03-23T09:45:37Z

Datadog/Example/Scenarios/RUM/StopSessionScenario/KioskSendEventsViewController.swift

+        rumMonitor.stopView(viewController: self)
+    }
+
+    @IBAction func didTapDownloadResourceButton(_ sender: Any) {


If I'm not mistaken you can change the sender type to UIButton and avoid force cast

maciejburda · 2023-03-23T09:50:16Z

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

+            if scope.isActive {
+                // False, but still active means we timed out or expired, refresh the session


From the comment it feels like isSessionActive should be checked here as well?

I don't see this param used elsewhere.

I'm confused by this? Do you mean applicationActive?

I don't get what False is referring to here, since we are inside of isActive, which is true

Ah, It's the return value of scope.process If it returned true return the scope, False, we check isActive. I'll change the comment to make it clearer.

maciejburda · 2023-03-23T09:50:39Z

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

+            return nil
+        })
+
+        // Sanity telemety, only end up with one active session


maciejburda · 2023-03-23T09:52:08Z

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

@@ -135,51 +158,63 @@ internal class RUMSessionScope: RUMScope, RUMContextProvider {
        }

        if !isSampled {
-            return true // discard all events in this session
+            // Make sure sessions end even if they are sampled


nice catch!

maciejburda · 2023-03-23T09:53:45Z

Sources/Datadog/RUMMonitor.swift

-                                self.applicationScope.sessionScope?.context ??
-                                self.applicationScope.context


I think this odd formatting is more common for ??, but if linter is not complaining I think it's fine

TBH I think the linter is what changed it.

maciejburda · 2023-03-23T09:54:17Z

Sources/Datadog/RUMMonitor.swift


                guard context.sessionID != .nullUUID else {
-                    // if Session was sampled or not yet started


Is that not true anymore?

I had some issues with this function I may have removed the comment by accident. I'll put it back.

Tests/DatadogIntegrationTests/Scenarios/RUM/RUMCommonAsserts.swift

…wift Co-authored-by: Maciej Burda <maciej.burda@datadoghq.com>

maxep · 2023-03-24T12:24:08Z

Sources/Datadog/RUM/RUMMonitor/RUMCommand.swift

+    var time: Date
+    var attributes: [AttributeKey: AttributeValue] = [:]
+    var canStartBackgroundView = false // no, stopping a session should not start a backgorund session
+    var isUserInteraction = false


/nit if we are not mutating any of those, they can be let 👍

They're var getters in the RUMCommand protocol. Is it possible to change them to let in the implementation?

Yes, if they are get only, they can be `let 👍

maxep · 2023-03-24T15:30:23Z

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

+        var deactivating = false
+        if isActive {
+            if command is RUMStopSessionCommand {
+                isActive = false
+                deactivating = true
+            } else if let startApplicationCommand = command as? RUMApplicationStartCommand {
+                startApplicationLaunchView(on: startApplicationCommand, context: context, writer: writer)
+            } else if let startViewCommand = command as? RUMStartViewCommand {
+                // Start view scope explicitly on receiving "start view" command
+                startView(on: startViewCommand, context: context)
+            } else if !hasActiveView {
+                // Otherwise, if there is no active view scope, consider starting artificial scope for handling this command
+                let handlingRule = RUMOffViewEventsHandlingRule(
+                    sessionState: state,
+                    isAppInForeground: context.applicationStateHistory.currentSnapshot.state.isRunningInForeground,
+                    isBETEnabled: backgroundEventTrackingEnabled
+                )

-            switch handlingRule {
-            case .handleInBackgroundView where command.canStartBackgroundView:
-                startBackgroundView(on: command, context: context)
-            default:
-                if !(command is RUMKeepSessionAliveCommand) { // it is expected to receive 'keep alive' while no active view (when tracking WebView events)
-                    // As no view scope will handle this command, warn the user on dropping it.
-                    DD.logger.warn(
+                switch handlingRule {
+                case .handleInBackgroundView where command.canStartBackgroundView:
+                    startBackgroundView(on: command, context: context)
+                default:
+                    if !(command is RUMKeepSessionAliveCommand) { // it is expected to receive 'keep alive' while no active view (when tracking WebView events)
+                        // As no view scope will handle this command, warn the user on dropping it.
+                        DD.logger.warn(
                        """
                        \(String(describing: command)) was detected, but no view is active. To track views automatically, try calling the
                        DatadogConfiguration.Builder.trackUIKitRUMViews() method. You can also track views manually using
                        the RumMonitor.startView() and RumMonitor.stopView() methods.
                        """
-                    )
+                        )
+                    }
                }
            }
        }


The conditions and state management are difficult to follow and error prone IMO. It might be helpful to split this into multiple methods, would that be possible?

I moved the "no active view" portion to its own function. Let me know if that helps or it you think it needs more.

Perfect, thank you!

maxep

LGTM 🚀

fuzzybinary added 3 commits March 10, 2023 15:59

RUMM-2872 Initial work for stopping sessions

2878276

This includes most of the work for the SessionScope for stopping a session, but does not include any ApplicationScope work for restarting sessions or ViewScope work for sending session status to intake.

RUMM-2872 Update RUM protocol schema

412f5fa

Adds `is_active` to Session information. Default it to true for the time being.

RUMM-2872 Add session restart logic to ApplicationScope

4686c79

This requires ApplicationScope keep multiple sessions, and discard them as they're completed.

fuzzybinary requested a review from a team as a code owner March 21, 2023 21:21

RUMM-2872 Add integration tests for stopped sessions

93db921

fuzzybinary force-pushed the jward/RUMM-2872-manually-end-session branch from b594e2d to 93db921 Compare March 22, 2023 14:44

fuzzybinary commented Mar 22, 2023

View reviewed changes

fuzzybinary force-pushed the jward/RUMM-2872-manually-end-session branch from 4a04e61 to e64a3e8 Compare March 22, 2023 19:41

RUMM-2872 Restart last known view on a new session

076b87a

fuzzybinary force-pushed the jward/RUMM-2872-manually-end-session branch from e64a3e8 to 076b87a Compare March 22, 2023 20:54

maciejburda approved these changes Mar 23, 2023

View reviewed changes

Update Tests/DatadogIntegrationTests/Scenarios/RUM/RUMCommonAsserts.s…

046151a

…wift Co-authored-by: Maciej Burda <maciej.burda@datadoghq.com>

maxep reviewed Mar 24, 2023

View reviewed changes

RUMM-2872 Apply code review feedback

05a8278

fuzzybinary force-pushed the jward/RUMM-2872-manually-end-session branch from 0155739 to 05a8278 Compare March 27, 2023 15:03

maxep approved these changes Mar 27, 2023

View reviewed changes

fuzzybinary merged commit 5a657d8 into develop Mar 27, 2023

fuzzybinary deleted the jward/RUMM-2872-manually-end-session branch March 27, 2023 16:56

ncreated mentioned this pull request Mar 31, 2023

Dogfood recent changes #1232

Merged

6 tasks

maxep mentioned this pull request Apr 3, 2023

Update feature/v2 with develop #1234

Merged

6 tasks

maciejburda mentioned this pull request Apr 19, 2023

Release 1.18.0 #1254

Merged

ncreated mentioned this pull request Nov 14, 2023

RUM-1335 Add sessionPrecondition to RUM events #1542

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow users to manually end a RUM session #1219

Allow users to manually end a RUM session #1219

fuzzybinary commented Mar 21, 2023

fuzzybinary Mar 22, 2023

maciejburda left a comment

maciejburda Mar 23, 2023

maciejburda Mar 23, 2023

fuzzybinary Mar 23, 2023

maciejburda Mar 23, 2023

fuzzybinary Mar 23, 2023

maciejburda Mar 23, 2023

maciejburda Mar 23, 2023

maciejburda Mar 23, 2023

fuzzybinary Mar 23, 2023

maciejburda Mar 23, 2023

fuzzybinary Mar 23, 2023

maxep Mar 24, 2023

fuzzybinary Mar 24, 2023

maxep Mar 27, 2023

fuzzybinary Mar 27, 2023

maxep Mar 24, 2023

fuzzybinary Mar 27, 2023

maxep Mar 27, 2023

maxep left a comment

		if scope.isActive {
		// False, but still active means we timed out or expired, refresh the session

		self.applicationScope.sessionScope?.context ??
		self.applicationScope.context


		guard context.sessionID != .nullUUID else {
		// if Session was sampled or not yet started

Allow users to manually end a RUM session #1219

Allow users to manually end a RUM session #1219

Conversation

fuzzybinary commented Mar 21, 2023

What and why?

How?

Review checklist

Custom CI job configuration (optional)

Choose a reason for hiding this comment

maciejburda 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

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

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

maxep left a comment

Choose a reason for hiding this comment