RUM-5182: Add precheck conditions when registering session replay feature by ambushwork · Pull Request #2264 · DataDog/dd-sdk-android

ambushwork · 2024-09-17T09:01:28Z

What does this PR do?

Add SystemRequirementConfiguration class to contain requirements to check
Add SystemRequirementChecker to check if every condition can be met:
- MemoryRequirementChecker implementation to check max RAM size
- CpuRequirementChecker implementation to check cpu cores number
Add SessionReplayRequirement class to use SystemRequirementConfiguration and all the checkers to determine if the task can be executed after checking the conditions.

Motivation

RUN-5182

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)

codecov-commenter · 2024-09-17T11:57:01Z

Codecov Report

Attention: Patch coverage is 66.01942% with 35 lines in your changes missing coverage. Please review.

Project coverage is 70.10%. Comparing base (35991f3) to head (70b09b1).
Report is 2 commits behind head on feature/sr-dynamic-optimisation.

Files with missing lines	Patch %	Lines
...d/sessionreplay/SystemRequirementsConfiguration.kt	50.00%	17 Missing ⚠️
.../internal/prerequisite/MemoryRequirementChecker.kt	62.50%	4 Missing and 5 partials ⚠️
...lay/internal/prerequisite/CPURequirementChecker.kt	73.33%	2 Missing and 2 partials ⚠️
.../android/core/internal/persistence/file/FileExt.kt	0.00%	2 Missing ⚠️
...ndroid/sessionreplay/SessionReplayConfiguration.kt	71.43%	2 Missing ⚠️
...com/datadog/android/sessionreplay/SessionReplay.kt	95.24%	1 Missing ⚠️

Additional details and impacted files

@@                         Coverage Diff                         @@
##           feature/sr-dynamic-optimisation    #2264      +/-   ##
===================================================================
+ Coverage                            70.09%   70.10%   +0.01%     
===================================================================
  Files                                  731      734       +3     
  Lines                                27241    27328      +87     
  Branches                              4598     4609      +11     
===================================================================
+ Hits                                 19092    19156      +64     
- Misses                                6845     6870      +25     
+ Partials                              1304     1302       -2

Files with missing lines	Coverage Δ
...com/datadog/android/sessionreplay/SessionReplay.kt	`62.50% <95.24%> (+8.33%)`	⬆️
.../android/core/internal/persistence/file/FileExt.kt	`85.00% <0.00%> (-2.93%)`	⬇️
...ndroid/sessionreplay/SessionReplayConfiguration.kt	`96.92% <71.43%> (-3.08%)`	⬇️
...lay/internal/prerequisite/CPURequirementChecker.kt	`73.33% <73.33%> (ø)`
.../internal/prerequisite/MemoryRequirementChecker.kt	`62.50% <62.50%> (ø)`
...d/sessionreplay/SystemRequirementsConfiguration.kt	`50.00% <50.00%> (ø)`

... and 33 files with indirect coverage changes

ambushwork · 2024-09-19T13:23:17Z

@0xnm @jonathanmos I just refactored the PR, please review it again if you want, main changes:

Add public API to SessionReplayConfiguration to set SystemRequirementConfigutation
Switch back to main thread to register feature after checking the requirements.

ambushwork · 2024-09-24T12:27:50Z

I applied the @0xnm 's change in my branch, please check again, thanks!

0xnm · 2024-09-25T08:45:57Z

+  companion object 
+    val DEFAULT: SystemRequirementConfiguration
+    val NONE: SystemRequirementConfiguration


should these be exposed to the user? what is the case for using NONE on the client-side - always enable Session Replay?

Yes, I think we need to provide an option to the client that session replay can be enabled in any case without checking any system requirements.

0xnm · 2024-09-25T08:47:09Z

        private var textAndInputPrivacy = TextAndInputPrivacy.MASK_ALL
        private var extensionSupport: ExtensionSupport = NoOpExtensionSupport()
        private var dynamicOptimizationEnabled = true
+        private var systemRequirementConfiguration = SystemRequirementConfiguration.NONE


shouldn't it be DEFAULT? because the default value for the default Session Replay configuration is NONE, while DEFAULT also exists.

Well, from my point of view of product design, I assume that at the beginning, most of user will not be aware of the existence of this API, in this case should we add a default limiter to disable the session replay? with this concern I use NONE as the default configuration, for the client who is aware of this API, they can use DEFAULT one to setup manually.

my point is that default is not called DEFAULT, but NONE, but I see that it is for the backward compatibility

You are right, but I wonder what name we should give to them, let's check the opinion of the other reviewers.

Note
I share @0xnm 's concern, mostly in term of naming.
The default value is NONE, so maybe the other value could be BASIC. Another option could be to use a naming similar to Facebook's device-year-class library. That lib would categorize the device with the year when devices with the same capabilities were most popular. E.g.: it would tell you that a low-end phone purchased today is equivalent to a 2018 standard phone.
Our "default" setting (minCPUCores = 2, minRAMSizeMb = 1024) looks like a 2011 phone specs.

0xnm

I think this one mostly lgtm, but I would like to have another pair of eyes having a fresh look here, especially on the public API changes.

@xgouchet or @mariusc83 can you please take a look?

0xnm · 2024-09-27T13:54:43Z

        private var textAndInputPrivacy = TextAndInputPrivacy.MASK_ALL
        private var extensionSupport: ExtensionSupport = NoOpExtensionSupport()
        private var dynamicOptimizationEnabled = true
+        private var systemRequirementConfiguration = SystemRequirementConfiguration.NONE


my point is that default is not called DEFAULT, but NONE, but I see that it is for the backward compatibility

xgouchet · 2024-09-30T07:43:52Z

        private var textAndInputPrivacy = TextAndInputPrivacy.MASK_ALL
        private var extensionSupport: ExtensionSupport = NoOpExtensionSupport()
        private var dynamicOptimizationEnabled = true
+        private var systemRequirementConfiguration = SystemRequirementConfiguration.NONE


Note
I share @0xnm 's concern, mostly in term of naming.
The default value is NONE, so maybe the other value could be BASIC. Another option could be to use a naming similar to Facebook's device-year-class library. That lib would categorize the device with the year when devices with the same capabilities were most popular. E.g.: it would tell you that a low-end phone purchased today is equivalent to a 2018 standard phone.
Our "default" setting (minCPUCores = 2, minRAMSizeMb = 1024) looks like a 2011 phone specs.

xgouchet · 2024-09-30T07:44:52Z

+        /**
+         * Defines the minimum system requirements for enabling the Session Replay feature.
+         * When [SessionReplay.enable] is invoked, the system configuration is verified against these requirements.
+         * If the system meets the specified criteria, Session Replay will be successfully enabled.


Note
We should add in the kdoc here that by default, we do not have any minimum requirements and Session Replay will be applied to all devices

@xgouchet Should our approach not be to optimize performance by default (including not running by default on weak devices), but give users a way to opt-out of our optimizations? If so, the default precheck would be applied for all users with the caveat that they can turn it off with the flag that we've provided in the configuration.

@jonathanmos as we discussed with @MaelNamNam before, this precheck should not be covered by the flag of the dynamic optimisation, they should be different API to enable/disable. I think we can keep it disable by default, I will add doc to elaborate

xgouchet · 2024-09-30T08:09:59Z

+        val checkers = listOf(
+            CPURequirementChecker(minCPUCores, internalLogger = internalLogger),
+            MemoryRequirementChecker(minRAMSizeMb, internalLogger = internalLogger)
+        )
+        (sdkCore as InternalSdkCore).getPersistenceExecutorService().submitSafe(OPERATION_NAME, internalLogger) {
+            val checkResult = checkers.all {
+                it.checkMinimumRequirement()
+            }


Warning
So I have a bit of a concern here. Right now it's not a problem, but it could become one eventually.
We create a new Checker instance and perform the checks every time we call the runIfMeetRequirements() method. The thing is that the cpu and ram size will never change on a given device, meaning we could memoize the result of the check for the current session and ensure we don't perform the check on every call. Not necessary for this PR but we should probably add a task for this.

I see the point here, so the way of memorising it for me is to persist result in shared preference or a file, but I may wonder if this solution will have the same effect with the current one because current checkers actually just read files. So the difference of two solutions is just the file that they are reading if I am not wrong?

Well, I was more thinking on an in memory check (technically some device coud have a Ram upgrade, or we could have some different checks in the future), to limit the check to once per app run.
In the current use case (the SystemRequirementConfiguration class being only used in one place) it wouldn't change anything, as I said, which is why just keeping a Jira task (and maybe a TODO comment in the class) is enough for now

0xnm

Lgtm. I left a few suggestions, but nothing blocking.

…ture

ambushwork force-pushed the yl/sr-opt/add-min-specs branch 4 times, most recently from 2072869 to 7ebd002 Compare September 17, 2024 11:15

ambushwork changed the title ~~RUN-5182: Add precheck conditions when registering session replay feature~~ RUM-5182: Add precheck conditions when registering session replay feature Sep 17, 2024

ambushwork force-pushed the yl/sr-opt/add-min-specs branch from 7ebd002 to 4275f94 Compare September 17, 2024 11:38

ambushwork force-pushed the yl/sr-opt/add-min-specs branch from 4275f94 to a17dc90 Compare September 17, 2024 12:13

ambushwork marked this pull request as ready for review September 17, 2024 12:13

ambushwork requested review from a team as code owners September 17, 2024 12:13

0xnm reviewed Sep 17, 2024

View reviewed changes

jonathanmos reviewed Sep 18, 2024

View reviewed changes

ambushwork force-pushed the yl/sr-opt/add-min-specs branch 2 times, most recently from 6792490 to d57126f Compare September 18, 2024 09:41

ambushwork requested review from 0xnm and jonathanmos September 18, 2024 09:42

0xnm reviewed Sep 18, 2024

View reviewed changes

ambushwork force-pushed the yl/sr-opt/add-min-specs branch from d57126f to d15d780 Compare September 18, 2024 12:01

jonathanmos previously approved these changes Sep 19, 2024

View reviewed changes

Comment thread ...tlin/com/datadog/android/sessionreplay/internal/prerequisite/MemoryRequirementCheckerTest.kt Outdated

ambushwork dismissed jonathanmos’s stale review via 92aee94 September 19, 2024 12:51

ambushwork force-pushed the yl/sr-opt/add-min-specs branch 3 times, most recently from de30f0a to 3ed945f Compare September 19, 2024 13:18

ambushwork force-pushed the yl/sr-opt/add-min-specs branch 2 times, most recently from 6669428 to 6cc73ca Compare September 19, 2024 13:40

0xnm reviewed Sep 19, 2024

View reviewed changes

ambushwork force-pushed the yl/sr-opt/add-min-specs branch from 6cc73ca to 281cb42 Compare September 20, 2024 13:12

ambushwork requested a review from 0xnm September 20, 2024 13:13

jonathanmos reviewed Sep 23, 2024

View reviewed changes

ambushwork force-pushed the yl/sr-opt/add-min-specs branch 3 times, most recently from 355b137 to c089683 Compare September 24, 2024 10:58

ambushwork requested a review from jonathanmos September 24, 2024 12:28

0xnm reviewed Sep 25, 2024

View reviewed changes

ambushwork force-pushed the yl/sr-opt/add-min-specs branch 2 times, most recently from d00334f to 277ad5a Compare September 25, 2024 14:20

ambushwork requested a review from 0xnm September 27, 2024 09:04

0xnm reviewed Sep 27, 2024

View reviewed changes

ambushwork force-pushed the yl/sr-opt/add-min-specs branch 2 times, most recently from a9cd682 to 72d14db Compare September 27, 2024 15:53

jonathanmos previously approved these changes Sep 29, 2024

View reviewed changes

xgouchet previously approved these changes Sep 30, 2024

View reviewed changes

ambushwork dismissed stale reviews from xgouchet and jonathanmos via b52034f September 30, 2024 11:50

ambushwork force-pushed the yl/sr-opt/add-min-specs branch 2 times, most recently from b52034f to 24d1af6 Compare September 30, 2024 12:14

xgouchet previously approved these changes Oct 1, 2024

View reviewed changes

ambushwork requested review from 0xnm and jonathanmos October 1, 2024 10:04

0xnm previously approved these changes Oct 1, 2024

View reviewed changes

RUM-5182: Add precheck conditions when registering session replay fea…

70b09b1

…ture

ambushwork dismissed stale reviews from 0xnm and xgouchet via 70b09b1 October 2, 2024 08:00

ambushwork force-pushed the yl/sr-opt/add-min-specs branch from 24d1af6 to 70b09b1 Compare October 2, 2024 08:00

0xnm approved these changes Oct 2, 2024

View reviewed changes

ambushwork merged commit 2b56563 into feature/sr-dynamic-optimisation Oct 2, 2024

ambushwork deleted the yl/sr-opt/add-min-specs branch October 2, 2024 09:44

Uh oh!

Conversation

ambushwork commented Sep 17, 2024

What does this PR do?

Motivation

Review checklist (to be filled by reviewers)

Uh oh!

codecov-commenter commented Sep 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ambushwork commented Sep 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ambushwork commented Sep 24, 2024

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

0xnm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

codecov-commenter commented Sep 17, 2024 •

edited

Loading

ambushwork commented Sep 19, 2024 •

edited

Loading