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-1265 Correctly handle RUM events when app in background #504

Merged

Conversation

mariusc83
Copy link
Member

@mariusc83 mariusc83 commented May 28, 2021

What and why?

Currently if the application is not in foreground (there is no active View scope), any resource or action event will be ignored as there is no active View on which to attach it.
In this PR we are adding a change in the way we handle the received commands in the RUMSessionScope in order to be able to record RUM events while the application is in background.

How?

We are introducing the concept of RUM background View scope which by definition is a normal View scope with predefined name and url. Once the RUMSessionScope needs to handle a RUMStartAction or RUMStartResource command and there is no active RUMViewScope, instead of ignoring the command as before it will automatically create a special RUMViewScope on which the received resource or action will be attached. For more information please check: RUM Background events RFC

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

@mariusc83 mariusc83 self-assigned this May 28, 2021
@mariusc83 mariusc83 requested a review from a team as a code owner May 28, 2021 11:36
@mariusc83 mariusc83 force-pushed the mconstantin/rumm-1265/support-rum-background-events branch 2 times, most recently from e1367e3 to d11bb58 Compare May 28, 2021 11:55
Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

Looks very good 💪! I left few convention feedbacks.

  • Also, please update the PR description 🙏, especially on "how" - as we implement a very custom way of determining if the app is in background (notably, without the app's life cycle events) I think it's worth explaining it for future references.

Comment on lines 11 to 12
static let RUM_BACKGROUND_VIEW_URL = "com/datadog/background/view"
static let RUM_BACKGROUND_VIEW_NAME = "Background"
Copy link
Member

Choose a reason for hiding this comment

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

✏️ We don't use capitals for defining constants in Swift. Also, in dd-sdk-ios we adopt the convention of grouping constants in Constants struct acting as a namespace:

internal class RUMViewScope: RUMScope, RUMContextProvider {
   struct Constants {
      static let backgroundViewURL = "com/datadog/background/view"
      static let backgroundViewName = "Background"
   }
   
   // ...
}

XCTAssertEqual(scope.viewScopes[0].viewPath, RUMViewScope.RUM_BACKGROUND_VIEW_URL)
}

func testWhenActiveViewScope_andReceivingStartCommand_doesNotCreateNewViewScope() {
Copy link
Member

Choose a reason for hiding this comment

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

✏️ missing "it":

Suggested change
func testWhenActiveViewScope_andReceivingStartCommand_doesNotCreateNewViewScope() {
func testWhenActiveViewScope_andReceivingStartCommand_itDoesNotCreateNewViewScope() {

XCTAssertEqual(scope.viewScopes.count, 1)
}

func testWhenNoActiveViewScope_andReceivingNotValidStartCommand_doesNotCreateNewViewScope() {
Copy link
Member

Choose a reason for hiding this comment

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

✏️ missing "it":

Suggested change
func testWhenNoActiveViewScope_andReceivingNotValidStartCommand_doesNotCreateNewViewScope() {
func testWhenNoActiveViewScope_andReceivingNotValidStartCommand_itDoesNotCreateNewViewScope() {


let scope = RUMSessionScope(parent: parent, dependencies: .mockAny(), samplingRate: 100, startTime: Date())
_ = scope.process(command: RUMStartViewCommand.mockAny())
_ = scope.process(command: RUMAddUserActionCommand.mockWith(time: currentTime))
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 we want to call generateRandomValidStartCommand() here, no?

@mariusc83 mariusc83 force-pushed the mconstantin/rumm-1265/support-rum-background-events branch from d11bb58 to 6459555 Compare May 31, 2021 11:38
@mariusc83 mariusc83 force-pushed the mconstantin/rumm-1265/support-rum-background-events branch from 6459555 to 6e9aaf8 Compare May 31, 2021 11:50
@ncreated
Copy link
Member

ncreated commented Jun 1, 2021

💯 👌

@mariusc83 mariusc83 merged commit 66c0dac into master Jun 2, 2021
@mariusc83 mariusc83 deleted the mconstantin/rumm-1265/support-rum-background-events branch June 2, 2021 07:48
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