RUM-6195: Allow ResourcesLruCache to work with any key #2418

jonathanmos · 2024-11-28T12:12:32Z

What does this PR do?

Modifies ResourcesLruCache to accept a caller-provided key as input rather than always generating the key from the resource. Add the customResourceIdCacheKey parameter to ImageWireframeHelper so that the key can be passed from the outside.

Motivation

This is necessary to support resources passed from Jetpack Compose and React Native.

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)

codecov-commenter · 2024-11-28T13:18:44Z

Codecov Report

Attention: Patch coverage is 94.33962% with 3 lines in your changes missing coverage. Please review.

Project coverage is 70.22%. Comparing base (4590058) to head (006743f).
Report is 63 commits behind head on develop.

Files with missing lines	Patch %	Lines
...sessionreplay/internal/recorder/resources/Cache.kt	0.00%	2 Missing ⚠️
...internal/recorder/resources/BitmapCachesManager.kt	75.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2418      +/-   ##
===========================================
+ Coverage    70.12%   70.22%   +0.10%     
===========================================
  Files          767      767              
  Lines        28490    28513      +23     
  Branches      4778     4782       +4     
===========================================
+ Hits         19977    20021      +44     
+ Misses        7170     7156      -14     
+ Partials      1343     1336       -7

Files with missing lines	Coverage Δ
...ionreplay/material/internal/ChipWireframeMapper.kt	`92.86% <100.00%> (+0.26%)`	⬆️
...nternal/recorder/mapper/CheckableTextViewMapper.kt	`62.07% <100.00%> (+0.67%)`	⬆️
...lay/internal/recorder/mapper/SwitchCompatMapper.kt	`88.43% <100.00%> (+0.19%)`	⬆️
...onreplay/internal/recorder/resources/BitmapPool.kt	`87.50% <100.00%> (ø)`
.../recorder/resources/DefaultImageWireframeHelper.kt	`98.42% <100.00%> (+3.31%)`	⬆️
...ay/internal/recorder/resources/ResourceResolver.kt	`92.54% <100.00%> (+0.67%)`	⬆️
...y/internal/recorder/resources/ResourcesLRUCache.kt	`67.57% <100.00%> (-6.12%)`	⬇️
...order/mapper/BaseAsyncBackgroundWireframeMapper.kt	`94.83% <100.00%> (+0.09%)`	⬆️
...d/sessionreplay/recorder/mapper/ImageViewMapper.kt	`80.00% <100.00%> (+0.45%)`	⬆️
...id/sessionreplay/recorder/mapper/TextViewMapper.kt	`90.10% <100.00%> (+0.10%)`	⬆️
... and 3 more

... and 35 files with indirect coverage changes

...eplay/src/main/kotlin/com/datadog/android/sessionreplay/internal/recorder/resources/Cache.kt

0xnm · 2024-12-09T08:28:44Z

...com/datadog/android/sessionreplay/internal/recorder/resources/DefaultImageWireframeHelper.kt

+                wireframeIndex++
+                val resourceCacheKey = if (resourceIdCacheKey != null) {
+                    "$resourceIdCacheKey" + "_$wireframeIndex"
+                } else {
+                    null
+                }


does it have any impact on the cache hit metric?

I've done some measurements and the hit metric isn't impacted

0xnm · 2024-12-09T08:30:13Z

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

@@ -82,18 +84,25 @@ interface ImageWireframeHelper {
        clipping: MobileSegment.WireframeClip? = null,
        shapeStyle: MobileSegment.ShapeStyle? = null,
        border: MobileSegment.ShapeBorder? = null,
-        prefix: String? = DRAWABLE_CHILD_NAME
+        prefix: String? = DRAWABLE_CHILD_NAME,
+        resourceIdCacheKey: String?


are we not assigning here the default null value on purpose? other nullable args in this function have it.

ambushwork · 2024-12-09T08:58:06Z

...com/datadog/android/sessionreplay/internal/recorder/resources/DefaultImageWireframeHelper.kt

@@ -115,7 +115,8 @@ internal class DefaultImageWireframeHelper(
        clipping: MobileSegment.WireframeClip?,
        shapeStyle: MobileSegment.ShapeStyle?,
        border: MobileSegment.ShapeBorder?,
-        prefix: String?
+        prefix: String?,
+        resourceIdCacheKey: String?


why not give a null value as default since in a lot of case it's not used?

I wanted to avoid as much as possible default values so that it's more explicit what is being passed

ambushwork · 2024-12-09T09:13:42Z

...com/datadog/android/sessionreplay/internal/recorder/resources/DefaultImageWireframeHelper.kt

+                    "$resourceIdCacheKey" + "_$wireframeIndex"
+                } else {
+                    null
+                }


what's the reason this modification is only applied for CompoundDrawable?

CompoundDrawables can have multiple drawables associated, so in the case that an override key is provided the drawables need to be differentiated in the cache but if no key is provided then it will default to the original behavior

0xnm · 2024-12-10T08:24:26Z

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

@@ -65,6 +65,8 @@ interface ImageWireframeHelper {
     * @param shapeStyle provides a custom shape (e.g. rounded corners) to the image wireframe
     * @param border provides a custom border to the image wireframe
     * @param prefix a prefix identifying the drawable in the parent view's context
+     * @param customResourceIdCacheKey an optional key with which to cache or retrieve from the resource cache.
+     * If this key is not provided then one will be generated from the drawable.
     */
    // TODO RUM-3666 limit the number of params to this function
    fun createImageWireframeByDrawable(


side note: it is a bit strange that detekt doesn't flag this method as a method with long list of arguments. Do we have this rule enabled?

...otlin/com/datadog/android/sessionreplay/internal/recorder/resources/ResourcesLRUCacheTest.kt

jonathanmos · 2024-12-17T10:53:00Z

Closed as the contents was squashed into this pr: #2448

jonathanmos changed the title ~~RUM-6195: Allow ResourceCache to work with any key~~ RUM-6195: Allow ResourcesLruCache to work with any key Nov 28, 2024

jonathanmos force-pushed the jmoskovich/rum-6195/caches-any-key branch 2 times, most recently from d4f5994 to 256a4aa Compare November 28, 2024 12:28

RUM-6195: Allow ResourcesLruCache to work with any key

8743eb1

jonathanmos force-pushed the jmoskovich/rum-6195/caches-any-key branch from 256a4aa to 8743eb1 Compare November 28, 2024 12:50

jonathanmos force-pushed the jmoskovich/rum-6195/caches-any-key branch 3 times, most recently from d26f4d8 to 56c49e9 Compare December 5, 2024 15:23

jonathanmos added 2 commits December 8, 2024 13:07

RUM-6195: Add cache key to ImageWireframeHelper

d8939fe

RUM-6195: Add missing ImageWireframeHelper tests

ebafe41

jonathanmos force-pushed the jmoskovich/rum-6195/caches-any-key branch from 56c49e9 to ebafe41 Compare December 8, 2024 13:12

jonathanmos marked this pull request as ready for review December 8, 2024 19:56

jonathanmos requested review from a team as code owners December 8, 2024 19:56

0xnm reviewed Dec 9, 2024

View reviewed changes

ambushwork reviewed Dec 9, 2024

View reviewed changes

RUM-6195: Rename cache key to customResourceIdCacheKey

1dcc621

jonathanmos requested review from ambushwork and 0xnm December 9, 2024 15:21

ambushwork approved these changes Dec 9, 2024

View reviewed changes

xgouchet approved these changes Dec 10, 2024

View reviewed changes

0xnm approved these changes Dec 10, 2024

View reviewed changes

RUM-6195: Minor fixes to test titles

006743f

jonathanmos closed this Dec 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUM-6195: Allow ResourcesLruCache to work with any key #2418

RUM-6195: Allow ResourcesLruCache to work with any key #2418

jonathanmos commented Nov 28, 2024 •

edited

Loading

codecov-commenter commented Nov 28, 2024 •

edited

Loading

0xnm Dec 9, 2024

jonathanmos Dec 9, 2024

0xnm Dec 9, 2024

ambushwork Dec 9, 2024

jonathanmos Dec 9, 2024

ambushwork Dec 9, 2024

jonathanmos Dec 9, 2024

0xnm Dec 10, 2024

jonathanmos commented Dec 17, 2024

RUM-6195: Allow ResourcesLruCache to work with any key #2418

RUM-6195: Allow ResourcesLruCache to work with any key #2418

Conversation

jonathanmos commented Nov 28, 2024 • edited Loading

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

codecov-commenter commented Nov 28, 2024 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanmos commented Dec 17, 2024

jonathanmos commented Nov 28, 2024 •

edited

Loading

codecov-commenter commented Nov 28, 2024 •

edited

Loading