-
-
Notifications
You must be signed in to change notification settings - Fork 318
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
ref: Stops session replay if rate limiting is activated #4496
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4496 +/- ##
=============================================
+ Coverage 91.439% 91.460% +0.021%
=============================================
Files 616 616
Lines 68794 68989 +195
Branches 24677 24758 +81
=============================================
+ Hits 62905 63098 +193
- Misses 5796 5799 +3
+ Partials 93 92 -1
... and 8 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
2c603bf | 1226.66 ms | 1243.06 ms | 16.40 ms |
455619d | 1231.40 ms | 1237.70 ms | 6.30 ms |
15b8c61 | 1223.16 ms | 1244.83 ms | 21.67 ms |
2f8b3a8 | 1233.76 ms | 1260.24 ms | 26.48 ms |
df2986b | 1215.20 ms | 1235.02 ms | 19.82 ms |
94e1968 | 1238.85 ms | 1252.02 ms | 13.17 ms |
216bdf9 | 1199.84 ms | 1209.53 ms | 9.69 ms |
6e342ac | 1220.24 ms | 1240.14 ms | 19.90 ms |
dbd1f86 | 1235.51 ms | 1247.20 ms | 11.69 ms |
2d0e628 | 1229.04 ms | 1252.79 ms | 23.75 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
2c603bf | 21.58 KiB | 417.88 KiB | 396.30 KiB |
455619d | 20.76 KiB | 432.87 KiB | 412.11 KiB |
15b8c61 | 20.76 KiB | 419.67 KiB | 398.91 KiB |
2f8b3a8 | 20.76 KiB | 434.72 KiB | 413.96 KiB |
df2986b | 22.85 KiB | 406.89 KiB | 384.04 KiB |
94e1968 | 21.58 KiB | 614.74 KiB | 593.16 KiB |
216bdf9 | 21.58 KiB | 418.13 KiB | 396.54 KiB |
6e342ac | 20.76 KiB | 436.66 KiB | 415.90 KiB |
dbd1f86 | 21.58 KiB | 707.43 KiB | 685.85 KiB |
2d0e628 | 21.90 KiB | 708.34 KiB | 686.43 KiB |
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 would rather check the active rate limits than having a flag.
@@ -46,6 +47,8 @@ @implementation SentrySessionReplayIntegration { | |||
SentryReplayOptions *_replayOptions; | |||
SentryNSNotificationCenterWrapper *_notificationCenter; | |||
SentryOnDemandReplay *_resumeReplayMaker; | |||
id<SentryRateLimits> _rateLimits; | |||
BOOL _rateLimited; |
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.
h
: What stops us from checking the rate limit for every session replay we start? Checking the rate active limits is cheap. This flag can quickly get out of sync with the rate limit, leading to potentially dropping data even if we don't need to.
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.
This flag can quickly get out of sync with the rate limit
This is the intention. Once the app was rate limited, we should not start the session replay again for the entire 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.
If we always limit this to an entire session, we lose flexibility. Why can't the server simply send a long enough timeout? Why does it have to be an entire 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.
Right now, this is the easiest way to ensure segment 0 will always reach the server, because session replay absolutely needs segment 0 to make replay work.
We could add more steps to check whether the envelope carrying the segment 0 was delivered, but it seems an overkill for an edge case (rate limiting).
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.
Please add this essential info as a code comment somewhere, because the future me will forget and get confused 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.
LGTM, when adding a comment why we rate limit until the next session.
@@ -46,6 +47,8 @@ @implementation SentrySessionReplayIntegration { | |||
SentryReplayOptions *_replayOptions; | |||
SentryNSNotificationCenterWrapper *_notificationCenter; | |||
SentryOnDemandReplay *_resumeReplayMaker; | |||
id<SentryRateLimits> _rateLimits; | |||
BOOL _rateLimited; |
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.
Please add this essential info as a code comment somewhere, because the future me will forget and get confused again.
📜 Description
Stop session replay if the app is being rate limited.
💡 Motivation and Context
https://github.com/getsentry/team-replay/issues/496
💚 How did you test it?
📝 Checklist
You have to check all boxes before merging:
sendDefaultPII
is enabled.🔮 Next steps