feat(Session Replay): Session Replay Integration#3671
feat(Session Replay): Session Replay Integration#3671brustolin merged 104 commits intofeat/session-replayfrom
Conversation
…coa into feat(SR)/ReplayEvent
brustolin
requested review from
armcknight and
philipphofmann
as code owners
|
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.
|
armcknight
left a comment
armcknight
left a comment
There was a problem hiding this comment.
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.
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.
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.
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 |
philipphofmann
left a comment
philipphofmann
left a comment
There was a problem hiding this comment.
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
7 participants
📜 Description
Adding Session replay integration
💡 Motivation and Context
💚 How did you test it?
Unit test
#skip-changelog