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

test(profiling): fix broken tests for new API #4998

Open
wants to merge 3 commits into
base: armcknight/profiling/new-continuous-apis/reevaluate-sample-rate-on-fg
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
15 changes: 11 additions & 4 deletions Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm
Original file line number Diff line number Diff line change
Expand Up @@ -80,12 +80,17 @@
void
_unsafe_cleanUpContinuousTraceProfiler()
{
if (SENTRY_CASSERT_RETURN(_gInFlightRootSpans > 0,
@"Attempted to decrement count of root spans to less than zero.")) {
_gInFlightRootSpans -= 1;
_gInFlightRootSpans -= 1;

if (_gInFlightRootSpans < 0) {
# if SENTRY_TEST || SENTRY_TEST_CI

Check warning on line 86 in Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm

View check run for this annotation

Codecov / codecov/patch

Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm#L86

Added line #L86 was not covered by tests
SENTRY_CASSERT(NO, @"Attempted to decrement count of root spans to less than zero.");
# else
SENTRY_LOG_DEBUG(@"Attempted to decrement count of root spans to less than zero.");
# endif // SENTRY_TEST || SENTRY_TEST_CI
}

if (_gInFlightRootSpans == 0) {
if (_gInFlightRootSpans <= 0) {
SENTRY_LOG_DEBUG(@"Last root span ended, stopping profiler.");
[SentryContinuousProfiler stop];
} else {
Expand Down Expand Up @@ -157,6 +162,8 @@
return;
}
if (!traceSampled) {
SENTRY_LOG_DEBUG(@"The trace associated with the profiler was not sampled, so the "
@"profiler was never started and there is nothing to discard.");
return;
}
_unsafe_cleanUpContinuousTraceProfiler();
Expand Down
4 changes: 4 additions & 0 deletions Sources/Sentry/include/SentryProfiledTracerConcurrency.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ void sentry_captureTransactionWithProfile(SentryHub *hub, SentryDispatchQueueWra
SentryId *_Nullable sentry_startProfiler(SentryTracerConfiguration *configuration, SentryHub *hub,
SentryTransactionContext *transactionContext);

/**
* @note Only called for transaction-based profiling or continuous profiling V2 with trace lifecycle
* option configured.
*/
SENTRY_EXTERN void sentry_stopProfilerDueToFinishedTransaction(
SentryHub *hub, SentryDispatchQueueWrapper *dispatchQueue, SentryTransaction *transaction,
BOOL isProfiling, NSDate *traceStartTimestamp, uint64_t startSystemTime
Expand Down
18 changes: 11 additions & 7 deletions Tests/SentryProfilerTests/SentryProfilingPublicAPITests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,9 @@ class SentryProfilingPublicAPITests: XCTestCase {
}
}

// MARK: continuous profiling v1
extension SentryProfilingPublicAPITests {
func testStartingContinuousProfilerWithSampleRateZero() throws {
func testStartingContinuousProfilerV1WithSampleRateZero() throws {
givenSdkWithHub()

fixture.options.profilesSampleRate = 0
Expand All @@ -74,7 +75,7 @@ extension SentryProfilingPublicAPITests {
XCTAssertFalse(SentryContinuousProfiler.isCurrentlyProfiling())
}

func testStartingContinuousProfilerWithSampleRateNil() throws {
func testStartingContinuousProfilerV1WithSampleRateNil() throws {
givenSdkWithHub()

// nil is the default initial value for profilesSampleRate, so we don't have to explicitly set it on the fixture
Expand All @@ -86,7 +87,7 @@ extension SentryProfilingPublicAPITests {
try stopProfiler()
}

func testNotStartingContinuousProfilerWithSampleRateBlock() throws {
func testNotStartingContinuousProfilerV1WithSampleRateBlock() throws {
givenSdkWithHub()

fixture.options.profilesSampler = { _ in 0 }
Expand All @@ -95,7 +96,7 @@ extension SentryProfilingPublicAPITests {
XCTAssertFalse(SentryContinuousProfiler.isCurrentlyProfiling())
}

func testNotStartingContinuousProfilerWithSampleRateNonZero() throws {
func testNotStartingContinuousProfilerV1WithSampleRateNonZero() throws {
givenSdkWithHub()

fixture.options.profilesSampleRate = 1
Expand All @@ -104,7 +105,7 @@ extension SentryProfilingPublicAPITests {
XCTAssertFalse(SentryContinuousProfiler.isCurrentlyProfiling())
}

func testStartingAndStoppingContinuousProfiler() throws {
func testStartingAndStoppingContinuousProfilerV1() throws {
givenSdkWithHub()
SentrySDK.startProfiler()
XCTAssert(SentryContinuousProfiler.isCurrentlyProfiling())
Expand All @@ -114,18 +115,21 @@ extension SentryProfilingPublicAPITests {
XCTAssertFalse(SentryContinuousProfiler.isCurrentlyProfiling())
}

func testStartingContinuousProfilerBeforeStartingSDK() {
func testStartingContinuousProfilerV1BeforeStartingSDK() {
SentrySDK.startProfiler()
XCTAssertFalse(SentryContinuousProfiler.isCurrentlyProfiling())
}

func testStartingContinuousProfilerAfterStoppingSDK() {
func testStartingContinuousProfilerV1AfterStoppingSDK() {
givenSdkWithHub()
SentrySDK.close()
SentrySDK.startProfiler()
XCTAssertFalse(SentryContinuousProfiler.isCurrentlyProfiling())
}
}

// MARK: continuous profiling v2
extension SentryProfilingPublicAPITests {
func testManuallyStartingAndStoppingContinuousProfilerV2Sampled() throws {
// Arrange
fixture.options.profiling.sessionSampleRate = 1
Expand Down
Loading