RUMM-2355 Add Session Replay lifecycle callbacks by mariusc83 · Pull Request #993 · DataDog/dd-sdk-android

mariusc83 · 2022-07-21T14:12:17Z

What does this PR do?

In this PR we are adding the Session Replay application lifecycle callbacks which will be controlled from the SessionReplayFeature level through the Public API.
We are also adding the base Recorder - Processor architecture, preparing the steps for the next PR where we're going to capture the screen snapshot and send it to the processor.

Motivation

What inspired you to submit this pull request?

Additional Notes

Anything else we should know when reviewing?

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)

0xnm · 2022-07-22T12:16:43Z

+    // region Internal
+
+    private fun registerCallback(context: Context) {
+        if (context is Application) {


we can declare context as Application type directly and avoid this check

Yeah but it doesn't follow the pattern for all the other features. If we do this it will make sense to change this also for RumFeature, LogsFeature, etc.

0xnm · 2022-07-22T12:20:57Z

+
+        // Then
+        verify(mockSessionReplayLifecycleCallback)
+            .register(appContext.mockInstance)


should we verify unregister here?

0xnm · 2022-07-22T12:21:24Z

+        // Then
+        countDownLatch.await(5, TimeUnit.SECONDS)
+        verify(mockSessionReplayLifecycleCallback)
+            .register(appContext.mockInstance)


seems like verification of unregister is missing

yup, copy pasted from the first one :)

0xnm · 2022-07-22T12:22:21Z

+
+        // Then
+        verify(mockSessionReplayLifecycleCallback)
+            .register(appContext.mockInstance)


also we need to check that unregister was called once.

0xnm · 2022-07-22T12:23:36Z

+        // When
+        repeat(3) {
+            Thread {
+                testedFeature.stopRecording()


test says startRecording, but stopRecording is called here

0xnm · 2022-07-22T12:31:58Z

+) : ViewTreeObserver
+    .OnDrawListener {


Suggested change

) : ViewTreeObserver

.OnDrawListener {

) : ViewTreeObserver.OnDrawListener {

0xnm · 2022-07-22T12:33:53Z

+) : ViewTreeObserver
+    .OnDrawListener {
+    private var currentOrientation = Configuration.ORIENTATION_UNDEFINED
+    private val trackedActivity: WeakReference<Activity> = WeakReference(activity)


why do we need to use WeakReference if we know activity lifecycle and can just release trackedActivity property when activity goes into a destroyed state?

Yes, I know but I want to be better safe than sorry. I want to totally avoid keeping those references and it will not be too costly to just use WeakRefs

The problem with weak references is that they can be collected at some point and everything will stop working all of the sudden, recording will just stop, it is very hard to debug such things.

The way I know is that they will never be destroyed if they still have a strong reference from somewhere in the process. @xgouchet I really want also your input here, should we keep WeakRefs or just go with direct reference and hope that will never go into trouble.

ok, I refreshed my knowledge (didn't use WeakReference for a long time) and that is true: they are cleared only when referent is weakly reachable.

So I guess we can keep them. But still, I'm curious why do we need it when we have a complete access to the lifecycle.

Yup, we can debate this, waiting for @xgouchet's opinion also.

0xnm · 2022-07-22T12:35:47Z

+    Application.ActivityLifecycleCallbacks {
+
+    internal var recorder: Recorder = ScreenRecorder(SnapshotProcessor())
+    internal val resumedActivities: WeakHashMap<Activity, Any?> = WeakHashMap()


Why we use WeakHashMap if we can properly control the reference to the activity by listening to the lifecycle?

Same as above

0xnm · 2022-07-22T12:39:57Z

+    }
+
+    @Test
+    fun `M stop recording all resumed activities W W unregisterAndStopRecorders()`() {


Suggested change

fun `M stop recording all resumed activities W W unregisterAndStopRecorders()`() {

fun `M stop recording all resumed activities W unregisterAndStopRecorders()`() {

0xnm · 2022-07-22T12:43:46Z

+    lateinit var mockActivity: Activity
+    lateinit var mockDecorView: View
+    lateinit var mockWindow: Window
+    lateinit var mockResources: Resources
+    lateinit var configuration: Configuration


we can also use Mock annotation here directly?

Usually I use that but whenever I want to stub a method inside I use mock { }

codecov-commenter · 2022-07-22T13:21:32Z

Codecov Report

Merging #993 (b7158ed) into feature/session-replay-vo (f175f09) will increase coverage by 0.09%.
The diff coverage is 84.34%.

❗ Current head b7158ed differs from pull request most recent head 0ca3a80. Consider uploading reports for the commit 0ca3a80 to get more accurate results

@@                      Coverage Diff                      @@
##           feature/session-replay-vo     #993      +/-   ##
=============================================================
+ Coverage                      82.85%   82.94%   +0.09%     
=============================================================
  Files                            298      306       +8     
  Lines                           9772     9853      +81     
  Branches                        1595     1616      +21     
=============================================================
+ Hits                            8096     8172      +76     
+ Misses                          1194     1184      -10     
- Partials                         482      497      +15

Impacted Files	Coverage Δ
...com/datadog/android/sessionreplay/recorder/Node.kt	`0.00% <0.00%> (ø)`
...oid/src/main/kotlin/com/datadog/android/Datadog.kt	`72.34% <50.00%> (-2.08%)`	⬇️
...android/sessionreplay/recorder/SnapshotProducer.kt	`50.00% <50.00%> (ø)`
.../kotlin/com/datadog/android/v2/core/DatadogCore.kt	`65.85% <66.67%> (+1.45%)`	⬆️
...g/android/sessionreplay/recorder/ScreenRecorder.kt	`84.62% <84.62%> (ø)`
...d/sessionreplay/recorder/RecorderOnDrawListener.kt	`86.36% <86.36%> (ø)`
...id/sessionreplay/SessionReplayLifecycleCallback.kt	`92.31% <92.31%> (ø)`
...oid/sessionreplay/internal/SessionReplayFeature.kt	`96.00% <94.12%> (-4.00%)`	⬇️
...droid/sessionreplay/processor/SnapshotProcessor.kt	`100.00% <100.00%> (ø)`
...m/datadog/android/sessionreplay/recorder/IntExt.kt	`100.00% <100.00%> (ø)`
... and 8 more

0xnm

lgtm!

RUMM-2335 Integrate Session Replay Hooks with the feature Public API

dee4a3f

mariusc83 self-assigned this Jul 21, 2022

mariusc83 force-pushed the mconstantin/rumm-2355/create-and-integrate-session-replay-hooks branch from 2a27976 to 19ef3d2 Compare July 21, 2022 14:16

xgouchet added the size-large This PR is large sized label Jul 21, 2022

mariusc83 force-pushed the mconstantin/rumm-2355/create-and-integrate-session-replay-hooks branch 2 times, most recently from 94205c2 to 6d80acd Compare July 22, 2022 08:13

mariusc83 marked this pull request as ready for review July 22, 2022 08:17

mariusc83 requested a review from a team as a code owner July 22, 2022 08:17

Base automatically changed from mconstantin/rumm-2265/create-json-schemas-for-session-replay to feature/session-replay-vo July 22, 2022 08:17

mariusc83 force-pushed the mconstantin/rumm-2355/create-and-integrate-session-replay-hooks branch from cf87ff5 to 808031a Compare July 22, 2022 09:38

0xnm reviewed Jul 22, 2022

View reviewed changes

mariusc83 force-pushed the mconstantin/rumm-2355/create-and-integrate-session-replay-hooks branch 2 times, most recently from 209a40a to b7158ed Compare July 25, 2022 07:06

0xnm approved these changes Jul 25, 2022

View reviewed changes

mariusc83 added 3 commits July 25, 2022 10:29

RUMM-2335 Add Session Replay hooks into the application lifecycle

b1d5ef2

Small fix in the schema generator tasks

9491cca

Adjust nightly tests coverage

0ca3a80

mariusc83 force-pushed the mconstantin/rumm-2355/create-and-integrate-session-replay-hooks branch from b7158ed to 0ca3a80 Compare July 25, 2022 08:34

mariusc83 merged commit 4e78a34 into feature/session-replay-vo Jul 25, 2022

mariusc83 deleted the mconstantin/rumm-2355/create-and-integrate-session-replay-hooks branch July 25, 2022 11:39

xgouchet added this to the internal milestone Dec 13, 2023

	) : ViewTreeObserver
	.OnDrawListener {
	) : ViewTreeObserver.OnDrawListener {

	fun `M stop recording all resumed activities W W unregisterAndStopRecorders()`() {
	fun `M stop recording all resumed activities W unregisterAndStopRecorders()`() {

Uh oh!

Conversation

mariusc83 commented Jul 21, 2022

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

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!

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!

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!

codecov-commenter commented Jul 22, 2022 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Jul 22, 2022 •

edited

Loading