-
-
Notifications
You must be signed in to change notification settings - Fork 342
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
feat(Session Replay): Session Replay Integration #3671
Conversation
…coa into feat(SR)/ReplayEvent
|
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
0de47cc | 1219.55 ms | 1245.96 ms | 26.41 ms |
4b92944 | 1236.31 ms | 1254.06 ms | 17.76 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
0de47cc | 21.58 KiB | 549.65 KiB | 528.06 KiB |
4b92944 | 21.58 KiB | 547.02 KiB | 525.44 KiB |
Previous results on branch: feat(SR)/replay-integration
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
a90f1ad | 1218.00 ms | 1236.04 ms | 18.04 ms |
f578a85 | 1196.49 ms | 1211.60 ms | 15.11 ms |
e0754d5 | 1235.65 ms | 1252.04 ms | 16.39 ms |
dd01832 | 1214.80 ms | 1227.48 ms | 12.68 ms |
0ab9fa3 | 1231.39 ms | 1252.20 ms | 20.82 ms |
dd705a9 | 1202.49 ms | 1225.38 ms | 22.89 ms |
305cf66 | 1201.24 ms | 1217.76 ms | 16.51 ms |
c203967 | 1208.16 ms | 1225.16 ms | 17.00 ms |
65442f4 | 1226.58 ms | 1240.06 ms | 13.48 ms |
6ce4076 | 1237.80 ms | 1250.39 ms | 12.59 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
a90f1ad | 21.58 KiB | 577.42 KiB | 555.84 KiB |
f578a85 | 21.58 KiB | 433.83 KiB | 412.25 KiB |
e0754d5 | 21.58 KiB | 576.65 KiB | 555.07 KiB |
dd01832 | 21.58 KiB | 576.64 KiB | 555.05 KiB |
0ab9fa3 | 21.58 KiB | 576.60 KiB | 555.01 KiB |
dd705a9 | 21.58 KiB | 440.43 KiB | 418.85 KiB |
305cf66 | 21.58 KiB | 440.48 KiB | 418.90 KiB |
c203967 | 21.58 KiB | 439.79 KiB | 418.21 KiB |
65442f4 | 21.58 KiB | 576.63 KiB | 555.05 KiB |
6ce4076 | 21.58 KiB | 576.86 KiB | 555.28 KiB |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/session-replay #3671 +/- ##
=========================================================
- Coverage 89.346% 88.766% -0.580%
=========================================================
Files 546 554 +8
Lines 59650 60141 +491
Branches 21416 21526 +110
=========================================================
+ Hits 53295 53385 +90
- Misses 5310 5749 +439
+ Partials 1045 1007 -38
... and 70 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
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.
Here's some stuff just from a quick glance through the diff. You could separate out the Swift rewrite of SentryReplayOptions to a separate PR. I don't mind the size otherwise.
How can we demo this, if it's at that stage? When it becomes ready for review, please add instructions with a demo. I wasn't able to see anything in the replays on the web dashboard when trying this. I also set the sessionSampleRate
to 1 in the options in AppDelegate with no change. I used the button in the sample app to catch errors, but didn't see those in the errors timeline on the web.
Other stuff:
- Please fix the typo on SentryOptions.h line 277:
@node
->@note
. - Make a note there as well that it's not available for visionOS and only for iOS 16+ (and the tvOS equivalent)
Sources/Swift/Integrations/SessionReplay/SentryReplayOptions.swift
Outdated
Show resolved
Hide resolved
Sources/Swift/Integrations/SessionReplay/SentryReplayOptions.swift
Outdated
Show resolved
Hide resolved
Co-authored-by: Andrew McKnight <[email protected]>
This reverts commit 3252a83.
Co-authored-by: Andrew McKnight <[email protected]>
…ntry-cocoa into feat(SR)/replay-integration
actualEnd = frame.time | ||
frames.append(frame.imagePath) | ||
} | ||
let (frames, start, end) = filterFrames(beginning: beginning, end: beginning.addingTimeInterval(duration)) |
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'd stay away from Swift tuples: they're not objc-compatible, and they're not Codable
.
If filterFrames
returned a struct instead, we could just pass that whole struct to SentryVideoInfo.init
below on line 146, instead of each thing individually.
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 understand. But for private functions inside the class itself I think it's fine. No need to declare another struct for this.
var videoWidth = 200 | ||
var videoHeight = 434 | ||
|
||
var bitRate = 20_000 | ||
var frameRate = 1 | ||
var cacheMaxSize = UInt.max |
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 fine with not bringing in the dependency, but please record the assumptions somewhere. Don't assume others will understand what the units are. Having a variable named "bitRate" that has a unit of "byte," which is 8 bits, is confusing. 20,000 bits vs 20,000 bytes is quite a difference. it should be byteRate
at the very least.
CI errors in the PR will be fixed in the next one: #3816 |
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, for merging this to main hidden behind a feature flag. As discussed, I'm going to give this a full review after it's merged to main, and I didn't check the functionality of the code yet.
Relates to: [Epic] Mobile Replay: Private Alpha Release sentry#63255 Added and experimental sub option to SentryOption to expose experimental features
📜 Description
Adding Session replay integration
💡 Motivation and Context
💚 How did you test it?
Unit test
#skip-changelog