RUMM-2873 Allow users to stop a RUM session by fuzzybinary · Pull Request #1356 · DataDog/dd-sdk-android

fuzzybinary · 2023-03-27T23:27:05Z

What does this PR do?

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).

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.

I've modified RumApplicationScope to support holding multiple RumSessionScopes in a similar way to how RumViewManager 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 (to be filled by reviewers)

Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
Make sure you discussed the feature or bugfix with the maintaining team in an Issue
Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

Still need to add tests that ensure that the RUM Context is updated on the core at the proper times and add integration tests.

0xnm

I've added few comments, most of them are quite minor and not related to the functional part of the change, but some are for clarifying the functional part.

0xnm · 2023-03-28T07:08:56Z

+        app:layout_constraintHorizontal_bias="0.498"
+        app:layout_constraintStart_toStartOf="parent"
+        app:layout_constraintTop_toBottomOf="@+id/end_session" />
+
+    <Button
+        android:id="@+id/end_session"
+        android:layout_width="wrap_content"
+        android:layout_height="wrap_content"
+        android:layout_marginTop="140dp"
+        android:text="@string/end_session"
+        app:layout_constraintEnd_toEndOf="parent"
+        app:layout_constraintHorizontal_bias="0.5"


minor: probably we don't need ConstraintLayout with bias 0.5/0.498 (and 0.498/0.501 for another layout). Good old LinearLayout with gravity should work.

Just used to doing it from iOS at this point 🙂

0xnm · 2023-03-28T07:11:35Z

+    lateinit var endSessionButton: Button
+    lateinit var startKioskButton: Button


no need to have properties, can be just variables inside method.

0xnm · 2023-03-28T07:21:35Z

                    session = ViewEvent.ViewEventSession(
                        id = rumContext.sessionId,
                        type = ViewEvent.ViewEventSessionType.USER,
-                        hasReplay = hasReplay
+                        hasReplay = hasReplay,
+                        isActive = rumContext.isSessionActive


I believe we are hitting the problem here by relying on the rumContext. Imagine the following situation: session was stopped, but view needs to wait for the child events to complete. But before they complete, another session was started, meaning by the time this view will receive terminal view update, it will be another session in the RumContext (active=true with another session ID), different from the one which view was attributed to originally.

I think the only solution in this case is to take session ID at the time view is constructed and rely on it, instead of relying on the shared RumContext.

I'll double check, but I think we're good here because this isn't the shared global RUM context. It's created by going up / down the scope tree, so a new session starting will be in a different tree than this.

0xnm · 2023-03-28T07:22:55Z

 internal data class RumContext(
    val applicationId: String = NULL_UUID,
    val sessionId: String = NULL_UUID,
+    val isSessionActive: Boolean = true,


I believe it should be false by default, to have a consistency with default sessionId which is NULL_UUID, meaning there is no session.

0xnm · 2023-03-28T07:45:46Z

+
+        // Then
+        val newSession = testedScope.activeSession
+        check(newSession is RumSessionScope)


I guess check is used instead of assertThat(...).isInstanceOf(...) to benefit from smart case below?

Yes. I was following other examples but that seems to be the reason it was done there so I kept it 🙂

0xnm · 2023-03-28T07:51:35Z

+                sessionIsActive = false
+            )
+        )
+
+        onView(withId(R.id.end_session)).perform(click())


why do we expect sessionIsActive = false before we click on End Session? shouldn't onView(withId(R.id.end_session)).perform(click()) be above?

This is a weird thing about how these tests are written. I need to capture current scope before I actually end the session, so I have to create the expected event before I click the button. The same reason the other tests add the expected "stop view" before actually stopping the view.

Co-authored-by: Nikita Ogorodnikov <[email protected]>

Add more tests checking for session state.

Also add stopSession to untestable APIs

This is used over pulling the RUMViewScope during session stop.

codecov-commenter · 2023-03-31T19:39:42Z

Codecov Report

Merging #1356 (c08ad2f) into develop (0b7868b) will increase coverage by 0.08%.
The diff coverage is 86.23%.

@@             Coverage Diff             @@
##           develop    #1356      +/-   ##
===========================================
+ Coverage    81.87%   81.95%   +0.08%     
===========================================
  Files          362      363       +1     
  Lines        12903    13014     +111     
  Branches      2164     2187      +23     
===========================================
+ Hits         10564    10665     +101     
- Misses        1679     1688       +9     
- Partials       660      661       +1

Impacted Files	Coverage Δ
.../main/kotlin/com/datadog/android/rum/RumMonitor.kt	`79.63% <ø> (ø)`
.../android/rum/internal/monitor/DatadogRumMonitor.kt	`83.66% <33.33%> (-0.84%)`	⬇️
...g/android/rum/internal/domain/scope/RumRawEvent.kt	`82.21% <66.67%> (-0.29%)`	⬇️
...d/rum/internal/domain/scope/RumApplicationScope.kt	`86.08% <84.51%> (-13.92%)`	⬇️
...d/rum/internal/domain/scope/RumViewManagerScope.kt	`90.70% <85.71%> (-1.34%)`	⬇️
...droid/rum/internal/domain/scope/RumSessionScope.kt	`91.01% <94.12%> (+1.27%)`	⬆️
.../android/rum/internal/domain/scope/RumViewScope.kt	`94.31% <94.12%> (-0.03%)`	⬇️
.../datadog/android/rum/internal/domain/RumContext.kt	`97.44% <100.00%> (+0.07%)`	⬆️
...um/internal/domain/scope/RumViewChangedListener.kt	`100.00% <100.00%> (ø)`

... and 12 files with indirect coverage changes

0xnm

lgtm! I've left some comments, but they are mostly not related to the main functionality.

0xnm · 2023-04-03T08:33:23Z

+        applicationDisplayed
    )

+    internal val lastActiveViewScope: RumViewScope?


I think it is not needed anymore?

Yes, good catch.

0xnm · 2023-04-03T08:48:53Z

+        childScopes.add(newSession)
+        if (event !is RumRawEvent.StartView) {
+            lastActiveViewInfo?.let {
+                it.keyRef.get()?.let { key ->


should we log the case when key is missing? just to simplify debugging on the customer side.

Added a message. Can you review that it makes sense?

yes, totally makes sense!

0xnm · 2023-04-03T08:53:29Z

+        )
+
+        // Then
+        assertThat(testedScope.childScopes.count()).isEqualTo(1)


it will give more meaningful error

Suggested change

assertThat(testedScope.childScopes.count()).isEqualTo(1)

assertThat(testedScope.childScopes).hasSize(1)

0xnm · 2023-04-03T08:54:38Z

+    }
+
+    @Test
+    fun `M create a new session W handleEvent { no active sessions, user interaction } `(


it seems that name of the test is a bit misleading, here we send the view event and not an action, so I guess it should not be user interaction in the name?

0xnm

lgtm! I've added a minor comment to help debugging.

0xnm · 2023-04-03T14:07:08Z


+    val activeSession: RumScope?
+        get() {
+            return childScopes.find { it.isActive() }


just a question out of my head: find will return first element matching the criteria. Normally we shouldn't expect childScopes to contain multiple isActive sessions, but for the debugging do you think it will be useful to log when this invariant (only one active session) is broken? wdyt?

Yes, I actually have a similar telemetry in iOS. Maybe not here, though as it could get noisy. I'll see if I can find a better place to add it.

0xnm · 2023-04-03T14:07:41Z

+        childScopes.add(newSession)
+        if (event !is RumRawEvent.StartView) {
+            lastActiveViewInfo?.let {
+                it.keyRef.get()?.let { key ->


yes, totally makes sense!

0xnm · 2023-04-03T14:08:15Z

+                    internalLogger.log(
+                        InternalLogger.Level.WARN,
+                        InternalLogger.Target.USER,
+                        LAST_ACTIVE_VIEW_GONE_WARNING_MESSAGE.format(it.name)


minor: better to explicitly specify the Locale

Suggested change

LAST_ACTIVE_VIEW_GONE_WARNING_MESSAGE.format(it.name)

LAST_ACTIVE_VIEW_GONE_WARNING_MESSAGE.format(Locale.US, it.name)

fuzzybinary · 2023-04-03T15:36:15Z

@0xnm I added telemetry for checking if we create multiple active sessions during session start. I think that should handle the case you're talking about.

fuzzybinary added 2 commits March 23, 2023 17:08

RUMM-2873 Add ability to end a RUM session (WIP)

8050341

Still need to add tests that ensure that the RUM Context is updated on the core at the proper times and add integration tests.

RUMM-2873 stopSession context and integration tests.

1ad4708

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

0xnm reviewed Mar 28, 2023

View reviewed changes

Apply suggestions from code review

1d5f647

Co-authored-by: Nikita Ogorodnikov <[email protected]>

fuzzybinary force-pushed the jward/RUMM-2873-stop-session branch 2 times, most recently from ff44dfb to 6fafbb3 Compare March 28, 2023 17:57

RUMM-2873 Apply suggestions from code review

c6414ac

Add more tests checking for session state.

fuzzybinary force-pushed the jward/RUMM-2873-stop-session branch from 6fafbb3 to c6414ac Compare March 28, 2023 18:14

fuzzybinary added 3 commits March 28, 2023 14:45

RUMM-2873 Fix force dereference

28d5bdf

Also add stopSession to untestable APIs

Merge branch 'develop' into jward/RUMM-2873-stop-session

79ed73e

RUMM-2873 Change to RumViewChangedListener

30ab2b6

This is used over pulling the RUMViewScope during session stop.

fuzzybinary requested a review from 0xnm March 31, 2023 19:17

xgouchet approved these changes Apr 3, 2023

View reviewed changes

0xnm reviewed Apr 3, 2023

View reviewed changes

fuzzybinary force-pushed the jward/RUMM-2873-stop-session branch from 788c379 to 57edb19 Compare April 3, 2023 13:51

0xnm approved these changes Apr 3, 2023

View reviewed changes

fuzzybinary force-pushed the jward/RUMM-2873-stop-session branch from 57edb19 to 61192e1 Compare April 3, 2023 15:34

RUMM-2873 Apply suggestions from code review.

c08ad2f

fuzzybinary force-pushed the jward/RUMM-2873-stop-session branch from 61192e1 to c08ad2f Compare April 3, 2023 15:38

0xnm approved these changes Apr 3, 2023

View reviewed changes

fuzzybinary merged commit c1fb449 into develop Apr 3, 2023

fuzzybinary deleted the jward/RUMM-2873-stop-session branch April 3, 2023 19:02

xgouchet added this to the 1.19.0 milestone Dec 13, 2023

		lateinit var endSessionButton: Button
		lateinit var startKioskButton: Button

	assertThat(testedScope.childScopes.count()).isEqualTo(1)
	assertThat(testedScope.childScopes).hasSize(1)

	LAST_ACTIVE_VIEW_GONE_WARNING_MESSAGE.format(it.name)
	LAST_ACTIVE_VIEW_GONE_WARNING_MESSAGE.format(Locale.US, it.name)

Uh oh!

Conversation

fuzzybinary commented Mar 27, 2023

What does this PR do?

Review checklist (to be filled by reviewers)

Uh oh!

0xnm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Mar 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

0xnm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xnm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fuzzybinary commented Apr 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Mar 31, 2023 •

edited

Loading