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.

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-1765 Do not start "ApplicationLaunch" view when app launches in background - part 2 #685

Conversation

ncreated
Copy link
Member

@ncreated ncreated commented Dec 10, 2021

What and why?

📦 This PR limits application launch events tracking feature to only consider events received while app is in foreground. It adds necessary implementation to not start "ApplicationLaunch" view if app is not in active state.

When app launches in background, its events can still be tracked if Background Events Tracking feature is enabled. This makes a clear separation of "ApplicationLaunch" and "Background" activities.

How?

Determining app state is done using existing AppStateListener - it is now injected to RUM feature as well. It is used to check the app state and consider starting either:

  • "ApplicationLaunch" view - if app receives off-view events in active state;
  • "Background" view - if app receives off-view events in not active state and BET is enabled.

Below, list of cases covered with new and existing (#677) tests.


✅ (1) Receive off-view events when app launches in foreground and BET is enabled.

The "ApplicationLaunch" view spans from SDK init to starting another view.

SC1

✅ (2) Receive off-view events when app launches in background and BET is enabled.

There is no "ApplicationLaunch" view. Instead, "Background" view is created and it starts at the time of first event.

SC2

✅ (3) Receive events in background when BET is enabled.

No change - receiving off-view event after app goes to background starts the "Background" view at the time of the event.

SC3

✅ (4) Receive off-view events when app launches in foreground and BET is disabled.

Same as scenario 1, the "ApplicationLaunch" view tracks off-view event and the view starts on SDK init.

SC4

✅ (5) Receive events in background when BET is disabled.

Nothing is tracked in this case. If BET is disabled, we do not create any view for tracking off-view background events.

✅ (6) Receive events in background when BET is disabled.

Only foreground events are tracked in user-defined view. "Background" view is not started as BET is disabled.

SC6

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)
  • Make sure each commit and the PR mention the Issue number or JIRA reference

with ensuring that only one listener is created for the entire SDK.
by ensuring that app starts "ApplicationLaunch" view only if launched in foreground and it starts
 "Background" view when launched in background with "Background Events Tracking" option enabled.
@ncreated ncreated self-assigned this Dec 10, 2021
@ncreated ncreated changed the title RUMM-1765 Do not start "ApplicationLaunch" view when app launches in background RUMM-1765 Do not start "ApplicationLaunch" view when app launches in background - part 2 Dec 10, 2021
Comment on lines 173 to 182
if userActionScope == nil {
addDiscreteUserAction(on: command)
if command.actionType == .custom {
// send it instantly without waiting for child events (e.g. resource associated to this action)
sendDiscreteCustomUserAction(on: command)
} else {
addDiscreteUserAction(on: command)
}
} else if command.actionType == .custom {
// still let it go, just instantly without any dependencies
// still let it go, just instantly without waiting for child events (e.g. resource associated to this action)
sendDiscreteCustomUserAction(on: command)
Copy link
Member Author

Choose a reason for hiding this comment

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

While testing ALT feature, I found this bug in .custom actions tracking. Here, I fix it - now we treat .custom actions the same, no matter if added while any other action scope exists or not. I also added test for this case.

@@ -548,6 +548,74 @@ class RUMViewScopeTests: XCTestCase {
XCTAssertEqual(event.model.view.action.count, 2, "View should record 2 actions: non-custom + instant custom")
}

func testGivenViewWithPendingAction_whenCustomActionIsAdded_itSendsItInstantly() throws {
Copy link
Member Author

Choose a reason for hiding this comment

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

These two tests ensure we track .custom actions properly, no matter if added while any action scope exists or not. In both cases, .custom action is sent instantly w/o waiting for their side effects.

@ncreated ncreated marked this pull request as ready for review December 13, 2021 15:30
@ncreated ncreated requested a review from a team as a code owner December 13, 2021 15:30
Copy link
Member

@maxep maxep left a comment

Choose a reason for hiding this comment

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

LGTM 👍 I've a couple of non-blocking suggestions

Comment on lines 173 to +181
if userActionScope == nil {
addDiscreteUserAction(on: command)
if command.actionType == .custom {
// send it instantly without waiting for child events (e.g. resource associated to this action)
sendDiscreteCustomUserAction(on: command)
} else {
addDiscreteUserAction(on: command)
}
} else if command.actionType == .custom {
// still let it go, just instantly without any dependencies
// still let it go, just instantly without waiting for child events (e.g. resource associated to this action)
Copy link
Member

Choose a reason for hiding this comment

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

/nit this can be:

if command.actionType == .custom {
    // send it instantly without waiting for child events (e.g. resource associated to this action)
    sendDiscreteCustomUserAction(on: command)
} else if userActionScope == nil {
    addDiscreteUserAction(on: command)
} else {
    reportActionDropped(type: command.actionType, name: command.name)
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, it's simpler 👍. Applied in 99ffde2 to avoid rebasing.

@@ -36,6 +37,9 @@ internal class RUMApplicationScope: RUMScope, RUMContextProvider {
/// RUM Sessions sampling rate.
internal let samplingRate: Float

/// The start time of the application, measured in device date. It equals the time of SDK init.
private let applicationStartTime: Date
Copy link
Member

Choose a reason for hiding this comment

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

I think the name can be confused with application launch time. Can we rename it to something like scopeStartTime or sdkInitTime?

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, makes sense - for that reason I called it sdkInitTime in upstream, but in RUMMonitor this information was translated to applicationStartTime. What proposed could be simpler 👍 - applied in 99ffde2 to avoid rebasing.

ncreated added a commit that referenced this pull request Dec 20, 2021
@ncreated ncreated merged commit 83f75a9 into ncreated/RUMM-1765-app-launch-events-tracking Dec 20, 2021
@ncreated ncreated deleted the ncreated/RUMM-1765-do-not-start-application-launch-view-when-app-in-background branch December 20, 2021 17:46
ncreated added a commit that referenced this pull request Dec 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants