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

mariusc83 · 2024-02-09T16:03:45Z

What does this PR do?

Opened from User property from future set on View Loads · Issue #1836 · DataDog/dd-sdk-android

As of now view has 3 states: active, stopped and complete. Stopped - view was stopped, because another view started or it was stopped explicitly, but it still exists in the scope hierarchy and will send view updates until all the child scopes are complete.

Imagine the following flow:

View A |--- Active ---|--- Stopped---|Complete
View B |--- Active -------------....
Here we have View A which moves to the stopped state once View B starts.

Customer is using global RUM attributes, which are updated with the current screen name (or any attribute specific to the current active screen) on each screen transition.

Say on View B activation, global attributes receive foo:bar. The expectation of the customer is not to see foo:bar in View A attributes (this screen is not active anymore), but it will be there, because View A updates will be sent even when View B is already active with the current state of the global attributes.

View updates for View A should stop reading global attributes if it is stopped, but view reduction process on the backend should keep attributes which were received for the view updates during the active state.

Motivation

What inspired you to submit this pull request?

Additional Notes

Anything else we should know when reviewing?

Review checklist (to be filled by reviewers)

Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
Make sure you discussed the feature or bugfix with the maintaining team in an Issue
Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

...dk-android-rum/src/main/kotlin/com/datadog/android/rum/internal/domain/scope/RumViewScope.kt

0xnm · 2024-02-12T08:58:07Z

...dk-android-rum/src/main/kotlin/com/datadog/android/rum/internal/domain/scope/RumViewScope.kt

@@ -850,6 +850,17 @@ internal open class RumViewScope(
            .submit()
    }

+    private fun updateGlobalAttributes(sdkCore: InternalSdkCore, event: RumRawEvent): Map<String, Any?> {
+        if (!stopped && event !is RumRawEvent.StopView) {


Why we don't want to update global attributes when we receive StopView event?

because as you explained in the description of the ticket this will propagate the global attribute in case the user started a new view. A new view start will stopped the previous one automatically sending a stopView if I remember correctly. If the user added a global attribute to be propagated with the new view at start the old view will get this new attribute also if the stop was not called before and it is triggered by the start of the new view.

No, StopView event is created only from RumMonitor#stopView. If new view starts, it won't create StopView event under the hood of RumMonitor#startView (instead, view will be stopped if it receives StartView, so maybe you want to address this case?).

oh...damn...this is so complicated, yup in that case I will update the ticket, keep the stopView update and do not update it in case of StartView instead.

0xnm · 2024-02-12T09:01:58Z

...dk-android-rum/src/main/kotlin/com/datadog/android/rum/internal/domain/scope/RumViewScope.kt

@@ -896,8 +906,6 @@ internal open class RumViewScope(
        pendingActionCount++
        val rumContext = getRumContext()

-        val globalAttributes = GlobalRumMonitor.get(sdkCore).getAttributes().toMutableMap()


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.

codecov-commenter · 2024-02-12T09:03:46Z

Codecov Report

Merging #1851 (39c7938) into develop (35dc427) will decrease coverage by 0.18%.
The diff coverage is 100.00%.

❗ Current head 39c7938 differs from pull request most recent head 8050229. Consider uploading reports for the commit 8050229 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1851      +/-   ##
===========================================
- Coverage    83.54%   83.35%   -0.18%     
===========================================
  Files          470      470              
  Lines        16594    16579      -15     
  Branches      2494     2494              
===========================================
- Hits         13862    13819      -43     
- Misses        2049     2077      +28     
  Partials       683      683

Files	Coverage Δ
.../android/rum/internal/domain/scope/RumViewScope.kt	`93.27% <100.00%> (-0.58%)`	⬇️

... and 26 files with indirect coverage changes

0xnm

lgtm, I've added some comments, but they are not blocking

0xnm · 2024-02-13T08:42:32Z

...ndroid-rum/src/test/kotlin/com/datadog/android/rum/internal/domain/scope/RumViewScopeTest.kt

+            trackFrustrations = fakeTrackFrustrations,
+            sampleRate = fakeSampleRate
+        )
+        mockSessionReplayContext(testedScope)


why session replay comes into play here?

good point, I think it was a request left over from other tests but as I do not assess the session replay status here I think I can remove it.

0xnm · 2024-02-13T08:43:18Z

...ndroid-rum/src/test/kotlin/com/datadog/android/rum/internal/domain/scope/RumViewScopeTest.kt

+            trackFrustrations = fakeTrackFrustrations,
+            sampleRate = fakeSampleRate
+        )
+        mockSessionReplayContext(testedScope)


same, not clear why it is needed here (same for the tests below)

0xnm · 2024-02-13T08:44:25Z

...ndroid-rum/src/test/kotlin/com/datadog/android/rum/internal/domain/scope/RumViewScopeTest.kt

+
+        // When
+        val result = testedScope.handleEvent(
+            RumRawEvent.StopView(fakeKey, emptyMap()),


should we add attributes here to make sure that they are taken into account as well at the assertion stage?

maybe let me think about it 🤔

0xnm · 2024-02-13T08:45:55Z

...ndroid-rum/src/test/kotlin/com/datadog/android/rum/internal/domain/scope/RumViewScopeTest.kt

+
+        // When
+        val result = testedScope.handleEvent(
+            RumRawEvent.StartView(fakeKey, emptyMap()),


in theory we cannot receive StartView event with the same key as the active scope already has (it should be the key inherited from another view)

yes that's correct, let me change that. For me was more important the attributes update trigger but yes to be 100% correct a different key here would be needed.

…topped

mariusc83 commented Feb 9, 2024

View reviewed changes

...dk-android-rum/src/main/kotlin/com/datadog/android/rum/internal/domain/scope/RumViewScope.kt Show resolved Hide resolved

mariusc83 marked this pull request as ready for review February 9, 2024 16:08

mariusc83 requested review from a team as code owners February 9, 2024 16:08

mariusc83 force-pushed the mconstantin/rum-3012/freeze-global-attributes-for-stopped-views branch 2 times, most recently from c0736cb to d322a79 Compare February 9, 2024 16:33

mariusc83 self-assigned this Feb 9, 2024

0xnm reviewed Feb 12, 2024

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-3012/freeze-global-attributes-for-stopped-views branch 2 times, most recently from 092c8fc to 2db9a34 Compare February 12, 2024 12:48

xgouchet approved these changes Feb 13, 2024

View reviewed changes

mariusc83 requested a review from 0xnm February 13, 2024 08:39

0xnm approved these changes Feb 13, 2024

View reviewed changes

jonathanmos approved these changes Feb 13, 2024

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-3012/freeze-global-attributes-for-stopped-views branch 3 times, most recently from 39c7938 to 2cbac29 Compare February 13, 2024 09:58

RUM-3012 Do not update RUM View global properties after the view is s…

8050229

…topped

mariusc83 force-pushed the mconstantin/rum-3012/freeze-global-attributes-for-stopped-views branch from 2cbac29 to 8050229 Compare February 13, 2024 09:59

mariusc83 merged commit 86983e8 into develop Feb 13, 2024
23 checks passed

mariusc83 deleted the mconstantin/rum-3012/freeze-global-attributes-for-stopped-views branch February 13, 2024 10:46

xgouchet added this to the 2.6.0 milestone Feb 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mariusc83 commented Feb 9, 2024

0xnm Feb 12, 2024

mariusc83 Feb 12, 2024

0xnm Feb 12, 2024

mariusc83 Feb 12, 2024

0xnm Feb 12, 2024

codecov-commenter commented Feb 12, 2024 •

edited

Loading

0xnm left a comment

0xnm Feb 13, 2024

mariusc83 Feb 13, 2024

0xnm Feb 13, 2024

0xnm Feb 13, 2024

mariusc83 Feb 13, 2024

0xnm Feb 13, 2024

mariusc83 Feb 13, 2024

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

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

Conversation

mariusc83 commented Feb 9, 2024

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 12, 2024 • edited Loading

Codecov Report

0xnm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 12, 2024 •

edited

Loading