ADFA-4646: Remove ch.qos.logback#1557
Open
davidschachterADFA wants to merge 10 commits into
Open
Conversation
Adds a Logback-free SLF4J 2.x provider (IdeSlf4jServiceProvider, IdeLogger, IdeLoggerFactory, IdeLogRouter, IdeGlobalLogBuffer, IdeLogFormatter) plus an external sink registry on IdeLogRouter for later cross-process forwarding (tooling-api RPC, Sentry breadcrumbs). Not yet registered via @autoservice, so Logback stays the active SLF4J backend and nothing else changes yet. This is groundwork for removing ch.qos.logback in a follow-up change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…logger Registers IdeSlf4jServiceProvider via @autoservice so it becomes the active SLF4J 2.x backend, and deletes the Logback-based classes it replaces: IDELoggingConfigurator, LogcatAppender, StdErrAppender, JvmStdErrAppender, GlobalBufferAppender, and the encoder/ layout classes. Drops ch.qos.logback from logger/build.gradle.kts and the version catalog, adding an explicit slf4j-api dependency (previously pulled in transitively via logback-classic). Trims now-dead pattern-layout constants from LogUtils. Updates IDELogFragment, the one real GlobalBufferAppender consumer, to the ported IdeGlobalLogBuffer. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Deletes ToolingLoggingConfigurator and LogbackStatusListener (Logback-only Configurator/status-diagnostics classes with no equivalent needed on the new backend). Rewrites ToolingApiAppender as a plain IdeLogRouter.ExternalSink that forwards log lines to the Android app over RPC, registered at the top of Main.main() instead of via Logback's Configurator SPI. Adds IdeLogRouter.PROP_JVM_STDERR_ENABLED so ToolingServerRunner can still toggle stderr output in the standalone tooling-api JVM process it launches, replacing the deleted JvmStdErrAppender's system property of the same purpose. Also drops the now-obsolete -Dlogback.statusListenerClass flag from both ToolingServerRunner and ToolingApiTestLauncher. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
TestingLoggingConfigurator's only job was attaching a stderr appender for the JVM test harness; that's now automatic behavior of IdeLogRouter for any plain-JVM process, so no replacement class is needed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
sentry-logback required casting SLF4J's ILoggerFactory to Logback's LoggerContext to manually attach a SentryAppender. Since crash events were already gated off (setMinimumEventLevel(Level.OFF) - events only ever come from explicit Sentry.captureException calls elsewhere) and only WARN+ logs were forwarded as breadcrumbs, that's replicated directly via an IdeLogRouter sink calling Sentry.addBreadcrumb, with no Logback (or Timber) dependency needed. Also removes the last remaining Logback traces: the buildscript classpath entry, the EnvUtil/DesugarEnvUtil desugaring bytecode patch (needed only because EnvUtil.logbackVersion() used Class.getModule(), unavailable on Android), and the sentry-logback catalog entry. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Auto-fixed by spotlessApply (tabs, trailing commas, wrapping) on the files touched by the logback removal; no semantic changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
Contributor
📝 Walkthrough
WalkthroughThe PR replaces Logback-based logging with a custom SLF4J provider, formatter, router, global buffer, Sentry sink, and Tooling API sink. Application log consumers and JVM logging properties are updated, while Logback dependencies, configurators, appenders, and status-listener wiring are removed. ChangesLogging pipeline migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SLF4J
participant IdeLogger
participant IdeLogRouter
participant IdeGlobalLogBuffer
participant ToolingApiAppender
participant ToolingClient
SLF4J->>IdeLogger: emit normalized log call
IdeLogger->>IdeLogRouter: dispatch log event
IdeLogRouter->>IdeGlobalLogBuffer: append formatted line
IdeLogRouter->>ToolingApiAppender: deliver external sink event
ToolingApiAppender->>ToolingClient: send LogMessageParams
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
logger/src/main/java/com/itsaky/androidide/logging/provider/IdeLoggerFactory.kt (1)
24-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the public factory contract.
The new public
IdeLoggerFactoryhas author-only KDoc. Add a concise contract describing per-name caching and thread-safegetLoggerbehavior.As per coding guidelines, public classes and functions must document their contracts and non-obvious behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@logger/src/main/java/com/itsaky/androidide/logging/provider/IdeLoggerFactory.kt` around lines 24 - 30, Add KDoc to the public IdeLoggerFactory class and its getLogger method, documenting that logger instances are cached per name and that retrieval is thread-safe. Retain the existing computeIfAbsent implementation and author attribution.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/src/main/java/com/itsaky/androidide/app/DeviceProtectedApplicationLoader.kt`:
- Around line 87-99: Update the IdeLogRouter sink in
DeviceProtectedApplicationLoader so arbitrary WARN+ log messages are not passed
directly to Sentry.addBreadcrumb. Only forward vetted structured metadata, or
route messages through the centralized allowlist/redaction mechanism before
assigning Breadcrumb.message, ensuring secrets, PII, paths, file contents, and
request bodies are excluded.
In
`@logger/src/main/java/com/itsaky/androidide/logging/provider/IdeGlobalLogBuffer.kt`:
- Around line 92-94: Report failures from both logging dispatch paths instead of
silently discarding them: update Consumer.consume handling in
IdeGlobalLogBuffer.kt (lines 92-94) and ExternalSink.onLog handling in
IdeLogSink.kt (line 77) to use a minimal non-recursive fallback such as Logcat
or System.err, preserving the existing dispatch behavior.
- Line 47: Replace the synchronized-list implementation of consumers in
IdeGlobalLogBuffer with CopyOnWriteArrayList, or otherwise make dispatch()
iterate over a synchronized snapshot. Ensure concurrent registerConsumer and
unregisterConsumer calls cannot cause ConcurrentModificationException during
dispatch() while preserving existing consumer delivery behavior.
In
`@logger/src/main/java/com/itsaky/androidide/logging/provider/IdeLogFormatter.kt`:
- Around line 43-48: Add KDoc to the public format function documenting its
formatting contract, including the omitMessage parameter, trailing-newline
behavior, and that timestamps use local time. Keep the documentation focused on
externally observable behavior and parameter semantics.
- Around line 38-56: Add unit tests for the replacement logging pipeline: in
logger/src/main/java/com/itsaky/androidide/logging/provider/IdeLogFormatter.kt
lines 38-56, cover abbreviateLoggerName and format output, levels, timestamps,
and omitted messages; in
logger/src/main/java/com/itsaky/androidide/logging/provider/IdeGlobalLogBuffer.kt
lines 53-94, cover replay, level filtering, capacity, and concurrent consumer
changes; in
logger/src/main/java/com/itsaky/androidide/logging/provider/IdeLogSink.kt lines
55-88, cover sink registration, removal, dispatch, and failure isolation; and in
subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/logging/ToolingApiAppender.kt
lines 31-41, cover LogMessageParams level, tag, message, and throwable
serialization.
---
Nitpick comments:
In
`@logger/src/main/java/com/itsaky/androidide/logging/provider/IdeLoggerFactory.kt`:
- Around line 24-30: Add KDoc to the public IdeLoggerFactory class and its
getLogger method, documenting that logger instances are cached per name and that
retrieval is thread-safe. Retain the existing computeIfAbsent implementation and
author attribution.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6eb22fd1-c944-4f91-bd57-c7f110d62ec8
📒 Files selected for processing (26)
app/build.gradle.ktsapp/src/main/java/com/itsaky/androidide/app/DeviceProtectedApplicationLoader.ktapp/src/main/java/com/itsaky/androidide/fragments/output/IDELogFragment.ktapp/src/main/java/com/itsaky/androidide/services/builder/ToolingServerRunner.ktgradle/libs.versions.tomllogger/build.gradle.ktslogger/src/main/java/com/itsaky/androidide/logging/GlobalBufferAppender.ktlogger/src/main/java/com/itsaky/androidide/logging/IDELoggingConfigurator.ktlogger/src/main/java/com/itsaky/androidide/logging/JvmStdErrAppender.javalogger/src/main/java/com/itsaky/androidide/logging/LogcatAppender.javalogger/src/main/java/com/itsaky/androidide/logging/StdErrAppender.javalogger/src/main/java/com/itsaky/androidide/logging/encoder/IDELogFormatEncoder.javalogger/src/main/java/com/itsaky/androidide/logging/encoder/IDELogFormatLayout.javalogger/src/main/java/com/itsaky/androidide/logging/provider/IdeGlobalLogBuffer.ktlogger/src/main/java/com/itsaky/androidide/logging/provider/IdeLogFormatter.ktlogger/src/main/java/com/itsaky/androidide/logging/provider/IdeLogSink.ktlogger/src/main/java/com/itsaky/androidide/logging/provider/IdeLogger.ktlogger/src/main/java/com/itsaky/androidide/logging/provider/IdeLoggerFactory.ktlogger/src/main/java/com/itsaky/androidide/logging/provider/IdeSlf4jServiceProvider.ktlogger/src/main/java/com/itsaky/androidide/logging/utils/LogUtils.javasubprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/Main.ktsubprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/logging/ToolingApiAppender.ktsubprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/logging/ToolingLoggingConfigurator.ktsubprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/util/LogbackStatusListener.kttesting/common/src/main/java/com/itsaky/androidide/testing/common/logging/TestingLoggingConfigurator.kttesting/tooling/src/main/java/com/itsaky/androidide/testing/tooling/ToolingApiTestLauncher.kt
💤 Files with no reviewable changes (12)
- subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/util/LogbackStatusListener.kt
- logger/src/main/java/com/itsaky/androidide/logging/JvmStdErrAppender.java
- logger/src/main/java/com/itsaky/androidide/logging/LogcatAppender.java
- subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/logging/ToolingLoggingConfigurator.kt
- logger/src/main/java/com/itsaky/androidide/logging/StdErrAppender.java
- logger/src/main/java/com/itsaky/androidide/logging/GlobalBufferAppender.kt
- logger/src/main/java/com/itsaky/androidide/logging/encoder/IDELogFormatLayout.java
- logger/src/main/java/com/itsaky/androidide/logging/encoder/IDELogFormatEncoder.java
- gradle/libs.versions.toml
- logger/src/main/java/com/itsaky/androidide/logging/IDELoggingConfigurator.kt
- testing/tooling/src/main/java/com/itsaky/androidide/testing/tooling/ToolingApiTestLauncher.kt
- testing/common/src/main/java/com/itsaky/androidide/testing/common/logging/TestingLoggingConfigurator.kt
- Stop sending pre-formatted log lines over the tooling-api RPC channel; the client re-logs them through its own SLF4J logger, which was formatting them a second time (doubled timestamp/level/thread/tag prefix, embedded stray newline). - Fix the GlitchTip breadcrumb threshold: Sentry's SentryAppender gates breadcrumbs via setMinimumBreadcrumbLevel independent of setMinimumLevel, so the original code recorded breadcrumbs from INFO up, not just WARN+. Also fix the level mapping to Sentry's actual DEBUG/INFO/WARNING/ERROR levels instead of collapsing everything non-ERROR to WARNING. - Contain exceptions in IdeLogRouter.dispatch() so a failure formatting or writing a log line can no longer propagate into the caller's business logic, matching Logback's old per-appender exception containment. Log (not silently discard) external-sink failures too. - Restore TRACE suppression: Logback's root logger defaulted to DEBUG (no .setLevel() call existed anywhere), so TRACE was always suppressed before this migration; IdeLogger's isTraceEnabled() now matches that instead of always returning true. - Extract the throwable-flattening logic duplicated between IdeLogRouter and ToolingApiAppender into a shared IdeLogFormatter.appendThrowable() helper. - Route the logcat tag through the existing LogUtils.processLogTag() sanitize/cap convention instead of an uncapped substring. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- IdeLogFormatter.format(): add KDoc documenting the output format, the omitMessage parameter's effect, the trailing-newline guarantee, and that timestamps use the JVM's local time zone. - IdeGlobalLogBuffer: switch consumers from a synchronized list to CopyOnWriteArrayList - iterating a synchronized list during dispatch() without also synchronizing on it can throw ConcurrentModificationException under concurrent registerConsumer/unregisterConsumer calls. Also log (instead of silently discard) a failing consumer, matching the external-sink failure logging already added to IdeLogRouter. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Covers IdeLogFormatter (abbreviateLoggerName, format output/levels/ timestamps/omitted messages, appendThrowable), IdeGlobalLogBuffer (replay on register, level filtering, unregister, 1000-entry capacity eviction, concurrent register/unregister during dispatch), IdeLogRouter (sink registration/removal, dispatch delivery, failure isolation), and ToolingApiAppender (LogMessageParams level/tag/message/throwable serialization). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@logger/src/test/java/com/itsaky/androidide/logging/provider/IdeGlobalLogBufferTest.kt`:
- Around line 100-119: Update the concurrency test around the churner thread and
join flow so it waits for a signal from the churn loop after at least one
consumer registration, ensuring churn actually occurred before dispatching
assertions. After setting stop and joining, assert that churner is no longer
alive, while preserving the existing uncaught-exception check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c99575da-4da5-4c76-87a9-cd7d5412e1a3
📒 Files selected for processing (10)
app/src/main/java/com/itsaky/androidide/app/DeviceProtectedApplicationLoader.ktlogger/src/main/java/com/itsaky/androidide/logging/provider/IdeGlobalLogBuffer.ktlogger/src/main/java/com/itsaky/androidide/logging/provider/IdeLogFormatter.ktlogger/src/main/java/com/itsaky/androidide/logging/provider/IdeLogSink.ktlogger/src/main/java/com/itsaky/androidide/logging/provider/IdeLogger.ktlogger/src/test/java/com/itsaky/androidide/logging/provider/IdeGlobalLogBufferTest.ktlogger/src/test/java/com/itsaky/androidide/logging/provider/IdeLogFormatterTest.ktlogger/src/test/java/com/itsaky/androidide/logging/provider/IdeLogRouterTest.ktsubprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/logging/ToolingApiAppender.ktsubprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/logging/ToolingApiAppenderTest.kt
🚧 Files skipped from review as they are similar to previous changes (6)
- subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/logging/ToolingApiAppender.kt
- logger/src/main/java/com/itsaky/androidide/logging/provider/IdeLogFormatter.kt
- logger/src/main/java/com/itsaky/androidide/logging/provider/IdeGlobalLogBuffer.kt
- logger/src/main/java/com/itsaky/androidide/logging/provider/IdeLogger.kt
- logger/src/main/java/com/itsaky/androidide/logging/provider/IdeLogSink.kt
- app/src/main/java/com/itsaky/androidide/app/DeviceProtectedApplicationLoader.kt
itsaky-adfa
requested changes
Jul 22, 2026
Comment on lines
+25
to
+29
| private fun setMainClient(client: IToolingApiClient?) { | ||
| val field = Main::class.java.getDeclaredField("_client") | ||
| field.isAccessible = true | ||
| field.set(Main, client) | ||
| } |
Contributor
There was a problem hiding this comment.
This is fragile and may break in case we rename/move the field later. Since we own the Main class implementation and it sits in the same module, we can introduce a setter to set the private field. This setter should be made internal with a @VisibleForTesting annotation to indicate that the setter is specifically for testing purposes. This should not make the private property internal though - only the setter.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Removes
ch.qos.logback(ADFA-4646) to shrink the APK, replacing it with a small hand-written SLF4J 2.x provider backed directly byandroid.util.Log/System.err— SLF4J 2.x requires theServiceLoader-basedSLF4JServiceProviderSPI, and no maintained Timber binding exists for it, so a custom provider was the only viable path.IdeSlf4jServiceProvider,IdeLogger,IdeLoggerFactory,IdeLogRouter,IdeGlobalLogBuffer,IdeLogFormatter) inlogger/, plus anIdeLogRouter.addSink/removeSinkregistry replacing the old per-context LogbackAppenders.Configurator/appender/encoder classes inlogger.tooling-api-implJVM process (separate from Android, launched viaProcessBuilder) off its own Logback wiring, keeping RPC log forwarding via a plainExternalSink.testing/common's Logback configurator (now automatic JVM behavior).sentry-logbackbridge (manualSentryAppendercast against Logback'sLoggerContext) with a directSentry.addBreadcrumbsink, preserving the existing WARN+-only threshold; removes the last Logback traces from build files (buildscriptclasspath entry, anEnvUtil/DesugarEnvUtildesugaring patch, version catalog entries).Commit-by-commit review is recommended — see individual commit messages for the reasoning behind each module's migration.
Test plan
:app:assembleV8Debugbuilds clean:logger,:subprojects:tooling-api-impl,:testing:common,:testing:toolingall compileIdeGlobalLogBufferconsumer-level filtering and theExternalSinkmechanism behave correctly (removed before landing)logbackreferences in source/build files🤖 Generated with Claude Code