-
Notifications
You must be signed in to change notification settings - Fork 133
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RUM-3029: Stopped view shouldn't send values added to the global attributes after it was stopped #2143
base: develop
Are you sure you want to change the base?
Conversation
a1c9e96
to
b2c3dd8
Compare
Datadog ReportBranch report: ✅ 0 Failed, 968 Passed, 2664 Skipped, 43.36s Total duration (1m 46.24s time saved) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great effort in working on the integration testing 👍👌. It's clear you've put thought into it 🏅.
However, this PR doesn’t address the underlying issue outlined in RUM-3029
, which still exists. I’ve highlighted the details of the problem in a few comments throughout the PR.
Datadog/IntegrationUnitTests/RUM/RUMViewScopeIntegrationTests.swift
Outdated
Show resolved
Hide resolved
monitor.startView(key: "key", name: viewName, attributes: initialAttributes) | ||
monitor.stopView(key: "key") | ||
// Flushing the commands in the current thread to prevent data races accessing the attributes | ||
core.flush() | ||
monitor.addAttribute(forKey: "additionalKey", value: "additionalValue") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change-request/ By calling core.flush()
, this scenario masks the actual issue we need to address.
Removing it:
monitor.startView(key: "key", name: viewName, attributes: initialAttributes)
monitor.stopView(key: "key")
monitor.addAttribute(forKey: "additionalKey", value: "additionalValue")
exposes two problems:
"additionalKey": "additionalValue"
is incorrectly included in the"MyView"
view, even though the view was stopped.- Both
initialAttributes
and"additionalKey": "additionalValue"
are mistakenly included in the"ApplicationLaunch"
view (!), even though they are only set for"MyView"
. This is curious and warrants further investigation to understand how they end up in"ApplicationLaunch"
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. The flush
should be avoided and indeed we have a data race accessing the attributes.
To solve the issue, the attributes are shared only through the process(command: RUMCommand
function in the RUMViewScope
. This way we limit the mutations to only some commands.
Also the second point, with local attributes leaked to previous views is fixed. The integration tests do validate it.
Datadog/IntegrationUnitTests/RUM/RUMViewScopeIntegrationTests.swift
Outdated
Show resolved
Hide resolved
} | ||
} | ||
|
||
func testWhenViewReceivesActions_theViewAttributesAreNotUpdated_withActionAttributes() throws { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change-request/ This test seems unusual as it doesn't validate existing behavior but instead asserts that a non-existent behavior doesn't occur. Event attributes are not meant to be inherited by other events, which is intentional. This test essentially verifies that this behavior doesn't happen.
A more valuable approach would be to add coverage for global attributes, which should be included in all events. Specifically, it would be useful to test that:
- Global attributes are added to action events.
- Global attributes are added to resource events.
- Global attributes are added to error events.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is not a behaviour in the product but the idea of the test is to give us confidence we don't have regressions on this regard.
The attributes are shared through commands for all types (actions, resources, views,..) and so it is very easy to have wrong assumptions by not knowing the playbook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As suggested I added tests to validate the addition of global attributes to actions, resources and errors
Datadog/IntegrationUnitTests/RUM/RUMViewScopeIntegrationTests.swift
Outdated
Show resolved
Hide resolved
let viewControllerClassName: String = .mockRandom() | ||
let view = createMockView(viewControllerClassName: viewControllerClassName) | ||
let view = ViewControllerMock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question/ I’m not entirely clear on this change or if it’s missing the removal of the createMockView()
implementation. From the PR description, I understand that createMockView()
is causing issues in Xcode 16. Ideally, I would expect to see that method either fixed or completely replaced with an alternative approach. While we’ve replaced it in a few places, the original problematic method still appears to be present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a good point.
I'm going to address it in a contextualized PR, just to remove this noise from the main focus of the PR.
b2c3dd8
to
9e1ff61
Compare
// When | ||
monitor.addAttribute(forKey: "sameKey", value: "globalValue") | ||
monitor.startView(key: "key", name: viewName, attributes: initialAttributes) | ||
//monitor.stopView(key: "key") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic behind this test is being discussed
…ibutes after it was stopped
9e1ff61
to
214827c
Compare
What and why?
This PR ensures that a
View
collects attributes only during its active state.Once the View is stopped, resources, actions, and global attributes are no longer associated with it, preventing potential leaks.
How?
The
View
attributes' updates are validated through:RUMViewScopeTests
RUMViewScopeIntegrationTests
Additionally, it fixes tests that are failing on Xcode 16 due to the use of not declared
ViewControllers
that didn't have an explicit initializer.The error that was being thrown is:
Review checklist
make api-surface
)