RUMM-2672: Make Storage#writeCurrentBatch async #1094
Conversation
Codecov Report
@@ Coverage Diff @@
## feature/sdkv2 #1094 +/- ##
=================================================
+ Coverage 82.52% 82.66% +0.14%
=================================================
Files 347 349 +2
Lines 11196 11229 +33
Branches 1838 1838
=================================================
+ Hits 9239 9282 +43
+ Misses 1395 1388 -7
+ Partials 562 559 -3
|
| override fun withWriteContext(callback: (DatadogContext, EventBatchWriter) -> Unit) { | ||
| // TODO RUMM-0000 thread safety. Thread switch happens in Storage right now. Open questions: | ||
| // * what if called wants to have a sync operation, without thread switch |
There was a problem hiding this comment.
I am thinking when will be the case/need for that. Do you have some situations in mind ?
There was a problem hiding this comment.
Crash reporting - we need write it as log or RUM event in a blocking manner.
There was a problem hiding this comment.
true, forgot about that.
| } else { | ||
| listener.onDataWriteFailed(eventId) | ||
| FileEventBatchWriter( |
There was a problem hiding this comment.
are we creating a writer every time we are writing something ?
There was a problem hiding this comment.
yes, every time we request write context (which should be short-living), because writer is associated with a particular batch file
There was a problem hiding this comment.
Damn...I am thinking that for SR I will have to write tons of events....meaning that we will be creating those writers all the time. Aren't we afraid of too many objects there ? I mean I know they are not so heavy but was thinking that maybe if we could do it differently 🤔
There was a problem hiding this comment.
We can profile it. Anyway, the number of these objects will be much less that number of snapshot/wireframes objects to write :)
| @@ -16,13 +18,17 @@ import com.datadog.tools.annotation.NoOpImplementation | |||
| @NoOpImplementation | |||
| internal interface Storage { | |||
|
|
|||
| // TODO RUMM-0000 It seems only write part is useful to be async. Having upload part as async | |||
0xnm
deleted the
nogorodnikov/rumm-2672/make-storage-writecurrentbatch-async
branch
What does this PR do?
This PR may look big, but essentially it does the following:
Storage#writeCurrentBatchimplementation is now async (ConsentAwareStoragetakes an executor), meaning I/O access inside this method is happening on the worker thread and callback is invoked on the worker thread as well.EventBatchWriterinto a dedicated class (and moved tests as well), becauseConsentAwareStorageand its tests were too big already. Introduced also a no-op implementation of writer if there is no batch file or consent is not granted - this is where the most of the diff comes from.This change is needed to make
FeatureScope#withWriteContexttruly async to be able to implement event write context further.Review checklist (to be filled by reviewers)