-
Notifications
You must be signed in to change notification settings - Fork 124
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
Allow users to manually end a RUM session #1219
Conversation
This includes most of the work for the SessionScope for stopping a session, but does not include any ApplicationScope work for restarting sessions or ViewScope work for sending session status to intake.
Adds `is_active` to Session information. Default it to true for the time being.
This requires ApplicationScope keep multiple sessions, and discard them as they're completed.
b594e2d
to
93db921
Compare
|
||
if command is RUMStopSessionCommand { | ||
// Reach in and grab the last active view | ||
lastActiveView = activeSession?.viewScopes.first(where: { $0.isActiveView } ) |
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.
I'm interested in thoughts on this approach. This feels like it's pulling in some extra dependencies in the ViewScope, but I couldn't find a good way to make activating the previous SessionScope's view work, even with the existing refresh logic.
4a04e61
to
e64a3e8
Compare
e64a3e8
to
076b87a
Compare
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!
Left a couple of minor suggestions 🙌
rumMonitor.stopView(viewController: self) | ||
} | ||
|
||
@IBAction func didTapDownloadResourceButton(_ sender: Any) { |
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.
If I'm not mistaken you can change the sender type to UIButton
and avoid force cast
if scope.isActive { | ||
// False, but still active means we timed out or expired, refresh the session |
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.
From the comment it feels like isSessionActive
should be checked here as well?
I don't see this param used elsewhere.
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.
I'm confused by this? Do you mean applicationActive
?
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.
I don't get what False
is referring to here, since we are inside of isActive
, which is true
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.
Ah, It's the return value of scope.process
If it returned true
return the scope, False, we check isActive. I'll change the comment to make it clearer.
return nil | ||
}) | ||
|
||
// Sanity telemety, only end up with one active session |
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.
Nice! 💪
@@ -135,51 +158,63 @@ internal class RUMSessionScope: RUMScope, RUMContextProvider { | |||
} | |||
|
|||
if !isSampled { | |||
return true // discard all events in this session | |||
// Make sure sessions end even if they are sampled |
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.
nice catch!
Sources/Datadog/RUMMonitor.swift
Outdated
self.applicationScope.sessionScope?.context ?? | ||
self.applicationScope.context |
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.
I think this odd formatting is more common for ??
, but if linter is not complaining I think it's fine
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.
TBH I think the linter is what changed it.
Sources/Datadog/RUMMonitor.swift
Outdated
|
||
guard context.sessionID != .nullUUID else { | ||
// if Session was sampled or not yet started |
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.
Is that not true anymore?
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.
I had some issues with this function I may have removed the comment by accident. I'll put it back.
Tests/DatadogIntegrationTests/Scenarios/RUM/RUMCommonAsserts.swift
Outdated
Show resolved
Hide resolved
…wift Co-authored-by: Maciej Burda <maciej.burda@datadoghq.com>
var time: Date | ||
var attributes: [AttributeKey: AttributeValue] = [:] | ||
var canStartBackgroundView = false // no, stopping a session should not start a backgorund session | ||
var isUserInteraction = false |
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.
/nit if we are not mutating any of those, they can be let
👍
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.
They're var
getters in the RUMCommand
protocol. Is it possible to change them to let in the implementation?
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, if they are get
only, they can be `let 👍
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.
Fixed
var deactivating = false | ||
if isActive { | ||
if command is RUMStopSessionCommand { | ||
isActive = false | ||
deactivating = true | ||
} else if let startApplicationCommand = command as? RUMApplicationStartCommand { | ||
startApplicationLaunchView(on: startApplicationCommand, context: context, writer: writer) | ||
} else if let startViewCommand = command as? RUMStartViewCommand { | ||
// Start view scope explicitly on receiving "start view" command | ||
startView(on: startViewCommand, context: context) | ||
} else if !hasActiveView { | ||
// Otherwise, if there is no active view scope, consider starting artificial scope for handling this command | ||
let handlingRule = RUMOffViewEventsHandlingRule( | ||
sessionState: state, | ||
isAppInForeground: context.applicationStateHistory.currentSnapshot.state.isRunningInForeground, | ||
isBETEnabled: backgroundEventTrackingEnabled | ||
) | ||
|
||
switch handlingRule { | ||
case .handleInBackgroundView where command.canStartBackgroundView: | ||
startBackgroundView(on: command, context: context) | ||
default: | ||
if !(command is RUMKeepSessionAliveCommand) { // it is expected to receive 'keep alive' while no active view (when tracking WebView events) | ||
// As no view scope will handle this command, warn the user on dropping it. | ||
DD.logger.warn( | ||
switch handlingRule { | ||
case .handleInBackgroundView where command.canStartBackgroundView: | ||
startBackgroundView(on: command, context: context) | ||
default: | ||
if !(command is RUMKeepSessionAliveCommand) { // it is expected to receive 'keep alive' while no active view (when tracking WebView events) | ||
// As no view scope will handle this command, warn the user on dropping it. | ||
DD.logger.warn( | ||
""" | ||
\(String(describing: command)) was detected, but no view is active. To track views automatically, try calling the | ||
DatadogConfiguration.Builder.trackUIKitRUMViews() method. You can also track views manually using | ||
the RumMonitor.startView() and RumMonitor.stopView() methods. | ||
""" | ||
) | ||
) | ||
} | ||
} | ||
} | ||
} |
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 conditions and state management are difficult to follow and error prone IMO. It might be helpful to split this into multiple methods, would that be possible?
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.
I moved the "no active view" portion to its own function. Let me know if that helps or it you think it needs more.
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.
Perfect, thank you!
0155739
to
05a8278
Compare
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.
LGTM 🚀
What and why?
This adds a method to manually end a RUM session with
stopRumSession
. This immediately stops the active view and sets the current session to inactive. A new session starts automatically when a user action is sent (startView
oraddUserAction
).How?
First, may RUM events now have a new property:
session.isActive
. This can be null, and is only currently set in RUMViewEvents. The state of this member is pulled from the parent session context.Second, I've modified
RUMApplicationScope
to support holding multipleRUMSessionScope
s in a similar way to how Sessions can handle multiple Views until all of their events have been processed.When a
RUMStopSession
event is received, the Application Scope forwards it to the Session, which marks itself as inactive, then forwards the event to the all views, which mark themselves as inactive and send a ViewUpdate event which captures the session now being inactive.Provided views have finished processing all of their user actions and resources, the session is removed from available sessions on the ApplicationScope. Otherwise, it sicks around (marked as inactive) until all events are complete.
Review checklist
Custom CI job configuration (optional)