RUM-9977: Update RUM feature context only after event processing completion#2650
Conversation
99091ac to
e27d65f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## nogorodnikov/rum-8170/feature-context-sync #2650 +/- ##
==============================================================================
- Coverage 70.11% 70.10% -0.01%
==============================================================================
Files 819 819
Lines 30611 30594 -17
Branches 5111 5107 -4
==============================================================================
- Hits 21460 21446 -14
+ Misses 7734 7726 -8
- Partials 1417 1422 +5
🚀 New features to boost your workflow:
|
| executorService.executeSafe("Rum event handling", sdkCore.internalLogger) { | ||
| synchronized(rootScope) { | ||
| rootScope.handleEvent(event, datadogContext, writeScope, writer) | ||
| updateFeatureContext() |
There was a problem hiding this comment.
Question: If I understand correctly, prior to this PR, RumViewSceope only update RUM feature context for onStartAction, with this change, we try to update the context for every event, is that correct?
There was a problem hiding this comment.
No, RUM feature context was updated from multiple places, not only from RumViewScope. Even RumViewScope was doing it from multiple places.
There was a problem hiding this comment.
If we take RumRawEvent.ActionSent as an example, this event will be delegated to RumViewScope.onActionSent, when handling this event, I see it will only update the context for has_replay attribute.
However with this PR, it will have an additional updateFeatureContext here when it is completely handled, if it is true, we are updating feature context more frequently than before?
There was a problem hiding this comment.
@ambushwork I understand what you are saying but I don't think it is too much harm in that, I guess removing this complexity is worthy, what do you think ?
There was a problem hiding this comment.
We will update context more often, this is true, but in fact any event can change the RUM context. Imagine some event triggering session validity check and invalidating it, for example.
So we bring more predictability and stability here.
Also, if taking RumViewScope, before this change it was updating RUM view context from different places: when RUM view scope is created, when stopped, when action is started and stopped, and, most importantly, at the sendViewUpdate, which is called for many events.
The last call site is still there, but it is just because there is dependency with SR. I will have to handle SR context handling as well (in another PR) before removing this call site (ideally).
There was a problem hiding this comment.
OK, make sense, I am not against simplify the call site of updating feature context, my questions is just to make sure that you are aware of the additional call of update feature context (especially because updateFeatureContext has synchronized block inside, we need to pay attention). if it's an expected behavior, then LGTM.
There was a problem hiding this comment.
yes, it is an expected behavior. Later on we can even do an optimization - since RumContext is a data class, we can store the previous snapshot and call updateFeatureContext only if something changed in the class.
| return childScopes.find { it.isActive() } | ||
| val activeSessions = childScopes.filter { it.isActive() } | ||
| if (activeSessions.size > 1) { | ||
| sdkCore.internalLogger.log( |
There was a problem hiding this comment.
is this related with this PR ?
There was a problem hiding this comment.
yes, it is. It is just a safeguard to inform that we have to make a selection from the multiple active sessions.
| executorService.executeSafe("Rum event handling", sdkCore.internalLogger) { | ||
| synchronized(rootScope) { | ||
| rootScope.handleEvent(event, datadogContext, writeScope, writer) | ||
| updateFeatureContext() |
There was a problem hiding this comment.
@ambushwork I understand what you are saying but I don't think it is too much harm in that, I guess removing this complexity is worthy, what do you think ?
4 participants
What does this PR do?
This PR brings the following change: now we will update RUM feature context only once, once RUM events processing is complete.
This change requires breaking a bit encapsulation by exposing concrete sub-types of
RumScope, but advantage is that we remove multiple feature context update call sites and leave only one place where it is done. Anyway, the hierarchy of the scopes is always the same, we never changed it.This is similar to what iOS SDK is doing https://github.com/DataDog/dd-sdk-ios/blob/61010e401dfa42bd7a98495f6e2cd04547c2f75a/DatadogRUM/Sources/RUMMonitor/Monitor.swift#L141-L164.
Review checklist (to be filled by reviewers)