feat(core): [Logs and Metrics Enable Flags 9] Warn for legacy Logs configuration - #5950
feat(core): [Logs and Metrics Enable Flags 9] Warn for legacy Logs configuration#5950adinauer wants to merge 2 commits into
Conversation
Sorry, something went wrong.
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>
|
Sorry, something went wrong.
📲 Install BuildsAndroid
|
Sorry, something went wrong.
Performance metrics 🚀
Previous results on branch: feat/warn-legacy-logs-externalStartup times
App size
|
Sorry, something went wrong.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8a414c2. Configure here.
Sorry, something went wrong.
| "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.
Sorry, something went wrong.
| + "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
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 AgentReview 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.
Sorry, something went wrong.
| 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?
Sorry, something went wrong.
| assertLegacyLogsConfigurationDoesNotDisableCapture(options) | ||
| } | ||
|
|
||
| private fun assertLegacyLogsConfigurationDoesNotDisableCapture(options: SentryOptions) { |
There was a problem hiding this comment.
Should we cleanup the sentry instance created by createTestScopes afterwards?
Sorry, something went wrong.
PR Stack (Logs and Metrics Enable Flags)
📜 Description
Keeps parsing the nullable legacy logs.enabled external option and emits a tailored migration warning from SentryOptions.merge for explicit true and false values.
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?
📝 Checklist
🔮 Next steps
Add the equivalent migration warning for legacy Spring Boot Logs configuration.
#skip-changelog