Conversation
Keep parsing the nullable logs.enabled external option and emit tailored migration warnings for explicit true and false values without changing manual Logs capture. Co-Authored-By: Claude <noreply@anthropic.com>
|
📲 Install BuildsAndroid
|
Performance metrics 🚀
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| "The 'logs.enabled' option no longer disables manual Sentry.logger() calls. Automatic " | ||
| + "logging integrations remain disabled unless enabled through their own opt-ins."); | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
Reviewed by Cursor Bugbot for commit 8a414c2. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| + "logging integrations remain disabled unless enabled through their own opt-ins."); | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
| withPropertiesFile("logs.enabled=true") { options -> assertTrue(options.isEnableLogs == true) } | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
Related: Should we deprecate the logs.enabled flag in ExternalOptions.java?
| assertLegacyLogsConfigurationDoesNotDisableCapture(options) | ||
| } | ||
|
|
||
| private fun assertLegacyLogsConfigurationDoesNotDisableCapture(options: SentryOptions) { |
There was a problem hiding this comment.
Should we cleanup the sentry instance created by createTestScopes afterwards?
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6492416. Configure here.
| LoggerApi(scopes).info("test log") | ||
|
|
||
| verify(client).captureLog(any(), anyOrNull()) | ||
| } |
There was a problem hiding this comment.
Tests leave Sentry globally initialized
Low Severity
assertLegacyLogsConfigurationDoesNotDisableCapture calls createTestScopes, which runs Sentry.init on the shared options, and SentryOptionsTest never calls Sentry.close(). Later tests in the same JVM can observe leftover global scopes, a mock logger, and zero shutdown timeouts.
Triggered by project rule: PR Review Guidelines for Cursor Bugbot
Reviewed by Cursor Bugbot for commit 6492416. Configure here.
This branch has not been deployed
3 participants
PR Stack (Logs and Metrics Enable Flags)
📜 Description
Keeps parsing the nullable legacy
logs.enabledexternal option and emits a tailored migration warning fromSentryOptions.mergefor explicittrueandfalsevalues.The legacy value no longer changes SDK behavior. Manual
Sentry.logger()capture remains active for either value, while absent configuration emits no warning.💡 Motivation and Context
The aggregate Logs enable flag was removed earlier in this stack. Existing
sentry.properties, environment-variable, and system-property configurations need a clear migration diagnostic rather than being silently ignored.💚 How did you test it?
./gradlew :sentry:test./gradlew spotlessApply apiDumptrue, andfalsevalues📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps
Add the equivalent migration warning for legacy Spring Boot Logs configuration.
#skip-changelog