-
Notifications
You must be signed in to change notification settings - Fork 136
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
RUM-5536 [SR] Start / Stop API #1986
Conversation
7ccf8ad
to
bce906e
Compare
bce906e
to
2c62e43
Compare
2c62e43
to
55891cc
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.
Looks great 💪 and I appreciate the work on tests coverage 🏅 👌. I left one blocking commend on potential problem in synchronizing the isSampled
state (I think now it can be mutated from different threads).
Also, let's move the new option to SessionReplay.Configuration
to be consistent with other feature APIs 🙏.
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.
Looks great 🎯. I left one small feedback on API comment.
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.
Looks great! I have left couple of question/suggestion.
DatadogSessionReplay/Sources/Recorder/RecordingCoordinator.swift
Outdated
Show resolved
Hide resolved
@maxep Following our discussion, I’ve updated the logic to use the scheduler’s queue. Sharing here the reasoning for later reference. With this approach, all state changes are synchronized on the same queue. It maintains consistency without the overhead of locks. A trade-off of this approach is that This change prioritizes thread and synchronization safety, which can avoid hard-to-debug issues. This should save us time and prevent potential problems in the future. |
Note: I logged a ticket to deal with webviews' snapshots later, as this should be an edge case not blocking GA. |
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.
Looks great, thanks!
What and why?
This PR adds the ability for customers to:
See RFC for more details.
How?
startRecordingImmediately
optional parameter when enabling Session Replay, defaulting totrue
start
andstop
public APIs to the existingSessionReplay
public enum, through static functionsrecordingEnabled
variable toRecordingCoordinator
to track when the user wants to start or stop a recordinghas_replay
accordingly when starting or stopping a recordingRecordingCoordinator
:has_replay
flag from the RUM view ID; whether it'snil
or not has no impact on this flag anymoreNote: ignoring webviews' snapshots will be tackled separately
Review checklist