-
-
Notifications
You must be signed in to change notification settings - Fork 226
Session Replay for iOS #4664
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
Session Replay for iOS #4664
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## version6 #4664 +/- ##
===========================================
Coverage ? 73.26%
===========================================
Files ? 480
Lines ? 17432
Branches ? 3439
===========================================
Hits ? 12771
Misses ? 3815
Partials ? 846 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| { | ||
| // For replay to work on iOS, session tracking must be enabled in the Cocoa SDKt | ||
| options.AutoSessionTracking = false; | ||
| nativeOptions.EnableAutoSessionTracking = 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.
I'm not sure how we now want to handle situations like this:
sentry-dotnet/src/Sentry/SentryClient.cs
Lines 350 to 362 in 9d1b4fe
| var hasTerminalException = processedEvent.HasTerminalException(); | |
| if (hasTerminalException) | |
| { | |
| // Event contains a terminal exception -> end session as crashed | |
| _options.LogDebug("Ending session as Crashed, due to unhandled exception."); | |
| scope.SessionUpdate = _sessionManager.EndSession(SessionEndStatus.Crashed); | |
| } | |
| else if (processedEvent.HasException()) | |
| { | |
| // Event contains a non-terminal exception -> report error | |
| // (this might return null if the session has already reported errors before) | |
| scope.SessionUpdate = _sessionManager.ReportError(); | |
| } |
... or this:
sentry-dotnet/src/Sentry/Internal/Hub.cs
Lines 376 to 384 in 9d1b4fe
| // Start a new session | |
| try | |
| { | |
| var sessionUpdate = _sessionManager.StartSession(); | |
| if (sessionUpdate is not null) | |
| { | |
| CaptureSession(sessionUpdate); | |
| } | |
| } |
If the CocoaSdk owns the session, do we still need to be able to start/stop sessions from the .NET SDK?
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.
these sessions are not the same as session replay. let's not conflate these 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.
@Flash0ver it's relatively complex to work around this. We could go with what's here for a 1st experimental release... basically if folks have session replay enabled for iOS then they can't manually start/stop sessions (since all of that has to be done via the Cocoa SDK). This is not ideal, but would allow us to ship some version of this functionality with version 6... get some feedback and then iterate from there.
If you're OK with that, this is ready for review again.
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.
Releasing as experimental feature for early feedback that we can improve on sounds great ... if #4704 is included.
Co-authored-by: GitHub <[email protected]> Co-authored-by: James Crosswell <[email protected]>
Co-authored-by: GitHub <[email protected]>
| /// to <c>false</c>. | ||
| /// </summary> | ||
| /// <remarks>Defaults to <c>true</c></remarks> | ||
| public bool EnableViewRendererV2 { get; set; } = 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.
question: are EnableViewRendererV2 and EnableFastViewRendering mutually exclusive?
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 separate flags in the underlying Cocoa SDK... we don't actually need to surface these in .NET though. I could just remove them unless/until we have some need to expose them.
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 was just wondering.
Since they're under ExperimentalOptions, I'm fine exposing the Option, so various combinations can be tested.
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
### Features
- Session Replay for iOS ([#4664](https://github.com/getsentry/sentry-dotnet/pull/4664))If none of the above apply, you can opt out of this check by adding |
| #elif __IOS__ | ||
| string? nativeId = null; | ||
| SentryCocoaSdk.ConfigureScope(scope => nativeId = scope.ReplayId); | ||
| return (nativeId is { } id) ? SentryId.Parse(id) : null; |
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.
Part of #2136
This PR adds basic support for Session Replay, including associating replays with exceptions and traces.
It does not implement any way to configure granular masks... only basic options to mask all text / images or none are provided. That can be done in a separate PR.
Note
Note that there are no new tests here as the enabling functionality was added in #4133 (those tests cover this functionality already)
Warning
We should bump the Cocoa SDK to 8.57.2 before releasing this