RUMM-3133 Don't override ViewScope attributes with UserActionScope #1250
Conversation
…tributes ### What and why? Today, when a `UserAction`, `Resource` or `LongTask` event is sent with attributes attached, if the attributes have conflicting keys with View event, the View attributes are overwritten by the `UserAction`/`Resource`/`LongTask` attributes. `UserActionScope` events must not update the `ViewScope` attributes, which has been the case for Android SDK. ### How? Updated logic that overwrites the `ViewScope` attributes with `UserActionScope` attributes with unit tests to ensure that the `ViewScope` attributes are not overwritten. Fixed `Resource` events where parent attributes were reported when there is a conflict.
Datadog ReportBranch report: ❌ ❌ Failed Tests (1)
|
There was a problem hiding this comment.
Awesome @ganeshnj !
This contribution is on point and the tests properly follow our pattern 👏
I've left a couple of comments and I would be interested to know your opinion on how we select the command types that can propagate attributes.
| attributes.merge(rumCommandAttributes: command.attributes) | ||
| attributes.mergeWithoutOverwriting(rumCommandAttributes: command.attributes) |
There was a problem hiding this comment.
I think we should keep overwriting the attributes here: it is consuming a RUMStopResourceCommand in a RUMResourceScope.
There was a problem hiding this comment.
The issue I found here was as below (from test case)
monitor.addAttribute(forKey: "abc", value: "123")
// when
monitor.startView(key: "View", name: nil, attributes: [:])
monitor.addUserAction(type: .custom, name: "action1", attributes: ["abc": "456"])
monitor.startResourceLoading(resourceKey: "/resource1", url: URL(string: "https://foo.com/1")!, attributes: ["abc": "456"])
monitor.stopResourceLoading(resourceKey: "/resource1", response: .mockAny(), size: nil, attributes: ["def": "789"])here want to keep the abc attribute value reported in the resource event as 456 (along with def: 789). Currently, we overwrite it to 123 which is incorrect from my understanding. Anything that is child scope must take a precedence over the parent scope.
Do you have an example where this logic will go wrong? I can investigate further.
| // RUMM-3133 Don't override View attributes with UserAction, Resource or LongTask attributes | ||
| switch command { | ||
| case is RUMUserActionCommand, is RUMResourceCommand, is RUMAddLongTaskCommand: | ||
| break | ||
| default: | ||
| attributes.merge(rumCommandAttributes: command.attributes) | ||
| } |
There was a problem hiding this comment.
I think we should do the opposite and check for view-related command types instead.
Maybe it could use a
internal protocol RUMViewCommand: RUMCommand { }for selecting which command types can propagate attributes to the view-scope. It would be less error prone when we add command types. WDYT?
There was a problem hiding this comment.
For sure having one base protocol for all the commands that we don't want to propagate would be ideal. I like the idea but I'm not sure what other commands we can expect here. This is the same rational behind using break with current default behavior.
Any hints on commands that we don't want propagation for?
ganeshnj
changed the title
| let rumEventMatchers = try core.waitAndReturnRUMEventMatchers() | ||
|
|
||
| let viewEvents = rumEventMatchers.filterRUMEvents(ofType: RUMViewEvent.self) { event in | ||
| return event.view.name != RUMOffViewEventsHandlingRule.Constants.applicationLaunchViewName | ||
| } | ||
| XCTAssertEqual(viewEvents.count, 4) | ||
| XCTAssertEqual(try viewEvents[0].attribute(forKeyPath: "context.abc"), "123") | ||
| XCTAssertEqual(try viewEvents[1].attribute(forKeyPath: "context.abc"), "123") | ||
| XCTAssertEqual(try viewEvents[2].attribute(forKeyPath: "context.abc"), "123") | ||
| XCTAssertEqual(try viewEvents[3].attribute(forKeyPath: "context.abc"), "123") | ||
|
|
||
| let actionEvents = rumEventMatchers.filterRUMEvents(ofType: RUMActionEvent.self) { event in | ||
| return event.view.name != RUMOffViewEventsHandlingRule.Constants.applicationLaunchViewName | ||
| } | ||
| XCTAssertEqual(actionEvents.count, 1) | ||
| XCTAssertEqual(try actionEvents[0].attribute(forKeyPath: "context.abc"), "456") | ||
|
|
||
| let resourceEvents = rumEventMatchers.filterRUMEvents(ofType: RUMResourceEvent.self) { event in | ||
| return event.view.name != RUMOffViewEventsHandlingRule.Constants.applicationLaunchViewName | ||
| } | ||
| XCTAssertEqual(resourceEvents.count, 1) | ||
| XCTAssertEqual(try resourceEvents[0].attribute(forKeyPath: "context.abc"), "789") | ||
| XCTAssertEqual(try resourceEvents[0].attribute(forKeyPath: "context.def"), "789") |
There was a problem hiding this comment.
non-blocking/ This is really good 👌, but we use legacy RUMEventMatchers concept here - perhaps it was inspired by surrounding tests 🙂 that weren't yet migrated. Not a change request, but I wanted to share a bit on how we could facilitate such tests today.
Recently we introduced two concepts that could make this code simpler and make assertions better: the DatadogCoreProxy and handy DDAssert* variants. If leveraging both, we could get rid of matchers and assert on models directly (including full assertion on dictionaries vs only partial ones with matchers). There would be no throwing and the entire test will run slightly faster.
Here's how it could look in the our new model. I'd love to see it even simplified with some contributions to DatadogCoreProxy and DDAssert* (e.g. type declarations seem redundant and force unwrapping isn't nice):
let viewEvents: [RUMViewEvent] = core.waitAndReturnEvents(of: RUMFeature.self, ofType: RUMViewEvent.self)
DDAssertDictionariesEqual(viewEvents[0].context!.contextInfo, ["abc": "123"])
DDAssertDictionariesEqual(viewEvents[1].context!.contextInfo, ["abc": "123"])
DDAssertDictionariesEqual(viewEvents[2].context!.contextInfo, ["abc": "123"])
DDAssertDictionariesEqual(viewEvents[3].context!.contextInfo, ["abc": "123"])
let actionEvents: [RUMActionEvent] = core.waitAndReturnEvents(of: RUMFeature.self, ofType: RUMActionEvent.self)
.filter { $0.view.name != RUMOffViewEventsHandlingRule.Constants.applicationLaunchViewName }
DDAssertDictionariesEqual(actionEvents[0].context!.contextInfo, ["abc": "456"])
let resourceEvents: [RUMResourceEvent] = core.waitAndReturnEvents(of: RUMFeature.self, ofType: RUMResourceEvent.self)
.filter { $0.view.name != RUMOffViewEventsHandlingRule.Constants.applicationLaunchViewName }
DDAssertDictionariesEqual(resourceEvents[0].context!.contextInfo, ["abc": "789", "def": "789"])
What and why?
Today, when a
UserAction,ResourceorLongTaskevent is sent with attributes attached, if the attributes have conflicting keys with View event, the View attributes are overwritten by theUserAction/Resource/LongTaskattributes.UserActionScopeevents must not update theViewScopeattributes, which has been the case for Android SDK.How?
Updated logic that overwrites the
ViewScopeattributes withUserActionScopeattributes with unit tests to ensure that theViewScopeattributes are not overwritten.Fixed
Resourceevents where parent attributes were reported when there is a conflict.Review checklist
Custom CI job configuration (optional)