Skip to content

Commit 37a0cee

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

2 files changed

Lines changed: 198 additions & 2 deletions

File tree

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

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

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

4143
@WorkerThread
@@ -83,7 +85,17 @@ internal class RumDataWriter(
8385
@WorkerThread
8486
internal fun onDataWritten(data: Any, rawData: ByteArray) {
8587
when (data) {
86-
is ViewEvent -> sdkCore.writeLastViewEvent(rawData)
88+
is ViewEvent -> onViewEventWritten(data, rawData)
89+
}
90+
}
91+
92+
@WorkerThread
93+
private fun onViewEventWritten(data: ViewEvent, rawData: ByteArray) {
94+
val isPersistedView = data.view.id == lastPersistedViewId
95+
val isViewStart = data.dd.documentVersion == FIRST_VIEW_DOCUMENT_VERSION
96+
if (isPersistedView || isViewStart) {
97+
sdkCore.writeLastViewEvent(rawData)
98+
lastPersistedViewId = data.view.id
8799
}
88100
}
89101

@@ -92,6 +104,8 @@ internal class RumDataWriter(
92104
companion object {
93105
val EMPTY_BYTE_ARRAY = ByteArray(0)
94106

107+
internal const val FIRST_VIEW_DOCUMENT_VERSION = 2L
108+
95109
private const val UNKNOWN_EVENT_TYPE = "unknown"
96110

97111
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: 183 additions & 1 deletion
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
@@ -336,14 +338,190 @@ internal class RumDataWriterTest {
336338
fun `M persist the event into the NDK crash folder W onDataWritten(){ViewEvent+dir exists}`(
337339
@Forgery viewEvent: ViewEvent
338340
) {
341+
// Given - the first event of a view
342+
val viewStartEvent = viewEvent.copy(
343+
dd = viewEvent.dd.copy(documentVersion = RumDataWriter.FIRST_VIEW_DOCUMENT_VERSION)
344+
)
345+
339346
// When
340-
testedWriter.onDataWritten(viewEvent, fakeSerializedData)
347+
testedWriter.onDataWritten(viewStartEvent, fakeSerializedData)
341348

342349
// Then
343350
verify(rumMonitor.mockSdkCore).writeLastViewEvent(fakeSerializedData)
344351
verifyNoInteractions(mockInternalLogger)
345352
}
346353

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

349527
// region accessibility
@@ -401,6 +579,10 @@ internal class RumDataWriterTest {
401579
// endregion
402580

403581
companion object {
582+
private const val VIEW_A_ID = "view-a"
583+
private const val VIEW_B_ID = "view-b"
584+
private const val FAKE_VIEW_UPDATE_DOCUMENT_VERSION = 7L
585+
404586
val rumMonitor = GlobalRumMonitorTestConfiguration()
405587

406588
@TestConfigurationsProvider

0 commit comments

Comments
 (0)