refactor: [SDK-5065] remove the otel observability path and OpenTelemetry dependency - #2724
refactor: [SDK-5065] remove the otel observability path and OpenTelemetry dependency#2724abdulraqeeb33 wants to merge 28 commits into
Conversation
…etry dependency The multiplatform logger module is validated in production, so the legacy OpenTelemetry pipeline it was built to replace is now dead weight. Keeping both meant shipping two ANR detectors, two crash reporters, two platform providers and two lifecycle managers behind a startup feature-flag branch, and it kept the io.opentelemetry tree on every integrator's classpath — the source of the recurring R8 "Missing class" failures in SDK-4820 and SDK-5006. The logger pipeline is now unconditional. LoggerModuleSwitch, the SDK_CUSTOM_LOGGING gate and resolveCustomLoggingEnabled are gone, which also fixes the first-launch gap: with no cached config the switch defaulted to otel, so a freshly installed app would have had no observability at all once otel was deleted. Code the logger path shared with otel is kept and renamed off the otel prefix rather than deleted: OtelPlatformProvider now implements ILoggerPlatformProvider directly (retiring the adapter), OtelIdResolver becomes LoggerIdResolver, and the OtelConfig/OtelSdkSupport pair becomes ObservabilityConfig/ObservabilitySdkSupport. The crash directory keeps its `onesignal/otel/crashes` path on purpose. Renaming it would orphan logger-owned records an upgrading install still has pending; OTel-format records left in it are reclaimed by the existing suffix-based purge. Verified: no io.opentelemetry in any published module's releaseRuntimeClasspath or POM, none in the release APK, and the example app minifies under R8 full mode for both flavors with no missing-class diagnostics. Co-authored-by: Cursor <cursoragent@cursor.com>
📊 Diff Coverage ReportDiff Coverage Report (Changed Lines Only)Gate: aggregate coverage on changed executable lines must be ≥ 80% (JaCoCo line data for lines touched in the diff). Changed Files Coverage
Overall (aggregate gate)404/436 touched executable lines covered (92.7% — requires ≥ 80%) Per-file detail (informational; gate is aggregate above):
|
Robolectric loads classes through its own instrumenting classloader, which strips the source-location metadata JaCoCo uses to attribute execution. Every class exercised only by a @RobolectricTest therefore reported 0% coverage no matter how well tested it was, while plain-JVM tests in the same module reported ~96-100%. CrashDirCleanup's doc comment already alludes to this, noting that keeping the logic free of Robolectric is what gets it "counted by Jacoco on the plain JVM". The gap was invisible until the otel removal renamed ~180 lines of Robolectric-only-tested code, which moved them into the diff-coverage denominator and failed the changed-lines gate at 11%. Enabling includeNoLocationClasses fixes the attribution. Nothing about the tests changed, only what the report can see: LoggerPlatformProvider 1.2% -> 98.8% LoggerIdResolver 0.0% -> 90.7% LoggerLifecycleManager 0.0% -> 84.7% OneSignalCrashUploaderWrapper 0.0% -> 82.6% Logging 47.0% -> 84.0% OneSignalImp 28.7% -> 71.9% Untouched Robolectric-tested classes are now measured honestly too (AndroidLogAnrDetector 0% -> 49.5%, FileLogStore 0% -> 34.5%), so the reported figures reflect real coverage rather than a measurement artifact. Co-authored-by: Cursor <cursoragent@cursor.com>
…st seams Removing :otel took its disk-buffering config with it, including the 72h maxFileAgeForRead and the per-file/per-folder size limits. FileLogStore only had a lower age bound, and the purge deliberately skips owned .otlp records at any age, so a record that never uploaded — including one written while remote logging is off, which is never even read — would be retried every launch forever. Restore both bounds and delete over-limit records rather than merely hiding them from listReadable. The fold-in of OtelLifecycleManager also dropped its injectable factories, which left the surviving pipeline's try/catch isolation, ANR start/stop, and remote-sink wiring untestable. Restore the seams with production defaults so runtime wiring is unchanged, and port the fault matrix. Also correct the migration guide: the otel artifact is no longer published and Logging.setOtelTelemetry is gone, so "no API change" was wrong. Co-authored-by: Cursor <cursoragent@cursor.com>
…pgrade docs Round-2 review found the accumulation caps were enforced only in save(), so an install carrying a backlog from a build without caps — which includes the large 5.9.x cohort already on the logger path — was fully listed and re-POSTed every launch until a new crash happened to trim it. Both bounds now run on listReadable and deleteUnrecognizedEntries too, reclaiming before payloads are read so an over-cap directory is never fully loaded. The crash path keeps only a cheap bounded trim; bulk reclaim happens on the uploader's IO paths. The byte cap also treated the first over-budget record as a cutoff, so one oversized payload evicted the entire older backlog — the opposite of what the cap is for. Skip it instead, and add a per-record cap so an outsized payload is dropped alone. selectOverflowOwnedEntries now also pins the record save() just wrote, so a backwards clock step cannot make it sort oldest and delete it. disableFeatures cleared each field only after the teardown call returned, so a throwing stop()/unregister() left the field set and the start guards then treated the dead component as running for the rest of the process. The migration guide claimed all pre-upgrade crash records are deleted. That is true only for OTel-format records; logger-path records are uploaded normally, and telling integrators otherwise would misdirect support. Tests: JVM coverage for both selectors including boundary, tie-break, oversized and keepName cases; re-enable-after-teardown-failure cases; the enable-twice case now asserts something; and the fault suite no longer leaks a mock sink into the global Logging object. Co-authored-by: Cursor <cursoragent@cursor.com>
…cklog Round-3 review found two ways the retention policy could delete crash reports it was meant to protect. keepName pinned the just-written record but charged its full length to the shared budget. An oversized payload therefore started the budget over cap, every sibling failed the remaining-budget check, and the whole backlog was evicted -- then the uploader, which runs without keepName, dropped the oversized record too. One bad payload destroyed everything including itself. The test covering that path used a single-entry directory, so it could observe the retention but never the consequence. Separately, the cheap exit in enforceAccumulationCaps checked count and total bytes but not the per-record cap, so a lone 600 KiB report survived save() and was then deleted by the uploader before any upload was attempted. Fixed at the source instead of patching the selector: save() now refuses a payload over the per-record limit and says so, which makes "every stored record is within the shared budget" an invariant. Size is no longer grounds for eviction -- deleting a captured crash unread is worse than keeping it -- and each record now claims at most the per-record cap against the budget, so an oversized record inherited from a build without the write-time limit still gets an upload attempt without displacing anything. Also: startLogging never received the clear-before-teardown fix disableFeatures got, so a throwing shutdown() stranded a dead sink that NoChange would never replace; expired-but-undeletable records were filtered out of the byte accounting and could hold the directory over cap indefinitely; and three lifecycle tests spawned real ANR watchdog daemon threads that outlived the spec and wrote into the cache dir other specs assert on. Co-authored-by: Cursor <cursoragent@cursor.com>
applyAction committed currentConfig even when a component never came up. Since a stable remote payload produces an identical config on the next refresh, the evaluator returned NoChange and the dead crash handler, ANR detector or sink stayed down for the rest of the process. enableFeatures now reports whether everything started, the config is only committed once it did, and startLogging is null-guarded like its siblings so a retry cannot tear down a healthy sink. startLogging also only had half the teardown invariant: it cleared its own field but left Logging's global pointing at the old sink while shutting it down. Every log emitted between shutdown and the replacement being installed -- including the warn in that window -- went to a telemetry whose consumer was already cancelled, where it queued and was never drained. On a throwing factory the global stayed on the dead instance for the session. Reverts the ExpiryOutcome split from the previous commit. It was added on the theory that an expired record whose delete failed could hold the directory over cap while invisible to the byte accounting. Writing the test disproved it: expired records are by definition the oldest, so the selector always picks them for eviction rather than retention, and only retained records claim budget. Including them in the candidate set changes no outcome, so the two-set bookkeeping was inert complexity. Kept a test that the record stays unreadable when its delete fails, which is the part that does matter. Also drops a tautological assertion that passed regardless of keepName now that size is not grounds for eviction, replaces a counter mutated from six concurrent coroutines with an AtomicInteger, stops building a throwaway platform provider just to read a path the pure helper computes, and corrects two KDocs that still claimed the byte cap bounds disk rather than claim. Co-authored-by: Cursor <cursoragent@cursor.com>
Removing the OpenTelemetry path also removed the synthetic Throwable the ANR detector used to build, so ANR records stopped being serialized via stackTraceToString() and were hand-joined instead — no `type: message` header and no `\tat ` frame prefix. Ordinary crashes still went through stackTraceToString(), so the pipeline emitted two different stacktrace formats depending on record type, and consumers that parse `exception.stacktrace` as a Java stacktrace (frame extraction, grouping/fingerprinting, symbolication, the Grafana `^\s*at ` transform) silently stopped matching ANR records only. Both ANR paths now go through shared `buildAnrCrashData` / `buildBackgroundBlockCrashData` builders backed by one `formatJvmStacktrace` helper that emits the canonical layout. Chose hand-formatting over re-synthesizing a Throwable for two reasons: this runs on the ANR watchdog thread while reporting a possibly-wedged app, so avoiding a throwable allocation and its stack fill keeps it cheap and non-throwing; and a real exception class would put its fully-qualified name in the header, which would no longer match the bare `exceptionType` the record reports. Against drift, a test pins `formatJvmStacktrace` output against a real `Throwable.stackTraceToString()`, so the ANR and crash paths cannot diverge again without a red test. `exceptionType` values are unchanged — this touches the `stacktrace` field only. Co-authored-by: Cursor <cursoragent@cursor.com>
… size CrashDirCleanup is a near-duplicate of the shared CrashRetention policy in the KMP submodule and is slated for deletion once that lands and the pin is bumped. This PR may merge first, so the two correctness defects are fixed here rather than left to merge ordering. Ports commit a95117a from the KMP repo, deliberately excluding its CrashRetentionPolicy value type: that change exists to shorten Swift call sites, which have no Kotlin default arguments after the Objective-C export. Android has no such boundary, so the parameter-heavy signatures stay and the diff stays reviewable. Reclaim records dated far enough into the future to be unrecoverable. The read path gates on `now - lastModifiedMs >= minAgeMillis`, which a future timestamp never satisfies, and selectExpiredOwnedEntries ignored every negative age, so such a record was unreadable for its entire life while still holding a count slot and budget — and it sorted newest during overflow, so it displaced genuine records that could still have been uploaded. The threshold is a full retention window ahead of now, not merely "in the future". That preserves the deliberate protection against a modest backwards clock step, which is what the negative-age handling was there for: a record dated modestly ahead is still left to wait until the clock agrees it is old. Clamping the timestamp for ordering alone would not have been sufficient — a record clamped to nowMs still ranks as the newest entry and keeps its slot. selectOverflowOwnedEntries now takes nowMs and applies the same judgement when ordering. This is needed on Android for the same reason it is on iOS: FileLogStore.enforceAccumulationCaps runs on the crash write path and enforces caps without running an expiry pass first, so ordering cannot assume the zombie has already been removed. Ordinary future dates clamp to nowMs; unrecoverable ones sort last. The two uploader-side callers already had a `now` in scope. Make CrashDirEntry.lengthBytes required. Budget claim is `min(lengthBytes, maxRecordBytes)`, so the previous `= 0L` default meant a caller that omitted the size claimed nothing and disabled the byte budget for that record. Both production call sites already passed a real length, so this was latent — but several test cases relied on the default, which is exactly the hazard. Tests now pass an explicit size. Replace the test that pinned the bug as correct. It asserted a record dated two full retention windows into the future was correctly ignored, citing backwards- clock protection — but two windows ahead is not a clock step, and the case it described is an hour of skew. It is now split into a plausible one-hour backwards step that must be left alone and a boundary case at exactly one window, matching KMP. Also coerce formatCrashDirInventory's maxSample to at least zero. Both callers pass literals today, but List.take throws on a negative argument and this is a logging helper on a crash-adjacent path. Each new test was confirmed red against the reverted production change and green after: reverting the expiry clause fails only "reclaims a record dated past the window into the future"; reverting the overflow sort key fails only "a future-dated record is evicted before any record that could still upload"; reverting the maxSample coercion fails only "treats a negative sample size as zero". The three tests guarding the lower bound — the one-hour step, the exactly-one-window boundary, and the modestly-future ordering case — were confirmed red against an over-correction that reclaims any future date, since no under-correction can fail them. Behavior now matches the KMP implementation exactly; only the signatures differ, which is what the comparison should find when the duplicate is deleted. Co-authored-by: Cursor <cursoragent@cursor.com>
…licy Moves the pin from 87e87fd to 64ce06b, picking up: - #20 shared crash-record retention policy (CrashRetention / CrashRetentionPolicy / CrashDirEntry in commonMain, with 29 commonTest cases running on both JVM and iOS) - #21 bounded retry/backoff for remote log export Pointer change only; Android still uses its local duplicate of the retention logic, which the next commit removes. Co-authored-by: Cursor <cursoragent@cursor.com>
CrashDirCleanup.kt was a near-duplicate of KMP's CrashRetention, written only because the shared version did not exist yet. Now that it does, Android consumes it and the local copy goes, leaving FileLogStore responsible for nothing but File I/O — turning a directory listing into CrashDirEntrys and applying the decisions the shared selectors return. Pure refactor, no behaviour change. The shared bounds are identical to the ones the deleted constants carried (72h read age, 50 records, 2 MiB budget, 512 KiB per record, ".otlp"), and the selector bodies match line for line, including the full-window future-date threshold and the clamp-vs-sort-last ordering. Shape differs deliberately: the shared API groups the bounds into a CrashRetentionPolicy that every selector takes, so FileLogStore holds one CrashRetention.defaultPolicy instance and passes the same one everywhere rather than relying on per-call defaults. The inline cheap-exit in enforceAccumulationCaps is now CrashRetention.isWithinCaps, which shares the selector's capped accounting instead of restating it. CrashDirCleanupTest goes with the implementation it covered: KMP's CrashRetentionTest is a strict superset of its 22 cases, and runs them on both JVM and iOS. FileLogStoreTest covers Android's own file I/O and stays, asserting against the shared policy rather than copies of its numbers. Co-authored-by: Cursor <cursoragent@cursor.com>
Applies the review standard already applied to the KMP side: a comment should state a constraint the code cannot show, never provenance, never a narration of the next line, never an argument aimed at a reviewer that the change is correct. Cut across FileLogStore, AnrCheckEvaluator, LoggerLifecycleManager, AndroidLogAnrDetector, OneSignalCrashUploaderWrapper and their tests: - Provenance: references to the removed OpenTelemetry disk-buffering library, "mirrors the old otel behavior", "ported from the deleted otel equivalent". That history lives in this PR body and in the commits that removed the module. - Reviewer-facing justification: paragraphs defending the one-time cost of a crash-path trim, the testability of the pure decision core, and why the crash-dir path helper is preferred over building a provider. - Repetition: the AnrCheckResult doc comments were restated verbatim on the BlockClassification entries; the teardown-ordering invariant was spelled out in the production code and again in two test comments; the stack fingerprint rationale appeared in three places. Kept the comments where the obvious reading is wrong: the byte cap bounds the budget claim rather than disk bytes, expired names are returned even when the unlink fails, save() must use raw Logcat because Logging.info can run app listeners, keepName exists so save() cannot evict its own record, and ANR stacktraces must stay byte-identical to the crash path's format. Comments and KDoc only — no non-comment line is touched. Co-authored-by: Cursor <cursoragent@cursor.com>
`save never evicts the record it just wrote` passed with `keepName` removed from `enforceAccumulationCaps` entirely, so the wiring was unverified. Both sort keys clamp to `nowMs`, so the record `save` just wrote can never sort strictly oldest; it lands in a tie group with any backlog dated at or ahead of the clock, and its position inside that group is whatever the filesystem happens to list. Only the explicit reservation keeps it. Measured on the old fixture, eviction without `keepName` was a coin flip that landed the safe way 7 times in 25, which is why a single attempt looked green. The fixture now dates the backlog ahead of the clock and repeats, so a false pass is vanishingly unlikely; it fails without the reservation and passes with it. Also restores the note explaining why the `isWithinCaps` short-circuit is what makes a full sort acceptable on the crashing thread, and passes the policy to `formatInventory` explicitly so every shared-selector call site reads alike. Co-authored-by: Cursor <cursoragent@cursor.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Multi-model review (Opus 5, GPT 5.6 Sol, Grok 4.6) of the OTel removal and remaining logger path.
OTel deletion, leftover-reference cleanup, ANR stacktrace formatting, and the intentional {cacheDir}/onesignal/otel/crashes path look consistent with the stated intent. The untagged KMP pin is already listed as a merge prerequisite.
Act on
- Remote disable / log-level updates can be skipped after a partial Enable (3/3).
applyActiononly commitscurrentConfigwhen every component starts. The evaluator then treats prior state as disabled, so a laterisEnabled=falseHYDRATE isNoChangeand never callsdisableFeatures(). The same stuck-null config makes a later level change evaluate asEnableinstead ofUpdateLogLevel, andif (remoteTelemetry == null)leavesshouldSendpinned at the original level. Fault tests cover retry-on-identical-enable and disable-after-full-enable, not disable or level-change after a partial start.
Consider
FileLogStore.save()still lists and stats the whole crash directory on the uncaught-exception thread before the cheapisWithinCapscheck, including any inherited OTel backlog (1/3).initialize()/start()install process-global state, then log throughLogging(app listeners). A throwing listener leaves the field null, so retry can install a second UEH / ANR watchdog (1/3).
Noted / dismissed
- Untagged KMP pin vs the publish
vX.Y.Zgate — already documented as a merge blocker. - Write path skipping
selectExpiredOwned— crash-thread cost; class KDoc overclaims both bounds on every path. formatJvmStacktrace“byte-identical” vs the bare ANR type name — comment accuracy only.
Sent by Cursor Automation: PR Reviews
| private fun applyAction(action: ObservabilityConfigAction, newConfig: ObservabilityConfig) { | ||
| val applied = | ||
| when (action) { | ||
| is ObservabilityConfigAction.Enable -> enableFeatures(newConfig.logLevel ?: LogLevel.ERROR) | ||
| is ObservabilityConfigAction.UpdateLogLevel -> updateLogLevel(action.newLevel) | ||
| is ObservabilityConfigAction.Disable -> { | ||
| disableFeatures() | ||
| true | ||
| } | ||
| is ObservabilityConfigAction.NoChange -> { | ||
| Logging.debug("OneSignal: logger config unchanged") | ||
| true | ||
| } | ||
| } | ||
| if (applied) currentConfig = newConfig |
There was a problem hiding this comment.
Act on (3/3): holding currentConfig back until every component starts breaks the remote kill switch and log-level updates.
ObservabilityConfigEvaluator derives wasEnabled from old?.isEnabled == true. After a partial Enable (e.g. crash handler and remote sink up, ANR throws), currentConfig stays null:
- HYDRATE
isEnabled=false→NoChange→disableFeatures()never runs. Healthy sinks keep shipping and the UEH stays installed. - HYDRATE enabled at a new level →
Enableagain, notUpdateLogLevel.if (remoteTelemetry == null)then skipsstartLogging, soshouldSendstays closed over the original level. If the missing component later succeeds, committed config and the live predicate disagree for the rest of the process.
This is new relative to the old always-commit currentConfig = newConfig. LoggerLifecycleManagerFaultTest retries on an identical enable and disables after a full start, but never follows a partial start with a disable or a level change.
Track desired config separately from component health: always commit the last observed remote snapshot, call disableFeatures() whenever !new.isEnabled and anything is still up, and on Enable retry reinstall shouldSend if the requested level differs.
There was a problem hiding this comment.
Addressed, and this comment deserves more credit than it got — it called both bugs a day before a human reviewer independently found the first one, and I did not act on it at the time.
Point 1 (kill switch after a partial Enable) — fixed in 209132e81. applyAction now short-circuits on real liveness before consulting the evaluator's verdict:
if (!newConfig.isEnabled && isAnyFeatureLive()) {
disableFeatures()
currentConfig = newConfig
return
}Pinned by disable tears down live components after a partial-failure enable, verified red without the guard (ILogAnrDetector.stop() was not called).
Point 2 (Enable retry skipping a live sink) — fixed in the same commit. The guard now compares against the level actually in force rather than treating any live sink as correct:
if (remoteTelemetry == null || activeLogLevel != logLevel) startLogging(logLevel)Where I went wrong, which is the useful part. Your recommendation had three parts: always commit the last observed remote snapshot, reconcile teardown from liveness, and reinstall shouldSend when the level differs. I implemented the second and third but not the first — I kept the conditional commit and bolted liveness onto the disable path only.
That left the enabled paths still trusting currentConfig, and a later review found exactly the hole that leaves: if the sink factory throws during UpdateLogLevel, currentConfig keeps {enabled, ERROR} with no sink, and every subsequent identical HYDRATE returns NoChange — hardcoded to succeed, never inspecting liveness — so remote logging stays dead for the process. enableFeatures would repair it, but is unreachable because Enable requires !wasEnabled. Had I taken your first bullet as well, that state would not have been constructible.
Being fixed now, symmetrically rather than as another spot patch.
Two smaller notes from the same follow-up review, for the record: ObservabilityConfigEvaluator diffs raw nullable levels while both branches normalize null to ERROR, so (true, null) → (true, ERROR) is a no-op that still forces a full sink rebuild. And a separate cold-start path has the same desired-vs-actual shape — resolveRemoteLoggingEnabled derives enablement purely from log level and never reads the persisted isEnabled, so a kill switch delivered by omitting log_level is ignored until the next successful config fetch. Both are in the current fix.
|
Multi-model review:
Both come from the same gap: desired config vs actual component health. Treat Disable as “ensure off if anything is live,” and on Enable retry apply Consider
Noted / dismissed Crash-export “one failed file aborts the pass” is in the KMP pin, not Android-authored code. Retention KDoc drift and the non-root unlink test are nits. No dangling |
…iveness LoggerLifecycleManager conflated desired config with actual component health. currentConfig only advances once every component started, so a partial-failure Enable leaves components running under a config that was never committed. Two bugs followed from that gap: - A later disabled payload evaluated as null -> disabled, which the evaluator reads as NoChange, so the remote kill switch never tore anything down and the crash handler, ANR watchdog and remote sink kept running. - An Enable retry carrying a newer level skipped startLogging because the sink was healthy, then committed the new level while the sink still filtered at the old one, permanently for the process. Disable now runs whenever any component is actually live, and the sink guard compares against the level the sink was really started at rather than assuming a live sink is at the requested level. Also unwinds a partially started crash handler or ANR detector: initialize() can chain onto the process-global uncaught-exception handler before throwing, so dropping the reference let a retry install a second one and double-report. The component is unregistered before the field is cleared, which preserves the existing invariant that a field never points at something dead. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Thanks — both consensus findings were real and are fixed in 1. Partial Enable then Disable — confirmed and fixedConfirmed the exact path. With
if (!newConfig.isEnabled && isAnyFeatureLive()) {
disableFeatures()
currentConfig = newConfig
return
}
private fun isAnyFeatureLive(): Boolean =
crashHandler != null || anrDetector != null || remoteTelemetry != nullThe liveness gate is also what keeps it from thrashing: after teardown nothing is live, so repeat disabled payloads fall through to Proof. That is the kill switch being ignored. Restored, the suite is green. 2. Enable retry commits a level the sink never adopted — confirmed and fixedAlso confirmed. The guard treated any live sink as correct, so a retry entering The manager now tracks the level the sink was actually started at: if (remoteTelemetry == null || activeLogLevel != logLevel) startLogging(logLevel)Proof. 3. Host listener throwing after
|
Moves the pin from 64ce06b to 0513bf5, picking up: - #22 drop unused swiftVersion from the logger contract - #23 date crash records by name when attributes are unreadable (CrashDirEntry.lastModifiedMs is now Long?, age resolves through CrashRetention.effectiveWriteTimeMs, selectOverflowOwned takes keepNames: Set<String>) 0513bf5 is the head of an unmerged PR branch, not a commit on main. The pin must be re-pointed at the squashed merge commit once #23 lands. Pointer change only; the Android adoption is the next commit. Co-authored-by: Cursor <cursoragent@cursor.com>
…er declares KMP #22 removed swiftVersion from ILoggerPlatformProvider, so the Android override stopped overriding anything and failed compilation once the submodule pin moved. The override returned null on every path and the property was never read on Android, so nothing is emitted differently; the test that asserted the null is removed with it. Co-authored-by: Cursor <cursoragent@cursor.com>
…ecord File.lastModified() returns 0 on I/O failure, which the retention policy read back as an age of "since the epoch" — past every ceiling, so a record the crash handler had just written was reclaimed as ancient. Adopts the shared API from KMP #23, which makes CrashDirEntry.lastModifiedMs nullable and resolves age through CrashRetention.effectiveWriteTimeMs, recovering the write time from the {millis}-{uuid}.otlp name when the filesystem cannot supply it. - listEntries and the crash-dir inventory report a non-positive lastModified() as unknown instead of fabricating an epoch timestamp - the listReadable age gate goes through effectiveWriteTimeMs, so a record cannot be withheld from readers by one clock while being reclaimed by another - an undatable record (no readable timestamp, no millis in its name) is withheld from readers rather than treated as age zero, and is never expired: a failed read is not evidence of age. It still counts toward the caps and stays evictable, so it cannot leak - selectOverflowOwned takes keepNames: Set<String> FileLogStoreTest had no coverage for a zero or unknown mtime at all — its write() helper always set a real timestamp — so none of this was exercised. Adds five cases, each verified to fail with the behavior reverted. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Correction: my refutation of item 4 was wrong, and you were right. Flagging it before it gets buried. I claimed a failed val previous = remoteTelemetry
remoteTelemetry = null
activeLogLevel = null
Logging.setLoggerTelemetry(null) { false }
...
val telemetry = remoteTelemetryFactory(platformProvider, httpSender) // throws
This is the same desired-vs-actual divergence as the kill-switch bug, and my fix for that only taught Two changes rather than one:
My test Fix incoming; I will re-verify with a test that fails on the sequences above rather than the one that already passed. |
Two paths disagreed about what "enabled" means.
LoggerIdResolver.resolveRemoteLoggingEnabled derived it from the
cached log level alone, while the HYDRATE path read
remoteLoggingParams.isEnabled. Persisted state could hold both at
once: ConfigModelStoreListener only wrote logLevel when the backend
sent one, so a server disable — which is expressed by omitting
log_level — left a previous session's ERROR sitting beside a fresh
isEnabled=false.
Cold start then read enabled=true and brought up the crash handler,
the ANR detector and the remote sink, and the buffered-crash upload
with them. It stayed up until a HYDRATE disabled it, so on a session
whose params fetch never succeeded the kill switch did nothing at all.
- resolveRemoteLoggingEnabled requires both a usable level and an
isEnabled that does not veto it, reading both from the same
remoteLoggingParams object
- ConfigModelStoreListener writes logLevel unconditionally. Every
neighbouring field treats absent as "unchanged", but for remote
logging absent is the revocation itself, so the previous level must
not survive it
An absent isEnabled means "fall back to the level". Caches written
before the field existed carry a level and nothing else, and reading
that as off would take observability away from every install on
upgrade until a fetch landed. Nothing writes a disable without also
writing the field, so absent is never a disable.
LoggerIdResolverTest asserted the bug: {"logLevel":"ERROR",
"isEnabled":false} was pinned as resolving enabled=true. Reworked,
plus the upgrade case and cold-start coverage driving the real
platform provider off SharedPreferences.
Co-authored-by: Cursor <cursoragent@cursor.com>
startLogging tore down before it built: it detached the field and
Logging's global, shut the old sink down, and only then called the
factory. A throwing factory therefore cost a sink that was serving
perfectly well, and updateLogLevel returning false left currentConfig
still claiming {enabled, ERROR} over nothing.
Only one recovery path worked. A repeat of the failed config re-ran
UpdateLogLevel, and that is the case the tests covered. A HYDRATE back
to the old level collapsed to NoChange, which was hardcoded to succeed
and never looked at the sink; and with one params fetch per session
being the norm, usually no further HYDRATE arrived at all. Both left
remote logging dead for the session. enableFeatures could have
repaired it but was unreachable, since Enable requires !wasEnabled.
- startLogging builds the replacement first, so a failure costs
nothing. The field and the global still move together, now to the
new instance and before the old one is shut down, which keeps the
invariant the previous ordering existed to protect: neither ever
points at a dead sink, and no log falls into a cancelled consumer
- NoChange reconciles against actual liveness instead of asserting
success, mirroring the disable path
The fault test named "retried on the next identical config" only ever
exercised a repeat of the failed config; renamed to say so, and
supplemented with the revert-to-old-level case and one asserting the
incumbent sink is neither shut down nor detached.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Both fixed, in The failed level change no longer costs the sink. Verified by reverting to teardown-first: Being straight about coverage: with build-first in place, the Separately, the same divergence existed on cold start, found while fixing the above. Fixed on both sides, deliberately: the resolver now honors I reproduced this one myself. Making Absent One sharp edge found on the way, unrelated to this PR but worth knowing: |
Picks up KMP #23 through 7a3eea3. The one behavior change Android sees is LogCrashUploader.start() now gating on isRemoteLoggingEnabled as well as the level, so a device holding {"logLevel":"ERROR","isEnabled":false} no longer exports its buffered crash records while the lifecycle manager correctly stays off. Also carries the protected-names retention fixes and a CrashRetention comment trim. Co-authored-by: Cursor <cursoragent@cursor.com>
Cold start and HYDRATE disagreed about what NONE means. LoggerIdResolver.resolveRemoteLoggingEnabled reads a cached NONE as disabled, but RemoteLoggingParamsObject defaults isEnabled to logLevel != null, so the same payload arriving over the wire produced ObservabilityConfig(isEnabled = true, logLevel = NONE) and the evaluator returned Enable(NONE). The crash handler, the ANR detector and a sink whose shouldSend predicate is always false all came up. Nothing consumes what that produces. Crash records written during a NONE session are only ever shipped by LogCrashUploader, which returns early on a NONE level, so they sit in the crash directory until FileLogStore's cap enforcement evicts them. The components cost battery and disk and deliver nothing, and the next cold start reads the same cache as off, so the divergence is not even stable within an install. Fixed at the parse boundary rather than in the evaluator so the persisted cache agrees with itself: hydration now writes isEnabled=false beside logLevel=NONE, which is exactly what resolveRemoteLoggingEnabled reads on the next launch. ObservabilityConfig stays a faithful snapshot of the model rather than a second place NONE has to be special-cased. Three tests, each verified to fail with the behavior reverted: NONE off the wire parses disabled, a shippable level still parses enabled, and a NONE fetch caches disabled through ConfigModelStoreListener. Co-authored-by: Cursor <cursoragent@cursor.com>
Bumps the KMP submodule to ecdb9f0 and moves the call sites the two new
parameters there require. One commit rather than a bump followed by a
fixup: both signatures changed, so the bump alone does not compile.
`save` wrote `{millis}-{uuid}.otlp.tmp` by appending to the target name.
That does not end in `.otlp`, so it is foreign, and once name-derived
dating was restricted to entirely-numeric foreign names it stopped
parsing at all. `deleteUnrecognizedEntries` is the only pass that ever
reclaims a stray temp and it needs an age, so a write interrupted between
`writeBytes` and `renameTo` was stranded on disk for the life of the
install — and this class's own KDoc claimed the opposite. Both names now
come from the policy, which recognises its own `ownedTempSuffix`. The
bytes on disk are unchanged: the name this produces is the one it always
produced.
`enforceAccumulationCaps` checked `isWithinCaps` without the `keepNames`
it hands the selector. The selector excuses protected records their byte
claim and the check charged them, so five 500 KiB records with the newest
in flight read as over cap while the trim kept everything and returned
nothing. That is the steady state once the directory is near the ceiling,
since the newest record is protected on every write, and it means the
crashing thread sorts the whole directory and deletes nothing — on the
one path the cheap exit exists to keep cheap. The comment asserting the
two cannot disagree is replaced with what actually keeps them agreeing.
`listReadable` now passes the policy to `effectiveWriteTimeMs`, which
takes one as of the bump. No behavior change while this store uses
`defaultPolicy`, but the read gate and the reclaim passes are now
provably reading the same policy rather than coincidentally.
Two tests, each verified to fail with the KMP behavior reverted: an
interrupted write with an unreadable timestamp is purged while
`3-tmp.dat` is left alone, and one younger than the age gate survives.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Potential issues:
|
… KMP Picks up KMP 7c41f61 and makes the matching comment edit here. The previous commit said charging protected records reports over cap "on every write near the ceiling"; simulating repeated writes shows it is bounded — never twice in a row, once during ramp-up with uniform record sizes, and around a quarter of over-cap writes only when sizes vary. Still a full directory sort on the crashing thread that deletes nothing, just not on every write. Comments and the submodule pointer only, no behavior change. Co-authored-by: Cursor <cursoragent@cursor.com>
The previous commit swept in an unintended rewrite of the class KDoc that was not part of that change and dropped four constraints a reader could otherwise break: that [listReadable] takes age from [CrashRetention.effectiveWriteTimeMs] so a file the crashing process may still have been writing is never read; that `maxTotalBytes` bounds budget claim rather than raw disk bytes, and why the two differ; that both bounds are enforced on every path that touches the directory rather than only after a write; and that over-limit records are deleted rather than merely hidden from [listReadable]. Restored verbatim. The only intended edit in that commit, the cap-check comment, is kept. Co-authored-by: Cursor <cursoragent@cursor.com>
Constraints move to the declaration each one governs rather than accumulating in class KDoc, and the rationale behind them moves to the PR description. Co-authored-by: Cursor <cursoragent@cursor.com>
reclaimOverLimitRecords omitted keepNames and took the default, so the over-limit reclaim reached from listReadable and deleteUnrecognizedEntries ran with emptySet() while looking like it had been considered. An earlier review asked whether the uploader paths pass protected names; the default is why the answer was never visible in the code. Android has no in-flight write registry to pass, so this states emptySet() outright and names SDK-5129, which tracks building one. Making the gap legible is the point: KMP now requires the argument, so a future reader sees a deliberate empty set rather than an omission. The exposure is narrower than the ticket currently describes. save() writes to a .otlp.tmp name and renames, and the selector filters on isOwned, so it never sees the temp and the .otlp name only appears once the content is complete. The one window where a partially written owned record is visible is the renameTo fallback, which writes straight to the target when the filesystem refuses the rename. Also corrects the save() comment: the shared suffixes do not make an interrupted write datable on their own, the leading millis in the name does. spotlessCheck, detekt and :core tests pass as separate invocations, detekt with the seven pre-existing findings in OneSignalDispatchers and FeatureFlagsRefreshService. Co-authored-by: Cursor <cursoragent@cursor.com>
Moves the pin from 7c41f61 to 7051a24, the head of KMP's ar/sdk-5065-unknown-mtime. CrashRetention no longer defaults keepNames or policy on any selector, so every call site here must state both. FileLogStore already did after the previous commit; nothing else in this repo calls the selectors. The pin references an unmerged commit so this branch can build and be reviewed ahead of KMP #23, and has to be re-pointed at the squashed merge commit once that lands. spotlessCheck, detekt and :core tests pass against the new pin as separate invocations. Co-authored-by: Cursor <cursoragent@cursor.com>
Ticket IDs belong in history, not in the source. The constraint the comment exists for, that this path has no in-flight registry and the only partial-write exposure is save()'s renameTo fallback, is unchanged. Co-authored-by: Cursor <cursoragent@cursor.com>
The wrapper built remote telemetry in a `by lazy` and never shut it down. LogTelemetryRemoteImpl's constructor starts a LogBatchProcessor whose init launches a one-second coroutine loop, so every start left a timer running for the life of the process. Review reported this against the disabled path, where it is worst: the kill switch exists to stop background work and instead started some. But the leak is not specific to that path. Nothing in com.onesignal.debug calls shutdown() at all, and LogCrashUploader only ever calls exportEncoded, which posts directly and bypasses the batch queue. This processor therefore never receives a record on any path, enabled or disabled, and ticked until process death regardless. Composition moves into the pass and the remote is shut down in a finally. Gating construction in the wrapper was rejected: the enabled check lives in the shared LogCrashUploader.start(), and an early return here would skip purgeUnrecognizedEntries(), which must still reclaim legacy otel files when disabled. Fixing this in the shared LogCrashUploader was rejected too. iOS passes OSRemoteLogger's own long-lived telemetry to createCrashUploader and uses the same instance for all live remote logging, so shutting it down from the uploader's early return would tear down the iOS transport. The uploader does not own the remote it is handed; the wrapper does. iOS has no equivalent leak: OSRemoteLoggingController only constructs OSRemoteLogger when the config is enabled and tears it down through stopRemoteLogging(). shutdown() is safe here with nothing exported. The buffer is always empty, so the bounded flush returns without suspending, and shutdownRemote logs rather than throws so teardown cannot replace an upload failure. Two tests cover it, both wrapping the genuine remote so the real runBlocking drain runs. Reverting the finally times both out. The four KMP tests pinning the disabled-path purge and the Android reclaim test are unaffected. spotlessCheck, detekt and :core tests pass as separate invocations, detekt with the seven pre-existing findings in OneSignalDispatchers and FeatureFlagsRefreshService. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Good catch, and it is worse than you framed it. Fixed in Nothing in the Android And the batch processor never had anything to do in the first place. Fix: composition moved out of the val remote = LoggerFactory.createRemoteTelemetry(platformProvider, httpSender)
try {
val fileStore = FileLogStore(platformProvider.crashStoragePath)
LoggerFactory.createCrashUploader(platformProvider, remote, fileStore, logger).start()
} finally {
shutdownRemote(remote)
}On your first suggestion, gating construction: rejected, because the enabled check lives inside the shared We also tried to fix this in the shared Verified by removing the One deliberate trade: the wrapper now rebuilds the platform provider per |
2 participants
Description
One Line Summary
Deletes the legacy OpenTelemetry observability path, the
:otelmodule and the wholeio.opentelemetrydependency tree, and restores the retention and export-retry behavior OTel had been supplying implicitly, as shared KMP code.Closes SDK-5065.
Details
Motivation
The
loggermodule is validated in production, so OTel is dead weight, meaning two ANR detectors, two crash reporters, two platform providers and two lifecycle managers behind a startup feature-flag branch, plusio.opentelemetryon every integrator's classpath. That tree is the source of the recurring R8Missing classfailures (SDK-4820, SDK-5006) and theFileStoragecrash-reporting failures seen on 5.9.3 through 5.9.5. Net +1,859 / −5,916 across 89 files.Scope
Deleted: the
:otelmodule, all six OpenTelemetry artifacts and their version pins,OtelLifecycleManager,OtelAnrDetector,AndroidOtelLogger,OneSignalCrashHandlerFactory,LoggerModuleSwitch/resolveCustomLoggingEnabled/ theSDK_CUSTOM_LOGGINGgate, andLogging.setOtelTelemetry.Renamed rather than removed, because the
loggerpath uses them:OtelPlatformProvider→LoggerPlatformProvider(now implementingILoggerPlatformProviderdirectly, retiring the adapter),OtelIdResolver→LoggerIdResolver, andOtel{Config,ConfigEvaluator,SdkSupport}→Observability*.AnrConstantsandAnrCheckEvaluatorwere already shared and are untouched.Non-obvious decisions
otelpath segment. Renaming it orphans logger-owned.otlprecords an upgrading install still has pending upload. Pre-upgrade OTLP blobs in the same directory are unreadable now and are reclaimed by the suffix-based purge.listReadable, because otherwise a record that never uploads wedges the backlog forever.File.lastModified()returns0for an I/O error, indistinguishable from a genuine epoch mtime, and an age of "since the epoch" is past every ceiling, so that would reclaim a crash record seconds after the handler wrote it. The shared policy re-derives the write time from the{millis}-{uuid}.otlpname; a record datable by neither clock is withheld from readers but never expired, since a failed read is not evidence of age.FileLogStore.saveruns inline on the crashing thread. It must not throw, it reports through raw Logcat only (aLoggingcall invokes app listeners, and a throwing listener would flip a successful write tofalse), and a cap-enforcement failure must not fail the write. TheisWithinCapsguard in front of the eviction selector is what keeps that affordable, because the selector sorts the whole directory.disableFeaturesclears each reference before the teardown call, andstartLoggingbuilds the replacement sink before discarding the incumbent.currentConfigadvances only once the requested state is actually in place, so a component that failed to start is retried by the next HYDRATE instead of collapsing toNoChange.Enableleaves components live under a config that was never committed, so a later "disabled" payload would evaluate toNoChangeand be ignored. Remote logging correspondingly requires both a usable level andisEnabled, since a server disable rewrites onlyisEnabled. But an absentisEnabledmeans a cache written before the field existed and must read as enabled, or every upgrade loses observability until a params fetch lands.resolveCustomLoggingEnabled()returnedfalsewhenever there was no cached config, i.e. on every first launch. Deleting the switch fixes that.Throwablethe ANR detector built, so ANR records lost thetype: messageheader and\tatframe prefix, so anything parsingexception.stacktraceas a Java stacktrace would have silently stopped matching ANRs only. Both ANR paths now share oneformatJvmStacktracehelper. Unknown app state is treated as foreground, so a genuine ANR is never downgraded.Cross-repo sequencing
KMP #20 and #21 are merged. OneSignal-iOS-SDK#1725 adopts the same shared policy on iOS, independently of this PR; both platforms consume the same pin and build the module from pinned submodule source, so no KMP release is required.
The submodule pin is
7c41f61, which sits on the unmerged KMP #23 branch. This cannot merge before #23 does, and once #23 lands the pin has to be re-pointed and the:coresuite re-run.Testing
Unit testing
Full
:coresuite,spotlessCheckanddetektpass. Diff coverage is 94.0% against a required ≥ 80%.LoggerLifecycleManagerTestandLoggerLifecycleManagerFaultTest. The lifecycle manager is now the only observability path and had no direct tests. The fault suite pins that one failing component cannot stop the others and that nothing propagates to the caller, since this runs inside SDK init.LoggingRemoteTest, replacingLoggingOtelTest, which could only assert "does not crash" because OTel's types were invisible to mocks.FileLogStoreTestextended for the retention bounds, expiry, clock skew, unreadable timestamps and failed unlinks, plus an upgrade-path test inOneSignalCrashUploaderWrapperTest.keepNamewrite-path test repeats 25 times deliberately:selectOverflowOwnedclamps its sort key tomin(lastModified, now), so the guarantee is about tie ordering rather than age, and a single attempt passes roughly three times in four even with the wiring removed. Verified red against that removal.Coverage tooling. The gate initially read 11%, which was a measurement bug: Robolectric's instrumenting classloader strips the source-location metadata JaCoCo attributes execution with, so a Robolectric-only-tested class reports 0% however well tested it is. This is pre-existing and repo-wide (
AndroidLogAnrDetectorandAndroidLogCrashHandlerare 0% onmaintoday), and surfaced here only because renaming moved those lines into the diff denominator. EnablingincludeNoLocationClassesfixes attribution with no test change.Manual testing
No
io.opentelemetryin the release AARs, the published POMs or the release APK.:app:assembleRelease -Pandroid.enableR8.fullMode=truesucceeds for both GMS and Huawei flavors with zero missing-class diagnostics, and the consumer R8 rules are clean.MIGRATION_GUIDE.mdcovers the dependency removal and how pre-upgrade crash records on disk are handled.Merge prerequisites
7c41f61, and the suite is re-run.sdk_custom_loggingremotely is no longer a mitigation. Needs explicit sign-off.telemetry.sdk.*disappears from log records, and anything grouping on...OtelAnrDetector$ApplicationNotRespondingExceptionmust move toApplicationNotRespondingException(SDK-5053). Lives inOneSignal/infra:dashboards/sdk/.loggeris the fallback. Resolved by deleting the switch.Follow-up (out of scope)
SDK_CUSTOM_LOGGINGlives in KMP and nothing gates on it after this PR, but it is the onlyAPP_STARTUPflag in the catalog, so deleting it removes the sole test subject forFeatureManager's startup-latching behavior on both sides. It should land with a replacement flag or a test-only fixture. Leaving it is harmless.Affected code checklist
OneSignalLogHttpSender, which now retries with backoffMIGRATION_GUIDE.md), no source-compatible API changeChecklist
:otelinterfaces, and the retention and retry work restores behavior the removal would otherwise have dropped silentlyMade with Cursor