Skip to content

RUM-2572 WebView - force fullsnapshot in browser sdk when RUM native view changed#1807

Merged
mariusc83 merged 1 commit into
feature/sr-web-view-supportfrom
mconstantin/rum-2572/force-browser-sdk-fullsnapsot-when-context-changed
Jan 5, 2024
Merged

RUM-2572 WebView - force fullsnapshot in browser sdk when RUM native view changed#1807
mariusc83 merged 1 commit into
feature/sr-web-view-supportfrom
mconstantin/rum-2572/force-browser-sdk-fullsnapsot-when-context-changed

Conversation

@mariusc83

@mariusc83 mariusc83 commented Jan 4, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

What does this PR do?

Because the player always needs a fullsnapshot as a starting state to apply the mutations whenever it needs to play a RUM view event we need to make sure that for the WebView we always force a fullSnapshotRecord in the Datadog browser sdk that is tracking the loaded web page whenever the RUM view changes.

Motivation

What inspired you to submit this pull request?

Additional Notes

Anything else we should know when reviewing?

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)

@mariusc83 mariusc83 self-assigned this Jan 4, 2024
@mariusc83
mariusc83 marked this pull request as ready for review January 4, 2024 14:54
@mariusc83
mariusc83 requested review from a team as code owners January 4, 2024 14:54
@codecov-commenter

codecov-commenter commented Jan 4, 2024 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #1807 (e699ced) into feature/sr-web-view-support (d02623f) will increase coverage by 0.07%.
The diff coverage is 100.00%.

Additional details and impacted files 
@@                       Coverage Diff                       @@
##           feature/sr-web-view-support    #1807      +/-   ##
===============================================================
+ Coverage                        83.66%   83.74%   +0.07%     
===============================================================
  Files                              471      472       +1     
  Lines                            16497    16527      +30     
  Branches                          2473     2478       +5     
===============================================================
+ Hits                             13802    13839      +37     
+ Misses                            2015     2010       -5     
+ Partials                           680      678       -2
Files Coverage Δ
...dog/android/sessionreplay/SessionReplayRecorder.kt 94.81% <100.00%> (+0.07%) ⬆️
...essionreplay/internal/recorder/SnapshotProducer.kt 91.67% <100.00%> (+3.79%) ⬆️
.../recorder/webview/WebViewBrowserSnapshotHandler.kt 100.00% <100.00%> (ø)

... and 29 files with indirect coverage changes

0xnm
0xnm reviewed Jan 4, 2024
View reviewed changes
Comment thread detekt_custom.yml
Comment thread ...play/src/main/kotlin/com/datadog/android/sessionreplay/internal/recorder/SnapshotProducer.kt Outdated
Comment thread ...com/datadog/android/sessionreplay/internal/recorder/webview/WebViewBrowserSnapshotHandler.kt Outdated
Comment thread ...com/datadog/android/sessionreplay/internal/recorder/webview/WebViewBrowserSnapshotHandler.kt Outdated
Comment thread ...com/datadog/android/sessionreplay/internal/recorder/webview/WebViewBrowserSnapshotHandler.kt
Comment thread ...com/datadog/android/sessionreplay/internal/recorder/webview/WebViewBrowserSnapshotHandler.kt Outdated
@mariusc83
mariusc83 force-pushed the mconstantin/rum-2572/force-browser-sdk-fullsnapsot-when-context-changed branch from fd7fd93 to b2c9544 Compare January 5, 2024 09:21
0xnm
0xnm previously approved these changes Jan 5, 2024
View reviewed changes

@0xnm 0xnm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm, I've added few suggestions

Comment thread ...datadog/android/sessionreplay/internal/recorder/webview/WebViewBrowserSnapshotHandlerTest.kt
Comment on lines +164 to +167
val cacheOverload = forge.anInt(
min = WebViewBrowserSnapshotHandler.DATA_CACHE_ENTRIES_LIMIT,
max = WebViewBrowserSnapshotHandler.DATA_CACHE_ENTRIES_LIMIT + 10
)

@0xnm 0xnm Jan 5, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

broken formatting

Suggested change
val cacheOverload = forge.anInt(
min = WebViewBrowserSnapshotHandler.DATA_CACHE_ENTRIES_LIMIT,
max = WebViewBrowserSnapshotHandler.DATA_CACHE_ENTRIES_LIMIT + 10
)
val cacheOverload = forge.anInt(
min = WebViewBrowserSnapshotHandler.DATA_CACHE_ENTRIES_LIMIT,
max = WebViewBrowserSnapshotHandler.DATA_CACHE_ENTRIES_LIMIT + 10
)

Comment thread ...datadog/android/sessionreplay/internal/recorder/webview/WebViewBrowserSnapshotHandlerTest.kt Outdated

@Test
fun `M purge the cached data W purgeWebViewFullSnapshotStateMap{cach limit was exceeded}`(
forge: Forge

@0xnm 0xnm Jan 5, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
forge: Forge
forge: Forge

0xnm
0xnm reviewed Jan 5, 2024
View reviewed changes
Comment thread ...com/datadog/android/sessionreplay/internal/recorder/webview/WebViewBrowserSnapshotHandler.kt Outdated
private fun purgeWebViewFullSnapshotStateMap() {
val iterator = webViewsFullSnapshotState.entries.iterator()
@Suppress("UnsafeThirdPartyFunctionCall")
// next/remove can't fail: we checked hasNext and there is not concurrent access

@0xnm 0xnm Jan 5, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
// next/remove can't fail: we checked hasNext and there is not concurrent access
// next/remove can't fail: we checked hasNext and there is no concurrent access

@mariusc83
mariusc83 force-pushed the mconstantin/rum-2572/force-browser-sdk-fullsnapsot-when-context-changed branch 3 times, most recently from 874f865 to 8fe00af Compare January 5, 2024 10:30
@mariusc83
mariusc83 requested a review from 0xnm January 5, 2024 12:08
0xnm
0xnm previously approved these changes Jan 5, 2024
View reviewed changes
Comment thread ...datadog/android/sessionreplay/internal/recorder/webview/WebViewBrowserSnapshotHandlerTest.kt Outdated
}

@Test
fun `M purge the cached data W purgeWebViewFullSnapshotStateMap{cach limit was exceeded}`(

@0xnm 0xnm Jan 5, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
fun `M purge the cached data W purgeWebViewFullSnapshotStateMap{cach limit was exceeded}`(
fun `M purge the cached data W purgeWebViewFullSnapshotStateMap{cache limit was exceeded}`(

@mariusc83
mariusc83 force-pushed the mconstantin/rum-2572/force-browser-sdk-fullsnapsot-when-context-changed branch from 8fe00af to bd66167 Compare January 5, 2024 12:30
0xnm
0xnm reviewed Jan 5, 2024
View reviewed changes
Comment thread .editorconfig Outdated
# SPDX License Names
[buildSrc/src/main/kotlin/com/datadog/gradle/plugin/checklicenses/SPDXLicense.kt]
ktlint_standard_enum-entry-name-case = disabled No newline at end of file
ktlint_standard_enum-entry-name-case = di sabled No newline at end of file

@0xnm 0xnm Jan 5, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

that is some wrong change

@mariusc83 mariusc83 Jan 5, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yes...it is al this problems with ktlint that I have to do it manually...I will work on that task right now to fix it in develop

@0xnm 0xnm Jan 5, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ktlint shouldn't touch this file though

@mariusc83
mariusc83 force-pushed the mconstantin/rum-2572/force-browser-sdk-fullsnapsot-when-context-changed branch from bd66167 to e699ced Compare January 5, 2024 12:45
@mariusc83
mariusc83 requested a review from 0xnm January 5, 2024 12:45
@mariusc83
mariusc83 merged commit de212fc into feature/sr-web-view-support Jan 5, 2024
@mariusc83
mariusc83 deleted the mconstantin/rum-2572/force-browser-sdk-fullsnapsot-when-context-changed branch January 5, 2024 14:23
mariusc83 added a commit that referenced this pull request Jan 12, 2024
@mariusc83
Revert "Merge pull request #1807 from DataDog/mconstantin/rum-2572/fo…
1ad25a8 
…rce-browser-sdk-fullsnapsot-when-context-changed"

This reverts commit de212fc, reversing
changes made to d02623f.
mariusc83 added a commit that referenced this pull request Jan 15, 2024
@mariusc83
Merge pull request #1821 from DataDog/mconstantin/drop-force-full-sna…
08668d7 
…pshot-logic

Revert "Merge pull request #1807 from DataDog/mconstantin/rum-2572/fo…
mariusc83 added a commit that referenced this pull request Jan 17, 2024
@mariusc83
Revert "Merge pull request #1807 from DataDog/mconstantin/rum-2572/fo…
13a7578 
…rce-browser-sdk-fullsnapsot-when-context-changed"

This reverts commit de212fc, reversing
changes made to d02623f.
@xgouchet xgouchet added this to the 2.8.x milestone Apr 5, 2024
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

Assignees

@mariusc83 mariusc83

Labels

None yet

Projects

None yet

Milestone

2.8.x

Development

Successfully merging this pull request may close these issues.

4 participants

@mariusc83 @codecov-commenter @0xnm @xgouchet