Skip to content

Conversation

@denrase
Copy link
Collaborator

@denrase denrase commented Nov 26, 2025

📜 Description

  • Call captureLogs on batcher if app terminates
  • Call captureLogs on batcher if app resigns active

Introduces listeners on the AppState and observes it in a conditional (UIKit) integration.

💡 Motivation and Context

Implements missing behaviour from batch processor spec.

When the application shuts down gracefully, the BatchProcessor SHOULD forward all data in memory to the transport. The transport SHOULD keep its existing behavior, which usually stores the data to disk as an envelope. It is not required to call a transport flush. This is mostly relevant for mobile SDKs already subscribed to these hooks, such as applicationWillTerminate on iOS.

When the application moves to the background, the BatchProcessor SHOULD forward all the data in memory to the transport and stop the timer. The transport SHOULD keep its existing behavior, which usually stores the data to disk as an envelope. It is not required to call the transport flush. This is mostly relevant for mobile SDKs.

Closes #6478

💚 How did you test it?

Added tests and tested manually.

📝 Checklist

You have to check all boxes before merging:

  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • I updated the wizard if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

@denrase denrase changed the title Flush Logs on Terminate or Resign Active State Flush Logs on WillTerminate or WillResignActive` App State Nov 26, 2025
@denrase denrase changed the title Flush Logs on WillTerminate or WillResignActive` App State Flush Logs on WillTerminate or WillResignActive App State Nov 26, 2025
@codecov
Copy link

codecov bot commented Nov 26, 2025

❌ 2 Tests Failed:

Tests completed Failed Passed Skipped
4000 2 3998 22
View the top 2 failed test(s) by shortest run time
SentryTests.PrivateSentrySDKOnlyTests::testProfilingStartAndCollect
Stack Traces | 0s run time
.../Tests/SentryTests/PrivateSentrySDKOnlyTests.swift:271 - XCTAssertGreaterThan failed: ("0") is not greater than ("0")
SentryTests.SentryFileManagerTests::testCreateDirectoryIfNotExists_successful_shouldNotLogError
Stack Traces | 0s run time
.../SentryTests/Helper/SentryFileManagerTests.swift:1056 - XCTAssertEqual failed: ("1") is not equal to ("0")

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

@denrase
Copy link
Collaborator Author

denrase commented Nov 26, 2025

The app state manager is only enabled for UIKit, but the notification-equivalents for terminate/active should also be available for macOS. Is this intended behaviour?

@denrase denrase added the ready-to-merge Use this label to trigger all PR workflows label Nov 26, 2025
@denrase denrase marked this pull request as ready for review November 26, 2025 10:25
@github-actions
Copy link
Contributor

github-actions bot commented Nov 26, 2025

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1215.20 ms 1241.04 ms 25.84 ms
Size 24.14 KiB 1.01 MiB 1015.57 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
fae97e5 1229.20 ms 1256.27 ms 27.06 ms
275e0a6 1222.16 ms 1258.07 ms 35.91 ms
bce9765 1229.42 ms 1243.49 ms 14.07 ms
0dd7283 1230.47 ms 1255.94 ms 25.47 ms
b074ba9 1236.52 ms 1248.75 ms 12.23 ms
701acf0 1230.24 ms 1255.60 ms 25.36 ms
095d886 1226.31 ms 1257.05 ms 30.73 ms
5cfc768 1220.74 ms 1245.06 ms 24.32 ms
67e8e3e 1220.08 ms 1229.23 ms 9.15 ms
66147d5 1234.45 ms 1268.45 ms 34.00 ms

App size

Revision Plain With Sentry Diff
fae97e5 23.75 KiB 912.37 KiB 888.62 KiB
275e0a6 24.15 KiB 1.01 MiB 1014.90 KiB
bce9765 23.74 KiB 874.06 KiB 850.32 KiB
0dd7283 23.74 KiB 997.28 KiB 973.53 KiB
b074ba9 23.74 KiB 976.79 KiB 953.05 KiB
701acf0 23.75 KiB 987.96 KiB 964.21 KiB
095d886 24.15 KiB 1.01 MiB 1014.91 KiB
5cfc768 23.75 KiB 850.73 KiB 826.98 KiB
67e8e3e 23.75 KiB 919.91 KiB 896.16 KiB
66147d5 23.74 KiB 1.01 MiB 1008.77 KiB

Previous results on branch: feat/flush-logs-on-app-state-change

Startup times

Revision Plain With Sentry Diff
5a96fbe 1223.24 ms 1239.82 ms 16.57 ms
6b6ef93 1192.06 ms 1212.08 ms 20.02 ms

App size

Revision Plain With Sentry Diff
5a96fbe 24.14 KiB 1.01 MiB 1015.46 KiB
6b6ef93 24.14 KiB 1.01 MiB 1015.51 KiB

#import "SentrySDK+Private.h"
#import "SentrySwift.h"

#if SENTRY_HAS_UIKIT
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Platform mismatch between integration and manager

The SentryLogFlushIntegration is conditionally compiled only for SENTRY_HAS_UIKIT platforms (iOS, tvOS, visionOS), but macOS also has equivalent notifications (NSApplication.willTerminateNotification and NSApplication.willResignActiveNotification). The feature won't work on macOS despite the platform having the necessary app lifecycle notifications, creating an inconsistent behavior across platforms. The SentryAppStateManager and its listener infrastructure also exclude macOS, preventing log flushing on app termination or resign on that platform.

Additional Locations (1)

Fix in Cursor Fix in Web

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this. I found a few high-level issues we need to address before I give this a closer look.


NS_ASSUME_NONNULL_BEGIN

@interface SentryLogFlushIntegration () <SentryAppStateListener>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

h: We can write integrations in Swift after merging #6862. I think we should already convert this integration to Swift then.

return NO;
}

[[[SentryDependencyContainer sharedInstance] appStateManager] addListener:self];
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 drove you to use the app state manager here? I would rather subscribe and also unsubscribe to the notifications willResignActiveNotification and willTerminateNotification as the SentrySessionTracker does. Then we don't need to modify the app state manager for this.


- (void)flushLogs
{
[self.logBatcher captureLogs];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m: If the logBatcher then actually flushes the logs, shouldn't we rename the method to flush?

Suggested change
[self.logBatcher captureLogs];
[self.logBatcher flush];

Comment on lines +118 to +124
// Guard against sync call deadlock when we are already on the dispatchQueue.queue.
if dispatchQueue.isCurrentQueue() {
performCaptureLogs()
} else {
dispatchQueue.dispatchSync { [weak self] in
self?.performCaptureLogs()
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m: Wouldn't always calling performCaptureLogs directly not be the same? Or is it because we need to run it on the serial dispatch queue? If yes, I think it would actually be better to do that check in the performCaptureLogs

But actually this seems a bit wrong to me. Maybe we should ensure that the LogBatcher gets its own queue that nobody else MUST use. Then we can be certain nobody else uses this queue, and we can safely call dispatchQueue.dispatchSync. Also when the global dispatchQueue of Sentry is very busy we have to wait until its finished to proceed here.

Suggested change
// Guard against sync call deadlock when we are already on the dispatchQueue.queue.
if dispatchQueue.isCurrentQueue() {
performCaptureLogs()
} else {
dispatchQueue.dispatchSync { [weak self] in
self?.performCaptureLogs()
}
performCaptureLogs()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-merge Use this label to trigger all PR workflows

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Logs: Batch Processor Minimize Data Loss

3 participants