Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(profiling): reevaluate sample rate on fg with new API #4997

Merged
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
12 changes: 6 additions & 6 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,9 @@ GEM
json (>= 1.5.1)
artifactory (3.0.17)
atomos (0.1.3)
aws-eventstream (1.3.1)
aws-partitions (1.1056.0)
aws-sdk-core (3.219.0)
aws-eventstream (1.3.2)
aws-partitions (1.1067.0)
aws-sdk-core (3.220.1)
aws-eventstream (~> 1, >= 1.3.0)
aws-partitions (~> 1, >= 1.992.0)
aws-sigv4 (~> 1.9)
Expand Down Expand Up @@ -130,7 +130,7 @@ GEM
faraday_middleware (1.2.1)
faraday (~> 1.0)
fastimage (2.4.0)
fastlane (2.226.0)
fastlane (2.227.0)
CFPropertyList (>= 2.3, < 4.0.0)
addressable (>= 2.8, < 3.0.0)
artifactory (~> 3.0)
Expand Down Expand Up @@ -196,12 +196,12 @@ GEM
google-apis-core (>= 0.11.0, < 2.a)
google-apis-storage_v1 (0.31.0)
google-apis-core (>= 0.11.0, < 2.a)
google-cloud-core (1.7.1)
google-cloud-core (1.8.0)
google-cloud-env (>= 1.0, < 3.a)
google-cloud-errors (~> 1.0)
google-cloud-env (1.6.0)
faraday (>= 0.17.3, < 3.0)
google-cloud-errors (1.4.0)
google-cloud-errors (1.5.0)
google-cloud-storage (1.47.0)
addressable (~> 2.8)
digest-crc (~> 0.4)
Expand Down
10 changes: 3 additions & 7 deletions Sources/Sentry/Profiling/SentryContinuousProfiler.mm
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@

# pragma mark - Private

NSTimeInterval kSentryProfilerChunkExpirationInterval = 60;

namespace {
/** @warning: Must be used from a synchronized context. */
std::mutex _threadUnsafe_gContinuousProfilerLock;
Expand Down Expand Up @@ -152,8 +154,6 @@ + (void)stop
return;
}

SENTRY_LOG_DEBUG(@"Stopping continuous profiler after current chunk completes.");

# if defined(SENTRY_TEST) || defined(SENTRY_TEST_CI)
// we want to allow immediately stopping a continuous profile for a UI test, since those
// currently only test launch profiles, and there is no reliable way to make the UI test
Expand All @@ -168,11 +168,7 @@ + (void)stop
}
# endif // defined(SENTRY_TEST) || defined(SENTRY_TEST_CI)

if (![_threadUnsafe_gContinuousCurrentProfiler isRunning]) {
SENTRY_LOG_DEBUG(@"No continuous profiler is currently running.");
return;
}

SENTRY_LOG_DEBUG(@"Stopping continuous profiler after current chunk completes.");
_stopCalled = YES;
}

Expand Down
4 changes: 2 additions & 2 deletions Sources/Sentry/Profiling/SentryProfilerDefines.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ typedef NS_ENUM(NSUInteger, SentryProfilerTruncationReason) {
SentryProfilerTruncationReasonAppMovedToBackground,
};

static NSTimeInterval kSentryProfilerChunkExpirationInterval = 60;
static NSTimeInterval kSentryProfilerTimeoutInterval = 30;
SENTRY_EXTERN NSTimeInterval kSentryProfilerChunkExpirationInterval;
SENTRY_EXTERN NSTimeInterval kSentryProfilerTimeoutInterval;

NS_ASSUME_NONNULL_BEGIN

Expand Down
1 change: 1 addition & 0 deletions Sources/Sentry/Profiling/SentryTraceProfiler.mm
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
# pragma mark - Private

NSTimer *_Nullable _sentry_threadUnsafe_traceProfileTimeoutTimer;
NSTimeInterval kSentryProfilerTimeoutInterval = 30;

namespace {
/** @warning: Must be used from a synchronized context. */
Expand Down
8 changes: 7 additions & 1 deletion Sources/Sentry/SentryProfiler.mm
Original file line number Diff line number Diff line change
Expand Up @@ -49,13 +49,19 @@
return;
}

sentry_profilerSessionSampleDecision = sentry_sampleProfileSession(options);
sentry_reevaluateSessionSampleRate(options.profiling.sessionSampleRate);
}

} // namespace

# pragma mark - Public

void
sentry_reevaluateSessionSampleRate(float sessionSampleRate)
{
sentry_profilerSessionSampleDecision = sentry_sampleProfileSession(sessionSampleRate);
}

void
sentry_sdkInitProfilerTasks(SentryOptions *options, SentryHub *hub)
{
Expand Down
4 changes: 2 additions & 2 deletions Sources/Sentry/SentrySampling.m
Original file line number Diff line number Diff line change
Expand Up @@ -120,9 +120,9 @@
}

SentrySamplerDecision *
sentry_sampleProfileSession(SentryOptions *options)
sentry_sampleProfileSession(float sessionSampleRate)
{
return _sentry_calcSampleFromNumericalRate(@(options.profiling.sessionSampleRate));
return _sentry_calcSampleFromNumericalRate(@(sessionSampleRate));
}

#endif // SENTRY_TARGET_PROFILING_SUPPORTED
Expand Down
11 changes: 11 additions & 0 deletions Sources/Sentry/SentrySessionTracker.m
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@
#import "SentrySDK+Private.h"
#import "SentrySwift.h"

#import "SentryProfilingConditionals.h"
#if SENTRY_TARGET_PROFILING_SUPPORTED
# import "SentryProfiler+Private.h"
#endif // SENTRY_TARGET_PROFILING_SUPPORTED

#if SENTRY_TARGET_MACOS_HAS_UI
# import <Cocoa/Cocoa.h>
#endif
Expand Down Expand Up @@ -174,6 +179,12 @@ - (void)didBecomeActive
}
[[[hub getClient] fileManager] deleteTimestampLastInForeground];
self.lastInForeground = nil;

#if SENTRY_TARGET_PROFILING_SUPPORTED
if (hub.client.options.profiling != nil) {
sentry_reevaluateSessionSampleRate(hub.client.options.profiling.sessionSampleRate);
}
#endif // SENTRY_TARGET_PROFILING_SUPPORTED
}

/**
Expand Down
1 change: 1 addition & 0 deletions Sources/Sentry/include/SentryPrivate.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#import "SentryLevelHelper.h"
#import "SentryLogC.h"
#import "SentryMeta.h"
#import "SentryProfiler+Private.h"
#import "SentryRandom.h"
#import "SentryScreenshot.h"
#import "SentrySdkInfo.h"
Expand Down
2 changes: 2 additions & 0 deletions Sources/Sentry/include/SentryProfiler+Private.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ SENTRY_EXTERN void sentry_sdkInitProfilerTasks(SentryOptions *options, SentryHub
*/
SENTRY_EXTERN SentrySamplerDecision *_Nullable sentry_profilerSessionSampleDecision;

SENTRY_EXTERN void sentry_reevaluateSessionSampleRate(float sessionSampleRate);

/**
* A wrapper around the low-level components used to gather sampled backtrace profiles.
* @warning A main assumption is that profile start/stop must be contained within range of time of
Expand Down
2 changes: 1 addition & 1 deletion Sources/Sentry/include/SentrySampling.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ SENTRY_EXTERN SentrySamplerDecision *sentry_sampleTrace(
SENTRY_EXTERN SentrySamplerDecision *sentry_sampleTraceProfile(SentrySamplingContext *context,
SentrySamplerDecision *tracesSamplerDecision, SentryOptions *options);

SENTRY_EXTERN SentrySamplerDecision *sentry_sampleProfileSession(SentryOptions *options);
SENTRY_EXTERN SentrySamplerDecision *sentry_sampleProfileSession(float sessionSampleRate);
#endif // SENTRY_TARGET_PROFILING_SUPPORTED

NS_ASSUME_NONNULL_END
25 changes: 16 additions & 9 deletions Sources/Swift/Integrations/Performance/SentryProfileOptions.swift
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,18 @@ import Foundation

/// An object containing configuration for the Sentry profiler.
/// - warning: Continuous profiling is an experimental feature and may still contain bugs.
/// - note: If either `SentryOptions.profilesSampleRate` or `SentryOptions.profilesSampler` are
/// set to a non-nil value such that transaction-based profiling is being used, these settings
/// will have no effect, nor will `SentrySDK.startProfileSession()` or
/// `SentrySDK.stopProfileSession()`.
/// - note: Profiling is automatically disabled if a thread sanitizer is attached.
@objcMembers
public class SentryProfileOptions: NSObject {
/// Different modes for starting and stopping the profiler.
@objc public enum SentryProfileLifecycle: Int {
/// Profiling is controlled manually, and is independent of transactions & spans. Developers
/// must `SentrySDK.startProfileSession()` and `SentrySDK.stopProfileSession()` to control
/// the lifecycle of the profiler. If the session is sampled,
/// must use`SentrySDK.startProfileSession()` and `SentrySDK.stopProfileSession()` to
/// manage the profile session. If the session is sampled,
/// `SentrySDK.startProfileSession()` will always start profiling.
/// - warning: Continuous profiling is an experimental feature and may still contain bugs.
/// - note: Profiling is automatically disabled if a thread sanitizer is attached.
Expand All @@ -27,6 +31,10 @@ public class SentryProfileOptions: NSObject {
/// - note: If there are multiple overlapping root spans, where some are sampled and some or
/// not, profiling will continue until the end of the last sampled root span. Profiling data
/// will not be linked with spans that are not sampled.
/// - note: When the last root span finishes, the profiler will continue running until the
/// end of the current timed interval. If a new root span starts before this interval
/// completes, the profiler will instead continue running until the next root span stops, at
/// which time it will attempt to stop again in the same way.
/// - note: Profiling is automatically disabled if a thread sanitizer is attached.
case trace
}
Expand All @@ -38,13 +46,12 @@ public class SentryProfileOptions: NSObject {
public var lifecycle: SentryProfileLifecycle = .manual

/// The % of user sessions in which to enable profiling.
/// - warning: Continuous profiling is an experimental feature and may still contain bugs.
/// - note: Whether or not the session is sampled is determined once, when the SDK is initially
/// configured.
/// - note: If either `SentryOptions.profilesSampleRate` or `SentryOptions.profilesSampler` are
/// set to a non-nil value such that transaction-based profiling is being used, then setting
/// this property has no effect, and neither do `SentrySDK.startProfileSession()` or
/// `SentrySDK.stopProfileSession()`.
/// - warning: Continuous profiling is an experimental feature and may still contain bugs.
/// - note: The decision whether or not to sample profiles is computed using this sample rate
/// when the SDK is started, and applies to any requests to start the profiler–regardless of
/// `lifecycle`– until the app resigns its active status. It is then reevaluated on subsequent
/// foreground events. The duration of time that a sample decision prevails between
/// launch/foreground and background is referred to as a profile session.
/// - note: Backgrounding and foregrounding the app starts a new user session and sampling is
/// re-evaluated. If there is no active trace when the app is backgrounded, profiling stops
/// before the app backgrounds. If there is an active trace and profiling is in-flight when the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,14 @@ final class SentryContinuousProfilerTests: XCTestCase {
SentrySDK.close()
try assertContinuousProfileStoppage()
}


func testBackgroundNotificationStopsProfile() {
SentryContinuousProfiler.start()
XCTAssert(SentryContinuousProfiler.isCurrentlyProfiling())
fixture.notificationCenter.post(Notification(name: UIApplication.willResignActiveNotification))
XCTAssertFalse(SentryContinuousProfiler.isCurrentlyProfiling())
}

func testStartingAPerformanceTransactionDoesNotStartProfiler() throws {
let manualSpan = try fixture.newTransaction()
XCTAssertFalse(SentryContinuousProfiler.isCurrentlyProfiling())
Expand Down
46 changes: 44 additions & 2 deletions Tests/SentryProfilerTests/SentryProfilingPublicAPITests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,19 @@ class SentryProfilingPublicAPITests: XCTestCase {
return scope
}()

let random = TestRandom(value: 0.5)
var sessionTracker: SessionTracker?

var _random: TestRandom = TestRandom(value: 0.5)
var random: TestRandom {
get {
_random
}
set(newValue) {
_random = newValue
SentryDependencyContainer.sharedInstance().random = newValue
}
}

let currentDate = TestCurrentDateProvider()
lazy var timerFactory = TestSentryNSTimerFactory(currentDateProvider: currentDate)
lazy var client = TestClient(options: options)!
Expand All @@ -31,7 +43,6 @@ class SentryProfilingPublicAPITests: XCTestCase {

override func setUp() {
super.setUp()
SentryDependencyContainer.sharedInstance().random = fixture.random
SentryDependencyContainer.sharedInstance().timerFactory = fixture.timerFactory
SentryDependencyContainer.sharedInstance().dateProvider = fixture.currentDate
}
Expand Down Expand Up @@ -401,6 +412,37 @@ extension SentryProfilingPublicAPITests {
// Assert
XCTAssertFalse(SentryContinuousProfiler.isCurrentlyProfiling())
}

func testSessionSampleRateReevaluationOnAppBecomingActive() {
// Arrange
fixture.options.profiling.sessionSampleRate = 0.5
fixture.options.profiling.lifecycle = .manual
fixture.random = TestRandom(value: 0)
let nc = SentryDependencyContainer.sharedInstance().notificationCenterWrapper
fixture.sessionTracker = SessionTracker(options: fixture.options, notificationCenter: nc)
fixture.sessionTracker?.start()
givenSdkWithHub()

// Act
SentrySDK.startProfileSession()

// Assert
XCTAssert(SentryContinuousProfiler.isCurrentlyProfiling())

// Act
nc.post(Notification(name: UIApplication.willResignActiveNotification))
XCTAssertFalse(SentryContinuousProfiler.isCurrentlyProfiling())

// Arrange
fixture.random = TestRandom(value: 1)

// Act
nc.post(Notification(name: UIApplication.didBecomeActiveNotification))
SentrySDK.startProfileSession()

// Assert
XCTAssertFalse(SentryContinuousProfiler.isCurrentlyProfiling())
}
}

private extension SentryProfilingPublicAPITests {
Expand Down