Skip to content
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

RUM-3012 Do not update RUM View global properties after the view is stopped #1851

Merged
merged 1 commit into from
Feb 13, 2024
Merged

RUM-3012 Do not update RUM View global properties after the view is stopped #1851

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
45 changes: 26 additions & 19 deletions ...android-rum/src/main/kotlin/com/datadog/android/rum/internal/domain/scope/RumViewScope.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@ internal open class RumViewScope(

internal val url = key.url.replace('.', '/')

internal val attributes: MutableMap<String, Any?> = initialAttributes.toMutableMap()
internal val eventAttributes: MutableMap<String, Any?> = initialAttributes.toMutableMap()
private var globalAttributes: MutableMap<String, Any?> = resolveGlobalAttributes(sdkCore)

private var sessionId: String = parentScope.getRumContext().sessionId
internal var viewId: String = UUID.randomUUID().toString()
Expand Down Expand Up @@ -142,7 +143,6 @@ internal open class RumViewScope(
it.putAll(getRumContext().toMap())
it[RumFeature.VIEW_TIMESTAMP_OFFSET_IN_MS_KEY] = serverTimeOffsetInMs
}
attributes.putAll(GlobalRumMonitor.get(sdkCore).getAttributes())
cpuVitalMonitor.register(cpuVitalListener)
memoryVitalMonitor.register(memoryVitalListener)
frameRateVitalMonitor.register(frameRateVitalListener)
Expand All @@ -161,6 +161,7 @@ internal open class RumViewScope(
event: RumRawEvent,
writer: DataWriter<Any>
): RumScope? {
updateGlobalAttributes(sdkCore, event)
mariusc83 marked this conversation as resolved.
Show resolved Hide resolved
when (event) {
is RumRawEvent.ResourceSent -> onResourceSent(event, writer)
is RumRawEvent.ActionSent -> onActionSent(event, writer)
Expand Down Expand Up @@ -279,7 +280,7 @@ internal open class RumViewScope(
)
}
}
attributes.putAll(event.attributes)
eventAttributes.putAll(event.attributes)
stopped = true
sendViewUpdate(event, writer)
sendViewChanged()
Expand Down Expand Up @@ -702,7 +703,6 @@ internal open class RumViewScope(
@Suppress("LongMethod", "ComplexMethod")
private fun sendViewUpdate(event: RumRawEvent, writer: DataWriter<Any>) {
val viewComplete = isViewComplete()
attributes.putAll(GlobalRumMonitor.get(sdkCore).getAttributes())
version++

// make a local copy, so that closure captures the state as of now
Expand Down Expand Up @@ -735,7 +735,7 @@ internal open class RumViewScope(
val isSlowRendered = resolveRefreshRateInfo(refreshRateInfo) ?: false
// make a copy - by the time we iterate over it on another thread, it may already be changed
val eventFeatureFlags = featureFlags.toMutableMap()
val eventAdditionalAttributes = attributes.toMutableMap()
val eventAdditionalAttributes = (eventAttributes + globalAttributes).toMutableMap()

sdkCore.newRumEventWriteOperation(writer) { datadogContext ->
val currentViewId = rumContext.viewId.orEmpty()
Expand Down Expand Up @@ -850,6 +850,16 @@ internal open class RumViewScope(
.submit()
}

private fun updateGlobalAttributes(sdkCore: InternalSdkCore, event: RumRawEvent) {
if (!stopped && event !is RumRawEvent.StartView) {
globalAttributes = resolveGlobalAttributes(sdkCore)
}
}

private fun resolveGlobalAttributes(sdkCore: InternalSdkCore): MutableMap<String, Any?> {
return GlobalRumMonitor.get(sdkCore).getAttributes().toMutableMap()
}

private fun resolveViewDuration(event: RumRawEvent): Long {
val duration = event.eventTime.nanoTime - startedNanos
return if (duration <= 0) {
Expand Down Expand Up @@ -883,8 +893,7 @@ internal open class RumViewScope(
private fun addExtraAttributes(
attributes: Map<String, Any?>
): MutableMap<String, Any?> {
return attributes.toMutableMap()
.apply { putAll(GlobalRumMonitor.get(sdkCore).getAttributes()) }
return attributes.toMutableMap().apply { putAll(globalAttributes) }
}

@Suppress("LongMethod")
Expand All @@ -895,9 +904,7 @@ internal open class RumViewScope(
) {
pendingActionCount++
val rumContext = getRumContext()

val globalAttributes = GlobalRumMonitor.get(sdkCore).getAttributes().toMutableMap()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we still keep a local copy? like val globalAttributes = globalAttributes.toMap().

Event will be created on another thread, and by the time it will be created globalAttributes may have some other value already, so it is better to have a local copy.


val localCopyOfGlobalAttributes = globalAttributes.toMutableMap()
sdkCore.newRumEventWriteOperation(writer) { datadogContext ->
val user = datadogContext.userInfo
val syntheticsAttribute = if (
Expand Down Expand Up @@ -967,7 +974,7 @@ internal open class RumViewScope(
architecture = datadogContext.deviceInfo.architecture
),
context = ActionEvent.Context(
additionalProperties = globalAttributes
additionalProperties = localCopyOfGlobalAttributes
),
dd = ActionEvent.Dd(
session = ActionEvent.DdSession(
Expand Down Expand Up @@ -1109,17 +1116,17 @@ internal open class RumViewScope(
viewChangedListener?.onViewChanged(
RumViewInfo(
key = key,
attributes = attributes,
attributes = eventAttributes,
isActive = isActive()
)
)
}

private fun isViewComplete(): Boolean {
val pending = pendingActionCount +
pendingResourceCount +
pendingErrorCount +
pendingLongTaskCount
pendingResourceCount +
pendingErrorCount +
pendingLongTaskCount
// we use <= 0 for pending counter as a safety measure to make sure this ViewScope will
// be closed.
return stopped && activeResourceScopes.isEmpty() && (pending <= 0L)
Expand All @@ -1144,19 +1151,19 @@ internal open class RumViewScope(
internal val ONE_SECOND_NS = TimeUnit.SECONDS.toNanos(1)

internal const val ACTION_DROPPED_WARNING = "RUM Action (%s on %s) was dropped, because" +
" another action is still active for the same view"
" another action is still active for the same view"

internal const val RUM_CONTEXT_UPDATE_IGNORED_AT_STOP_VIEW_MESSAGE =
"Trying to update global RUM context when StopView event arrived, but the context" +
" doesn't reference this view."
" doesn't reference this view."
internal const val RUM_CONTEXT_UPDATE_IGNORED_AT_ACTION_UPDATE_MESSAGE =
"Trying to update active action in the global RUM context, but the context" +
" doesn't reference this view."
" doesn't reference this view."

internal val FROZEN_FRAME_THRESHOLD_NS = TimeUnit.MILLISECONDS.toNanos(700)
internal const val SLOW_RENDERED_THRESHOLD_FPS = 55
internal const val NEGATIVE_DURATION_WARNING_MESSAGE = "The computed duration for the " +
"view: %s was 0 or negative. In order to keep the view we forced it to 1ns."
"view: %s was 0 or negative. In order to keep the view we forced it to 1ns."

internal fun fromEvent(
parentScope: RumScope,
Expand Down
2 changes: 1 addition & 1 deletion .../src/test/kotlin/com/datadog/android/rum/internal/domain/scope/RumApplicationScopeTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ internal class RumApplicationScopeTest {
val viewScope = viewManager.childrenScopes.first()
check(viewScope is RumViewScope)
assertThat(viewScope.key).isEqualTo(fakeKey)
assertThat(viewScope.attributes).isEqualTo(mockAttributes)
assertThat(viewScope.eventAttributes).isEqualTo(mockAttributes)
}

@Test
Expand Down
4 changes: 2 additions & 2 deletions .../src/test/kotlin/com/datadog/android/rum/internal/domain/scope/RumViewManagerScopeTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ internal class RumViewManagerScopeTest {
.isEqualTo(resolveExpectedTimestamp(fakeEvent.eventTime.timestamp))
assertThat(it.key).isEqualTo(fakeEvent.key)
assertThat(it.type).isEqualTo(RumViewScope.RumViewType.FOREGROUND)
assertThat(it.attributes).containsAllEntriesOf(fakeEvent.attributes)
assertThat(it.eventAttributes).containsAllEntriesOf(fakeEvent.attributes)
assertThat(it.firstPartyHostHeaderTypeResolver).isSameAs(mockResolver)
assertThat(it.version).isEqualTo(2)
}
Expand All @@ -241,7 +241,7 @@ internal class RumViewManagerScopeTest {
.isEqualTo(resolveExpectedTimestamp(fakeEvent.eventTime.timestamp))
assertThat(it.key).isEqualTo(fakeEvent.key)
assertThat(it.type).isEqualTo(RumViewScope.RumViewType.FOREGROUND)
assertThat(it.attributes).containsAllEntriesOf(fakeEvent.attributes)
assertThat(it.eventAttributes).containsAllEntriesOf(fakeEvent.attributes)
assertThat(it.firstPartyHostHeaderTypeResolver).isSameAs(mockResolver)
assertThat(it.version).isEqualTo(2)
}
Expand Down
Loading
Loading