RUMM-1455 Add Session Start Callback #590
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This PR also surface API from recent PRs. Let me know if I should revert that! |
maxep
force-pushed
the
maxep/RUMM-1455/publish-session-id
branch
from
c19a626
to
840ba8d
Compare
ncreated
requested changes
Sep 7, 2021
| @@ -254,6 +254,11 @@ public class DDConfigurationBuilder: NSObject { | |||
| _ = sdkBuilder.set(rumSessionsSamplingRate: rumSessionsSamplingRate) | |||
| } | |||
|
|
|||
| @objc | |||
| public func set(onSessionStart handler: @escaping (String, Bool) -> Void) { | |||
There was a problem hiding this comment.
Nice 👌, let's also add smoke test for this new API here.
| @@ -87,6 +92,7 @@ internal class RUMApplicationScope: RUMScope, RUMContextProvider { | |||
| private func refresh(expiredSession: RUMSessionScope, on command: RUMCommand) { | |||
| let refreshedSession = RUMSessionScope(from: expiredSession, startTime: command.time) | |||
| sessionScope = refreshedSession | |||
| sessionScopeDidUpdate(refreshedSession) | |||
There was a problem hiding this comment.
We have no test coverage for this case - test added in RUMMonitorTests only checks the callback for initial session. Both "initial" and "refreshed" sessions are already tested in RUMApplicationScopeTests and I think we could add tests for both callback situations there as well, WDYT?
| /// (when `true` it means no event in this session will be kept). | ||
| /// | ||
| /// - Parameter handler: the callback handler to notify whenever a new Session starts. | ||
| public func onSessionStart(_ handler: @escaping (String, Bool) -> Void) -> Builder { |
There was a problem hiding this comment.
- About the naming - as this is a RUM feature, I think it's worth expressing it from the name as we do for other RUM options. How about
onRUMSessionStart? - About testing - it also requires updating tests in
DatadogConfigurationBuilderTests(we cover all public options in there)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What and why?
Adds an optional
onSessionStartcallback toRUMApplicationScopeto be called when a new session starts. The callback takes 2 arguments: the newly started Session ID and a boolean indicating whether or not the session is discarded by the sampling rate (whentrueit means no event in this session will be kept).Fix #526
Review checklist