-
-
Notifications
You must be signed in to change notification settings - Fork 463
feat(android): Add log flushing on app backgrounding #4873
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
base: main
Are you sure you want to change the base?
Conversation
Add flushLogs() method to ISentryClient interface and implement it in SentryClient to allow log processors to flush any buffered logs when the app transitions to the background. This is integrated into the lifecycle watcher and can be enabled via an opt-in SentryOptions flag. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/MainActivity.java
Outdated
Show resolved
Hide resolved
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| f634d01 | 359.58 ms | 433.88 ms | 74.30 ms |
| 9fbb112 | 401.87 ms | 515.87 ms | 114.00 ms |
| d5a29b6 | 298.62 ms | 391.78 ms | 93.16 ms |
| bbc35bb | 298.53 ms | 372.17 ms | 73.64 ms |
| b3d8889 | 420.46 ms | 453.71 ms | 33.26 ms |
| a5ab36f | 320.47 ms | 389.77 ms | 69.30 ms |
| ee747ae | 405.43 ms | 485.70 ms | 80.28 ms |
| 951caf7 | 323.66 ms | 392.82 ms | 69.16 ms |
| b6702b0 | 395.86 ms | 409.98 ms | 14.12 ms |
| 27d7cf8 | 369.82 ms | 422.62 ms | 52.80 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| f634d01 | 1.58 MiB | 2.10 MiB | 533.40 KiB |
| 9fbb112 | 1.58 MiB | 2.11 MiB | 539.18 KiB |
| d5a29b6 | 1.58 MiB | 2.12 MiB | 549.37 KiB |
| bbc35bb | 1.58 MiB | 2.12 MiB | 553.01 KiB |
| b3d8889 | 1.58 MiB | 2.10 MiB | 535.07 KiB |
| a5ab36f | 1.58 MiB | 2.12 MiB | 555.26 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| 951caf7 | 1.58 MiB | 2.13 MiB | 558.77 KiB |
| b6702b0 | 1.58 MiB | 2.12 MiB | 551.79 KiB |
| 27d7cf8 | 1.58 MiB | 2.12 MiB | 549.42 KiB |
Previous results on branch: feat/android-app-lifecycle-log-flushing
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 3a939dd | 310.96 ms | 361.33 ms | 50.37 ms |
| 1d7703b | 318.98 ms | 405.18 ms | 86.20 ms |
| 294dba2 | 334.12 ms | 406.31 ms | 72.19 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 3a939dd | 1.58 MiB | 2.13 MiB | 557.55 KiB |
| 1d7703b | 1.58 MiB | 2.12 MiB | 551.96 KiB |
| 294dba2 | 1.58 MiB | 2.13 MiB | 559.15 KiB |
| */ | ||
| void flush(long timeoutMillis); | ||
|
|
||
| void flushLogs(long timeoutMillis); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not the biggest fan of adding a new API here, an alternative would be to move this directly into ILoggerApi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we're not going for https://github.com/getsentry/sentry-java/pull/4873/files#r2581483713, then I think this makes sense to make it cleaner and bake it into ILoggerApi instead
|
Isn't log processor flushing every 5 seconds anyway? |
The spec mentions this:
|
| ``` | ||
|
|
||
| </details> | ||
| - Android: Flush logs when app enters background ([#4873](https://github.com/getsentry/sentry-java/pull/4873)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
| addAppBreadcrumb("background"); | ||
|
|
||
| if (enableLogFlushing) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, wondering if it's a good idea to bind everything to LifecycleWatcher.. Since we now can addAppStateListener from anywhere, wdyt of introducing an AndroidLoggerApi which delegates everything to LoggerApi, but in addition subscribes for onBackground/onForeground to trigger logs flush? And then we override the Java LoggerApi via Options as always
romtsn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a couple of suggestion, but not gonna block
| @Override | ||
| public void run() { | ||
| scopes | ||
| .getGlobalScope() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m in theory there could be a different SentryClient instance on each scope type but that's probably more of an edge case. Romans suggestion above should get rid of that problem tho.
As an alternative, on Scopes there's already an implementation:
@ApiStatus.Internal
@NotNull
public ISentryClient getClient() {
return getCombinedScopeView().getClient();
}
We'd just have to expose that on IScopes.
| public AndroidLoggerApi(final @NotNull Scopes scopes) { | ||
| super(scopes); | ||
| AppState.getInstance().addAppStateListener(this); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Forked scopes accumulate listeners causing memory leak
Every Scopes instance creates a new AndroidLoggerApi that registers itself as an AppStateListener. When scopes are forked via forkedScopes(), forkedCurrentScope(), pushScope(), withScope(), etc., each forked scope creates its own AndroidLoggerApi listener. However, forked scopes are never explicitly closed - their lifecycle tokens only restore the previous scope in the ThreadLocal. The close() method that removes the listener is only called on the root scopes during SDK shutdown. This causes unbounded listener accumulation over the app's lifetime, resulting in a memory leak and duplicate flushLogs() calls on every background transition.
Additional Locations (1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's a valid point, although not a concern in globalhub mode. @romtsn I'm wondering if we should completely decouple the flush-triggering logic from logger(api), as the changes blew up quite a bit. We could move it to SentryAndroid.init()
| if (this.options.isEnableAutoSessionTracking() | ||
| || this.options.isEnableAppLifecycleBreadcrumbs()) { | ||
| || this.options.isEnableAppLifecycleBreadcrumbs() | ||
| || this.options.getLogs().isEnabled()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Unnecessary LifecycleWatcher created when only logs enabled
The condition || this.options.getLogs().isEnabled() was added but the LifecycleWatcher doesn't handle log flushing - that's done by AndroidLoggerApi. When only logs are enabled (not session tracking or breadcrumbs), this creates an unnecessary LifecycleWatcher with a Timer that performs no useful work since both enableSessionTracking and enableAppLifecycleBreadcrumbs will be false.

Summary
Adds log flushing functionality that triggers when the Android app transitions to the background.