Skip to content

Commit 4ea458e

Browse files
ambushworkambushwork
committed
RUM-16113: Prevent stale views from overwriting last_view_event
1 parent 69bfb31 commit 4ea458e

2 files changed

Lines changed: 269 additions & 3 deletions

File tree

‎features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/internal/domain/RumDataWriter.kt‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,11 +36,17 @@ internal class RumDataWriter(
3636
private val sdkCore: InternalSdkCore
3737
) : DataWriter<Any> {
3838

39+
private var currentViewId: String? = null
40+
3941
// region DataWriter
4042

4143
@WorkerThread
4244
@Suppress("ReturnCount")
4345
override fun write(writer: EventBatchWriter, element: Any, eventType: EventType): Boolean {
46+
if (element is ViewEvent) {
47+
onViewEventSubmitted(element)
48+
}
49+
4450
val byteArray = eventSerializer.serializeToByteArray(element, sdkCore.internalLogger)
4551
?: return false
4652

@@ -83,7 +89,23 @@ internal class RumDataWriter(
8389
@WorkerThread
8490
internal fun onDataWritten(data: Any, rawData: ByteArray) {
8591
when (data) {
86-
is ViewEvent -> sdkCore.writeLastViewEvent(rawData)
92+
is ViewEvent -> onViewEventWritten(data, rawData)
93+
}
94+
}
95+
96+
@WorkerThread
97+
internal fun onViewEventSubmitted(data: ViewEvent) {
98+
synchronized(this) {
99+
if (data.dd.documentVersion == FIRST_VIEW_DOCUMENT_VERSION) {
100+
currentViewId = data.view.id
101+
}
102+
}
103+
}
104+
105+
@WorkerThread
106+
private fun onViewEventWritten(data: ViewEvent, rawData: ByteArray) {
107+
if (data.view.id == currentViewId) {
108+
sdkCore.writeLastViewEvent(rawData)
87109
}
88110
}
89111

@@ -92,6 +114,8 @@ internal class RumDataWriter(
92114
companion object {
93115
val EMPTY_BYTE_ARRAY = ByteArray(0)
94116

117+
internal const val FIRST_VIEW_DOCUMENT_VERSION = 2L
118+
95119
private const val UNKNOWN_EVENT_TYPE = "unknown"
96120

97121
private fun resolveEventType(event: Any): String = when (event) {

‎features/dd-sdk-android-rum/src/test/kotlin/com/datadog/android/rum/internal/domain/RumDataWriterTest.kt‎

Lines changed: 244 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import com.datadog.tools.unit.extensions.config.TestConfiguration
3030
import com.datadog.tools.unit.forge.aThrowable
3131
import fr.xgouchet.elmyr.Forge
3232
import fr.xgouchet.elmyr.annotation.Forgery
33+
import fr.xgouchet.elmyr.annotation.IntForgery
3334
import fr.xgouchet.elmyr.annotation.StringForgery
3435
import fr.xgouchet.elmyr.junit5.ForgeConfiguration
3536
import fr.xgouchet.elmyr.junit5.ForgeExtension
@@ -46,6 +47,7 @@ import org.mockito.kotlin.anyOrNull
4647
import org.mockito.kotlin.argumentCaptor
4748
import org.mockito.kotlin.doReturn
4849
import org.mockito.kotlin.doThrow
50+
import org.mockito.kotlin.never
4951
import org.mockito.kotlin.times
5052
import org.mockito.kotlin.verify
5153
import org.mockito.kotlin.verifyNoInteractions
@@ -334,16 +336,252 @@ internal class RumDataWriterTest {
334336

335337
@Test
336338
fun `M persist the event into the NDK crash folder W onDataWritten(){ViewEvent+dir exists}`(
337-
@Forgery viewEvent: ViewEvent
339+
@Forgery fakeViewEvent: ViewEvent
338340
) {
341+
// Given - the first event of a view
342+
val fakeViewStartEvent = fakeViewEvent.copy(
343+
dd = fakeViewEvent.dd.copy(documentVersion = RumDataWriter.FIRST_VIEW_DOCUMENT_VERSION)
344+
)
345+
testedWriter.onViewEventSubmitted(fakeViewStartEvent)
346+
339347
// When
340-
testedWriter.onDataWritten(viewEvent, fakeSerializedData)
348+
testedWriter.onDataWritten(fakeViewStartEvent, fakeSerializedData)
341349

342350
// Then
343351
verify(rumMonitor.mockSdkCore).writeLastViewEvent(fakeSerializedData)
344352
verifyNoInteractions(mockInternalLogger)
345353
}
346354

355+
@Test
356+
fun `M call writeLastViewEvent W onDataWritten() { ViewEvent of a new view }`(
357+
@Forgery fakeViewEvent: ViewEvent
358+
) {
359+
// Given - view A is persisted
360+
writeViewStart(fakeViewEvent, VIEW_A_ID)
361+
362+
// When - view B starts and emits its first event
363+
val fakeViewBData = writeViewStart(fakeViewEvent, VIEW_B_ID)
364+
365+
// Then - the new view takes over the persisted snapshot
366+
verify(rumMonitor.mockSdkCore).writeLastViewEvent(fakeViewBData)
367+
}
368+
369+
@Test
370+
fun `M call writeLastViewEvent W onDataWritten() { ViewEvent, update of the persisted view }`(
371+
@Forgery fakeViewEvent: ViewEvent
372+
) {
373+
// Given - view A is persisted
374+
writeViewStart(fakeViewEvent, VIEW_A_ID)
375+
376+
// When - the same view completes (stopped, no newer view)
377+
val fakeCompletedViewAData = writeViewUpdate(fakeViewEvent, VIEW_A_ID, isActive = false, marker = "complete")
378+
379+
// Then - the snapshot is refreshed in place with the final event
380+
verify(rumMonitor.mockSdkCore).writeLastViewEvent(fakeCompletedViewAData)
381+
}
382+
383+
@Test
384+
fun `M NOT call writeLastViewEvent W onDataWritten() { ViewEvent, stale view still marked active }`(
385+
@Forgery fakeViewEvent: ViewEvent
386+
) {
387+
// Given - view A was stopped while resources were still pending, then view B started
388+
writeViewStart(fakeViewEvent, VIEW_A_ID)
389+
writeViewStart(fakeViewEvent, VIEW_B_ID)
390+
391+
// When - a pending resource of view A completes: view A is not complete yet, so it still
392+
// emits an event with isActive = true
393+
val fakeStaleViewAData = writeViewUpdate(fakeViewEvent, VIEW_A_ID, isActive = true, marker = "stale")
394+
395+
// Then - the stale view does not overwrite the snapshot of view B
396+
verify(rumMonitor.mockSdkCore, never()).writeLastViewEvent(fakeStaleViewAData)
397+
}
398+
399+
@Test
400+
fun `M NOT call writeLastViewEvent W onDataWritten() { ViewEvent, stale view completing }`(
401+
@Forgery fakeViewEvent: ViewEvent
402+
) {
403+
// Given - view A was stopped while resources were still pending, view B started, then one
404+
// of view A's pending resources completed
405+
writeViewStart(fakeViewEvent, VIEW_A_ID)
406+
writeViewStart(fakeViewEvent, VIEW_B_ID)
407+
writeViewUpdate(fakeViewEvent, VIEW_A_ID, isActive = true, marker = "stale")
408+
409+
// When - the last pending event of view A completes it
410+
val fakeCompletedViewAData = writeViewUpdate(fakeViewEvent, VIEW_A_ID, isActive = false, marker = "complete")
411+
412+
// Then - the completion of the stale view does not overwrite the snapshot of view B
413+
verify(rumMonitor.mockSdkCore, never()).writeLastViewEvent(fakeCompletedViewAData)
414+
}
415+
416+
@Test
417+
fun `M NOT call writeLastViewEvent W onDataWritten() { ViewEvent, stale view, no active view }`(
418+
@Forgery fakeViewEvent: ViewEvent
419+
) {
420+
// Given - view A was stopped while resources were still pending, view B started and then
421+
// completed too, so no view is active anymore
422+
writeViewStart(fakeViewEvent, VIEW_A_ID)
423+
writeViewStart(fakeViewEvent, VIEW_B_ID)
424+
val fakeCompletedViewBData = writeViewUpdate(fakeViewEvent, VIEW_B_ID, isActive = false, marker = "complete")
425+
426+
// When - a pending event of view A completes it, after the last view completed
427+
val fakeCompletedViewAData = writeViewUpdate(fakeViewEvent, VIEW_A_ID, isActive = false, marker = "complete")
428+
429+
// Then - view A does not overwrite the snapshot of the last view
430+
verify(rumMonitor.mockSdkCore).writeLastViewEvent(fakeCompletedViewBData)
431+
verify(rumMonitor.mockSdkCore, never()).writeLastViewEvent(fakeCompletedViewAData)
432+
}
433+
434+
@Test
435+
fun `M NOT call writeLastViewEvent W onDataWritten() { ViewEvent, stale view after many views }`(
436+
@Forgery fakeViewEvent: ViewEvent,
437+
@IntForgery(min = 20, max = 100) fakeViewCount: Int
438+
) {
439+
// Given - view A was stopped while a resource was still pending, then the user navigated
440+
// through many other views
441+
writeViewStart(fakeViewEvent, VIEW_A_ID)
442+
repeat(fakeViewCount) {
443+
writeViewStart(fakeViewEvent, "view-$it")
444+
}
445+
446+
// When - the pending resource of view A finally completes
447+
val fakeStaleViewAData = writeViewUpdate(fakeViewEvent, VIEW_A_ID, isActive = true, marker = "stale")
448+
449+
// Then - the stale view does not overwrite the snapshot of the newest view
450+
verify(rumMonitor.mockSdkCore, never()).writeLastViewEvent(fakeStaleViewAData)
451+
}
452+
453+
@Test
454+
fun `M call writeLastViewEvent W onDataWritten() { ViewEvent, first write of the view failed }`(
455+
@Forgery fakeViewEvent: ViewEvent
456+
) {
457+
// Given - view A is persisted, then view B starts but its first event is not written
458+
writeViewStart(fakeViewEvent, VIEW_A_ID)
459+
submitViewStartWithFailedWrite(fakeViewEvent, VIEW_B_ID)
460+
461+
// When - a later event of view B is written
462+
val fakeViewBUpdateData = writeViewUpdate(fakeViewEvent, VIEW_B_ID, isActive = true, marker = "update")
463+
464+
// Then - view B still establishes its snapshot
465+
verify(rumMonitor.mockSdkCore).writeLastViewEvent(fakeViewBUpdateData)
466+
}
467+
468+
@Test
469+
fun `M NOT call writeLastViewEvent W onDataWritten() { ViewEvent, stale view, first write failed }`(
470+
@Forgery fakeViewEvent: ViewEvent
471+
) {
472+
// Given - view A was stopped while resources were still pending, then view B started but
473+
// its first event was not written
474+
writeViewStart(fakeViewEvent, VIEW_A_ID)
475+
submitViewStartWithFailedWrite(fakeViewEvent, VIEW_B_ID)
476+
477+
// When - a pending event of view A completes
478+
val fakeStaleViewAData = writeViewUpdate(fakeViewEvent, VIEW_A_ID, isActive = true, marker = "stale")
479+
480+
// Then - view A is not the current view anymore, even though nothing was persisted for B
481+
verify(rumMonitor.mockSdkCore, never()).writeLastViewEvent(fakeStaleViewAData)
482+
}
483+
484+
@Test
485+
fun `M keep the newest view persisted W onDataWritten() { ViewEvent, interleaved views }`(
486+
@Forgery fakeViewEvent: ViewEvent
487+
) {
488+
// When
489+
writeViewStart(fakeViewEvent, VIEW_A_ID)
490+
writeViewStart(fakeViewEvent, VIEW_B_ID)
491+
val fakeStaleViewAData = writeViewUpdate(fakeViewEvent, VIEW_A_ID, isActive = true, marker = "stale")
492+
val fakeViewBUpdateData = writeViewUpdate(fakeViewEvent, VIEW_B_ID, isActive = false, marker = "complete")
493+
494+
// Then
495+
verify(rumMonitor.mockSdkCore, never()).writeLastViewEvent(fakeStaleViewAData)
496+
argumentCaptor<ByteArray> {
497+
verify(rumMonitor.mockSdkCore, times(3)).writeLastViewEvent(capture())
498+
assertThat(lastValue).isEqualTo(fakeViewBUpdateData)
499+
}
500+
}
501+
502+
// endregion
503+
504+
// region Internal
505+
506+
/**
507+
* Notifies the writer that the first event of the given view was submitted and written, and
508+
* returns the serialized data used for that event.
509+
*/
510+
private fun writeViewStart(fakeViewEvent: ViewEvent, viewId: String): ByteArray {
511+
return writeViewEvent(
512+
fakeViewEvent = fakeViewEvent,
513+
viewId = viewId,
514+
isActive = true,
515+
documentVersion = RumDataWriter.FIRST_VIEW_DOCUMENT_VERSION,
516+
marker = "start"
517+
)
518+
}
519+
520+
/**
521+
* Notifies the writer that the first event of the given view was submitted, but that its batch
522+
* write failed, so it was never written.
523+
*/
524+
private fun submitViewStartWithFailedWrite(fakeViewEvent: ViewEvent, viewId: String) {
525+
testedWriter.onViewEventSubmitted(
526+
forgeViewEvent(
527+
fakeViewEvent = fakeViewEvent,
528+
viewId = viewId,
529+
isActive = true,
530+
documentVersion = RumDataWriter.FIRST_VIEW_DOCUMENT_VERSION
531+
)
532+
)
533+
}
534+
535+
/**
536+
* Notifies the writer that a subsequent event of the given view was submitted and written, and
537+
* returns the serialized data used for that event.
538+
*/
539+
private fun writeViewUpdate(
540+
fakeViewEvent: ViewEvent,
541+
viewId: String,
542+
isActive: Boolean,
543+
marker: String
544+
): ByteArray {
545+
return writeViewEvent(
546+
fakeViewEvent = fakeViewEvent,
547+
viewId = viewId,
548+
isActive = isActive,
549+
documentVersion = FAKE_VIEW_UPDATE_DOCUMENT_VERSION,
550+
marker = marker
551+
)
552+
}
553+
554+
private fun forgeViewEvent(
555+
fakeViewEvent: ViewEvent,
556+
viewId: String,
557+
isActive: Boolean,
558+
documentVersion: Long
559+
): ViewEvent {
560+
return fakeViewEvent.copy(
561+
view = fakeViewEvent.view.copy(id = viewId, isActive = isActive),
562+
dd = fakeViewEvent.dd.copy(documentVersion = documentVersion)
563+
)
564+
}
565+
566+
private fun writeViewEvent(
567+
fakeViewEvent: ViewEvent,
568+
viewId: String,
569+
isActive: Boolean,
570+
documentVersion: Long,
571+
marker: String
572+
): ByteArray {
573+
val serializedData = "$viewId-$marker".toByteArray(Charsets.UTF_8)
574+
val event = forgeViewEvent(
575+
fakeViewEvent = fakeViewEvent,
576+
viewId = viewId,
577+
isActive = isActive,
578+
documentVersion = documentVersion
579+
)
580+
testedWriter.onViewEventSubmitted(event)
581+
testedWriter.onDataWritten(event, serializedData)
582+
return serializedData
583+
}
584+
347585
// endregion
348586

349587
// region accessibility
@@ -401,6 +639,10 @@ internal class RumDataWriterTest {
401639
// endregion
402640

403641
companion object {
642+
private const val VIEW_A_ID = "view-a"
643+
private const val VIEW_B_ID = "view-b"
644+
private const val FAKE_VIEW_UPDATE_DOCUMENT_VERSION = 7L
645+
404646
val rumMonitor = GlobalRumMonitorTestConfiguration()
405647

406648
@TestConfigurationsProvider

0 commit comments

Comments
 (0)