REPLAY-1943: Refactor caches from singletons to class instances

[jonathanmos]

What does this PR do?

Refactor the base64 drawable cache and the bitmap pool to be class instances instead of singletons.

Motivation

Make things more straightforward to test and remove mutable variables

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]

what is the purpose of providing an implementation here and not a Mock ? I see that you are not doing anything in the entryRemoved method ?

[jonathanmos]

It's a slightly more accurate representation of the lrucache we're using. The default implementation calculates size by number of objects, and here we're calculating size by bitmap allocation the same as in the class. Regarding mocking, we discussed before to use the actual lrucache and not a mock in order to make use of the cache in the tests

[mariusc83]

but if you provide this custom implementation in your tests this means that you are altering the class functionality ? This would be acceptable if you want to mock other methods in order to isolate one test case but if you do this for the whole test - suite how is this going to be relevant anymore ? You say here: It's a slightly more accurate representation of the lrucache we're using . Would this mean that you are changing your internal logic ?

[codecov-commenter]

Codecov Report

Merging #1564 (0f66105) into feature/base64-jmoskovich (8002635) will decrease coverage by 0.14%.
Report is 1 commits behind head on feature/base64-jmoskovich.
The diff coverage is 68.80%.

@@                      Coverage Diff                      @@
##           feature/base64-jmoskovich    #1564      +/-   ##
=============================================================
- Coverage                      83.32%   83.19%   -0.14%     
=============================================================
  Files                            443      444       +1     
  Lines                          15070    15095      +25     
  Branches                        2272     2279       +7     
=============================================================
  Hits                           12557    12557              
- Misses                          1923     1941      +18     
- Partials                         590      597       +7     

Files Changed	Coverage Δ
...android/sessionreplay/internal/utils/CacheUtils.kt	15.79% <18.75%> (+15.79%)	⬆️
...roid/sessionreplay/internal/utils/DrawableUtils.kt	96.97% <50.00%> (ø)
...ay/internal/recorder/mapper/BaseWireframeMapper.kt	70.59% <58.33%> (-7.98%)	⬇️
...eplay/internal/recorder/base64/BitmapPoolHelper.kt	62.50% <62.50%> (ø)
...nreplay/internal/recorder/base64/Base64LRUCache.kt	76.74% <66.67%> (-8.10%)	⬇️
...eplay/internal/recorder/base64/Base64Serializer.kt	84.51% <78.95%> (-4.23%)	⬇️
...ssionreplay/internal/recorder/base64/BitmapPool.kt	87.27% <83.33%> (+25.30%)	⬆️
...adog/android/sessionreplay/SessionReplayPrivacy.kt	100.00% <100.00%> (ø)
...id/sessionreplay/internal/utils/InvocationUtils.kt	100.00% <100.00%> (ø)

... and 15 files with indirect coverage changes

[plousada]

Builder methods should be chainable, it's the pattern we've been using in the SDK.
E.g. Configuration.kt allows a use case such as this SampleApplication.kt .

[jonathanmos]

I'm modifying the builder in order to pass the dependencies in the builder constructor, so the setters won't exist anymore

[plousada]

This seems like a highly generic and non actionable message, could we improve it to give more context of what failed?

[jonathanmos]

I've added the stacktrace to the log to give additional context

[plousada]

Curiosity: Did we do any study into the optimal cache size?

[jonathanmos]

This is more of a starting point to improve upon later. All images are downscaled to have a maximum size of around 15kb, so we can calculate for 4mb a capacity of around 270 cached images - which is more than enough even for an app with only images in a grid. Working out the optimal cache size would be quite complicated, given the different use cases of customers - apps with lots of images vs apps with few.

We could think about implementing a cache that autoadjusts its size based upon recorded performance from previous runs (record the maximum percentage utilization and adjust the size of the cache accordingly) but this will add some complexity.

[mariusc83]

nicely done !!