Greenstand / Greenstand/treetracker-android

Logging diagnostics upgrade — Crashlytics correctness, custom keys, mapping uploads

Open
#1,291 13 comments 0 reactions 1 assignee Claimed by @thetwai-dev View on GitHub
2.3 chore Documentation refactor triage
Dominant language
Kotlin
Stars
101
Forks
116
PR merge metrics
No merged PRs in 30d

Description

## Summary

Three defects in the current logging stack make it hard to triage org-reported "trees aren't syncing" bugs. None are covered by #1236 (which is scoped to error-handling consistency across sync paths). This issue tracks the Crashlytics-side infrastructure work that gives #1236's cleanups real diagnostic value, plus the build-config fix needed for deobfuscated stack traces.

## Problem

1. **`ExceptionLogger` forges synthetic non-fatals.** Every `Timber.i` / `Timber.w` in release wraps the message as `Exception(message)` and calls `recordException` (`analytics/ExceptionLogger.kt:33-35`). Operational logs from `OrgRepo`, `SplashScreenViewModel`, `CaptureFlowNavigationController` flood Crashlytics as fake crashes — burying real ones and consuming quota.

2. **No R8 mapping-file upload.** `release` minifies (`app/build.gradle:72`) but no `firebaseCrashlytics { mappingFileUploadEnabled true }` is set. Production stack traces are obfuscated.

3. **Missing custom keys.** `ExceptionDataCollector` already wires nine custom keys today (`is_syncing`, `user_wallet`, `power_user_wallet`, `destination_wallet`, `session_note`, `organization_name`, `is_in_session`, `route`, `last_route`). Missing: `capture_session_uuid`, `pending_upload_count`, `last_sync_timestamp`, `installation_id`, `is_online`, `app_version`, `build_type`, `upload_queue_snapshot`. These are the fields needed to filter Crashlytics by planter, session, and device state when triaging a single org's reports.

## Proposed action items

### 1. Fix `ExceptionLogger` rule table (`analytics/ExceptionLogger.kt`)

- [ ] `Timber.i` / `Timber.w` → `firebaseCrashlytics.log(...)` breadcrumb; no `recordException`.
- [ ] `Timber.e(throwable, "msg")` → breadcrumb **and** `recordException(throwable)` with the real throwable.
- [ ] `Timber.e("msg")` (no throwable) → breadcrumb only. Never call `Exception(message)`.
- [ ] `Timber.v` / `Timber.d` → no Crashlytics API call.
- [ ] Add `ExceptionLoggerTest.kt` covering all four cases with a mock `FirebaseCrashlytics`.
- [ ] Add KDoc header documenting the rule table + three do/don't rules (no PII; no direct `Firebase.crashlytics.*` outside `analytics/`; `Timber.e("…")` ≠ non-fatal).

### 2. Build-config fixes (`app/build.gradle`, `application/TreeTrackerApplication.kt`)

- [ ] `firebaseCrashlytics { mappingFileUploadEnabled true }` in `release`.
- [ ] `firebaseCrashlytics { mappingFileUploadEnabled false }` in `debug`, `dev`, `beta`, `prerelease`.
- [ ] Forced-crash on a release build shows deobfuscated frames in Crashlytics.
- [ ] `TreeTrackerApplication.onCreate` plants `DebugTree` when `DEBUG_ENABLED || BETA`, `ExceptionLogger` otherwise (beta testers stop flooding Crashlytics).
- [ ] No regression in `DEBUG_ENABLED`-gated UI on the beta variant.

### 3. `CrashKey` registry + new custom keys (`analytics/CrashKey.kt`, `analytics/ExceptionDataCollector.kt`)

- [ ] New `CrashKey` sealed/enum registry; the nine existing string constants in `ExceptionDataCollector` migrated.
- [ ] `ExceptionDataCollector.set(CrashKey, String?)` and `set(CrashKey, Boolean)` overloads added.
- [ ] Existing keys still set as today (`user_wallet`, `org_name`, `is_in_session`, `route`, etc.) — no regression.
- [ ] **New** key `capture_session_uuid` set in `models/SessionTracker.kt` on session start, cleared on `endSession()`.
- [ ] **New** key `pending_upload_count` set in `background/TreeSyncWorker.doWork()` and on `SessionTracker.endSession()`.
- [ ] **New** key `last_sync_timestamp` set on both success and failure branches in `usecases/SyncDataUseCase.kt` (ISO-8601).
- [ ] **New** key `installation_id` set after `FirebaseInstallations.getInstance().id.await()` resolves in `usecases/SyncDataUseCase.kt:52-56`. Code comment clarifies this is **per-device, not per-planter** on shared phones.
- [ ] **New** key `is_online` updated on every `usecases/CheckForInternetUseCase.kt` call.
- [ ] **New** keys `app_version`, `build_type` set once in `TreeTrackerApplication.onCreate()` post-Koin start.
- [ ] **New** key `upload_queue_snapshot` updated whenever `TreeUploader.uploadTreeBundles()` runs.

### 4. First execution slice of #1236 (closes #813)

- [ ] `usecases/UploadImageUseCase.kt:38-47` → `Timber.tag("UploadImage").e(ace, "S3 upload failed for ${params.imagePath}")` (currently drops `ace`).
- [ ] `models/TreeUploader.kt:89-91` → `Timber.tag("TreeUploader").e(e, "Bundle upload failed for ${treeIdBundle.size} trees")` (currently drops `e`).
- [ ] `usecases/SyncDataUseCase.kt:91` → `Timber.tag(TAG).e(e, "Sync failed")` (currently drops `e`).
- [ ] `usecases/SyncDataUseCase.kt:161` → `Timber.tag(TAG).e(e, "$tag step failed")` (currently has tag but drops `e`).
- [ ] An `AmazonClientException` from image upload now produces a Crashlytics non-fatal with the real exception stack trace, not a synthetic one.
- [ ] `TreeCapturer.kt:99-101` already correct — leave alone.

## Open questions

1. Detekt `ForbiddenImport` rule banning `com.google.firebase.crashlytics` outside `analytics/` — useful guardrail or scope creep? Suggest filing as a follow-up.
2. `installation_id` is per-device, not per-planter on shared phones. OK to record with a code comment, or do we want a different identifier?
3. `upload_queue_snapshot` key shape — single short string `pending=N;oldest=X;last_error=Y;last_success=Z` vs. N flat keys?
4. Who owns the Firebase Console step (first mapping-file upload requires console access)?
5. After the four action items ship, is Crashlytics-with-breadcrumbs enough for org bug triage, or do we still need an on-device log file? Revisit after a month of real reports.

## References

- Parent: #1236 (Standardize error handling across sync and upload paths)
- Closes: #813 (`Timber.e("NewTree upload failed")` does not notify user, just loops)
- Sibling proposal pattern: #1289
- Already shipped: #1199 / #1201 / #1215 (HTTP body gating to DEBUG, `printStackTrace()` removal)

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.