[RUM-8652] Allow definition of custom implementations of specific SR methods by cdn34dd · Pull Request #2516 · DataDog/dd-sdk-android

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 getCurrentActivity 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 setInternalCallback only accessible through a internal proxy in order to not expose internal config options. In practice it would look something like this:

        val internalCallback = ReactNativeSessionReplayInternalCallback(reactContext)
        
        val configuration = SessionReplayConfiguration.Builder(replaySampleRate.toFloat())
            /* .... */
            .setImagePrivacy(/* .... */)
            .setTouchPrivacy(/* .... */)
            .setTextAndInputPrivacy(/* .... */)
            .let { builder ->
                _SessionReplayInternalProxy(builder).setInternalCallback(internalCallback)
            }

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

+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

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


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

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

+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:47:37Z

        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

+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

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


to be reverted

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

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


maybe some explanation/docs here will be useful

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

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


_SessionReplayInternalProxy.setCustomCallbacks(builder, customCallbacks)

_SessionReplayInternalProxy(builder).setCustomCallbacks(customCallbacks)

codecov-commenter · 2025-03-05T11:14:09Z

Codecov Report

Attention: Patch coverage is 83.87097% with 5 lines in your changes missing coverage. Please review.

Project coverage is 70.07%. Comparing base (a9007ed) to head (f0da63c).
Report is 2 commits behind head on develop.

Files with missing lines	Patch %	Lines
...nreplay/internal/recorder/SessionReplayRecorder.kt	40.00%	2 Missing and 1 partial ⚠️
...nreplay/internal/SessionReplayLifecycleCallback.kt	75.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2516      +/-   ##
===========================================
- Coverage    70.07%   70.07%   -0.01%     
===========================================
  Files          796      797       +1     
  Lines        29970    29990      +20     
  Branches      5016     5018       +2     
===========================================
+ Hits         21001    21013      +12     
- Misses        7574     7576       +2     
- Partials      1395     1401       +6

Files with missing lines	Coverage Δ
...com/datadog/android/sessionreplay/SessionReplay.kt	`72.73% <100.00%> (+0.63%)`	⬆️
...ndroid/sessionreplay/SessionReplayConfiguration.kt	`96.77% <100.00%> (+0.18%)`	⬆️
...droid/sessionreplay/_SessionReplayInternalProxy.kt	`100.00% <100.00%> (ø)`
.../sessionreplay/internal/DefaultRecorderProvider.kt	`93.80% <100.00%> (+0.10%)`	⬆️
...oid/sessionreplay/internal/SessionReplayFeature.kt	`98.75% <100.00%> (+0.01%)`	⬆️
...nreplay/internal/SessionReplayLifecycleCallback.kt	`62.50% <75.00%> (-4.17%)`	⬇️
...nreplay/internal/recorder/SessionReplayRecorder.kt	`95.10% <40.00%> (-2.00%)`	⬇️

... and 27 files with indirect coverage changes

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

… builder - have the option to set `getCurrentActivity` in order to allow fragments lifecycle callbacks to be registered after Application.onCreate phase - make `setInternalCallback`to be available only through internal proxy tmp

ambushwork · 2025-03-07T08:36:04Z


    private val currentActiveWindows = WeakHashMap<Window, Any?>()

+    fun setCurrentWindow(activity: Activity) {


Here the function name is setCurrentWindow but you are taking activity as the argument, which makes a bit confusing,
you can just change to setCurrentWindow(window: Window) and let the call site worry about how to retrieve the window.

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

cdn34dd force-pushed the carlosnogueira/RUM-8652/add-config-option-to-override-specific-sr-methods branch 4 times, most recently from cf978c3 to 8e98c06 Compare March 5, 2025 10:45

cdn34dd requested a review from ambushwork March 5, 2025 15:30

ambushwork previously approved these changes Mar 6, 2025

View reviewed changes

xgouchet previously approved these changes Mar 6, 2025

View reviewed changes

cdn34dd dismissed stale reviews from xgouchet and ambushwork via 5cfbd1c March 6, 2025 17:09

cdn34dd force-pushed the carlosnogueira/RUM-8652/add-config-option-to-override-specific-sr-methods branch from 8e98c06 to 5cfbd1c Compare March 6, 2025 17:09

cdn34dd requested a review from ambushwork March 6, 2025 17:12

cdn34dd force-pushed the carlosnogueira/RUM-8652/add-config-option-to-override-specific-sr-methods branch from 5cfbd1c to f0da63c Compare March 6, 2025 17:47

ambushwork approved these changes Mar 7, 2025

View reviewed changes

cdn34dd merged commit 5a98bd8 into develop Mar 7, 2025

cdn34dd deleted the carlosnogueira/RUM-8652/add-config-option-to-override-specific-sr-methods branch March 7, 2025 08:38

mariusc83 mentioned this pull request Apr 9, 2026

Reproduce RUMS-4124: SessionReplayInternalCallback missing resource forwarding methods #3355

Closed

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


		private val currentActiveWindows = WeakHashMap<Window, Any?>()

		fun setCurrentWindow(activity: Activity) {

Uh oh!

Conversation

cdn34dd commented Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

0xnm commented Mar 3, 2025

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!

ambushwork Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ambushwork Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xnm commented Mar 3, 2025

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!

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

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cdn34dd commented Feb 27, 2025 •

edited

Loading

ambushwork Mar 3, 2025 •

edited

Loading

ambushwork Mar 3, 2025 •

edited

Loading

codecov-commenter commented Mar 5, 2025 •

edited

Loading