Skip to content
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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
- Improve frames tracker performance (#4469)
- Log a warning when dropping envelopes due to rate-limiting (#4463)
- Expose `SentrySessionReplayIntegration-Hybrid.h` as `private` (#4486)
- Stops session replay if rate limiting is activated (#4496)
- Add `maskedViewClasses` and `unmaskedViewClasses` to SentryReplayOptions init via dict (#4492)

## 8.39.0
Expand Down
8 changes: 4 additions & 4 deletions Sources/Sentry/SentryClient.m
Original file line number Diff line number Diff line change
Expand Up @@ -99,10 +99,10 @@ - (instancetype)initWithOptions:(SentryOptions *)options
fileManager:(SentryFileManager *)fileManager
deleteOldEnvelopeItems:(BOOL)deleteOldEnvelopeItems
{
NSArray<id<SentryTransport>> *transports = [SentryTransportFactory
initTransports:options
sentryFileManager:fileManager
currentDateProvider:SentryDependencyContainer.sharedInstance.dateProvider];
NSArray<id<SentryTransport>> *transports =
[SentryTransportFactory initTransports:options
sentryFileManager:fileManager
rateLimits:SentryDependencyContainer.sharedInstance.rateLimits];

SentryTransportAdapter *transportAdapter =
[[SentryTransportAdapter alloc] initWithTransports:transports options:options];
Expand Down
24 changes: 24 additions & 0 deletions Sources/Sentry/SentryDependencyContainer.m
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,12 @@
#import <SentryCrash.h>
#import <SentryCrashWrapper.h>
#import <SentryDebugImageProvider.h>
#import <SentryDefaultRateLimits.h>
#import <SentryDependencyContainer.h>
#import <SentryHttpDateParser.h>
#import <SentryNSNotificationCenterWrapper.h>
#import <SentryRateLimitParser.h>
#import <SentryRetryAfterHeaderParser.h>
#import <SentrySDK+Private.h>
#import <SentrySwift.h>
#import <SentrySwizzleWrapper.h>
Expand Down Expand Up @@ -215,6 +219,26 @@ - (SentryNSNotificationCenterWrapper *)notificationCenterWrapper
}
}

- (id<SentryRateLimits>)rateLimits
{
@synchronized(sentryDependencyContainerLock) {
if (_rateLimits == nil) {
SentryRetryAfterHeaderParser *retryAfterHeaderParser =
[[SentryRetryAfterHeaderParser alloc]
initWithHttpDateParser:[[SentryHttpDateParser alloc] init]
currentDateProvider:self.dateProvider];
SentryRateLimitParser *rateLimitParser =
[[SentryRateLimitParser alloc] initWithCurrentDateProvider:self.dateProvider];

_rateLimits = [[SentryDefaultRateLimits alloc]
initWithRetryAfterHeaderParser:retryAfterHeaderParser
andRateLimitParser:rateLimitParser
currentDateProvider:self.dateProvider];
}
return _rateLimits;
}
}

#if SENTRY_HAS_UIKIT
- (SentryUIDeviceWrapper *)uiDeviceWrapper SENTRY_DISABLE_THREAD_SANITIZER(
"double-checked lock produce false alarms")
Expand Down
20 changes: 20 additions & 0 deletions Sources/Sentry/SentrySessionReplayIntegration.m
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
# import "SentryNSNotificationCenterWrapper.h"
# import "SentryOptions.h"
# import "SentryRandom.h"
# import "SentryRateLimits.h"
# import "SentryReachability.h"
# import "SentrySDK+Private.h"
# import "SentryScope+Private.h"
Expand Down Expand Up @@ -46,6 +47,8 @@ @implementation SentrySessionReplayIntegration {
SentryReplayOptions *_replayOptions;
SentryNSNotificationCenterWrapper *_notificationCenter;
SentryOnDemandReplay *_resumeReplayMaker;
id<SentryRateLimits> _rateLimits;
BOOL _rateLimited;
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

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).

Copy link
Member

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.

}

- (instancetype)init
Expand Down Expand Up @@ -78,6 +81,7 @@ - (void)setupWith:(SentryReplayOptions *)replayOptions enableTouchTracker:(BOOL)
{
_replayOptions = replayOptions;
_viewPhotographer = [[SentryViewPhotographer alloc] initWithRedactOptions:replayOptions];
_rateLimits = SentryDependencyContainer.sharedInstance.rateLimits;

if (touchTracker) {
_touchTracker = [[SentryTouchTracker alloc]
Expand Down Expand Up @@ -416,6 +420,12 @@ - (void)resume

- (void)start
{
if (_rateLimited) {
SENTRY_LOG_WARN(
@"This session was rate limited. Not starting session replay until next app session");
return;
}

if (self.sessionReplay != nil) {
if (self.sessionReplay.isFullSession == NO) {
[self.sessionReplay captureReplay];
Expand Down Expand Up @@ -447,6 +457,7 @@ - (void)sentrySessionEnded:(SentrySession *)session

- (void)sentrySessionStarted:(SentrySession *)session
{
_rateLimited = NO;
[self startSession];
}

Expand Down Expand Up @@ -553,6 +564,15 @@ - (void)sessionReplayNewSegmentWithReplayEvent:(SentryReplayEvent *)replayEvent
replayRecording:(SentryReplayRecording *)replayRecording
videoUrl:(NSURL *)videoUrl
{
if ([_rateLimits isRateLimitActive:kSentryDataCategoryReplay] ||
[_rateLimits isRateLimitActive:kSentryDataCategoryAll]) {
SENTRY_LOG_DEBUG(
@"Rate limiting is active for replays. Stopping session replay until next session.");
_rateLimited = YES;
[self stop];
return;
}

[SentrySDK.currentHub captureReplayEvent:replayEvent
replayRecording:replayRecording
video:videoUrl];
Expand Down
13 changes: 1 addition & 12 deletions Sources/Sentry/SentryTransportFactory.m
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ @implementation SentryTransportFactory

+ (NSArray<id<SentryTransport>> *)initTransports:(SentryOptions *)options
sentryFileManager:(SentryFileManager *)sentryFileManager
currentDateProvider:(id<SentryCurrentDateProvider>)currentDateProvider
rateLimits:(id<SentryRateLimits>)rateLimits
{
NSURLSession *session;

Expand All @@ -42,17 +42,6 @@ @implementation SentryTransportFactory
id<SentryRequestManager> requestManager =
[[SentryQueueableRequestManager alloc] initWithSession:session];

SentryHttpDateParser *httpDateParser = [[SentryHttpDateParser alloc] init];
SentryRetryAfterHeaderParser *retryAfterHeaderParser =
[[SentryRetryAfterHeaderParser alloc] initWithHttpDateParser:httpDateParser
currentDateProvider:currentDateProvider];
SentryRateLimitParser *rateLimitParser =
[[SentryRateLimitParser alloc] initWithCurrentDateProvider:currentDateProvider];
id<SentryRateLimits> rateLimits =
[[SentryDefaultRateLimits alloc] initWithRetryAfterHeaderParser:retryAfterHeaderParser
andRateLimitParser:rateLimitParser
currentDateProvider:currentDateProvider];

SentryEnvelopeRateLimit *envelopeRateLimit =
[[SentryEnvelopeRateLimit alloc] initWithRateLimits:rateLimits];

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
@class SentryThreadInspector;
@protocol SentryRandom;
@protocol SentryCurrentDateProvider;
@protocol SentryRateLimits;

#if SENTRY_HAS_METRIC_KIT
@class SentryMXManager;
Expand Down Expand Up @@ -71,6 +72,7 @@ SENTRY_NO_INIT
@property (nonatomic, strong) SentryExtraContextProvider *extraContextProvider;
@property (nonatomic, strong) SentrySysctl *sysctlWrapper;
@property (nonatomic, strong) SentryThreadInspector *threadInspector;
@property (nonatomic, strong) id<SentryRateLimits> rateLimits;

#if SENTRY_UIKIT_AVAILABLE
@property (nonatomic, strong) SentryFramesTracker *framesTracker;
Expand Down
3 changes: 2 additions & 1 deletion Sources/Sentry/include/SentryTransportFactory.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

@class SentryOptions, SentryFileManager;
@protocol SentryCurrentDateProvider;
@protocol SentryRateLimits;

NS_ASSUME_NONNULL_BEGIN

Expand All @@ -12,7 +13,7 @@ NS_SWIFT_NAME(TransportInitializer)

+ (NSArray<id<SentryTransport>> *)initTransports:(SentryOptions *)options
sentryFileManager:(SentryFileManager *)sentryFileManager
currentDateProvider:(id<SentryCurrentDateProvider>)currentDateProvider;
rateLimits:(id<SentryRateLimits>)rateLimits;

@end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,110 @@ class SentrySessionReplayIntegrationTests: XCTestCase {
XCTAssertEqual(sessionReplay.sessionReplayId, replayId)
}

func testStopBecauseOfReplayRateLimit() throws {
let rateLimiter = TestRateLimits()
SentryDependencyContainer.sharedInstance().rateLimits = rateLimiter
rateLimiter.rateLimits.append(.replay)

startSDK(sessionSampleRate: 1, errorSampleRate: 1)
let sut = try getSut()
let sessionReplay = sut.sessionReplay

XCTAssertTrue(sessionReplay?.isRunning ?? false)

let videoUrl = URL(fileURLWithPath: "video.mp4")
let videoInfo = SentryVideoInfo(path: videoUrl, height: 1_024, width: 480, duration: 5, frameCount: 5, frameRate: 1, start: Date(), end: Date(), fileSize: 10, screens: [])
let replayEvent = SentryReplayEvent(eventId: SentryId(), replayStartTimestamp: Date(), replayType: .session, segmentId: 0)

(sut as SentrySessionReplayDelegate).sessionReplayNewSegment(replayEvent: replayEvent,
replayRecording: SentryReplayRecording(segmentId: 0, video: videoInfo, extraEvents: []),
videoUrl: videoUrl)

XCTAssertFalse(sessionReplay?.isRunning ?? true)
XCTAssertNil(sut.sessionReplay)
}

func testStopBecauseOfAllRateLimit() throws {
let rateLimiter = TestRateLimits()
SentryDependencyContainer.sharedInstance().rateLimits = rateLimiter
rateLimiter.rateLimits.append(.all)

startSDK(sessionSampleRate: 1, errorSampleRate: 1)
let sut = try getSut()
let sessionReplay = sut.sessionReplay

XCTAssertTrue(sessionReplay?.isRunning ?? false)

let videoUrl = URL(fileURLWithPath: "video.mp4")
let videoInfo = SentryVideoInfo(path: videoUrl, height: 1_024, width: 480, duration: 5, frameCount: 5, frameRate: 1, start: Date(), end: Date(), fileSize: 10, screens: [])
let replayEvent = SentryReplayEvent(eventId: SentryId(), replayStartTimestamp: Date(), replayType: .session, segmentId: 0)

(sut as SentrySessionReplayDelegate).sessionReplayNewSegment(replayEvent: replayEvent,
replayRecording: SentryReplayRecording(segmentId: 0, video: videoInfo, extraEvents: []),
videoUrl: videoUrl)

XCTAssertFalse(sessionReplay?.isRunning ?? true)
XCTAssertNil(sut.sessionReplay)
}

func testDontRestartAfterRateLimit() throws {
let rateLimiter = TestRateLimits()
SentryDependencyContainer.sharedInstance().rateLimits = rateLimiter
rateLimiter.rateLimits.append(.all)

startSDK(sessionSampleRate: 1, errorSampleRate: 1)
let sut = try getSut()
let sessionReplay = sut.sessionReplay

XCTAssertTrue(sessionReplay?.isRunning ?? false)

let videoUrl = URL(fileURLWithPath: "video.mp4")
let videoInfo = SentryVideoInfo(path: videoUrl, height: 1_024, width: 480, duration: 5, frameCount: 5, frameRate: 1, start: Date(), end: Date(), fileSize: 10, screens: [])
let replayEvent = SentryReplayEvent(eventId: SentryId(), replayStartTimestamp: Date(), replayType: .session, segmentId: 0)

(sut as SentrySessionReplayDelegate).sessionReplayNewSegment(replayEvent: replayEvent,
replayRecording: SentryReplayRecording(segmentId: 0, video: videoInfo, extraEvents: []),
videoUrl: videoUrl)

XCTAssertFalse(sessionReplay?.isRunning ?? true)
XCTAssertNil(sut.sessionReplay)

sut.start()

XCTAssertFalse(sessionReplay?.isRunning ?? true)
XCTAssertNil(sut.sessionReplay)
}

func testAlowStartForNewSessionAfterRateLimit() throws {
let rateLimiter = TestRateLimits()
SentryDependencyContainer.sharedInstance().rateLimits = rateLimiter
rateLimiter.rateLimits.append(.all)

startSDK(sessionSampleRate: 0, errorSampleRate: 1)
let sut = try getSut()
let sessionReplay = sut.sessionReplay
sut.start()

XCTAssertTrue(sessionReplay?.isRunning ?? false)

let videoUrl = URL(fileURLWithPath: "video.mp4")
let videoInfo = SentryVideoInfo(path: videoUrl, height: 1_024, width: 480, duration: 5, frameCount: 5, frameRate: 1, start: Date(), end: Date(), fileSize: 10, screens: [])
let replayEvent = SentryReplayEvent(eventId: SentryId(), replayStartTimestamp: Date(), replayType: .session, segmentId: 0)

(sut as SentrySessionReplayDelegate).sessionReplayNewSegment(replayEvent: replayEvent,
replayRecording: SentryReplayRecording(segmentId: 0, video: videoInfo, extraEvents: []),
videoUrl: videoUrl)
XCTAssertNil(sut.sessionReplay)

sut.start()
XCTAssertNil(sut.sessionReplay)

(sut as SentrySessionListener).sentrySessionStarted(SentrySession(releaseName: "", distinctId: ""))

sut.start()
XCTAssertTrue(sut.sessionReplay?.isRunning ?? false)
}

func testStartWithBufferSessionReplay() throws {
startSDK(sessionSampleRate: 0, errorSampleRate: 1)
let sut = try getSut()
Expand Down
16 changes: 12 additions & 4 deletions Tests/SentryTests/Networking/SentryTransportFactoryTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import XCTest
class SentryTransportFactoryTests: XCTestCase {

private static let dsnAsString = TestConstants.dsnAsString(username: "SentryTransportFactoryTests")

func testIntegration_UrlSessionDelegate_PassedToRequestManager() throws {
let urlSessionDelegateSpy = UrlSessionDelegateSpy()

Expand All @@ -19,7 +19,7 @@ class SentryTransportFactoryTests: XCTestCase {
options.urlSessionDelegate = urlSessionDelegateSpy

let fileManager = try! SentryFileManager(options: options, dispatchQueueWrapper: TestSentryDispatchQueueWrapper())
let transports = TransportInitializer.initTransports(options, sentryFileManager: fileManager, currentDateProvider: TestCurrentDateProvider())
let transports = TransportInitializer.initTransports(options, sentryFileManager: fileManager, rateLimits: rateLimiting())
let httpTransport = transports.first
let requestManager = try XCTUnwrap(Dynamic(httpTransport).requestManager.asObject as? SentryQueueableRequestManager)

Expand All @@ -44,7 +44,7 @@ class SentryTransportFactoryTests: XCTestCase {
options.urlSession = sessionConfiguration

let fileManager = try! SentryFileManager(options: options, dispatchQueueWrapper: TestSentryDispatchQueueWrapper())
let transports = TransportInitializer.initTransports(options, sentryFileManager: fileManager, currentDateProvider: TestCurrentDateProvider())
let transports = TransportInitializer.initTransports(options, sentryFileManager: fileManager, rateLimits: rateLimiting())

let httpTransport = transports.first
let requestManager = try XCTUnwrap(Dynamic(httpTransport).requestManager.asObject as? SentryQueueableRequestManager)
Expand All @@ -60,7 +60,7 @@ class SentryTransportFactoryTests: XCTestCase {
func testShouldReturnTwoTransports_WhenSpotlightEnabled() throws {
let options = Options()
options.enableSpotlight = true
let transports = TransportInitializer.initTransports(options, sentryFileManager: try SentryFileManager(options: options), currentDateProvider: TestCurrentDateProvider())
let transports = TransportInitializer.initTransports(options, sentryFileManager: try SentryFileManager(options: options), rateLimits: rateLimiting())

XCTAssert(transports.contains {
$0.isKind(of: SentrySpotlightTransport.self)
Expand All @@ -70,5 +70,13 @@ class SentryTransportFactoryTests: XCTestCase {
$0.isKind(of: SentryHttpTransport.self)
})
}

func rateLimiting() -> RateLimits {
let dateProvider = TestCurrentDateProvider()
let retryAfterHeaderParser = RetryAfterHeaderParser(httpDateParser: HttpDateParser(), currentDateProvider: dateProvider)
let rateLimitParser = RateLimitParser(currentDateProvider: dateProvider)

return DefaultRateLimits(retryAfterHeaderParser: retryAfterHeaderParser, andRateLimitParser: rateLimitParser, currentDateProvider: dateProvider)
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ class SentryTransportInitializerTests: XCTestCase {
func testDefault() throws {
let options = try Options(dict: ["dsn": SentryTransportInitializerTests.dsnAsString])

let result = TransportInitializer.initTransports(options, sentryFileManager: fileManager, currentDateProvider: TestCurrentDateProvider())
let result = TransportInitializer.initTransports(options, sentryFileManager: fileManager, rateLimits: SentryDependencyContainer.sharedInstance().rateLimits)
XCTAssertEqual(result.count, 1)

let firstTransport = result.first
Expand Down
Loading