Skip to content

RUM-16113: Prevent inactive views from overwriting last_view_event #3707

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ambushwork wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

RUM-16113: Prevent inactive views from overwriting last_view_event #3707

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 25 additions & 1 deletion ...-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/internal/domain/RumDataWriter.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,17 @@ internal class RumDataWriter(
private val sdkCore: InternalSdkCore
) : DataWriter<Any> {

private var currentViewId: String? = null
Comment thread
ambushwork marked this conversation as resolved.

// region DataWriter

@WorkerThread
@Suppress("ReturnCount")
override fun write(writer: EventBatchWriter, element: Any, eventType: EventType): Boolean {
if (element is ViewEvent) {
onViewEventSubmitted(element)
}
Comment thread
ambushwork marked this conversation as resolved.

val byteArray = eventSerializer.serializeToByteArray(element, sdkCore.internalLogger)
?: return false

Expand Down Expand Up @@ -83,7 +89,23 @@ internal class RumDataWriter(
@WorkerThread
internal fun onDataWritten(data: Any, rawData: ByteArray) {
when (data) {
is ViewEvent -> sdkCore.writeLastViewEvent(rawData)
is ViewEvent -> onViewEventWritten(data, rawData)
}
}

@WorkerThread
internal fun onViewEventSubmitted(data: ViewEvent) {
synchronized(this) {
if (data.dd.documentVersion == FIRST_VIEW_DOCUMENT_VERSION) {
currentViewId = data.view.id
}
}
}

@WorkerThread
private fun onViewEventWritten(data: ViewEvent, rawData: ByteArray) {
if (data.view.id == currentViewId) {
Comment thread
satween marked this conversation as resolved.
Comment thread
ambushwork marked this conversation as resolved.
sdkCore.writeLastViewEvent(rawData)
}
}

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

internal const val FIRST_VIEW_DOCUMENT_VERSION = 2L

private const val UNKNOWN_EVENT_TYPE = "unknown"

private fun resolveEventType(event: Any): String = when (event) {
Expand Down
246 changes: 244 additions & 2 deletions ...-android-rum/src/test/kotlin/com/datadog/android/rum/internal/domain/RumDataWriterTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import com.datadog.tools.unit.extensions.config.TestConfiguration
import com.datadog.tools.unit.forge.aThrowable
import fr.xgouchet.elmyr.Forge
import fr.xgouchet.elmyr.annotation.Forgery
import fr.xgouchet.elmyr.annotation.IntForgery
import fr.xgouchet.elmyr.annotation.StringForgery
import fr.xgouchet.elmyr.junit5.ForgeConfiguration
import fr.xgouchet.elmyr.junit5.ForgeExtension
Expand All @@ -46,6 +47,7 @@ import org.mockito.kotlin.anyOrNull
import org.mockito.kotlin.argumentCaptor
import org.mockito.kotlin.doReturn
import org.mockito.kotlin.doThrow
import org.mockito.kotlin.never
import org.mockito.kotlin.times
import org.mockito.kotlin.verify
import org.mockito.kotlin.verifyNoInteractions
Expand Down Expand Up @@ -334,16 +336,252 @@ internal class RumDataWriterTest {

@Test
fun `M persist the event into the NDK crash folder W onDataWritten(){ViewEvent+dir exists}`(
@Forgery viewEvent: ViewEvent
@Forgery fakeViewEvent: ViewEvent
) {
// Given - the first event of a view
val fakeViewStartEvent = fakeViewEvent.copy(
dd = fakeViewEvent.dd.copy(documentVersion = RumDataWriter.FIRST_VIEW_DOCUMENT_VERSION)
)
testedWriter.onViewEventSubmitted(fakeViewStartEvent)

// When
testedWriter.onDataWritten(viewEvent, fakeSerializedData)
testedWriter.onDataWritten(fakeViewStartEvent, fakeSerializedData)

// Then
verify(rumMonitor.mockSdkCore).writeLastViewEvent(fakeSerializedData)
verifyNoInteractions(mockInternalLogger)
}

@Test
fun `M call writeLastViewEvent W onDataWritten() { ViewEvent of a new view }`(
@Forgery fakeViewEvent: ViewEvent
) {
// Given - view A is persisted
writeViewStart(fakeViewEvent, VIEW_A_ID)

// When - view B starts and emits its first event
val fakeViewBData = writeViewStart(fakeViewEvent, VIEW_B_ID)

// Then - the new view takes over the persisted snapshot
verify(rumMonitor.mockSdkCore).writeLastViewEvent(fakeViewBData)
}

@Test
fun `M call writeLastViewEvent W onDataWritten() { ViewEvent, update of the persisted view }`(
@Forgery fakeViewEvent: ViewEvent
) {
// Given - view A is persisted
writeViewStart(fakeViewEvent, VIEW_A_ID)

// When - the same view completes (stopped, no newer view)
val fakeCompletedViewAData = writeViewUpdate(fakeViewEvent, VIEW_A_ID, isActive = false, marker = "complete")

// Then - the snapshot is refreshed in place with the final event
verify(rumMonitor.mockSdkCore).writeLastViewEvent(fakeCompletedViewAData)
}

@Test
fun `M NOT call writeLastViewEvent W onDataWritten() { ViewEvent, stale view still marked active }`(
@Forgery fakeViewEvent: ViewEvent
) {
// Given - view A was stopped while resources were still pending, then view B started
writeViewStart(fakeViewEvent, VIEW_A_ID)
writeViewStart(fakeViewEvent, VIEW_B_ID)

// When - a pending resource of view A completes: view A is not complete yet, so it still
// emits an event with isActive = true
val fakeStaleViewAData = writeViewUpdate(fakeViewEvent, VIEW_A_ID, isActive = true, marker = "stale")

// Then - the stale view does not overwrite the snapshot of view B
verify(rumMonitor.mockSdkCore, never()).writeLastViewEvent(fakeStaleViewAData)
}

@Test
fun `M NOT call writeLastViewEvent W onDataWritten() { ViewEvent, stale view completing }`(
@Forgery fakeViewEvent: ViewEvent
) {
// Given - view A was stopped while resources were still pending, view B started, then one
// of view A's pending resources completed
writeViewStart(fakeViewEvent, VIEW_A_ID)
writeViewStart(fakeViewEvent, VIEW_B_ID)
writeViewUpdate(fakeViewEvent, VIEW_A_ID, isActive = true, marker = "stale")

// When - the last pending event of view A completes it
val fakeCompletedViewAData = writeViewUpdate(fakeViewEvent, VIEW_A_ID, isActive = false, marker = "complete")

// Then - the completion of the stale view does not overwrite the snapshot of view B
verify(rumMonitor.mockSdkCore, never()).writeLastViewEvent(fakeCompletedViewAData)
}

@Test
fun `M NOT call writeLastViewEvent W onDataWritten() { ViewEvent, stale view, no active view }`(
@Forgery fakeViewEvent: ViewEvent
) {
// Given - view A was stopped while resources were still pending, view B started and then
// completed too, so no view is active anymore
writeViewStart(fakeViewEvent, VIEW_A_ID)
writeViewStart(fakeViewEvent, VIEW_B_ID)
val fakeCompletedViewBData = writeViewUpdate(fakeViewEvent, VIEW_B_ID, isActive = false, marker = "complete")

// When - a pending event of view A completes it, after the last view completed
val fakeCompletedViewAData = writeViewUpdate(fakeViewEvent, VIEW_A_ID, isActive = false, marker = "complete")

// Then - view A does not overwrite the snapshot of the last view
verify(rumMonitor.mockSdkCore).writeLastViewEvent(fakeCompletedViewBData)
verify(rumMonitor.mockSdkCore, never()).writeLastViewEvent(fakeCompletedViewAData)
}

@Test
fun `M NOT call writeLastViewEvent W onDataWritten() { ViewEvent, stale view after many views }`(
@Forgery fakeViewEvent: ViewEvent,
@IntForgery(min = 20, max = 100) fakeViewCount: Int
) {
// Given - view A was stopped while a resource was still pending, then the user navigated
// through many other views
writeViewStart(fakeViewEvent, VIEW_A_ID)
repeat(fakeViewCount) {
writeViewStart(fakeViewEvent, "view-$it")
}

// When - the pending resource of view A finally completes
val fakeStaleViewAData = writeViewUpdate(fakeViewEvent, VIEW_A_ID, isActive = true, marker = "stale")

// Then - the stale view does not overwrite the snapshot of the newest view
verify(rumMonitor.mockSdkCore, never()).writeLastViewEvent(fakeStaleViewAData)
}

@Test
fun `M call writeLastViewEvent W onDataWritten() { ViewEvent, first write of the view failed }`(
@Forgery fakeViewEvent: ViewEvent
) {
// Given - view A is persisted, then view B starts but its first event is not written
writeViewStart(fakeViewEvent, VIEW_A_ID)
submitViewStartWithFailedWrite(fakeViewEvent, VIEW_B_ID)

// When - a later event of view B is written
val fakeViewBUpdateData = writeViewUpdate(fakeViewEvent, VIEW_B_ID, isActive = true, marker = "update")

// Then - view B still establishes its snapshot
verify(rumMonitor.mockSdkCore).writeLastViewEvent(fakeViewBUpdateData)
}

@Test
fun `M NOT call writeLastViewEvent W onDataWritten() { ViewEvent, stale view, first write failed }`(
@Forgery fakeViewEvent: ViewEvent
) {
// Given - view A was stopped while resources were still pending, then view B started but
// its first event was not written
writeViewStart(fakeViewEvent, VIEW_A_ID)
submitViewStartWithFailedWrite(fakeViewEvent, VIEW_B_ID)

// When - a pending event of view A completes
val fakeStaleViewAData = writeViewUpdate(fakeViewEvent, VIEW_A_ID, isActive = true, marker = "stale")

// Then - view A is not the current view anymore, even though nothing was persisted for B
verify(rumMonitor.mockSdkCore, never()).writeLastViewEvent(fakeStaleViewAData)
}

@Test
fun `M keep the newest view persisted W onDataWritten() { ViewEvent, interleaved views }`(
@Forgery fakeViewEvent: ViewEvent
) {
// When
writeViewStart(fakeViewEvent, VIEW_A_ID)
writeViewStart(fakeViewEvent, VIEW_B_ID)
val fakeStaleViewAData = writeViewUpdate(fakeViewEvent, VIEW_A_ID, isActive = true, marker = "stale")
val fakeViewBUpdateData = writeViewUpdate(fakeViewEvent, VIEW_B_ID, isActive = false, marker = "complete")

// Then
verify(rumMonitor.mockSdkCore, never()).writeLastViewEvent(fakeStaleViewAData)
argumentCaptor<ByteArray> {
verify(rumMonitor.mockSdkCore, times(3)).writeLastViewEvent(capture())
assertThat(lastValue).isEqualTo(fakeViewBUpdateData)
}
}

// endregion

// region Internal

/**
* Notifies the writer that the first event of the given view was submitted and written, and
* returns the serialized data used for that event.
*/
private fun writeViewStart(fakeViewEvent: ViewEvent, viewId: String): ByteArray {
return writeViewEvent(
fakeViewEvent = fakeViewEvent,
viewId = viewId,
isActive = true,
documentVersion = RumDataWriter.FIRST_VIEW_DOCUMENT_VERSION,
marker = "start"
)
}

/**
* Notifies the writer that the first event of the given view was submitted, but that its batch
* write failed, so it was never written.
*/
private fun submitViewStartWithFailedWrite(fakeViewEvent: ViewEvent, viewId: String) {
testedWriter.onViewEventSubmitted(
forgeViewEvent(
fakeViewEvent = fakeViewEvent,
viewId = viewId,
isActive = true,
documentVersion = RumDataWriter.FIRST_VIEW_DOCUMENT_VERSION
)
)
}

/**
* Notifies the writer that a subsequent event of the given view was submitted and written, and
* returns the serialized data used for that event.
*/
private fun writeViewUpdate(
fakeViewEvent: ViewEvent,
viewId: String,
isActive: Boolean,
marker: String
): ByteArray {
return writeViewEvent(
fakeViewEvent = fakeViewEvent,
viewId = viewId,
isActive = isActive,
documentVersion = FAKE_VIEW_UPDATE_DOCUMENT_VERSION,
marker = marker
)
}

private fun forgeViewEvent(
fakeViewEvent: ViewEvent,
viewId: String,
isActive: Boolean,
documentVersion: Long
): ViewEvent {
return fakeViewEvent.copy(
view = fakeViewEvent.view.copy(id = viewId, isActive = isActive),
dd = fakeViewEvent.dd.copy(documentVersion = documentVersion)
)
}

private fun writeViewEvent(
fakeViewEvent: ViewEvent,
viewId: String,
isActive: Boolean,
documentVersion: Long,
marker: String
): ByteArray {
val serializedData = "$viewId-$marker".toByteArray(Charsets.UTF_8)
val event = forgeViewEvent(
fakeViewEvent = fakeViewEvent,
viewId = viewId,
isActive = isActive,
documentVersion = documentVersion
)
testedWriter.onViewEventSubmitted(event)
testedWriter.onDataWritten(event, serializedData)
return serializedData
}

// endregion

// region accessibility
Expand Down Expand Up @@ -401,6 +639,10 @@ internal class RumDataWriterTest {
// endregion

companion object {
private const val VIEW_A_ID = "view-a"
private const val VIEW_B_ID = "view-b"
private const val FAKE_VIEW_UPDATE_DOCUMENT_VERSION = 7L

val rumMonitor = GlobalRumMonitorTestConfiguration()

@TestConfigurationsProvider
Expand Down
Loading