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 @@ -4,6 +4,7 @@

### Features

- Add an explicit Logs opt-in to the Android Timber integration ([#5943](https://github.com/getsentry/sentry-java/pull/5943))
- Add an explicit Logs opt-in to the JUL handler ([#5942](https://github.com/getsentry/sentry-java/pull/5942))
- Add an explicit Logs opt-in to the Log4j2 appender ([#5941](https://github.com/getsentry/sentry-java/pull/5941))
- Add an explicit Logs opt-in to the Logback appender ([#5940](https://github.com/getsentry/sentry-java/pull/5940))
Expand Down
2 changes: 2 additions & 0 deletions sentry-android-core/api/sentry-android-core.api
Original file line number Diff line number Diff line change
Expand Up @@ -439,6 +439,7 @@ public final class io/sentry/android/core/SentryAndroidOptions : io/sentry/Sentr
public fun isEnableStandaloneAppStartTracing ()Z
public fun isEnableSystemEventBreadcrumbs ()Z
public fun isEnableSystemEventBreadcrumbsExtras ()Z
public fun isEnableTimberLogs ()Z
public fun isReportHistoricalAnrs ()Z
public fun isReportHistoricalTombstones ()Z
public fun isTombstoneEnabled ()Z
Expand Down Expand Up @@ -472,6 +473,7 @@ public final class io/sentry/android/core/SentryAndroidOptions : io/sentry/Sentr
public fun setEnableStandaloneAppStartTracing (Z)V
public fun setEnableSystemEventBreadcrumbs (Z)V
public fun setEnableSystemEventBreadcrumbsExtras (Z)V
public fun setEnableTimberLogs (Z)V
public fun setFrameMetricsCollector (Lio/sentry/android/core/internal/util/SentryFrameMetricsCollector;)V
public fun setNativeHandlerStrategy (Lio/sentry/android/core/NdkHandlerStrategy;)V
public fun setNativeSdkName (Ljava/lang/String;)V
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,7 @@ static void installDefaultIntegrations(
}

if (isTimberAvailable) {
options.addIntegration(new SentryTimberIntegration());
options.addIntegration(new SentryTimberIntegration(options.isEnableTimberLogs()));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Timber logs option ignored at init

High Severity

Auto-installed SentryTimberIntegration copies isEnableTimberLogs in its constructor during installDefaultIntegrations, which runs before the SentryAndroid.init configuration callback. Setting enableTimberLogs in that callback therefore never reaches the planted SentryTimberTree, so the Java/Kotlin opt-in silently has no effect. Manifest metadata still works because it is applied earlier.

Additional Locations (1)
Fix in CursorFix in Web

Reviewed by Cursor Bugbot for commit 4f677fb. Configure here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

fixed in a later PR

Comment on lines 475 to +476

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: SentryTimberIntegration is created before the user's configuration callback runs, so it caches the default enableLogs value, ignoring any changes made in the callback.
Severity: MEDIUM

Suggested Fix

The SentryTimberIntegration should not cache the enableLogs value in its constructor. Instead, it should read the value directly from the SentryOptions object whenever it needs to decide whether to capture a log. This ensures that the user's configuration, applied via the callback, is always respected at runtime. The check inside SentryTimberTree should be changed to query options.isEnableTimberLogs instead of using the cached enableLogs field.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location:
sentry-android-core/src/main/java/io/sentry/android/core/AndroidOptionsInitializer.java#L475-L476

Potential issue: The `SentryTimberIntegration` is instantiated during
`AndroidOptionsInitializer.installDefaultIntegrations(...)`, which occurs before the
user's configuration callback is invoked. The integration's constructor caches the value
of `options.isEnableTimberLogs()` at that moment. Consequently, if a user attempts to
enable Timber logging programmatically within the `SentryAndroid.init { options -> ...
}` block, their setting will be silently ignored because the integration has already
been created with the default value (usually `false`). This violates the documented rule
that integrations must not cache option values in their constructors. Only configuration
via `AndroidManifest.xml` works as intended.

Also affects:

  • sentry-android-timber/src/main/java/io/sentry/android/timber/SentryTimberIntegration.kt:24~30

Did we get this right? 馃憤 / 馃憥 to inform future reviews.

}
options.addIntegration(new AppComponentsBreadcrumbsIntegration(context));
options.addIntegration(new SystemEventsBreadcrumbsIntegration(context));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,8 @@ final class ManifestMetadataReader {

static final String ENABLE_LOGS = "io.sentry.logs.enabled";

static final String ENABLE_TIMBER_LOGS = "io.sentry.timber.logs.enabled";

static final String ENABLE_METRICS = "io.sentry.metrics.enabled";

static final String ENABLE_AUTO_TRACE_ID_GENERATION =
Expand Down Expand Up @@ -706,6 +708,9 @@ static void applyMetadata(
.getLogs()
.setEnabled(readBool(metadata, logger, ENABLE_LOGS, options.getLogs().isEnabled()));

options.setEnableTimberLogs(
readBool(metadata, logger, ENABLE_TIMBER_LOGS, options.isEnableTimberLogs()));

options
.getMetrics()
.setEnabled(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,9 @@ public final class SentryAndroidOptions extends SentryOptions {
/** Enable or disable automatic breadcrumbs for Network Events Using NetworkCallback */
private boolean enableNetworkEventBreadcrumbs = true;

/** Enable or disable automatic Sentry Logs capture from Timber. Default is disabled. */
private boolean enableTimberLogs = false;

/**
* Enables the Auto instrumentation for Activity lifecycle tracing.
*
Expand Down Expand Up @@ -457,6 +460,14 @@ public void setEnableNetworkEventBreadcrumbs(boolean enableNetworkEventBreadcrum
this.enableNetworkEventBreadcrumbs = enableNetworkEventBreadcrumbs;
}

public boolean isEnableTimberLogs() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what do you think about naming this TimberLogCapture?

IMO, the current API isn't clear if we are outputting to Timber or capturing from Timber.

Suggested change
public boolean isEnableTimberLogs() {
public boolean setTimberLogCaptureEnabled() {

return enableTimberLogs;
}

public void setEnableTimberLogs(boolean enableTimberLogs) {
this.enableTimberLogs = enableTimberLogs;
}

/**
* Enable or disable all the automatic breadcrumbs
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -679,8 +679,21 @@ class AndroidOptionsInitializerTest {
fun `SentryTimberIntegration added to the integration list if available on classpath`() {
fixture.initSutWithClassLoader(isTimberAvailable = true)

val actual = fixture.sentryOptions.integrations.firstOrNull { it is SentryTimberIntegration }
assertNotNull(actual)
val actual =
fixture.sentryOptions.integrations.firstOrNull { it is SentryTimberIntegration }
as SentryTimberIntegration
assertFalse(actual.enableLogs)
}

@Test
fun `SentryTimberIntegration receives Timber logs option`() {
fixture.sentryOptions.isEnableTimberLogs = true
fixture.initSutWithClassLoader(isTimberAvailable = true)

val actual =
fixture.sentryOptions.integrations.firstOrNull { it is SentryTimberIntegration }
as SentryTimberIntegration
assertTrue(actual.enableLogs)
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1967,6 +1967,25 @@ class ManifestMetadataReaderTest {
assertTrue(fixture.options.logs.isEnabled)
}

@Test
fun `applyMetadata keeps Timber logs disabled if not found`() {
val context = fixture.getContext()

ManifestMetadataReader.applyMetadata(context, fixture.options, fixture.buildInfoProvider)

assertFalse(fixture.options.isEnableTimberLogs)
}

@Test
fun `applyMetadata reads Timber logs enabled to options`() {
val bundle = bundleOf(ManifestMetadataReader.ENABLE_TIMBER_LOGS to true)
val context = fixture.getContext(metaData = bundle)

ManifestMetadataReader.applyMetadata(context, fixture.options, fixture.buildInfoProvider)

assertTrue(fixture.options.isEnableTimberLogs)
}

@Test
fun `applyMetadata reads metrics enabled and keep default value if not found`() {
// Arrange
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,21 @@ class SentryAndroidOptionsTest {
assertTrue(sentryOptions.isEnableScopeSync)
}

@Test
fun `Timber logs are disabled by default`() {
val sentryOptions = SentryAndroidOptions()

assertFalse(sentryOptions.isEnableTimberLogs)
}

@Test
fun `Timber logs can be enabled`() {
val sentryOptions = SentryAndroidOptions()
sentryOptions.isEnableTimberLogs = true

assertTrue(sentryOptions.isEnableTimberLogs)
}

@Test
fun `attach screenshots disabled by default for Android`() {
val sentryOptions = SentryAndroidOptions()
Expand Down
4 changes: 4 additions & 0 deletions sentry-android-timber/api/sentry-android-timber.api
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@ public final class io/sentry/android/timber/SentryTimberIntegration : io/sentry/
public fun <init> ()V
public fun <init> (Lio/sentry/SentryLevel;Lio/sentry/SentryLevel;Lio/sentry/SentryLogLevel;)V
public synthetic fun <init> (Lio/sentry/SentryLevel;Lio/sentry/SentryLevel;Lio/sentry/SentryLogLevel;ILkotlin/jvm/internal/DefaultConstructorMarker;)V
public fun <init> (Lio/sentry/SentryLevel;Lio/sentry/SentryLevel;Lio/sentry/SentryLogLevel;Z)V
public fun <init> (Z)V
public fun close ()V
public final fun getEnableLogs ()Z
public final fun getMinBreadcrumbLevel ()Lio/sentry/SentryLevel;
public final fun getMinEventLevel ()Lio/sentry/SentryLevel;
public final fun getMinLogsLevel ()Lio/sentry/SentryLogLevel;
Expand All @@ -21,6 +24,7 @@ public final class io/sentry/android/timber/SentryTimberIntegration : io/sentry/
public final class io/sentry/android/timber/SentryTimberTree : timber/log/Timber$Tree {
public fun <init> (Lio/sentry/IScopes;Lio/sentry/SentryLevel;Lio/sentry/SentryLevel;Lio/sentry/SentryLogLevel;)V
public synthetic fun <init> (Lio/sentry/IScopes;Lio/sentry/SentryLevel;Lio/sentry/SentryLevel;Lio/sentry/SentryLogLevel;ILkotlin/jvm/internal/DefaultConstructorMarker;)V
public fun <init> (Lio/sentry/IScopes;Lio/sentry/SentryLevel;Lio/sentry/SentryLevel;Lio/sentry/SentryLogLevel;Z)V
public fun d (Ljava/lang/String;[Ljava/lang/Object;)V
public fun d (Ljava/lang/Throwable;)V
public fun d (Ljava/lang/Throwable;Ljava/lang/String;[Ljava/lang/Object;)V
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,22 @@ public class SentryTimberIntegration(
public val minBreadcrumbLevel: SentryLevel = SentryLevel.INFO,
public val minLogsLevel: SentryLogLevel = SentryLogLevel.INFO,
) : Integration, Closeable {
public var enableLogs: Boolean = false
private set

public constructor(enableLogs: Boolean) : this() {
this.enableLogs = enableLogs
}

public constructor(
minEventLevel: SentryLevel,
minBreadcrumbLevel: SentryLevel,
minLogsLevel: SentryLogLevel,
enableLogs: Boolean,
) : this(minEventLevel, minBreadcrumbLevel, minLogsLevel) {
this.enableLogs = enableLogs
}

private lateinit var tree: SentryTimberTree
private lateinit var logger: ILogger

Expand All @@ -31,7 +47,7 @@ public class SentryTimberIntegration(
override fun register(scopes: IScopes, options: SentryOptions) {
logger = options.logger

tree = SentryTimberTree(scopes, minEventLevel, minBreadcrumbLevel, minLogsLevel)
tree = SentryTimberTree(scopes, minEventLevel, minBreadcrumbLevel, minLogsLevel, enableLogs)
Timber.plant(tree)

logger.log(SentryLevel.DEBUG, "SentryTimberIntegration installed.")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,18 @@ public class SentryTimberTree(
private val minBreadcrumbLevel: SentryLevel,
private val minLogLevel: SentryLogLevel = SentryLogLevel.INFO,
) : Timber.Tree() {
private var enableLogs: Boolean = false

public constructor(
scopes: IScopes,
minEventLevel: SentryLevel,
minBreadcrumbLevel: SentryLevel,
minLogLevel: SentryLogLevel,
enableLogs: Boolean,
) : this(scopes, minEventLevel, minBreadcrumbLevel, minLogLevel) {
this.enableLogs = enableLogs
}

private val pendingTag = ThreadLocal<String?>()

private fun retrieveTag(): String? {
Expand Down Expand Up @@ -185,7 +197,9 @@ public class SentryTimberTree(

captureEvent(level, tag, sentryMessage, throwable)
addBreadcrumb(level, sentryMessage, throwable)
addLog(logLevel, message, tag, throwable, *args)
if (enableLogs) {
addLog(logLevel, message, tag, throwable, *args)
}
}

/** do not log if it's lower than min. required level. */
Expand Down
Original file line number Diff line number Diff line change
@@ -1,39 +1,59 @@
package io.sentry.android.timber

import io.sentry.Breadcrumb
import io.sentry.IScopes
import io.sentry.ITransportFactory
import io.sentry.ScopesAdapter
import io.sentry.Sentry
import io.sentry.SentryLevel
import io.sentry.SentryLogLevel
import io.sentry.SentryOptions
import io.sentry.logger.ILoggerApi
import io.sentry.logger.SentryLogParameters
import io.sentry.protocol.SdkVersion
import io.sentry.transport.ITransport
import kotlin.test.BeforeTest
import kotlin.test.Test
import kotlin.test.assertEquals
import kotlin.test.assertFalse
import kotlin.test.assertTrue
import org.mockito.kotlin.any
import org.mockito.kotlin.mock
import org.mockito.kotlin.verify
import org.mockito.kotlin.verifyNoInteractions
import org.mockito.kotlin.whenever
import timber.log.Timber

class SentryTimberIntegrationTest {
private class Fixture {
val scopes = mock<IScopes>()
val logs = mock<ILoggerApi>()
val options = SentryOptions().apply { sdkVersion = SdkVersion("test", "1.2.3") }

init {
whenever(scopes.logger()).thenReturn(logs)
}

fun getSut(
minEventLevel: SentryLevel = SentryLevel.ERROR,
minBreadcrumbLevel: SentryLevel = SentryLevel.INFO,
minLogsLevel: SentryLogLevel = SentryLogLevel.INFO,
enableLogs: Boolean? = null,
): SentryTimberIntegration =
SentryTimberIntegration(
minEventLevel = minEventLevel,
minBreadcrumbLevel = minBreadcrumbLevel,
minLogsLevel = minLogsLevel,
)
if (enableLogs == null) {
SentryTimberIntegration(
minEventLevel = minEventLevel,
minBreadcrumbLevel = minBreadcrumbLevel,
minLogsLevel = minLogsLevel,
)
} else {
SentryTimberIntegration(
minEventLevel = minEventLevel,
minBreadcrumbLevel = minBreadcrumbLevel,
minLogsLevel = minLogsLevel,
enableLogs = enableLogs,
)
}
}

private val fixture = Fixture()
Expand Down Expand Up @@ -64,6 +84,30 @@ class SentryTimberIntegrationTest {
verify(fixture.scopes).captureEvent(any())
}

@Test
fun `Manual integration defaults logs to disabled while capturing events and breadcrumbs`() {
val sut = fixture.getSut()
sut.register(fixture.scopes, fixture.options)

assertFalse(sut.enableLogs)
Timber.e("message")

verify(fixture.scopes).captureEvent(any())
verify(fixture.scopes).addBreadcrumb(any<Breadcrumb>())
verifyNoInteractions(fixture.logs)
}

@Test
fun `Manual integration captures logs when enabled`() {
val sut = fixture.getSut(enableLogs = true)
sut.register(fixture.scopes, fixture.options)

assertTrue(sut.enableLogs)
Timber.i("message")

verify(fixture.logs).log(any(), any<SentryLogParameters>(), any<String>())
}

@Test
fun `Integrations removes a tree from Timber on close integration`() {
val sut = fixture.getSut()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,16 @@ class SentryTimberTreeTest {
minEventLevel: SentryLevel = SentryLevel.ERROR,
minBreadcrumbLevel: SentryLevel = SentryLevel.INFO,
minLogsLevel: SentryLogLevel = SentryLogLevel.INFO,
enableLogs: Boolean? = true,
): SentryTimberTree {
logs = mock<ILoggerApi>()
scopes = mock<Scopes>()
whenever(scopes.logger()).thenReturn(logs)
return SentryTimberTree(scopes, minEventLevel, minBreadcrumbLevel, minLogsLevel)
return if (enableLogs == null) {
SentryTimberTree(scopes, minEventLevel, minBreadcrumbLevel, minLogsLevel)
} else {
SentryTimberTree(scopes, minEventLevel, minBreadcrumbLevel, minLogsLevel, enableLogs)
}
}
}

Expand Down Expand Up @@ -296,6 +301,17 @@ class SentryTimberTreeTest {
sut.d("test %s, %s", 1, 1)
}

@Test
fun `Tree defaults logs to disabled while capturing events and breadcrumbs`() {
val sut = fixture.getSut(enableLogs = null)

sut.e("message")

verify(fixture.scopes).captureEvent(any())
verify(fixture.scopes).addBreadcrumb(any<Breadcrumb>())
verifyNoInteractions(fixture.logs)
}

@Test
fun `Tree adds a log with message and arguments, when provided`() {
val sut = fixture.getSut()
Expand Down
Loading