Implement Android SR for ReactTextView and ReactEditText #569

jonathanmos · 2023-12-10T22:26:38Z

What does this PR do?

Implementation of Android Session Replay for ReactTextView and ReactEditText - "allow" only (masking requires a change on the Android sdk side before it can be implemented here)

Device

Replay

Motivation

Necessary for Session Replay support for Android

Additional Notes

Anything else we should know when reviewing?

Review checklist (to be filled by reviewers)

Feature or bugfix MUST have appropriate tests
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)
If this PR is auto-generated, please make sure also to manually update the code related to the change

...ay/android/src/main/kotlin/com/datadog/reactnative/sessionreplay/ReactTextShadowNodeUtils.kt

louiszawadzki

As it's hard to put extensive testing in place here, I believe we should rely on comments to make sure that if we want to revisit this code later we understand exactly what it does.

I've also suggested a few improvements.

Your screenshot also seems to be missing a few use cases, can you add all the examples on the screen?

...ay/android/src/main/kotlin/com/datadog/reactnative/sessionreplay/ReactTextShadowNodeUtils.kt

...lay/android/src/main/kotlin/com/datadog/reactnative/sessionreplay/mappers/ReactTextMapper.kt

...android/src/main/kotlin/com/datadog/reactnative/sessionreplay/ReactTextPropertiesResolver.kt

...ain/kotlin/com/datadog/reactnative/sessionreplay/ReactNativeSessionReplayExtensionSupport.kt

...on-replay/android/src/test/kotlin/com/datadog/reactnative/sessionreplay/DrawableUtilsTest.kt

...kotlin/com/datadog/reactnative/sessionreplay/ReactNativeSessionReplayExtensionSupportTest.kt

...oid/src/test/kotlin/com/datadog/reactnative/sessionreplay/ReactTextPropertiesResolverTest.kt

0xnm · 2023-12-18T10:06:02Z

...oid/src/test/kotlin/com/datadog/reactnative/sessionreplay/ReactTextPropertiesResolverTest.kt

+            MobileSegment.TextStyle(
+                family = "Serif",
+                size = forge.aPositiveLong(),
+                "#000000"
+            ),


let's extract this to the ForgeryFactory, maybe?

I'll extract this, but I'm a bit concerned that it duplicates logic in the native sdk. I'm wondering if we shouldn't change the visibility of the forgery factories there

...eplay/android/src/main/kotlin/com/datadog/reactnative/sessionreplay/utils/ReflectionUtils.kt

...droid/src/main/kotlin/com/datadog/reactnative/sessionreplay/DdSessionReplayImplementation.kt

...ain/kotlin/com/datadog/reactnative/sessionreplay/ReactNativeSessionReplayExtensionSupport.kt

...android/src/main/kotlin/com/datadog/reactnative/sessionreplay/ReactTextPropertiesResolver.kt

...lay/android/src/main/kotlin/com/datadog/reactnative/sessionreplay/mappers/ReactTextMapper.kt

...eplay/android/src/main/kotlin/com/datadog/reactnative/sessionreplay/utils/ReflectionUtils.kt

...on-replay/android/src/main/kotlin/com/datadog/reactnative/sessionreplay/ShadowNodeWrapper.kt

...kotlin/com/datadog/reactnative/sessionreplay/ReactNativeSessionReplayExtensionSupportTest.kt

...oid/src/test/kotlin/com/datadog/reactnative/sessionreplay/ReactTextPropertiesResolverTest.kt

louiszawadzki

Looks good to me if @0xnm approves as well.

mariusc83 · 2023-12-20T08:13:34Z

...android/src/main/kotlin/com/datadog/reactnative/sessionreplay/ReactTextPropertiesResolver.kt

+        val padding = textPosition?.padding
+        val alignment = resolveTextAlignment(view, textWireframe)
+
+        return textWireframe.copy(


why do you create a copy here ? Couldn't we just resolve all the properties first and then after create the instance ?

I'll change to remove the copy in the drawable method and return pairs of values so we only copy the wireframe once. Copying the wireframe at least once is necessary (unless nothing changed in terms of properties), because the initial wireframe comes from the superclass map so it's either to do this or to duplicate the logic.

mariusc83 · 2023-12-20T08:25:23Z

...eplay/android/src/main/kotlin/com/datadog/reactnative/sessionreplay/utils/ReflectionUtils.kt

+import java.lang.reflect.Field
+
+internal class ReflectionUtils {
+    internal fun getDeclaredField(instance: Any, fieldName: String): Any? {


oh wow...are we going to use reflection ? How many times is this code going to be called ?

Unfortunately yes, since we need the values from the React shadownode. It would be called multiple times for every view

I would test the performances of these methods and see how they could affect the frame rate. These methods are being called on the UIThread and it will be problematic. I would rather have less quality of the replays than affected performance.

@mariusc83 performance benchmark is scheduled after this PR is merged. I agree that performance is more important than quality of replays.

...eplay/android/src/main/kotlin/com/datadog/reactnative/sessionreplay/utils/ReflectionUtils.kt

0xnm

I've left some final suggestions and then I think it is good to be merged.

...ain/kotlin/com/datadog/reactnative/sessionreplay/ReactNativeSessionReplayExtensionSupport.kt

0xnm · 2023-12-20T08:27:48Z

...lay/android/src/main/kotlin/com/datadog/reactnative/sessionreplay/SessionReplaySDKWrapper.kt

+
+    internal companion object {
+        internal fun getLogger(): InternalLogger? {
+            return (Datadog.getInstance() as? FeatureSdkCore)?.internalLogger


I'm not sure this is a best way to get a logger, because it has 2 problems:

it introduces nullability (it can be avoided if we get logger only once when feature is initialized, because it is guaranteed that core will be there at this point and its type is FeatureSdkCore)

each call to Datadog.getInstance introduces synchronization lock (not a big penalty, but still).

Normally in other features we are passing down the logger instance we got only once (or passing down SDK core instance), and anyway in the code of this module you have InternalLogger as constructor argument.

But it is not a blocker for me, if it works fine for you.

Oh this one is on me, I suggested this change.

I wasn't aware of the sync lock, let's watch how much of an impact this has in the perf benchmark.

probably just peanuts and we shouldn't care about this penalty. But if we have some tracing data, it will be good.

I've changed it back to passing the logger instance

...on-replay/android/src/main/kotlin/com/datadog/reactnative/sessionreplay/ShadowNodeWrapper.kt

...eplay/android/src/main/kotlin/com/datadog/reactnative/sessionreplay/utils/ReflectionUtils.kt

0xnm

lgtm!

...kotlin/com/datadog/reactnative/sessionreplay/ReactNativeSessionReplayExtensionSupportTest.kt

jonathanmos force-pushed the jmoskovich/rum-1817/record-android-text branch 2 times, most recently from b27e59a to a87b2e8 Compare December 10, 2023 23:12

louiszawadzki reviewed Dec 11, 2023

View reviewed changes

...ay/android/src/main/kotlin/com/datadog/reactnative/sessionreplay/ReactTextShadowNodeUtils.kt Outdated Show resolved Hide resolved

jonathanmos force-pushed the jmoskovich/rum-1817/record-android-text branch from 68aa3f2 to aef3116 Compare December 13, 2023 09:43

louiszawadzki reviewed Dec 14, 2023

View reviewed changes

jonathanmos marked this pull request as ready for review December 18, 2023 08:28

jonathanmos requested a review from a team as a code owner December 18, 2023 08:28

0xnm reviewed Dec 18, 2023

View reviewed changes

jonathanmos force-pushed the jmoskovich/rum-1817/record-android-text branch from 4f1fd12 to 8f2601e Compare December 18, 2023 10:15

0xnm reviewed Dec 18, 2023

View reviewed changes

...eplay/android/src/main/kotlin/com/datadog/reactnative/sessionreplay/utils/ReflectionUtils.kt Outdated Show resolved Hide resolved

jonathanmos requested review from 0xnm and louiszawadzki December 19, 2023 09:25

louiszawadzki reviewed Dec 19, 2023

View reviewed changes

...droid/src/main/kotlin/com/datadog/reactnative/sessionreplay/DdSessionReplayImplementation.kt Show resolved Hide resolved

0xnm requested changes Dec 19, 2023

View reviewed changes

louiszawadzki previously approved these changes Dec 19, 2023

View reviewed changes

jonathanmos requested a review from 0xnm December 19, 2023 16:12

mariusc83 reviewed Dec 20, 2023

View reviewed changes

jonathanmos commented Dec 20, 2023

View reviewed changes

...eplay/android/src/main/kotlin/com/datadog/reactnative/sessionreplay/utils/ReflectionUtils.kt Outdated Show resolved Hide resolved

0xnm reviewed Dec 20, 2023

View reviewed changes

jonathanmos dismissed louiszawadzki’s stale review via 1d1850d December 20, 2023 10:52

jonathanmos requested review from louiszawadzki, 0xnm and mariusc83 December 20, 2023 11:05

0xnm previously approved these changes Dec 20, 2023

View reviewed changes

...kotlin/com/datadog/reactnative/sessionreplay/ReactNativeSessionReplayExtensionSupportTest.kt Outdated Show resolved Hide resolved

jonathanmos dismissed 0xnm’s stale review via bdce711 December 20, 2023 12:31

jonathanmos force-pushed the jmoskovich/rum-1817/record-android-text branch from 1d1850d to bdce711 Compare December 20, 2023 12:31

louiszawadzki approved these changes Dec 20, 2023

View reviewed changes

0xnm approved these changes Dec 20, 2023

View reviewed changes

mariusc83 approved these changes Dec 21, 2023

View reviewed changes

Implement Android SR for ReactTextView and ReactEditText

7cc8400

jonathanmos force-pushed the jmoskovich/rum-1817/record-android-text branch from bdce711 to 7cc8400 Compare December 21, 2023 09:26

jonathanmos merged commit 13c2ee0 into develop Dec 21, 2023
4 checks passed

jonathanmos deleted the jmoskovich/rum-1817/record-android-text branch December 21, 2023 10:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Android SR for ReactTextView and ReactEditText #569

Implement Android SR for ReactTextView and ReactEditText #569

jonathanmos commented Dec 10, 2023 •

edited

Loading

louiszawadzki left a comment

0xnm Dec 18, 2023

jonathanmos Dec 18, 2023

louiszawadzki left a comment

mariusc83 Dec 20, 2023

jonathanmos Dec 20, 2023 •

edited

Loading

mariusc83 Dec 20, 2023

jonathanmos Dec 20, 2023

mariusc83 Dec 20, 2023

louiszawadzki Dec 20, 2023

0xnm left a comment

0xnm Dec 20, 2023

louiszawadzki Dec 20, 2023

0xnm Dec 20, 2023

jonathanmos Dec 20, 2023

0xnm left a comment

Implement Android SR for ReactTextView and ReactEditText #569

Implement Android SR for ReactTextView and ReactEditText #569

Conversation

jonathanmos commented Dec 10, 2023 • edited Loading

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

louiszawadzki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

louiszawadzki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanmos Dec 20, 2023 • edited Loading

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

0xnm 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

0xnm left a comment

Choose a reason for hiding this comment

jonathanmos commented Dec 10, 2023 •

edited

Loading

jonathanmos Dec 20, 2023 •

edited

Loading