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
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}
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.
| "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.
2 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