Skip to content
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ SentryAndroid.init(
```

</details>
- Android: Flush logs when app enters background ([#4873](https://github.com/getsentry/sentry-java/pull/4873))
Copy link
Member

Choose a reason for hiding this comment

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

maybe this should go above the previous entry, because it doesn't look right when rendered:
image



### Improvements
Expand Down
12 changes: 12 additions & 0 deletions sentry-android-core/api/sentry-android-core.api
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,18 @@ public final class io/sentry/android/core/AndroidLogger : io/sentry/ILogger {
public fun log (Lio/sentry/SentryLevel;Ljava/lang/Throwable;Ljava/lang/String;[Ljava/lang/Object;)V
}

public class io/sentry/android/core/AndroidLoggerApi : io/sentry/logger/LoggerApi, io/sentry/android/core/AppState$AppStateListener, java/io/Closeable {
public fun <init> (Lio/sentry/Scopes;)V
public fun close ()V
public fun onBackground ()V
public fun onForeground ()V
}

public final class io/sentry/android/core/AndroidLoggerApiFactory : io/sentry/logger/ILoggerApiFactory {
public fun <init> ()V
public fun create (Lio/sentry/Scopes;)Lio/sentry/logger/LoggerApi;
}

public class io/sentry/android/core/AndroidMemoryCollector : io/sentry/IPerformanceSnapshotCollector {
public fun <init> ()V
public fun collect (Lio/sentry/PerformanceCollectionData;)V
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
package io.sentry.android.core;

import io.sentry.Scopes;
import io.sentry.SentryLevel;
import io.sentry.logger.LoggerApi;
import io.sentry.logger.LoggerBatchProcessor;
import java.io.Closeable;
import java.io.IOException;
import org.jetbrains.annotations.ApiStatus;
import org.jetbrains.annotations.NotNull;

@ApiStatus.Internal
public class AndroidLoggerApi extends LoggerApi implements Closeable, AppState.AppStateListener {

public AndroidLoggerApi(final @NotNull Scopes scopes) {
super(scopes);
AppState.getInstance().addAppStateListener(this);
}
Copy link

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)

Fix in Cursor Fix in Web

Copy link
Member Author

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()


@Override
@ApiStatus.Internal
public void close() throws IOException {
AppState.getInstance().removeAppStateListener(this);
}

@Override
public void onForeground() {}

@Override
public void onBackground() {
try {
scopes
.getOptions()
.getExecutorService()
.submit(
new Runnable() {
@Override
public void run() {
scopes.getClient().flushLogs(LoggerBatchProcessor.FLUSH_AFTER_MS);
}
});
} catch (Throwable t) {
scopes
.getOptions()
.getLogger()
.log(SentryLevel.ERROR, t, "Failed to submit log flush runnable");
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package io.sentry.android.core;

import io.sentry.Scopes;
import io.sentry.logger.ILoggerApiFactory;
import io.sentry.logger.LoggerApi;
import org.jetbrains.annotations.ApiStatus;
import org.jetbrains.annotations.NotNull;

@ApiStatus.Internal
public final class AndroidLoggerApiFactory implements ILoggerApiFactory {

@Override
@NotNull
public LoggerApi create(@NotNull Scopes scopes) {
return new AndroidLoggerApi(scopes);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,8 @@ static void loadDefaultAndMetadataOptions(

readDefaultOptionValues(options, finalContext, buildInfoProvider);
AppState.getInstance().registerLifecycleObserver(options);

options.setLoggerApiFactory(new AndroidLoggerApiFactory());
}

@TestOnly
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@ public void register(final @NotNull IScopes scopes, final @NotNull SentryOptions
this.options.isEnableAppLifecycleBreadcrumbs());

if (this.options.isEnableAutoSessionTracking()
|| this.options.isEnableAppLifecycleBreadcrumbs()) {
|| this.options.isEnableAppLifecycleBreadcrumbs()
|| this.options.getLogs().isEnabled()) {
Copy link

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.

Fix in Cursor Fix in Web

try (final ISentryLifecycleToken ignored = lock.acquire()) {
if (watcher != null) {
return;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package io.sentry.android.core

import androidx.test.ext.junit.runners.AndroidJUnit4
import io.sentry.Scopes
import kotlin.test.assertIs
import org.junit.Test
import org.junit.runner.RunWith
import org.mockito.kotlin.mock

@RunWith(AndroidJUnit4::class)
class AndroidLoggerApiFactoryTest {

@Test
fun `factory creates AndroidLogger`() {
val factory = AndroidLoggerApiFactory()
val logger = factory.create(mock<Scopes>())
assertIs<AndroidLoggerApi>(logger)
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
package io.sentry.android.core

import androidx.test.ext.junit.runners.AndroidJUnit4
import io.sentry.Scopes
import io.sentry.SentryClient
import io.sentry.SentryOptions
import io.sentry.test.ImmediateExecutorService
import kotlin.test.assertTrue
import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
import org.mockito.kotlin.any
import org.mockito.kotlin.mock
import org.mockito.kotlin.verify
import org.mockito.kotlin.whenever

@RunWith(AndroidJUnit4::class)
class AndroidLoggerApiTest {

@Before
fun setup() {
AppState.getInstance().resetInstance()
}

@Test
fun `AndroidLogger registers and unregisters app state listener`() {
val scopes = mock<Scopes>()
val logger = AndroidLoggerApi(scopes)
assertTrue(AppState.getInstance().lifecycleObserver.listeners.isNotEmpty())

logger.close()
assertTrue(AppState.getInstance().lifecycleObserver.listeners.isEmpty())
}

@Test
fun `AndroidLogger triggers flushing if app goes in background`() {
val scopes = mock<Scopes>()

val client = mock<SentryClient>()
whenever(scopes.client).thenReturn(client)

val options = SentryOptions()
options.executorService = ImmediateExecutorService()
whenever(scopes.options).thenReturn(options)

val logger = AndroidLoggerApi(scopes)
logger.onBackground()

verify(client).flushLogs(any())
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -926,4 +926,10 @@ class AndroidOptionsInitializerTest {
fixture.initSut()
assertIs<AndroidRuntimeManager>(fixture.sentryOptions.runtimeManager)
}

@Test
fun `AndroidLoggerApiFactory is set in the options`() {
fixture.initSut()
assertIs<AndroidLoggerApiFactory>(fixture.sentryOptions.loggerApiFactory)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,14 @@ import io.sentry.DateUtils
import io.sentry.IContinuousProfiler
import io.sentry.IScope
import io.sentry.IScopes
import io.sentry.ISentryClient
import io.sentry.ReplayController
import io.sentry.ScopeCallback
import io.sentry.SentryLevel
import io.sentry.SentryOptions
import io.sentry.Session
import io.sentry.Session.State
import io.sentry.test.ImmediateExecutorService
import io.sentry.transport.ICurrentDateProvider
import kotlin.test.BeforeTest
import kotlin.test.Test
Expand All @@ -36,6 +38,8 @@ class LifecycleWatcherTest {
val replayController = mock<ReplayController>()
val continuousProfiler = mock<IContinuousProfiler>()

val client = mock<ISentryClient>()

fun getSUT(
sessionIntervalMillis: Long = 0L,
enableAutoSessionTracking: Boolean = true,
Expand All @@ -49,9 +53,13 @@ class LifecycleWatcherTest {
whenever(scopes.configureScope(argumentCaptor.capture())).thenAnswer {
argumentCaptor.value.run(scope)
}
whenever(scope.client).thenReturn(client)

options.setReplayController(replayController)
options.setContinuousProfiler(continuousProfiler)
options.executorService = ImmediateExecutorService()
whenever(scopes.options).thenReturn(options)
whenever(scopes.globalScope).thenReturn(scope)

return LifecycleWatcher(
scopes,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,10 @@ class SessionTrackingIntegrationTest {
TODO("Not yet implemented")
}

override fun flushLogs(timeoutMillis: Long) {
TODO("Not yet implemented")
}

override fun captureFeedback(feedback: Feedback, hint: Hint?, scope: IScope): SentryId {
TODO("Not yet implemented")
}
Expand Down
16 changes: 15 additions & 1 deletion sentry/api/sentry.api
Original file line number Diff line number Diff line change
Expand Up @@ -1053,6 +1053,7 @@ public abstract interface class io/sentry/ISentryClient {
public abstract fun close ()V
public abstract fun close (Z)V
public abstract fun flush (J)V
public abstract fun flushLogs (J)V
public abstract fun getRateLimiter ()Lio/sentry/transport/RateLimiter;
public abstract fun isEnabled ()Z
public fun isHealthy ()Z
Expand Down Expand Up @@ -2856,6 +2857,7 @@ public final class io/sentry/SentryClient : io/sentry/ISentryClient {
public fun close ()V
public fun close (Z)V
public fun flush (J)V
public fun flushLogs (J)V
public fun getRateLimiter ()Lio/sentry/transport/RateLimiter;
public fun isEnabled ()Z
public fun isHealthy ()Z
Expand Down Expand Up @@ -3407,6 +3409,7 @@ public class io/sentry/SentryOptions {
public fun getIntegrations ()Ljava/util/List;
public fun getInternalTracesSampler ()Lio/sentry/TracesSampler;
public fun getLogger ()Lio/sentry/ILogger;
public fun getLoggerApiFactory ()Lio/sentry/logger/ILoggerApiFactory;
public fun getLogs ()Lio/sentry/SentryOptions$Logs;
public fun getMaxAttachmentSize ()J
public fun getMaxBreadcrumbs ()I
Expand Down Expand Up @@ -3559,6 +3562,7 @@ public class io/sentry/SentryOptions {
public fun setInitPriority (Lio/sentry/InitPriority;)V
public fun setInstrumenter (Lio/sentry/Instrumenter;)V
public fun setLogger (Lio/sentry/ILogger;)V
public fun setLoggerApiFactory (Lio/sentry/logger/ILoggerApiFactory;)V
public fun setLogs (Lio/sentry/SentryOptions$Logs;)V
public fun setMaxAttachmentSize (J)V
public fun setMaxBreadcrumbs (I)V
Expand Down Expand Up @@ -5021,6 +5025,11 @@ public abstract interface class io/sentry/internal/viewhierarchy/ViewHierarchyEx
public abstract fun export (Lio/sentry/protocol/ViewHierarchyNode;Ljava/lang/Object;)Z
}

public final class io/sentry/logger/DefaultLoggerApiFactory : io/sentry/logger/ILoggerApiFactory {
public fun <init> ()V
public fun create (Lio/sentry/Scopes;)Lio/sentry/logger/LoggerApi;
}

public abstract interface class io/sentry/logger/ILoggerApi {
public abstract fun debug (Ljava/lang/String;[Ljava/lang/Object;)V
public abstract fun error (Ljava/lang/String;[Ljava/lang/Object;)V
Expand All @@ -5033,13 +5042,18 @@ public abstract interface class io/sentry/logger/ILoggerApi {
public abstract fun warn (Ljava/lang/String;[Ljava/lang/Object;)V
}

public abstract interface class io/sentry/logger/ILoggerApiFactory {
public abstract fun create (Lio/sentry/Scopes;)Lio/sentry/logger/LoggerApi;
}

public abstract interface class io/sentry/logger/ILoggerBatchProcessor {
public abstract fun add (Lio/sentry/SentryLogEvent;)V
public abstract fun close (Z)V
public abstract fun flush (J)V
}

public final class io/sentry/logger/LoggerApi : io/sentry/logger/ILoggerApi {
public class io/sentry/logger/LoggerApi : io/sentry/logger/ILoggerApi {
protected final field scopes Lio/sentry/Scopes;
public fun <init> (Lio/sentry/Scopes;)V
public fun debug (Ljava/lang/String;[Ljava/lang/Object;)V
public fun error (Ljava/lang/String;[Ljava/lang/Object;)V
Expand Down
2 changes: 2 additions & 0 deletions sentry/src/main/java/io/sentry/ISentryClient.java
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ public interface ISentryClient {
*/
void flush(long timeoutMillis);

void flushLogs(long timeoutMillis);
Copy link
Member Author

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

Copy link
Member

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


/**
* Captures the event.
*
Expand Down
3 changes: 3 additions & 0 deletions sentry/src/main/java/io/sentry/NoOpSentryClient.java
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ public void close() {}
@Override
public void flush(long timeoutMillis) {}

@Override
public void flushLogs(long timeoutMillis) {}

@Override
public @NotNull SentryId captureFeedback(
@NotNull Feedback feedback, @Nullable Hint hint, @NotNull IScope scope) {
Expand Down
7 changes: 5 additions & 2 deletions sentry/src/main/java/io/sentry/Scopes.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
import io.sentry.hints.SessionEndHint;
import io.sentry.hints.SessionStartHint;
import io.sentry.logger.ILoggerApi;
import io.sentry.logger.LoggerApi;
import io.sentry.protocol.*;
import io.sentry.transport.RateLimiter;
import io.sentry.util.HintUtils;
Expand Down Expand Up @@ -56,7 +55,7 @@ private Scopes(
final @NotNull SentryOptions options = getOptions();
validateOptions(options);
this.compositePerformanceCollector = options.getCompositePerformanceCollector();
this.logger = new LoggerApi(this);
this.logger = options.getLoggerApiFactory().create(this);
}

public @NotNull String getCreator() {
Expand Down Expand Up @@ -463,6 +462,10 @@ public void close(final boolean isRestarting) {
executorService.close(getOptions().getShutdownTimeoutMillis());
}

if (logger instanceof Closeable) {
((Closeable) logger).close();
}

// TODO: should we end session before closing client?
configureScope(ScopeType.CURRENT, scope -> scope.getClient().close(isRestarting));
configureScope(ScopeType.ISOLATION, scope -> scope.getClient().close(isRestarting));
Expand Down
7 changes: 6 additions & 1 deletion sentry/src/main/java/io/sentry/SentryClient.java
Original file line number Diff line number Diff line change
Expand Up @@ -1563,10 +1563,15 @@ public void close(final boolean isRestarting) {

@Override
public void flush(final long timeoutMillis) {
loggerBatchProcessor.flush(timeoutMillis);
flushLogs(timeoutMillis);
transport.flush(timeoutMillis);
}

@Override
public void flushLogs(final long timeoutMillis) {
loggerBatchProcessor.flush(timeoutMillis);
}

@Override
public @Nullable RateLimiter getRateLimiter() {
return transport.getRateLimiter();
Expand Down
Loading
Loading