Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

RUM-6093:Add TimeBank in session replay recorder for dynamic optimisation #2247

Merged
merged 1 commit into from
Sep 10, 2024
Merged

RUM-6093:Add TimeBank in session replay recorder for dynamic optimisation #2247

merged 1 commit into from
Sep 10, 2024

Conversation

ambushwork
Copy link
Member

What does this PR do?

Add TimeBank in session replay recorder for dynamic optimisation.

The main idea is to have a time balance in this TimeBank, every execution of session replay recording will consume this balance, and the balance will be recharged during the time passing until a maximum balance is reached.

Before every execution of recording, if the balance is negative, the execution will skipped until the balance is recharged to positive.

Motivation

RUM-6093

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)

@ambushwork ambushwork requested review from a team as code owners September 6, 2024 17:03
xgouchet
xgouchet reviewed Sep 9, 2024
View reviewed changes
...sion-replay/src/main/kotlin/com/datadog/android/sessionreplay/internal/recorder/Debouncer.kt Outdated
if (timePassedSinceLastExecution >= maxRecordDelayInNs) {
executeRunnable(runnable)
} else {
handler.postDelayed({ executeRunnable(runnable) }, DEBOUNCE_TIME_IN_MS)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning

Shouldn't you remove previously posted runnables, if there are multiple debounced executions ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is done above at line 33

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, so I changed the place where runInTimeBalance should wrap up.
Now runInTimeBalance wrap only the function of executeRunnable, so we check first the condition of debounce frequency, then check if it meets the time balance.

mariusc83
mariusc83 reviewed Sep 9, 2024
View reviewed changes
...sion-replay/src/main/kotlin/com/datadog/android/sessionreplay/internal/recorder/Debouncer.kt Outdated Show resolved Hide resolved
...sion-replay/src/main/kotlin/com/datadog/android/sessionreplay/internal/recorder/Debouncer.kt Outdated
if (timePassedSinceLastExecution >= maxRecordDelayInNs) {
executeRunnable(runnable)
} else {
handler.postDelayed({ executeRunnable(runnable) }, DEBOUNCE_TIME_IN_MS)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is done above at line 33

@ambushwork ambushwork force-pushed the yl/sr-opt/cpu-bank branch 2 times, most recently from ff54810 to fe0410a Compare September 9, 2024 08:32
@ambushwork ambushwork force-pushed the yl/sr-opt/cpu-bank branch 2 times, most recently from 8d854e7 to 705fa22 Compare September 9, 2024 13:42
@codecov-commenter
Copy link

codecov-commenter commented Sep 9, 2024 •
edited
Loading

Codecov Report

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

Project coverage is 69.99%. Comparing base (679de59) to head (86dac33).
Report is 2 commits behind head on feature/sr-dynamic-optimisation.

Files with missing lines Patch % Lines
...droid/sessionreplay/internal/recorder/Debouncer.kt 66.67% 4 Missing and 1 partial ⚠️
Additional details and impacted files 
@@                         Coverage Diff                         @@
##           feature/sr-dynamic-optimisation    #2247      +/-   ##
===================================================================
- Coverage                            70.12%   69.99%   -0.13%     
===================================================================
  Files                                  727      728       +1     
  Lines                                27062    27077      +15     
  Branches                              4560     4561       +1     
===================================================================
- Hits                                 18975    18951      -24     
- Misses                                6827     6842      +15     
- Partials                              1260     1284      +24
Files with missing lines Coverage Δ
...ssionreplay/internal/recorder/RecordingTimeBank.kt 100.00% <100.00%> (ø)
...nternal/recorder/listener/WindowsOnDrawListener.kt 97.22% <100.00%> (ø)
...droid/sessionreplay/internal/recorder/Debouncer.kt 81.48% <66.67%> (-18.52%) ⬇️

... and 28 files with indirect coverage changes

mariusc83
mariusc83 previously approved these changes Sep 9, 2024
View reviewed changes
0xnm
0xnm previously approved these changes Sep 10, 2024
View reviewed changes
Copy link
Member

@0xnm 0xnm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, I added some suggestions

...lay/src/main/kotlin/com/datadog/android/sessionreplay/internal/recorder/RecordingTimeBank.kt Outdated Show resolved Hide resolved
...lay/src/main/kotlin/com/datadog/android/sessionreplay/internal/recorder/RecordingTimeBank.kt Outdated Show resolved Hide resolved
...n-replay/src/test/kotlin/com/datadog/android/sessionreplay/recorder/RecordingTimeBankTest.kt Outdated Show resolved Hide resolved
...n-replay/src/test/kotlin/com/datadog/android/sessionreplay/recorder/RecordingTimeBankTest.kt Outdated Show resolved Hide resolved
...n-replay/src/test/kotlin/com/datadog/android/sessionreplay/recorder/RecordingTimeBankTest.kt Outdated Show resolved Hide resolved
...n-replay/src/test/kotlin/com/datadog/android/sessionreplay/recorder/RecordingTimeBankTest.kt Show resolved Hide resolved
...n-replay/src/test/kotlin/com/datadog/android/sessionreplay/recorder/RecordingTimeBankTest.kt Outdated Show resolved Hide resolved
...n-replay/src/test/kotlin/com/datadog/android/sessionreplay/recorder/RecordingTimeBankTest.kt Outdated
Comment on lines 106 to 107
fun `M allow everything W set max balance to 1000ms per sec`(forge: Forge) {
recordingTimeBank = RecordingTimeBank(1000)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if it is more than 1s? shouldn't we cover this as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, I updated the test to M allow everything W set max balance more than 1000ms per sec with a random value with 1000ms as min value.

@ambushwork ambushwork dismissed stale reviews from jonathanmos, 0xnm, and mariusc83 via 86dac33 September 10, 2024 13:02
@ambushwork ambushwork merged commit e0dd0d4 into feature/sr-dynamic-optimisation Sep 10, 2024
23 checks passed
@ambushwork ambushwork deleted the yl/sr-opt/cpu-bank branch September 10, 2024 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xnm 0xnm 0xnm approved these changes

@mariusc83 mariusc83 mariusc83 left review comments

@xgouchet xgouchet Awaiting requested review from xgouchet

@jonathanmos jonathanmos Awaiting requested review from jonathanmos

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ambushwork @codecov-commenter @xgouchet @mariusc83 @0xnm @jonathanmos