[Feat] [SDK-399] Capture logcat output as telemetry events#369
[Feat] [SDK-399] Capture logcat output as telemetry events#369buongarzoni wants to merge 9 commits intomasterfrom
Conversation
|
@claude review |
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, reopen this pull request to trigger a review.
|
@claude review |
There was a problem hiding this comment.
All previously-flagged blocking issues have been addressed in follow-up commits; only the two documentation/consistency nits posted inline remain.
Extended reasoning...
Overview
This PR adds an opt-in Android logcat capture feature that records app logcat output as Rollbar LOG telemetry events, with a configurable minimum level. The change touches AndroidConfiguration (new builder options), introduces LogcatTelemetryCapture (a small reader-thread wrapper around logcat -v threadtime -T 1 *:<priority>), wires lifecycle into Rollbar.init/close, and adds a focused unit test suite. A Gradle testOptions { unitTests.isReturnDefaultValues = true } flip lets the test JVM exercise code paths that touch the Android stubs.
Security risks
Low. The subprocess is invoked via ProcessBuilder with a fixed argv (no shell), and the only variable component (priorityFilter) comes from a closed enum mapping in logcatPriorityFor(), so command injection isn't reachable. The capture only sees logs already emitted to logcat by the app's own UID, so no privilege escalation. The main consideration is data exposure: third-party libraries logging PII/secrets at WARN+ would now flow into telemetry — but that is the documented contract and consumers opt in explicitly.
Level of scrutiny
Moderate. The feature touches the SDK init/close lifecycle and spawns a long-lived background reader thread, which warranted close review of restart semantics, process-death handling, and ring-buffer replay. The author has now addressed all of those: V-priority filtering aligned with mapPriorityToLevel (b5f0407), unexpected-EOF cleanup that resets running/process so start() can recover (858a713), -T 1 to skip historical buffer (0f13b1a), and recordLogEventFor instead of recordManualEventFor (e63b0af). The unit tests cover the parser, level threshold, self-tag filter, tracker-throw containment, and the restart-after-EOF path.
Other factors
The two remaining issues are nits and non-blocking: (1) the javadoc on captureLogsAsTelemetry still says "manual telemetry events" after the implementation switched to LOG, and (2) ConnectivityDetector.java:49 is the lone Rollbar-internal logger that doesn't use Rollbar.TAG, so a single one-shot warning could slip past the selfTag exact-match filter under a narrow trigger condition. Both are inline-commented for the author and neither warrants delaying merge.
There was a problem hiding this comment.
All previously-raised bugs and nits have been addressed across the recent commits; LGTM.
Extended reasoning...
Overview
This PR adds a new opt-in feature for the Android SDK that captures logcat output as telemetry events. Changes are concentrated in a new file (LogcatTelemetryCapture.java, ~200 lines) plus small additions to AndroidConfiguration (two new builder options), Rollbar.java (init/teardown wiring), ConnectivityDetector.java (one-line tag swap), and minor javadoc cleanups in the rollbar-java telemetry interface. A new test file (LogcatTelemetryCaptureTest.java) provides good coverage of priority mapping, threshold filtering, self-tag suppression, exception isolation, and post-EOF restart behavior.
Security risks
None significant. The feature spawns a logcat subprocess scoped to the app's UID — there is no command injection vector since arguments are fixed string literals (only the priority filter is interpolated, and it comes from a closed enum). No new network surface, no new data sources, no auth/crypto changes. The captured log content stays in memory and is only flushed alongside Rollbar payloads the user already opts into.
Level of scrutiny
Moderate: this is a new background-thread feature touching subprocess management and reader-thread lifecycle, which historically is a source of leaks and zombie state. The earlier review surfaced four real bugs in exactly those areas (filter mismatch, dead-process state leak, ring-buffer replay, wrong telemetry type). The author addressed all four in commit e63b0af, then addressed both follow-up nits (ConnectivityDetector tag in 41da663, javadoc wording in ea3a83c). The current state passes a re-read of the diff against the original concerns.
Other factors
- Feature is opt-in via
captureLogsAsTelemetry(true)(default false), so the blast radius for any latent issue is bounded to consumers who explicitly enable it. - Test coverage includes the previously-broken paths:
logcatPriorityFor(DEBUG) == "V", restart-after-EOF via theAtomicIntegerfactory call counter, andrecordLogEventFor(notrecordManualEventFor) verification. - The PR author iterated responsively through two review rounds. With no remaining concerns and no new bugs flagged this round, approval is warranted.
Description of the change
Automatically captures Android log output and records it as telemetry events, giving Rollbar users richer context in their telemetry trail at the time an error occurs.
How to enable
Pass
captureLogsAsTelemetry(true)when building your AndroidConfiguration:When enabled, log lines emitted via android.util.Log (and any other source writing to logcat from the app's UID, including third-party libraries) at or above the configured minimum level are recorded as TelemetryType.LOG events
with Source.CLIENT.
Configuring the minimum log level
By default only WARNING and above are captured. Use logTelemetryMinimumLevel to capture more or fewer entries:
Capture INFO and above
Capture only errors
Supported levels (mapped from logcat priorities):
Notes
Type of change
Related issues
Checklists
Development
Code review