Skip to content

Commit e2f0626

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

2 files changed

Lines changed: 172 additions & 1 deletion

File tree

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

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,41 @@ internal class RumDataWriter(
8383
@WorkerThread
8484
internal fun onDataWritten(data: Any, rawData: ByteArray) {
8585
when (data) {
86-
is ViewEvent -> sdkCore.writeLastViewEvent(rawData)
86+
is ViewEvent -> onViewEventWritten(data, rawData)
87+
}
88+
}
89+
90+
/**
91+
* Persists the view event as the `last_view_event` used to attribute a crash to a RUM view,
92+
* but only if it describes the current view.
93+
*
94+
* `view.isActive` cannot be used to make that decision: it is set to `!viewComplete`, so a
95+
* stopped view with pending resources/actions/errors keeps emitting `isActive = true` updates
96+
* even though a newer view is already displayed. The current view is resolved from the RUM
97+
* feature context instead.
98+
*/
99+
@WorkerThread
100+
private fun onViewEventWritten(data: ViewEvent, rawData: ByteArray) {
101+
// useContextThread = false is mandatory here: this runs on the RUM thread while the context
102+
// thread is blocked waiting for the event to be handled, dispatching the read to the
103+
// context thread would deadlock.
104+
val rumContext = sdkCore.getFeatureContext(Feature.RUM_FEATURE_NAME, useContextThread = false)
105+
val currentViewId = rumContext[RumContext.VIEW_ID] as? String
106+
val currentViewStartedAt = rumContext[RumContext.VIEW_TIMESTAMP] as? Long ?: 0L
107+
108+
// the event belongs to the current view: updates of that view, including its final
109+
// (complete) one, keep refreshing the snapshot
110+
val isCurrentView = data.view.id == currentViewId
111+
// the RUM feature context is only updated once the event has been written, so it is one
112+
// event behind: a view which started after the one held in context is a newer view, which
113+
// is about to become the current one. `ViewEvent.date` and `RumContext.viewTimestamp` are
114+
// both the view start timestamp, so they are comparable.
115+
val isNewerView = data.date > currentViewStartedAt
116+
117+
// anything else is an update of a view which was superseded by a newer one: dropping it
118+
// prevents crashes from being misattributed to that stale view
119+
if (isCurrentView || isNewerView) {
120+
sdkCore.writeLastViewEvent(rawData)
87121
}
88122
}
89123

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

Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
package com.datadog.android.rum.internal.domain
88

99
import com.datadog.android.api.InternalLogger
10+
import com.datadog.android.api.feature.Feature
1011
import com.datadog.android.api.storage.EventType
1112
import com.datadog.android.api.storage.RawBatchEvent
1213
import com.datadog.android.core.internal.storage.TelemetryAwareEventBatchWriter
@@ -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,6 +338,9 @@ 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 event belongs to the current view
342+
stubCurrentView(viewEvent.view.id, viewEvent.date)
343+
339344
// When
340345
testedWriter.onDataWritten(viewEvent, fakeSerializedData)
341346

@@ -344,6 +349,133 @@ internal class RumDataWriterTest {
344349
verifyNoInteractions(mockInternalLogger)
345350
}
346351

352+
@Test
353+
fun `M call writeLastViewEvent W onDataWritten() { ViewEvent, update of the current view }`(
354+
@Forgery viewEvent: ViewEvent
355+
) {
356+
// Given - view A is the current view, its update is older than nothing else
357+
stubCurrentView(VIEW_A_ID, VIEW_A_TIMESTAMP)
358+
359+
// When - view A emits an update, complete or not
360+
val fakeData = writeViewEvent(viewEvent, VIEW_A_ID, VIEW_A_TIMESTAMP, isActive = false)
361+
362+
// Then
363+
verify(rumMonitor.mockSdkCore).writeLastViewEvent(fakeData)
364+
}
365+
366+
@Test
367+
fun `M call writeLastViewEvent W onDataWritten() { ViewEvent of a newer view }`(
368+
@Forgery viewEvent: ViewEvent
369+
) {
370+
// Given - view A is still the current view in context: the context is only updated once
371+
// the event has been written, so it is one event behind
372+
stubCurrentView(VIEW_A_ID, VIEW_A_TIMESTAMP)
373+
374+
// When - view B, which started after view A, emits its first event
375+
val fakeViewBData = writeViewEvent(viewEvent, VIEW_B_ID, VIEW_B_TIMESTAMP, isActive = true)
376+
377+
// Then - the newer view takes over the persisted snapshot
378+
verify(rumMonitor.mockSdkCore).writeLastViewEvent(fakeViewBData)
379+
}
380+
381+
@Test
382+
fun `M call writeLastViewEvent W onDataWritten() { ViewEvent, no current view }`(
383+
@Forgery viewEvent: ViewEvent
384+
) {
385+
// Given - no view is active anymore
386+
whenever(
387+
rumMonitor.mockSdkCore.getFeatureContext(Feature.RUM_FEATURE_NAME, false)
388+
) doReturn emptyMap()
389+
390+
// When - the last view completes
391+
val fakeData = writeViewEvent(viewEvent, VIEW_A_ID, VIEW_A_TIMESTAMP, isActive = false)
392+
393+
// Then
394+
verify(rumMonitor.mockSdkCore).writeLastViewEvent(fakeData)
395+
}
396+
397+
@Test
398+
fun `M NOT call writeLastViewEvent W onDataWritten() { ViewEvent, stale view still marked active }`(
399+
@Forgery viewEvent: ViewEvent
400+
) {
401+
// Given - view A was stopped while resources were still pending, view B is the current view
402+
stubCurrentView(VIEW_B_ID, VIEW_B_TIMESTAMP)
403+
404+
// When - a pending resource of view A completes: view A is not complete yet, so it still
405+
// emits an event with isActive = true
406+
val fakeStaleViewAData = writeViewEvent(viewEvent, VIEW_A_ID, VIEW_A_TIMESTAMP, isActive = true)
407+
408+
// Then - the stale view does not overwrite the snapshot of view B
409+
verify(rumMonitor.mockSdkCore, never()).writeLastViewEvent(fakeStaleViewAData)
410+
}
411+
412+
@Test
413+
fun `M NOT call writeLastViewEvent W onDataWritten() { ViewEvent, stale view completing }`(
414+
@Forgery viewEvent: ViewEvent
415+
) {
416+
// Given - view B is the current view
417+
stubCurrentView(VIEW_B_ID, VIEW_B_TIMESTAMP)
418+
419+
// When - the last pending event of view A completes it
420+
val fakeCompletedViewAData = writeViewEvent(viewEvent, VIEW_A_ID, VIEW_A_TIMESTAMP, isActive = false)
421+
422+
// Then - the completion of the stale view does not overwrite the snapshot of view B
423+
verify(rumMonitor.mockSdkCore, never()).writeLastViewEvent(fakeCompletedViewAData)
424+
}
425+
426+
@Test
427+
fun `M NOT read the RUM context on the context thread W onDataWritten() { ViewEvent }`(
428+
@Forgery viewEvent: ViewEvent
429+
) {
430+
// Given
431+
stubCurrentView(viewEvent.view.id, viewEvent.date)
432+
433+
// When
434+
testedWriter.onDataWritten(viewEvent, fakeSerializedData)
435+
436+
// Then - reading on the context thread would deadlock: the context thread is waiting for
437+
// this event to be handled
438+
verify(rumMonitor.mockSdkCore).getFeatureContext(Feature.RUM_FEATURE_NAME, false)
439+
verify(rumMonitor.mockSdkCore, never()).getFeatureContext(Feature.RUM_FEATURE_NAME, true)
440+
}
441+
442+
// endregion
443+
444+
// region Internal
445+
446+
/**
447+
* Stubs the RUM feature context to describe the given view as the current one.
448+
*/
449+
private fun stubCurrentView(viewId: String, viewTimestamp: Long) {
450+
whenever(
451+
rumMonitor.mockSdkCore.getFeatureContext(Feature.RUM_FEATURE_NAME, false)
452+
) doReturn mapOf(
453+
RumContext.VIEW_ID to viewId,
454+
RumContext.VIEW_TIMESTAMP to viewTimestamp
455+
)
456+
}
457+
458+
/**
459+
* Notifies the writer that a view event was written for the given view, and returns the
460+
* serialized data used for that event.
461+
*/
462+
private fun writeViewEvent(
463+
viewEvent: ViewEvent,
464+
viewId: String,
465+
viewTimestamp: Long,
466+
isActive: Boolean
467+
): ByteArray {
468+
val serializedData = "$viewId-$viewTimestamp".toByteArray(Charsets.UTF_8)
469+
testedWriter.onDataWritten(
470+
viewEvent.copy(
471+
date = viewTimestamp,
472+
view = viewEvent.view.copy(id = viewId, isActive = isActive)
473+
),
474+
serializedData
475+
)
476+
return serializedData
477+
}
478+
347479
// endregion
348480

349481
// region accessibility
@@ -401,6 +533,11 @@ internal class RumDataWriterTest {
401533
// endregion
402534

403535
companion object {
536+
private const val VIEW_A_ID = "view-a"
537+
private const val VIEW_B_ID = "view-b"
538+
private const val VIEW_A_TIMESTAMP = 1_000L
539+
private const val VIEW_B_TIMESTAMP = 2_000L
540+
404541
val rumMonitor = GlobalRumMonitorTestConfiguration()
405542

406543
@TestConfigurationsProvider

0 commit comments

Comments
 (0)