RUM-3012 Do not update RUM View global properties after the view is stopped#1851
Conversation
c0736cb to
d322a79
Compare
| } | ||
|
|
||
| private fun updateGlobalAttributes(sdkCore: InternalSdkCore, event: RumRawEvent): Map<String, Any?> { | ||
| if (!stopped && event !is RumRawEvent.StopView) { |
There was a problem hiding this comment.
Why we don't want to update global attributes when we receive StopView event?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?).
There was a problem hiding this comment.
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.
| pendingActionCount++ | ||
| val rumContext = getRumContext() | ||
|
|
||
| val globalAttributes = GlobalRumMonitor.get(sdkCore).getAttributes().toMutableMap() |
There was a problem hiding this comment.
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 Report
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
|
092c8fc to
2db9a34
Compare
0xnm
left a comment
0xnm
left a comment
There was a problem hiding this comment.
lgtm, I've added some comments, but they are not blocking
| trackFrustrations = fakeTrackFrustrations, | ||
| sampleRate = fakeSampleRate | ||
| ) | ||
| mockSessionReplayContext(testedScope) |
There was a problem hiding this comment.
why session replay comes into play here?
There was a problem hiding this comment.
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.
| trackFrustrations = fakeTrackFrustrations, | ||
| sampleRate = fakeSampleRate | ||
| ) | ||
| mockSessionReplayContext(testedScope) |
There was a problem hiding this comment.
same, not clear why it is needed here (same for the tests below)
|
|
||
| // When | ||
| val result = testedScope.handleEvent( | ||
| RumRawEvent.StopView(fakeKey, emptyMap()), |
There was a problem hiding this comment.
should we add attributes here to make sure that they are taken into account as well at the assertion stage?
There was a problem hiding this comment.
maybe let me think about it 🤔
|
|
||
| // When | ||
| val result = testedScope.handleEvent( | ||
| RumRawEvent.StartView(fakeKey, emptyMap()), |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
39c7938 to
2cbac29
Compare
2cbac29 to
8050229
Compare
5 participants
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)