Skip to content

Commit

Permalink
ref(continuous profiling): clarify initial options values (#4216)
Browse files Browse the repository at this point in the history
  • Loading branch information
armcknight committed Aug 2, 2024
1 parent cc31630 commit 6d00d1b
Show file tree
Hide file tree
Showing 4 changed files with 67 additions and 5 deletions.
2 changes: 1 addition & 1 deletion Sources/Sentry/SentryOptions.m
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ - (instancetype)init
self.tracesSampleRate = nil;
#if SENTRY_TARGET_PROFILING_SUPPORTED
_enableProfiling = NO;
self.profilesSampleRate = nil;
self.profilesSampleRate = SENTRY_INITIAL_PROFILES_SAMPLE_RATE;
#endif // SENTRY_TARGET_PROFILING_SUPPORTED
self.enableCoreDataTracing = YES;
_enableSwizzling = YES;
Expand Down
12 changes: 8 additions & 4 deletions Sources/Sentry/SentrySDK.m
Original file line number Diff line number Diff line change
Expand Up @@ -546,8 +546,10 @@ + (void)startProfiler
{
if (![currentHub.client.options isContinuousProfilingEnabled]) {
SENTRY_LOG_WARN(
@"You must disable trace profiling by setting SentryOptions.profilesSampleRate to nil "
@"or 0 before using continuous profiling features.");
@"You must disable trace profiling by setting SentryOptions.profilesSampleRate and "
@"SentryOptions.profilesSampler to nil (which is the default initial value for both "
@"properties, so you can also just remove those lines from your configuration "
@"altogether) before attempting to start a continuous profiling session.");
return;
}

Expand All @@ -558,8 +560,10 @@ + (void)stopProfiler
{
if (![currentHub.client.options isContinuousProfilingEnabled]) {
SENTRY_LOG_WARN(
@"You must disable trace profiling by setting SentryOptions.profilesSampleRate to nil "
@"or 0 before using continuous profiling features.");
@"You must disable trace profiling by setting SentryOptions.profilesSampleRate and "
@"SentryOptions.profilesSampler to nil (which is the default initial value for both "
@"properties, so you can also just remove those lines from your configuration "
@"altogether) before attempting to stop a continuous profiling session.");
return;
}

Expand Down
11 changes: 11 additions & 0 deletions Sources/Sentry/include/SentryInternalDefines.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,17 @@ static NSString *const SentryPlatformName = @"cocoa";

#define SENTRY_DEFAULT_SAMPLE_RATE @1
#define SENTRY_DEFAULT_TRACES_SAMPLE_RATE @0

/**
* The value we give when initializing the options object, and what it will be if a consumer never
* modifies it in their SDK config.
* */
#define SENTRY_INITIAL_PROFILES_SAMPLE_RATE nil

/**
* The default value we will give for profiles sample rate if an invalid value is supplied for the
* options property in config or returned from the sampler function.
*/
#define SENTRY_DEFAULT_PROFILES_SAMPLE_RATE @0

/**
Expand Down
47 changes: 47 additions & 0 deletions Tests/SentryTests/SentryOptionsTest.m
Original file line number Diff line number Diff line change
Expand Up @@ -1141,7 +1141,11 @@ - (void)testDefaultProfilesSampleRate
{
SentryOptions *options = [self getValidOptions:@{}];
XCTAssertNil(options.profilesSampleRate);

// This property now only refers to trace-based profiling, but renaming it would require a major
// rev
XCTAssertFalse(options.isProfilingEnabled);

XCTAssert([options isContinuousProfilingEnabled]);
}

Expand All @@ -1150,7 +1154,11 @@ - (void)testProfilesSampleRate_SetToNil
SentryOptions *options = [[SentryOptions alloc] init];
options.profilesSampleRate = nil;
XCTAssertNil(options.profilesSampleRate);

// This property now only refers to trace-based profiling, but renaming it would require a major
// rev
XCTAssertFalse(options.isProfilingEnabled);

XCTAssert([options isContinuousProfilingEnabled]);
}

Expand Down Expand Up @@ -1193,7 +1201,11 @@ - (void)testProfilesSampleRateUpperBound
- (void)testIsProfilingEnabled_NothingSet_IsDisabled
{
SentryOptions *options = [[SentryOptions alloc] init];

// This property now only refers to trace-based profiling, but renaming it would require a major
// rev
XCTAssertFalse(options.isProfilingEnabled);

XCTAssertNil(options.profilesSampleRate);
XCTAssert([options isContinuousProfilingEnabled]);
}
Expand All @@ -1202,7 +1214,11 @@ - (void)testIsProfilingEnabled_ProfilesSampleRateSetToZero_IsDisabled
{
SentryOptions *options = [[SentryOptions alloc] init];
options.profilesSampleRate = @0.00;

// This property now only refers to trace-based profiling, but renaming it would require a major
// rev
XCTAssertFalse(options.isProfilingEnabled);

XCTAssertNotNil(options.profilesSampleRate);
XCTAssertFalse([options isContinuousProfilingEnabled]);
}
Expand All @@ -1211,7 +1227,11 @@ - (void)testIsProfilingEnabled_ProfilesSampleRateSet_IsEnabled
{
SentryOptions *options = [[SentryOptions alloc] init];
options.profilesSampleRate = @0.01;

// This property now only refers to trace-based profiling, but renaming it would require a major
// rev
XCTAssertTrue(options.isProfilingEnabled);

XCTAssertNotNil(options.profilesSampleRate);
XCTAssertFalse([options isContinuousProfilingEnabled]);
}
Expand All @@ -1223,7 +1243,11 @@ - (void)testIsProfilingEnabled_ProfilesSamplerSet_IsEnabled
XCTAssertNotNil(context);
return @0.0;
};

// This property now only refers to trace-based profiling, but renaming it would require a major
// rev
XCTAssertTrue(options.isProfilingEnabled);

XCTAssertNil(options.profilesSampleRate);
XCTAssertFalse([options isContinuousProfilingEnabled]);
}
Expand All @@ -1235,7 +1259,11 @@ - (void)testIsProfilingEnabled_EnableProfilingSet_IsEnabled
# pragma clang diagnostic ignored "-Wdeprecated-declarations"
options.enableProfiling = YES;
# pragma clang diagnostic pop

// This property now only refers to trace-based profiling, but renaming it would require a major
// rev
XCTAssertTrue(options.isProfilingEnabled);

XCTAssertNil(options.profilesSampleRate);
XCTAssertFalse([options isContinuousProfilingEnabled]);
}
Expand All @@ -1255,6 +1283,25 @@ - (void)testProfilesSampler
XCTAssertFalse([options isContinuousProfilingEnabled]);
}

// this is a tricky part of the API, because while a profilesSampleRate of nil enables continuous
// profiling, just having the profilesSampler set at all disables it, even if the sampler function
// would return nil
- (void)testProfilesSamplerReturnsNil_ContinuousProfilingNotEnabled
{
SentryTracesSamplerCallback sampler = ^(SentrySamplingContext *context) {
XCTAssertNotNil(context);
NSNumber *result = nil;
return result;
};

SentryOptions *options = [self getValidOptions:@{ @"profilesSampler" : sampler }];

SentrySamplingContext *context = [[SentrySamplingContext alloc] init];
XCTAssertNil(options.profilesSampler(context));
XCTAssertNil(options.profilesSampleRate);
XCTAssertFalse([options isContinuousProfilingEnabled]);
}

- (void)testDefaultProfilesSampler
{
SentryOptions *options = [self getValidOptions:@{}];
Expand Down

0 comments on commit 6d00d1b

Please sign in to comment.