Skip to content

Commit 1480798

Browse files
0xnm0xnm
authored
Merge pull request #2619 from DataDog/nogorodnikov/rum-9711/resolve-batch-file-only-during-write-call
RUM-9711: Resolve batch file only during the actual write call
2 parents ef654e8 + 00b3f6e commit 1480798

File tree

4 files changed

+91
-178
lines changed

4 files changed

+91
-178
lines changed

‎dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/internal/persistence/ConsentAwareStorage.kt

Lines changed: 10 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,6 @@ import com.datadog.android.core.internal.persistence.file.existsSafe
2424
import com.datadog.android.core.internal.persistence.file.lengthSafe
2525
import com.datadog.android.core.internal.privacy.ConsentProvider
2626
import com.datadog.android.core.internal.utils.executeSafe
27-
import com.datadog.android.core.metrics.MethodCallSamplingRate
28-
import com.datadog.android.core.metrics.TelemetryMetricType
2927
import com.datadog.android.privacy.TrackingConsent
3028
import java.io.File
3129
import java.util.Locale
@@ -60,41 +58,24 @@ internal class ConsentAwareStorage(
6058
forceNewBatch: Boolean,
6159
callback: (EventBatchWriter) -> Unit
6260
) {
63-
val metric = internalLogger.startPerformanceMeasure(
64-
callerClass = ConsentAwareStorage::class.java.name,
65-
metric = TelemetryMetricType.MethodCalled,
66-
samplingRate = MethodCallSamplingRate.RARE.rate,
67-
operationName = "writeCurrentBatch[$featureName]"
68-
)
6961
executorService.executeSafe("Data write", internalLogger) {
7062
val orchestrator = resolveOrchestrator()
63+
// TODO RUM-9712 Put performance metric for event processing + event write measurement
7164
if (orchestrator == null) {
7265
callback.invoke(NoOpEventBatchWriter())
73-
metric?.stopAndSend(false)
7466
return@executeSafe
7567
}
7668
synchronized(writeLock) {
77-
val batchFile = orchestrator.getWritableFile(forceNewBatch)
78-
val metadataFile = if (batchFile != null) {
79-
orchestrator.getMetadataFile(batchFile)
80-
} else {
81-
null
82-
}
83-
val writer = if (batchFile == null) {
84-
NoOpEventBatchWriter()
85-
} else {
86-
FileEventBatchWriter(
87-
batchFile = batchFile,
88-
metadataFile = metadataFile,
89-
eventsWriter = batchEventsReaderWriter,
90-
metadataReaderWriter = batchMetadataReaderWriter,
91-
filePersistenceConfig = filePersistenceConfig,
92-
batchWriteEventListener = this,
93-
internalLogger = internalLogger
94-
)
95-
}
69+
val writer = FileEventBatchWriter(
70+
fileOrchestrator = orchestrator,
71+
forceNewBatch = forceNewBatch,
72+
eventsWriter = batchEventsReaderWriter,
73+
metadataReaderWriter = batchMetadataReaderWriter,
74+
filePersistenceConfig = filePersistenceConfig,
75+
batchWriteEventListener = this,
76+
internalLogger = internalLogger
77+
)
9678
callback.invoke(writer)
97-
metric?.stopAndSend(writer !is NoOpEventBatchWriter)
9879
}
9980
}
10081
}

‎dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/internal/persistence/FileEventBatchWriter.kt

Lines changed: 34 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import com.datadog.android.api.InternalLogger
1111
import com.datadog.android.api.storage.EventBatchWriter
1212
import com.datadog.android.api.storage.EventType
1313
import com.datadog.android.api.storage.RawBatchEvent
14+
import com.datadog.android.core.internal.persistence.file.FileOrchestrator
1415
import com.datadog.android.core.internal.persistence.file.FilePersistenceConfig
1516
import com.datadog.android.core.internal.persistence.file.FileReaderWriter
1617
import com.datadog.android.core.internal.persistence.file.FileWriter
@@ -19,20 +20,37 @@ import java.io.File
1920
import java.util.Locale
2021

2122
internal class FileEventBatchWriter(
22-
private val batchFile: File,
23-
private val metadataFile: File?,
23+
private val fileOrchestrator: FileOrchestrator,
24+
private val forceNewBatch: Boolean,
2425
private val eventsWriter: FileWriter<RawBatchEvent>,
2526
private val metadataReaderWriter: FileReaderWriter,
2627
private val filePersistenceConfig: FilePersistenceConfig,
2728
private val batchWriteEventListener: BatchWriteEventListener,
2829
private val internalLogger: InternalLogger
2930
) : EventBatchWriter {
3031

32+
@get:WorkerThread
33+
private val batchFile: File? by lazy {
34+
@Suppress("ThreadSafety") // called in the worker context
35+
fileOrchestrator.getWritableFile(forceNewFile = forceNewBatch)
36+
}
37+
38+
@get:WorkerThread
39+
private val metadataFile: File?
40+
get() = batchFile?.let {
41+
@Suppress("ThreadSafety") // called in the worker context
42+
fileOrchestrator.getMetadataFile(it)
43+
}
44+
3145
@WorkerThread
3246
override fun currentMetadata(): ByteArray? {
33-
if (metadataFile == null || !metadataFile.existsSafe(internalLogger)) return null
34-
35-
return metadataReaderWriter.readData(metadataFile)
47+
return with(metadataFile) {
48+
if (this == null || !existsSafe(internalLogger)) {
49+
null
50+
} else {
51+
metadataReaderWriter.readData(this)
52+
}
53+
}
3654
}
3755

3856
@WorkerThread
@@ -41,6 +59,16 @@ internal class FileEventBatchWriter(
4159
batchMetadata: ByteArray?,
4260
eventType: EventType
4361
): Boolean {
62+
val (batchFile, metadataFile) = batchFile to metadataFile
63+
if (batchFile == null) {
64+
internalLogger.log(
65+
InternalLogger.Level.ERROR,
66+
targets = listOf(InternalLogger.Target.USER, InternalLogger.Target.TELEMETRY),
67+
{ NO_BATCH_FILE_AVAILABLE }
68+
)
69+
return false
70+
}
71+
4472
// prevent useless operation for empty event
4573
return if (event.data.isEmpty()) {
4674
true
@@ -99,5 +127,6 @@ internal class FileEventBatchWriter(
99127
companion object {
100128
internal const val WARNING_METADATA_WRITE_FAILED = "Unable to write metadata file: %s"
101129
internal const val ERROR_LARGE_DATA = "Can't write data with size %d (max item size is %d)"
130+
internal const val NO_BATCH_FILE_AVAILABLE = "No batch file available"
102131
}
103132
}

‎dd-sdk-android-core/src/test/kotlin/com/datadog/android/core/internal/persistence/ConsentAwareStorageTest.kt

Lines changed: 6 additions & 105 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,6 @@ import com.datadog.android.core.internal.persistence.file.FilePersistenceConfig
2121
import com.datadog.android.core.internal.persistence.file.FileReaderWriter
2222
import com.datadog.android.core.internal.persistence.file.batch.BatchFileReaderWriter
2323
import com.datadog.android.core.internal.privacy.ConsentProvider
24-
import com.datadog.android.core.metrics.PerformanceMetric
25-
import com.datadog.android.core.metrics.TelemetryMetricType
2624
import com.datadog.android.privacy.TrackingConsent
2725
import com.datadog.android.utils.forge.Configurator
2826
import com.datadog.android.utils.verifyLog
@@ -108,9 +106,6 @@ internal class ConsentAwareStorageTest {
108106
@Forgery
109107
lateinit var fakeEventType: EventType
110108

111-
@Mock
112-
lateinit var mockMetric: PerformanceMetric
113-
114109
@StringForgery(StringForgeryType.ALPHABETICAL)
115110
lateinit var fakeRootDirName: String
116111

@@ -141,15 +136,6 @@ internal class ConsentAwareStorageTest {
141136
whenever((mockPendingOrchestrator).decrementAndGetPendingFilesCount())
142137
.thenReturn(fakePendingBatches - 1)
143138

144-
whenever(
145-
mockInternalLogger.startPerformanceMeasure(
146-
"com.datadog.android.core.internal.persistence.ConsentAwareStorage",
147-
TelemetryMetricType.MethodCalled,
148-
0.001f,
149-
"writeCurrentBatch[$fakeFeatureName]"
150-
)
151-
) doReturn mockMetric
152-
153139
testedStorage = ConsentAwareStorage(
154140
// same thread executor
155141
executorService = FakeSameThreadExecutorService(),
@@ -170,123 +156,49 @@ internal class ConsentAwareStorageTest {
170156

171157
@Test
172158
fun `M provide writer W writeCurrentBatch() {consent=granted}`(
173-
@BoolForgery forceNewBatch: Boolean,
174-
@Forgery file: File,
175-
forge: Forge
176-
) {
177-
// Given
178-
val mockCallback = mock<(EventBatchWriter) -> Unit>()
179-
whenever(mockConsentProvider.getConsent()) doReturn TrackingConsent.GRANTED
180-
whenever(mockGrantedOrchestrator.getWritableFile(forceNewBatch)) doReturn file
181-
val mockMetaFile: File? = forge.aNullable { mock() }
182-
whenever(mockGrantedOrchestrator.getMetadataFile(file)) doReturn mockMetaFile
183-
184-
// When
185-
testedStorage.writeCurrentBatch(fakeDatadogContext, forceNewBatch, callback = mockCallback)
186-
187-
// Then
188-
verify(mockGrantedOrchestrator).getWritableFile(forceNewBatch)
189-
verify(mockGrantedOrchestrator).getMetadataFile(file)
190-
argumentCaptor<EventBatchWriter> {
191-
verify(mockCallback).invoke(capture())
192-
assertThat(firstValue).isInstanceOf(FileEventBatchWriter::class.java)
193-
}
194-
verify(mockMetric).stopAndSend(true)
195-
verifyNoMoreInteractions(
196-
mockGrantedOrchestrator,
197-
mockPendingOrchestrator,
198-
mockBatchReaderWriter,
199-
mockMetaReaderWriter,
200-
mockMetric
201-
)
202-
}
203-
204-
@Test
205-
fun `M provide no-op writer W writeCurrentBatch(){granted, no file}`(
206159
@BoolForgery forceNewBatch: Boolean
207160
) {
208161
// Given
209162
val mockCallback = mock<(EventBatchWriter) -> Unit>()
210163
whenever(mockConsentProvider.getConsent()) doReturn TrackingConsent.GRANTED
211-
whenever(mockGrantedOrchestrator.getWritableFile(forceNewBatch)) doReturn null
212164

213165
// When
214166
testedStorage.writeCurrentBatch(fakeDatadogContext, forceNewBatch, callback = mockCallback)
215167

216168
// Then
217-
verify(mockGrantedOrchestrator).getWritableFile(forceNewBatch)
218-
argumentCaptor<EventBatchWriter> {
219-
verify(mockCallback).invoke(capture())
220-
assertThat(firstValue).isInstanceOf(NoOpEventBatchWriter::class.java)
221-
}
222-
verify(mockMetric).stopAndSend(false)
223-
verifyNoMoreInteractions(
224-
mockGrantedOrchestrator,
225-
mockPendingOrchestrator,
226-
mockBatchReaderWriter,
227-
mockMetaReaderWriter,
228-
mockMetric
229-
)
230-
}
231-
232-
@Test
233-
fun `M provide writer W writeCurrentBatch() {consent=pending}`(
234-
@BoolForgery forceNewBatch: Boolean,
235-
@Forgery file: File,
236-
forge: Forge
237-
) {
238-
// Given
239-
val mockCallback = mock<(EventBatchWriter) -> Unit>()
240-
whenever(mockConsentProvider.getConsent()) doReturn TrackingConsent.PENDING
241-
whenever(mockPendingOrchestrator.getWritableFile(forceNewBatch)) doReturn file
242-
val mockMetaFile: File? = forge.aNullable { mock() }
243-
whenever(mockPendingOrchestrator.getMetadataFile(file)) doReturn mockMetaFile
244-
245-
// When
246-
testedStorage.writeCurrentBatch(fakeDatadogContext, forceNewBatch, callback = mockCallback)
247-
248-
// Then
249-
verify(mockPendingOrchestrator).getWritableFile(forceNewBatch)
250-
verify(mockPendingOrchestrator).getMetadataFile(file)
251169
argumentCaptor<EventBatchWriter> {
252170
verify(mockCallback).invoke(capture())
253171
assertThat(firstValue).isInstanceOf(FileEventBatchWriter::class.java)
254172
}
255-
verify(mockMetric).stopAndSend(true)
256173
verifyNoMoreInteractions(
257174
mockGrantedOrchestrator,
258175
mockPendingOrchestrator,
259176
mockBatchReaderWriter,
260-
mockMetaReaderWriter,
261-
mockMetric
177+
mockMetaReaderWriter
262178
)
263179
}
264180

265181
@Test
266-
fun `M provide no-op writer W writeCurrentBatch() {pending, no file}`(
182+
fun `M provide writer W writeCurrentBatch() {consent=pending}`(
267183
@BoolForgery forceNewBatch: Boolean
268184
) {
269185
// Given
270186
val mockCallback = mock<(EventBatchWriter) -> Unit>()
271187
whenever(mockConsentProvider.getConsent()) doReturn TrackingConsent.PENDING
272-
whenever(mockPendingOrchestrator.getWritableFile(forceNewBatch)) doReturn null
273188

274189
// When
275190
testedStorage.writeCurrentBatch(fakeDatadogContext, forceNewBatch, callback = mockCallback)
276191

277192
// Then
278-
verify(mockPendingOrchestrator).getWritableFile(forceNewBatch)
279193
argumentCaptor<EventBatchWriter> {
280194
verify(mockCallback).invoke(capture())
281-
assertThat(firstValue).isInstanceOf(NoOpEventBatchWriter::class.java)
195+
assertThat(firstValue).isInstanceOf(FileEventBatchWriter::class.java)
282196
}
283-
verify(mockMetric).stopAndSend(false)
284197
verifyNoMoreInteractions(
285198
mockGrantedOrchestrator,
286199
mockPendingOrchestrator,
287200
mockBatchReaderWriter,
288-
mockMetaReaderWriter,
289-
mockMetric
201+
mockMetaReaderWriter
290202
)
291203
}
292204

@@ -297,14 +209,6 @@ internal class ConsentAwareStorageTest {
297209
// Given
298210
val mockCallback = mock<(EventBatchWriter) -> Unit>()
299211
whenever(mockConsentProvider.getConsent()) doReturn TrackingConsent.NOT_GRANTED
300-
whenever(
301-
mockInternalLogger.startPerformanceMeasure(
302-
"com.datadog.android.core.internal.persistence.ConsentAwareStorage",
303-
TelemetryMetricType.MethodCalled,
304-
0.001f,
305-
"writeCurrentBatch[null]"
306-
)
307-
) doReturn mockMetric
308212

309213
// When
310214
testedStorage.writeCurrentBatch(fakeDatadogContext, forceNewBatch, callback = mockCallback)
@@ -314,13 +218,11 @@ internal class ConsentAwareStorageTest {
314218
verify(mockCallback).invoke(capture())
315219
assertThat(firstValue).isInstanceOf(NoOpEventBatchWriter::class.java)
316220
}
317-
verify(mockMetric).stopAndSend(false)
318221
verifyNoMoreInteractions(
319222
mockGrantedOrchestrator,
320223
mockPendingOrchestrator,
321224
mockBatchReaderWriter,
322-
mockMetaReaderWriter,
323-
mockMetric
225+
mockMetaReaderWriter
324226
)
325227
}
326228

@@ -365,8 +267,7 @@ internal class ConsentAwareStorageTest {
365267
mockGrantedOrchestrator,
366268
mockPendingOrchestrator,
367269
mockBatchReaderWriter,
368-
mockMetaReaderWriter,
369-
mockMetric
270+
mockMetaReaderWriter
370271
)
371272
}
372273

0 commit comments

Comments
 (0)