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
15 changes: 15 additions & 0 deletions sentry/src/main/java/io/sentry/SentryOptions.java
Original file line number Diff line number Diff line change
Expand Up @@ -3733,6 +3733,21 @@ public void merge(final @NotNull ExternalOptions options) {
}
}

if (options.isEnableLogs() != null) {
if (options.isEnableLogs()) {
logger.log(
SentryLevel.WARNING,
"The 'logs.enabled' option is no longer supported. Manual Sentry.logger() calls no "
+ "longer require it, and automatic logging integrations now require their own "
+ "opt-ins.");
} else {
logger.log(
SentryLevel.WARNING,
"The 'logs.enabled' option no longer disables manual Sentry.logger() calls. Automatic "
+ "logging integrations remain disabled unless enabled through their own opt-ins.");
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Legacy logs warning never surfaces

Medium Severity

The migration warning for legacy logs.enabled is written through SentryOptions.logger inside merge. External configuration is merged in preInitConfigurations while that logger is still NoOpLogger; initLogger runs only afterward. Users with sentry.properties, env vars, or system properties therefore get no diagnostic, so logs.enabled=false is ignored without notice.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 8a414c2. Configure here.

Comment on lines +3747 to +3749

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: The migration warning for the deprecated logs.enabled configuration will not be displayed by default because it is logged via DiagnosticLogger, which requires debug mode to be enabled.
Severity: MEDIUM

Suggested Fix

The warning should be logged using a mechanism that is not dependent on the debug flag. Consider using a different logger or logging at a level that bypasses the debug check to ensure the migration warning is always visible to users with the deprecated configuration.

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/src/main/java/io/sentry/SentryOptions.java#L3747-L3749

Potential issue: The migration warning for the deprecated `logs.enabled` configuration
is logged using a `DiagnosticLogger`. This logger's `log()` method is gated by
`options.isDebug()`, which defaults to `false`. As a result, users with the legacy
configuration will not see the intended migration diagnostic warning unless they have
explicitly enabled debug mode. This undermines the feature's goal of providing clear
migration guidance, as the warning will be silently suppressed for most users in
production environments.

Did we get this right? 👍 / 👎 to inform future reviews.


if (options.isEnableMetrics() != null) {
getMetrics().setEnabled(options.isEnableMetrics());
}
Expand Down
12 changes: 12 additions & 0 deletions sentry/src/test/java/io/sentry/ExternalOptionsTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -437,6 +437,18 @@ class ExternalOptionsTest {
withPropertiesFile("logs.enabled=true") { options -> assertTrue(options.isEnableLogs == true) }
}

@Test
fun `creates options with enableLogs set to false`() {
withPropertiesFile("logs.enabled=false") { options ->
assertTrue(options.isEnableLogs == false)
}
}

@Test
fun `creates options with enableLogs set to null when not set`() {
withPropertiesFile { assertNull(it.isEnableLogs) }
}

@Test
fun `creates options with enableMetrics set to true`() {
withPropertiesFile("metrics.enabled=true") { options ->
Expand Down
72 changes: 72 additions & 0 deletions sentry/src/test/java/io/sentry/SentryOptionsTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@ package io.sentry

import io.sentry.SentryOptions.RequestSize
import io.sentry.logger.ILoggerBatchProcessorFactory
import io.sentry.logger.LoggerApi
import io.sentry.test.createSentryClientMock
import io.sentry.test.createTestScopes
import io.sentry.util.StringUtils
import java.io.File
import java.net.Proxy
Expand All @@ -15,8 +18,11 @@ import kotlin.test.assertNotNull
import kotlin.test.assertNull
import kotlin.test.assertSame
import kotlin.test.assertTrue
import org.mockito.kotlin.any
import org.mockito.kotlin.anyOrNull
import org.mockito.kotlin.eq
import org.mockito.kotlin.mock
import org.mockito.kotlin.never
import org.mockito.kotlin.verify

class SentryOptionsTest {
Expand Down Expand Up @@ -501,6 +507,72 @@ class SentryOptionsTest {
assertTrue(options.metrics.isEnabled)
}

@Test
fun `merging options does not warn when legacy logs configuration is absent`() {
val logger = mock<ILogger>()
val options =
SentryOptions().also {
it.isDebug = true
it.setLogger(logger)
}

options.merge(ExternalOptions())

verify(logger, never()).log(eq(SentryLevel.WARNING), any<String>())
}

@Test
fun `merging options warns when legacy logs configuration is true`() {
val logger = mock<ILogger>()
val options =
SentryOptions().also {
it.isDebug = true
it.setLogger(logger)
}

options.merge(ExternalOptions().apply { isEnableLogs = true })

verify(logger)
.log(
SentryLevel.WARNING,
"The 'logs.enabled' option is no longer supported. Manual Sentry.logger() calls no " +
"longer require it, and automatic logging integrations now require their own opt-ins.",
*emptyArray(),
)
assertLegacyLogsConfigurationDoesNotDisableCapture(options)
}

@Test
fun `merging options warns when legacy logs configuration is false`() {
val logger = mock<ILogger>()
val options =
SentryOptions().also {
it.isDebug = true
it.setLogger(logger)
}

options.merge(ExternalOptions().apply { isEnableLogs = false })

verify(logger)
.log(
SentryLevel.WARNING,
"The 'logs.enabled' option no longer disables manual Sentry.logger() calls. Automatic " +
"logging integrations remain disabled unless enabled through their own opt-ins.",
*emptyArray(),
)
assertLegacyLogsConfigurationDoesNotDisableCapture(options)
}

private fun assertLegacyLogsConfigurationDoesNotDisableCapture(options: SentryOptions) {
options.dsn = "https://key@sentry.io/proj"
val client = createSentryClientMock()
val scopes = createTestScopes(options).apply { bindClient(client) }

LoggerApi(scopes).info("test log")

verify(client).captureLog(any(), anyOrNull())
}

@Test
fun `merging options merges and overwrites existing tag values`() {
val externalOptions = ExternalOptions()
Expand Down
Loading