-
-
Notifications
You must be signed in to change notification settings - Fork 316
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
fix: Delete old session replay files #4446
base: main
Are you sure you want to change the base?
Conversation
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
0ecf042 | 1228.25 ms | 1250.67 ms | 22.42 ms |
0d32275 | 1230.96 ms | 1237.90 ms | 6.94 ms |
e0f077c | 1224.65 ms | 1243.52 ms | 18.87 ms |
af1f4dd | 1207.33 ms | 1230.04 ms | 22.71 ms |
7bc3c0d | 1212.35 ms | 1228.94 ms | 16.59 ms |
3f366ee | 1242.28 ms | 1260.80 ms | 18.52 ms |
cb6ab62 | 1213.47 ms | 1248.46 ms | 34.99 ms |
42ef6ba | 1219.58 ms | 1245.37 ms | 25.78 ms |
b9d59f7 | 1222.23 ms | 1227.16 ms | 4.93 ms |
888a145 | 1228.63 ms | 1248.94 ms | 20.30 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
0ecf042 | 21.58 KiB | 631.82 KiB | 610.24 KiB |
0d32275 | 22.84 KiB | 403.14 KiB | 380.29 KiB |
e0f077c | 22.85 KiB | 412.59 KiB | 389.74 KiB |
af1f4dd | 22.85 KiB | 414.71 KiB | 391.86 KiB |
7bc3c0d | 20.76 KiB | 427.35 KiB | 406.59 KiB |
3f366ee | 20.76 KiB | 427.84 KiB | 407.08 KiB |
cb6ab62 | 22.85 KiB | 413.42 KiB | 390.57 KiB |
42ef6ba | 21.58 KiB | 417.86 KiB | 396.28 KiB |
b9d59f7 | 22.85 KiB | 405.77 KiB | 382.92 KiB |
888a145 | 21.58 KiB | 713.54 KiB | 691.96 KiB |
// Mapping replay folder here and not in dispatched queue to prevent a race condition between | ||
// listing files and creating a new replay session. |
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.
The dispatch queue in the dependency manager is a serial queue, so there shouldn't be a danger of race condition:
sentry-cocoa/Sources/Sentry/SentryDispatchQueueWrapper.m
Lines 13 to 14 in 2d2068d
dispatch_queue_attr_t attributes = dispatch_queue_attr_make_with_qos_class( | |
DISPATCH_QUEUE_SERIAL, DISPATCH_QUEUE_PRIORITY_DEFAULT, 0); |
In fact, you might even be creating a race condition by not placing this on the serial queue, depending on what other replay operations are or aren't also performed on the queue. Something that was previously deferred might be interleaved with this line and the next block enqueued below in various combinations.
For instance, what happens if multiple calls to SentryReplayApi.start
are received in quick succession?
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.
The dispatch queue in the dependency manager is a serial queue
But the SentrySessionReplay
class that creates a new replay session has it own queue.
what happens if multiple calls to SentryReplayApi.start
The Integration is initialized only once and the cleanUp will run only once.
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.
Hm, the only reason I mentioned the dependency container's queue is because that's what's being used in this file. Should we use one queue for all replay activity? Why the dependency container's here and the private queue in SentrySessionReplay.swift, wouldn't that open up the possibility of more race conditions?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4446 +/- ##
=============================================
+ Coverage 91.343% 91.351% +0.008%
=============================================
Files 610 610
Lines 49928 49964 +36
Branches 18039 18047 +8
=============================================
+ Hits 45606 45643 +37
Misses 4230 4230
+ Partials 92 91 -1
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>
🚨 Detected changes in high risk code 🚨High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:
|
It's just a new function to check whether a path points to a directory. |
📜 Description
Clean up for old session replay files
💚 How did you test it?
Unit tests
📝 Checklist
You have to check all boxes before merging:
sendDefaultPII
is enabled.🔮 Next steps