Skip to content
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.

Sign up for GitHub

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

RUMM-1779 Keep view active until all resources are consumed #702

Merged
merged 2 commits into from
Dec 31, 2021
Merged

RUMM-1779 Keep view active until all resources are consumed #702

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8a17f5c
RUMM-1779 Keep view active until all resources are consumed
maxep Dec 29, 2021
92a8105
RUMM-1779 Assert no event is sent after view is inactive
maxep Dec 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions Sources/Datadog/RUM/RUMMonitor/Scopes/RUMViewScope.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,8 +345,10 @@ internal class RUMViewScope: RUMScope, RUMContextProvider {
version += 1
attributes.merge(rumCommandAttributes: command.attributes)

let timeSpent = command.time.timeIntervalSince(viewStartTime)
// RUMM-1779 Keep view active as long as we have ongoing resources
let isActive = isActiveView || !resourceScopes.isEmpty
buranmert marked this conversation as resolved.
Show resolved Hide resolved

let timeSpent = command.time.timeIntervalSince(viewStartTime)
let cpuInfo = vitalInfoSampler.cpu
let memoryInfo = vitalInfoSampler.memory
let refreshRateInfo = vitalInfoSampler.refreshRate
Expand Down Expand Up @@ -384,7 +386,7 @@ internal class RUMViewScope: RUMScope, RUMContextProvider {
frozenFrame: .init(count: frozenFramesCount),
id: viewUUID.toRUMDataFormat,
inForegroundPeriods: nil,
isActive: isActiveView,
isActive: isActive,
isSlowRendered: isSlowRendered,
largestContentfulPaint: nil,
loadEvent: nil,
Expand Down
7 changes: 6 additions & 1 deletion Tests/DatadogTests/Datadog/RUM/RUMMonitor/Scopes/RUMViewScopeTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,14 +416,19 @@ class RUMViewScopeTests: XCTestCase {
scope.process(command: RUMStopResourceCommand.mockWith(resourceKey: "/resource/1")),
"The View should be kept alive as all its Resources haven't yet finished loading"
)

var event = try XCTUnwrap(output.recordedEvents(ofType: RUMEvent<RUMViewEvent>.self).last)
XCTAssertTrue(event.model.view.isActive ?? false, "View should stay active")

XCTAssertFalse(
scope.process(command: RUMStopResourceWithErrorCommand.mockWithErrorMessage(resourceKey: "/resource/2")),
"The View should stop as all its Resources finished loading"
)

let event = try XCTUnwrap(output.recordedEvents(ofType: RUMEvent<RUMViewEvent>.self).last)
event = try XCTUnwrap(output.recordedEvents(ofType: RUMEvent<RUMViewEvent>.self).last)
XCTAssertEqual(event.model.view.resource.count, 1, "View should record 1 successful Resource")
XCTAssertEqual(event.model.view.error.count, 1, "View should record 1 error due to second Resource failure")
XCTAssertFalse(event.model.view.isActive ?? true, "View should be inactive")
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test looks good 👍, although we could also test that "SDK doesn't send view updates to completed views (to ones which received their terminal, view.isActive == false update)". How about enhancing session-level validators in RUMSessionMatcher to also integrate this assertion? It already has the notion of ViewVisit aggregating all view updates in their time order and even applying some preliminary isActive checks.

We use RUMSessionMatcher in all integration and many unit tests, so this validation would be heavily applied.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I can have a look!
So we would basically stop sending view updates once the view is inactive?
What is the actual definition of an active view?
I worry if BE start processing inactive view events in the future, we will loose data.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we would basically stop sending view updates once the view is inactive?

This is exactly how I understand it 👍, but maybe @xgouchet can confirm this.

What is the actual definition of an active view?

I will share more context on Slack.

Copy link
Member Author

@maxep maxep Dec 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need further modification to actually prevent from sending update events after a view is inactive, here it just sets is_active: false
It's actually already done here, so it's just a matter of asserting this in test as you mentioned!


// MARK: - User Action Tracking
Expand Down
33 changes: 18 additions & 15 deletions Tests/DatadogTests/Matchers/RUMSessionMatcher.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,23 +213,26 @@ internal class RUMSessionMatcher {

// Validate ViewVisit's view.isActive for each events
try visits.forEach { visit in
var viewWasPreviouslyActive = false
var viewIsInactive = false
try visit.viewEvents.enumerated().forEach { index, viewEvent in
let viewIsActive = viewEvent.view.isActive!
if index == 0 {
if !viewIsActive {
throw RUMSessionConsistencyException(
description: "A `RUMSessionMatcher.ViewVisit` can't have a first event with an inactive `View`."
)
}
} else {
if !viewWasPreviouslyActive && viewIsActive {
throw RUMSessionConsistencyException(
description: "A `RUMSessionMatcher.ViewVisit` can't have an event where a `View` is active after the `View` was marked as inactive."
)
}
guard let viewIsActive = viewEvent.view.isActive else {
throw RUMSessionConsistencyException(
description: "A `RUMSessionMatcher.ViewVisit` can't have an event without the `isActive` parameter set."
)
}

if index == 0 && !viewIsActive {
throw RUMSessionConsistencyException(
description: "A `RUMSessionMatcher.ViewVisit` can't have a first event with an inactive `View`."
)
}

if viewIsInactive {
throw RUMSessionConsistencyException(
description: "A `RUMSessionMatcher.ViewVisit` can't have an event after the `View` was marked as inactive."
)
}
viewWasPreviouslyActive = viewIsActive
viewIsInactive = !viewIsActive
}
}

Expand Down