RUM-4098: Use the datastore for Session Replay resources#2041
Conversation
56e08c5 to
542d2b1
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2041 +/- ##
============================================
+ Coverage 69.56% 83.22% +13.67%
============================================
Files 715 511 -204
Lines 26553 18119 -8434
Branches 4452 2719 -1733
============================================
- Hits 18469 15079 -3390
+ Misses 6871 2293 -4578
+ Partials 1213 747 -466
|
6279225 to
564eb64
Compare
542d2b1 to
7986738
Compare
5e26358 to
921ee19
Compare
405c260 to
085ed01
Compare
c181326 to
f0ceab6
Compare
555637d to
8891749
Compare
05dcf33 to
1c3f646
Compare
307e5b3 to
41e1c72
Compare
bc635ab to
2e2c6f1
Compare
| fun removeValue( | ||
| key: String | ||
| key: String, | ||
| callback: DataStoreWriteCallback |
There was a problem hiding this comment.
what us the use-case when we are interested in the result?
There was a problem hiding this comment.
For when we remove the expired datastore entry, because this is an async operation and therefore we need to know when it's finished in order to be sure that the initialization of the datastoremanager is complete.
| key: String, | ||
| data: T, | ||
| version: Int = 0, | ||
| callback: DataStoreWriteCallback, |
There was a problem hiding this comment.
what us the use-case when we are interested in the result?
There was a problem hiding this comment.
At the moment for setValue there is no specific use-case, but I felt that it makes sense to also have a callback here, since we have one for removeValue
| val data = ResourceHashesEntry( | ||
| lastUpdateDateNs = storedLastUpdateDateNs, | ||
| resourceHashes = knownResources.toList() | ||
| ) |
There was a problem hiding this comment.
probably not the most efficient design. Imagine you have 1000 hashes, each is 36 characters (UUID length). All chars are ASCII, so each char is 1 byte. It means that once you add 1001th hash, you need to write (and also serialize) all 1000 hashes you wrote before, which is 36KB. So with each write operation you have data overhead of 36KB, instead of storing only the hash you need to.
There was a problem hiding this comment.
after discussion, we'll leave this for now and perhaps improve in a future pr
|
|
||
| private fun fetchStoredResourceHashes( | ||
| onFetchSuccessful: (dataStoreContent: DataStoreContent<ResourceHashesEntry>?) -> Unit, | ||
| onFetchFailure: () -> Unit |
There was a problem hiding this comment.
minor: there is no need to have this argument, because the only call site of this method gives empty lambda.
There was a problem hiding this comment.
This lambda should actually not be empty but should have a call to finishedInitializingManager. I'll add some tests to catch missing calls
| private val resourceHashesSerializer: Serializer<ResourceHashesEntry>, | ||
| private val resourceHashesDeserializer: Deserializer<String, ResourceHashesEntry> | ||
| ) { | ||
| private val knownResources = Collections.synchronizedSet(mutableSetOf<String>()) |
There was a problem hiding this comment.
note: if performance is critical here, it is better to use Collections.newSetFromMap(ConcurrentHashMap<String, Boolean>). Overall the performance of ConcurrentHashMap is better that synchronised map (but there are some differences in behavior).
There was a problem hiding this comment.
Good point - I've modified this
fa0553f to
fa9f3b1
Compare
0xnm
left a comment
0xnm
left a comment
There was a problem hiding this comment.
lgtm. but my concern about writing all the hashes each time we just need to add one more to storage stays, not sure how much it can become a bottleneck in the app with many resources.
fa9f3b1 to
4641fbf
Compare
5 participants
What does this PR do?
Use the datastore from Session Replay, so that resource hashes are persisted between sessions.
Motivation
The second part of the effort to persist bitmap hashes between sessions.
Additional Notes
Anything else we should know when reviewing?
Review checklist (to be filled by reviewers)