RUMM-2356 Add session replay recorder basic logic by mariusc83 · Pull Request #1007 · DataDog/dd-sdk-android

mariusc83 · 2022-08-03T14:53:24Z

What does this PR do?

This PR introduces the basic logic in our SnapshotProducer as a part of the ScreenRecorder component where we are actually going to capture the application screens and resolve them as Node tree equivalents.

Please note that for now the SnapshotProducer is quite dummy and can only handle basic application screens mostly from our Android Sample App. The way we will work on this component is through multiple iterations by running it with different sample applications and improving the output.

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)

ncreated

Looks really good, I can easily understand what's happening in there 👍. Left one arch suggestion, but non-blocking.

ncreated · 2022-08-04T06:29:53Z

+import com.datadog.android.sessionreplay.model.MobileSegment
+
+internal data class Node(
+    val id: Long,
+    val wireframes: List<MobileSegment.Wireframe>,


suggestion: I think it might be healthier to not depend directly on (HTTP) transport details in the core SR logic (the MobileSegment.Wireframe is what we serialize and send in payload). Introducing intermediate (DTO) and doing Node → indermediate → Wireframe transformation could IMO help on a long run by clearly separating concerns. Otherwise, the Recorder code will have to be changed if transport details change, which seems to be against SoC as both layers clearly overlap here. WDYT?

Yes I thought about it but then I think I prefer it this way even though maybe at some point I need to change the wireframes a bit. Main reason why is:

I don't want to introduce another layer which will require a transformer. If there's any change on the Wireframe I will still have to apply it at the transformer level from the intermediate DTO to Wireframe

Most important I want to avoid creating too many objects. With this approach the wireframes instances created here will be re - used at the Processor level.

Very clear 👌, was just checking what's the reasoning behind it 👍. I'll perhaps introduce a layer in between on iOS (have my reasons, mainly around threading), but I think it might be iOS-specific.

codecov-commenter · 2022-08-04T06:40:40Z

Codecov Report

Merging #1007 (25061da) into feature/session-replay-vo (922fd29) will increase coverage by 0.04%.
The diff coverage is 85.53%.

@@                      Coverage Diff                      @@
##           feature/session-replay-vo    #1007      +/-   ##
=============================================================
+ Coverage                      82.99%   83.03%   +0.04%     
=============================================================
  Files                            307      314       +7     
  Lines                           9980    10129     +149     
  Branches                        1628     1669      +41     
=============================================================
+ Hits                            8282     8410     +128     
- Misses                          1193     1195       +2     
- Partials                         505      524      +19

Impacted Files	Coverage Δ
...nal/tracking/AndroidXFragmentLifecycleCallbacks.kt	`93.48% <ø> (ø)`
...m/datadog/android/sessionreplay/recorder/IntExt.kt	`100.00% <ø> (ø)`
...com/datadog/android/sessionreplay/recorder/Node.kt	`66.67% <66.67%> (+66.67%)`	⬆️
...ionreplay/recorder/mapper/ButtonWireframeMapper.kt	`66.67% <66.67%> (ø)`
...android/sessionreplay/recorder/SnapshotProducer.kt	`77.14% <77.14%> (+27.14%)`	⬆️
...y/recorder/mapper/ViewScreenshotWireframeMapper.kt	`80.00% <80.00%> (ø)`
...onreplay/recorder/mapper/GenericWireframeMapper.kt	`85.71% <85.71%> (ø)`
...ssionreplay/recorder/mapper/ViewWireframeMapper.kt	`88.46% <88.46%> (ø)`
...ssionreplay/recorder/mapper/TextWireframeMapper.kt	`89.66% <89.66%> (ø)`
.../datadog/android/sessionreplay/recorder/LongExt.kt	`100.00% <100.00%> (ø)`
... and 8 more

urseberry

No docs impact.

xgouchet

Overall the SnapshotProducer class seems quite complex as you have many distinct usecases (TextView, ImageView, ButtonView, …).
I think it might be better to have a WireframeAdapter interface (or converter, or whatever name suits best), looking like

interface WireframeAdapter <T: View> {
    fun adapt(input: T): Node
}

and then have independent implementations for each type we want to support.

And then, if you really need shared extension functions, then place them in a separate file with documentation

xgouchet · 2022-08-08T07:56:52Z

+        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES
+            .LOLLIPOP
+        ) {


Nit pick but the formatting here seems really weird

xgouchet · 2022-08-08T07:59:00Z

+    }
+
+    @Suppress("ComplexMethod")
+    private fun View.toNode(pixelsDensity: Float): Node? {


Is an extension method really relevant here? This would be equally efficient as a fun convertViewToNode(view: View, pixelsDensity: Float): Node? and less ambiguous.

xgouchet · 2022-08-08T08:01:04Z

+
+    @Suppress("ComplexMethod")
+    private fun View.toNode(pixelsDensity: Float): Node? {
+        if (isNotValid()) {


Same here, using extension makes things ambiguous here. Does isNotValid refer to the implicit this: View, or to the parent this: SnapshotProducer?

xgouchet · 2022-08-08T08:01:08Z

+            return null
+        }
+
+        if (isSystemNoise()) {


xgouchet · 2022-08-08T08:01:14Z

+            return null
+        }
+
+        if (isToolbar()) {


xgouchet · 2022-08-08T08:04:17Z

+        return toNode(childNodes, pixelsDensity)
+    }
+
+    private fun View.toNode(childNodes: LinkedList<Node>, pixelsDensity: Float) =


Please avoid using block function when the implementation is this long, as it hides the return type and make things harder to read

xgouchet · 2022-08-08T08:05:33Z

+    private fun Drawable.resolveShapeStyleAndBorder(): Pair<MobileSegment
+        .ShapeStyle?, MobileSegment.ShapeBorder?>? {


Can we pick a consistent name for all those conversion functions? You have at least three in this class: toSomething, asSomething and resolveSomething.

mariusc83 · 2022-08-08T16:14:58Z

Overall the SnapshotProducer class seems quite complex as you have many distinct usecases (TextView, ImageView, ButtonView, …). I think it might be better to have a WireframeAdapter interface (or converter, or whatever name suits best), looking like
interface WireframeAdapter <T: View> {
    fun adapt(input: T): Node
}
and then have independent implementations for each type we want to support.

And then, if you really need shared extension functions, then place them in a separate file with documentation

I do agree that I might not need all these extension functions, will try to get rid of at least part of them.
Regarding the extraction of the logic into a WireframeAdapter I do have this in mind for later. For now I just wanted to keep it simple and have this basic functionality here but I do have in mind that this will require some logic extraction and more components as we will add more things in here. For now I really can't anticipate what we will add and where to see exactly how my Adapter will look like so I just want to wait a bit more.

xgouchet · 2022-08-16T11:26:26Z

+                )
+    }
+
+    private fun View.isNotValid(): Boolean {


What does valid means here? Do we mean trackable, visible on screen, …?

I will change it to trackable

xgouchet · 2022-08-16T12:11:30Z

+            // skip adding the children and just take a screenshot of the toolbar.
+            // It is too complex to de - structure this in multiple wireframes
+            // and we cannot actually get all the details here.


Should we add a TODO/Task in the backlog to handle this in the future?

It will already be handled in v1 once we introduce the logic in the screenshotWireframeMapper

xgouchet · 2022-08-16T12:12:50Z

+    WireframeMapper<T, S> {
+
+    protected fun resolveViewId(view: View): Long {
+        return if (view.id != View.NO_ID) view.id.toLong() else view.hashCode().toLong()


Aren't you afraid that there would be collision here? What would be the consequences if collisions happened?

Hmmm you are right...it might be a problem due the the view.hashcode maybe I should use the reference address there instead ? What do you think ? It can create problems if it happens as it might override other view properties in the mutation computation.

xgouchet · 2022-08-16T12:29:49Z

+import android.widget.TextView
+import com.datadog.android.sessionreplay.model.MobileSegment
+
+internal class RecorderWireframeMapper(


Why is this called RecorderWireframeMapper? The naming doesn't seem consistent with the TextWireframeMapper or ButtonWireframeMapper.
It seems to map Views to Wireframe, so what's the difference between this class and the ViewWireframeMapper? Same question goes for the ScreenshotWireframeMapper?

Yes...I think I need to find better names for these 🤔

mariusc83 self-assigned this Aug 3, 2022

mariusc83 force-pushed the mconstantin/rumm-2356/add-session-replay-recorder-logic branch 2 times, most recently from 504bb2c to a18cfe5 Compare August 3, 2022 15:04

mariusc83 marked this pull request as ready for review August 3, 2022 15:05

mariusc83 requested a review from a team as a code owner August 3, 2022 15:05

ncreated reviewed Aug 4, 2022

View reviewed changes

mariusc83 force-pushed the feature/session-replay-vo branch from 49fb615 to 8a5db0e Compare August 4, 2022 11:24

mariusc83 requested a review from a team as a code owner August 4, 2022 11:24

mariusc83 force-pushed the mconstantin/rumm-2356/add-session-replay-recorder-logic branch from a18cfe5 to c5bbbb6 Compare August 4, 2022 11:54

xgouchet added the size-large This PR is large sized label Aug 4, 2022

RUMM-2356 Update Session Replay schemas

bec8bb5

mariusc83 force-pushed the mconstantin/rumm-2356/add-session-replay-recorder-logic branch from c5bbbb6 to d9065f3 Compare August 4, 2022 15:55

urseberry reviewed Aug 5, 2022

View reviewed changes

xgouchet suggested changes Aug 8, 2022

View reviewed changes

mariusc83 force-pushed the mconstantin/rumm-2356/add-session-replay-recorder-logic branch from d9065f3 to 29b0cdd Compare August 8, 2022 17:15

mariusc83 requested a review from xgouchet August 10, 2022 13:58

xgouchet suggested changes Aug 16, 2022

View reviewed changes

xgouchet approved these changes Aug 17, 2022

View reviewed changes

RUMM-2356 Add Snapshot Producer first layer of logic

c1c8cba

mariusc83 force-pushed the mconstantin/rumm-2356/add-session-replay-recorder-logic branch from 25061da to c1c8cba Compare August 17, 2022 08:16

mariusc83 merged commit ca02455 into feature/session-replay-vo Aug 17, 2022

mariusc83 deleted the mconstantin/rumm-2356/add-session-replay-recorder-logic branch August 17, 2022 08:33

xgouchet added this to the internal milestone Dec 13, 2023

		private fun Drawable.resolveShapeStyleAndBorder(): Pair<MobileSegment
		.ShapeStyle?, MobileSegment.ShapeBorder?>? {

Uh oh!

Conversation

mariusc83 commented Aug 3, 2022

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

Uh oh!

ncreated 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!

codecov-commenter commented Aug 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

urseberry left a comment

Choose a reason for hiding this comment

Uh oh!

xgouchet left a comment

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!

Choose a reason for hiding this comment

Uh oh!

mariusc83 commented Aug 8, 2022

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Aug 4, 2022 •

edited

Loading