[RUM-8652] Allow definition of custom implementations of specific SR methods #2516

cdn34dd · 2025-02-27T10:17:33Z

What does this PR do?

Exposes a new SessionReplayConfigutation option to allow clients to pass their own implementations of specific methods used in the Session Replay SDK.
Updates the SDK code to use the overriden function if provided and falling back to the original implementation otherwise.
( Currently only getCurrentWIndows is allowed to be redefined, but there's a high likelihood that more will be needed )

Motivation

What inspired you to submit this pull request?

Certain parts of the session replay SDK don't work as intended when ran in a react-native environment, leading to broken replays. In order to keep platform specific code out of the android SDK, the goal is to allow each client to add their own implementations of the parts that don't work.

Additional Notes

We keep the setCustomCallbacks only accessible through a internal proxy in order to not expose internal config options. In practice it would look something like this:

        val customCallbacks = SessionReplayCustomCallbacks(
            getCurrentWindows = ::getCurrentWindows
            /* Other potential methods */
        )
        
        val configuration = SessionReplayConfiguration.Builder(replaySampleRate.toFloat())
            /* .... */
            .setImagePrivacy(/* .... */)
            .setTouchPrivacy(/* .... */)
            .setTextAndInputPrivacy(/* .... */)
            .let { builder ->
                _SessionReplayInternalProxy.setCustomCallbacks(builder, customCallbacks)
            }

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)

xgouchet · 2025-02-27T14:28:13Z

...n/kotlin/com/datadog/android/sessionreplay/internal/recorder/SessionReplayCustomCallbacks.kt

+data class SessionReplayCustomCallbacks(
+    val getCurrentWindows: (() -> List<Window>)? = null
+)


Note
Why did you chose to make it a data class instead of an interface?

The general idea was to make a simple configuration object we could add to the configuration builder that holds optional callbacks, allowing clients to set only the ones they needed. But here both would work.

0xnm · 2025-03-03T08:31:46Z

I think we shouldn't go this way (by exposing it in the public API along with other methods), it should be an internal API accessible from the proxy.

ambushwork · 2025-03-03T08:39:15Z

...sion-replay/src/main/kotlin/com/datadog/android/sessionreplay/_SessionReplayInternalProxy.kt

+ */
+@InternalApi
+@Suppress(
+    "UndocumentedPublicClass",


This class has been documented above, why do we still need this Suppression here?

ambushwork · 2025-03-03T08:45:08Z

...n/kotlin/com/datadog/android/sessionreplay/internal/recorder/SessionReplayCustomCallbacks.kt

+import android.view.Window
+
+data class SessionReplayCustomCallbacks(
+    val getCurrentWindows: (() -> List<Window>)? = null


I think instead of having a data class with only one callback inside, you can just make it as an interface

@NoOpImplementation interface SessionReplayCustomCallback{ fun getCurrentWindows() : List<Window> }

I'm still not sure how many callbacks there will be for sure, for now it's just one, if it turns out to be just this one, we will likely not even need this and I'll change this then.

Instead of having several callbacks feature, is that possible to have only one callback but with more functions in it?

Not sure what that means, can you explain it a bit more or give an example ? Just for context, all these functions we need to override from RN or other project can all be unrelated to each other and can affect different parts of the sdk.

If I understand correctly, what you intent to do should be like:

data class SessionReplayCustomCallbacks( val getCurrentWindows: (() -> List<Window>)? = null val doSomethingElseA: ((SomeInputA) -> SomeOutputA)? = null val doSomethingElseB: ((SomeInputB) -> List<SomeOutputB>)? = null )

Instead of doing that, you can do :

@NoOpImplementation interface SessionReplayCustomCallback{ fun getCurrentWindows() : List<Window> fun doSomethingElseA(input: SomeInputA) : SomeOutputA fun doSomethingElseA(input: SomeInputB) : SomeOutputB }

In that case you will have only one callback but can be called in different placesto serve for your need.

ambushwork · 2025-03-03T08:46:42Z

...ssion-replay/src/main/kotlin/com/datadog/android/sessionreplay/SessionReplayConfiguration.kt

+        /**
+         * Specifies callbacks functions to override standard behaviours in Session Replay
+         * that need to behave differently depending on the platform.
+         */


While reading this documentation, I would expect to understand what's the exact behaviours it will override. can you elaborate it?

ambushwork · 2025-03-03T08:47:37Z

...ssion-replay/src/main/kotlin/com/datadog/android/sessionreplay/SessionReplayConfiguration.kt

@@ -73,6 +75,7 @@ data class SessionReplayConfiguration internal constructor(
        private var extensionSupportSet: MutableSet<ExtensionSupport> = mutableSetOf()
        private var dynamicOptimizationEnabled = true
        private var systemRequirementsConfiguration = SystemRequirementsConfiguration.NONE
+        private var customCallbacks: SessionReplayCustomCallbacks = SessionReplayCustomCallbacks()


is it expected that the class name and field name are all pluriel?

I would say in this case yes, but what would you suggest ?

usually we keep class name singular to represent a single instance of an object, unless the class itself presents a pluriel collection such as data class Users(val users: List<User>), and for the field name it should be like:

private var customCallback : SessionReplayCustomCallback = SessionReplayCustomCallback() // or private var customCallbacks : List<SessionReplayCustomCallback> = SessionReplayCustomCallback()

I don't mind changing the name, but having something called SessionReplayCustomCallback, will be incorrect as soon as another callback is added no ?

0xnm · 2025-03-03T08:56:04Z

Please ignore my comment above, I had an old version of this PR opened. I will review a new one.

0xnm

I left few suggestions, but nothing blocking. LGTM.

0xnm · 2025-03-03T09:34:37Z

...n/kotlin/com/datadog/android/sessionreplay/internal/recorder/SessionReplayCustomCallbacks.kt

+import android.view.Window
+
+data class SessionReplayCustomCallbacks(
+    val getCurrentWindows: (() -> List<Window>)? = null


minor: there is no need to use get prefix for val property, it can be just val currentWindows

0xnm · 2025-03-03T09:34:52Z

...src/main/kotlin/com/datadog/android/sessionreplay/internal/recorder/SessionReplayRecorder.kt

-                        logger = internalLogger,
-                        resourceResolver = resourceResolver,
+                        logger = internalLogger, resourceResolver = resourceResolver,


to be reverted

0xnm · 2025-03-03T09:36:43Z

...n/kotlin/com/datadog/android/sessionreplay/internal/recorder/SessionReplayCustomCallbacks.kt

+data class SessionReplayCustomCallbacks(
+    val getCurrentWindows: (() -> List<Window>)? = null


maybe some explanation/docs here will be useful

ambushwork · 2025-03-03T14:35:55Z

...sion-replay/src/main/kotlin/com/datadog/android/sessionreplay/_SessionReplayInternalProxy.kt

+    "ClassNaming",
+    "VariableNaming"
+)
+class _SessionReplayInternalProxy {


_SessionReplayInternalProxy.setCustomCallbacks(builder, customCallbacks)

_SessionReplayInternalProxy(builder).setCustomCallbacks(customCallbacks)

… builder - have the option to override `getCurrentWindows` in order to improve cross-platform support - make `setInternalCallback`to be available only through internal proxy

xgouchet reviewed Feb 27, 2025

View reviewed changes

cdn34dd marked this pull request as ready for review March 3, 2025 07:12

cdn34dd requested review from a team as code owners March 3, 2025 07:12

cdn34dd force-pushed the carlosnogueira/RUM-8652/add-config-option-to-override-specific-sr-methods branch from 8d1bb14 to fdb1b74 Compare March 3, 2025 08:00

ambushwork reviewed Mar 3, 2025

View reviewed changes

0xnm previously approved these changes Mar 3, 2025

View reviewed changes

ambushwork reviewed Mar 3, 2025

View reviewed changes

cdn34dd dismissed 0xnm’s stale review via 504b964 March 3, 2025 17:59

RUM-8652: add internalCallback option to SessionReplayConfiguration…

83f6f78

… builder - have the option to override `getCurrentWindows` in order to improve cross-platform support - make `setInternalCallback`to be available only through internal proxy

cdn34dd force-pushed the carlosnogueira/RUM-8652/add-config-option-to-override-specific-sr-methods branch from 504b964 to 83f6f78 Compare March 3, 2025 18:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RUM-8652] Allow definition of custom implementations of specific SR methods #2516

[RUM-8652] Allow definition of custom implementations of specific SR methods #2516

cdn34dd commented Feb 27, 2025 •

edited

Loading

xgouchet Feb 27, 2025

cdn34dd Feb 28, 2025

0xnm commented Mar 3, 2025

ambushwork Mar 3, 2025

ambushwork Mar 3, 2025

cdn34dd Mar 3, 2025

ambushwork Mar 3, 2025

cdn34dd Mar 3, 2025

ambushwork Mar 3, 2025 •

edited

Loading

ambushwork Mar 3, 2025

ambushwork Mar 3, 2025

cdn34dd Mar 3, 2025

ambushwork Mar 3, 2025 •

edited

Loading

cdn34dd Mar 3, 2025

0xnm commented Mar 3, 2025

0xnm left a comment

0xnm Mar 3, 2025

0xnm Mar 3, 2025

0xnm Mar 3, 2025

ambushwork Mar 3, 2025

		data class SessionReplayCustomCallbacks(
		val getCurrentWindows: (() -> List<Window>)? = null

[RUM-8652] Allow definition of custom implementations of specific SR methods #2516

Are you sure you want to change the base?

[RUM-8652] Allow definition of custom implementations of specific SR methods #2516

Conversation

cdn34dd commented Feb 27, 2025 • edited Loading

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xnm commented Mar 3, 2025

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

ambushwork Mar 3, 2025 • 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

ambushwork Mar 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xnm commented Mar 3, 2025

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

cdn34dd commented Feb 27, 2025 •

edited

Loading

ambushwork Mar 3, 2025 •

edited

Loading

ambushwork Mar 3, 2025 •

edited

Loading