feat(logback): [Logs and Metrics Enable Flags 1] Add Logs opt-in - #5940
feat(logback): [Logs and Metrics Enable Flags 1] Add Logs opt-in#5940adinauer wants to merge 2 commits into
Conversation
Require an explicit Logback appender opt-in before forwarding records as Sentry Logs. Keep event and breadcrumb capture independent while the aggregate core Logs flag remains in place. Co-Authored-By: Claude <noreply@anthropic.com>
|
📲 Install BuildsAndroid
|
runningcode
left a comment
There was a problem hiding this comment.
I put a global suggestion on the naming, probably worth a discussion with the team but otherwise going to approve
| public fun getMinimumBreadcrumbLevel ()Lch/qos/logback/classic/Level; | ||
| public fun getMinimumEventLevel ()Lch/qos/logback/classic/Level; | ||
| public fun getMinimumLevel ()Lch/qos/logback/classic/Level; | ||
| public fun isEnableLogs ()Z |
There was a problem hiding this comment.
would it be more idiomatic to make this logsEnabled()?
There was a problem hiding this comment.
Afaik other SDKs are calling the option enableLogs too, so I wanted to align.
There was a problem hiding this comment.
good point, this is the classic tension of consistency vs feeling native to the platform. Given our new goal hierarchy, I would think it should be the one that feels native.
There was a problem hiding this comment.
Discussed in slack, we should make this feel native.
| public fun getMinimumEventLevel ()Lch/qos/logback/classic/Level; | ||
| public fun getMinimumLevel ()Lch/qos/logback/classic/Level; | ||
| public fun isEnableLogs ()Z | ||
| public fun setEnableLogs (Z)V |
There was a problem hiding this comment.
same suggestion as above
| public fun setEnableLogs (Z)V | |
| public fun setLogsEnabled (Z)V |
| private @NotNull Level minimumBreadcrumbLevel = Level.INFO; | ||
| private @NotNull Level minimumEventLevel = Level.ERROR; | ||
| private @NotNull Level minimumLevel = Level.INFO; | ||
| private boolean enableLogs = false; |
There was a problem hiding this comment.
| private boolean enableLogs = false; | |
| private boolean logsEnabled = false; |
runningcode
left a comment
There was a problem hiding this comment.
After a discussion, I think we should make this feel native!
PR Stack (Logs and Metrics Enable Flags)
📜 Description
Adds an
enableLogsoption to the LogbackSentryAppender. The option defaults tofalseand can be configured through Java or Logback XML.The appender now requires both this local opt-in and the existing aggregate core Logs flag before forwarding Logback records as Sentry Logs. Event and breadcrumb capture remain unchanged.
💡 Motivation and Context
Logging integrations need explicit local opt-ins before the aggregate core Logs flag can be removed later in this stack. This prevents applications from unexpectedly forwarding framework logs when core Logs capture becomes available without a global enable flag.
💚 How did you test it?
./gradlew spotlessApply apiDump./gradlew :sentry-logback:test📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps
Continue adding integration-local Logs opt-ins before removing the aggregate core Logs flag.