RUMM-2625 Update default values for RUM events#1139
Conversation
f14b51a to
ed65701
Compare
Codecov Report
@@ Coverage Diff @@
## develop #1139 +/- ##
===========================================
- Coverage 83.17% 83.02% -0.16%
===========================================
Files 274 275 +1
Lines 9462 9528 +66
Branches 1540 1556 +16
===========================================
+ Hits 7870 7910 +40
- Misses 1152 1162 +10
- Partials 440 456 +16
|
| session = ViewEvent.DdSession(plan = ViewEvent.Plan.PLAN_1) | ||
| ) | ||
| ), | ||
| connectivity = networkInfo.toViewConnectivity(), |
There was a problem hiding this comment.
Why are we adding the network info on the View? It was not added on purpose, since the network info can change many times during a View's lifetime. AFAIK the last value will override previous ones, meaning that having this information will be erroneous and unusable.
There was a problem hiding this comment.
I had considered it would be useful to get the last connectivity value the view had instead of no information, but like you said it's probably not all that actionable and most insight can already be taken from resource connectivity values. I'll remove it.
| if (trackFrustrations && errorCount > 0 && actualType == RumActionType.TAP) { | ||
| frustrations.add(ActionEvent.Type.ERROR_TAP) | ||
| } | ||
| val networkInfo = CoreFeature.networkInfoProvider.getLatestNetworkInfo() |
There was a problem hiding this comment.
for continuous action network info at the time of sending the action may be not relevant or not the same as it was at the beginning of the action. Like for the RumResourceScope we fix the network info when we create resource scope, because this is the state when that was when resource call started.
IMO we should either do the same here, or not add it.
There was a problem hiding this comment.
I'll follow the same pattern as in RumResourceScope, it makes more sense.
| ) | ||
| } | ||
|
|
||
| internal fun NetworkInfo.toActionConnectivity(): ActionEvent.Connectivity { |
There was a problem hiding this comment.
If we are going to add network info for actions, it would be nice to add tests.
|
|
||
| val context = getRumContext() | ||
| val user = CoreFeature.userInfoProvider.getUserInfo() | ||
| val hasUserInfo = user.id != null || user.name != null || |
There was a problem hiding this comment.
should we maybe extract it into extension/util method? the same check occurs multiple times in this PR
| } | ||
|
|
||
| fun hasNoUserInfo(): ErrorEventAssert { | ||
| assertThat(actual.usr).isNull() |
There was a problem hiding this comment.
minor: we can also add error message
|
|
||
| whenever(coreFeature.mockUserInfoProvider.getUserInfo()) doReturn fakeUserInfo | ||
| whenever(mockParentScope.getRumContext()) doReturn fakeParentContext | ||
| whenever(coreFeature.mockNetworkInfoProvider.getLatestNetworkInfo()) doReturn fakeAndroidNetworkInfo |
There was a problem hiding this comment.
We need to add the assertions for the changed/added fields. Same for other tests.
Updates some default attributes of RUM events in order to standardize and unify behaviour across all sdk platforms.
ed65701 to
240b52e
Compare
0xnm
left a comment
0xnm
left a comment
There was a problem hiding this comment.
lgtm! I've added one suggestion (the same assertion is repeated twice).
| hasServiceName(coreFeature.fakeServiceName) | ||
| hasVersion(coreFeature.mockAppVersionProvider.version) |
There was a problem hiding this comment.
Same assertion twice
| hasServiceName(coreFeature.fakeServiceName) | |
| hasVersion(coreFeature.mockAppVersionProvider.version) |
9b9cc1e to
3fb947c
Compare
5 participants
What does this PR do?
Updates some default attributes of RUM events in order to standardize and unify behaviour across all sdk platforms.
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)