refactor: [SDK-5065] remove the otel observability path and OpenTelemetry dependency - #2724
Open
abdulraqeeb33 wants to merge 13 commits into
Open
refactor: [SDK-5065] remove the otel observability path and OpenTelemetry dependency#2724abdulraqeeb33 wants to merge 13 commits into
abdulraqeeb33 wants to merge 13 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>
Contributor
📊 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)380/409 touched executable lines covered (92.9% — 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>
abdulraqeeb33
marked this pull request as ready for review
August 26, 2026 19:38
![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
Comment on lines
+148
to
+162
| 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.
Contributor
|
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>
Contributor
Author
|
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
|
abdulraqeeb33
enabled auto-merge (squash)
August 28, 2026 15:15
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.
Description
One Line Summary
Deletes the legacy OpenTelemetry observability path, the
:otelGradle module and the entireio.opentelemetrydependency tree, making the multiplatformloggerpipeline unconditional — and restores the disk-retention and export-retry behavior OTel had been providing by default, as shared KMP code.Closes SDK-5065.
Details
Motivation
The
loggermodule has been validated in production, so the OpenTelemetry pipeline it was built to replace is now dead weight. Keeping both meant:io.opentelemetryon every integrator's classpath. That tree is the source of the recurring R8Missing classfailures behind SDK-4820 (dontwarnrule) and SDK-5006 (shading work), and of theFailed resolution of: Lio/opentelemetry/contrib/disk/buffering/storage/impl/FileStoragecrash-reporting failures seen in production on 5.9.3–5.9.5 and 5.7.x/5.8.x.Net: +1,859 / −5,916 lines across 89 files.
Scope
Deleted
:otelGradle module in its entirety, plus itssettings.gradleinclude/substitution and the:coredependency.build.gradle. Transitive Jackson and AutoValue leave the classpath with them, so integrator APKs shrink and the optional-class-dontwarnrules become unnecessary (seeMIGRATION_GUIDE.md).OtelLifecycleManager,OtelAnrDetector,AndroidOtelLogger,OneSignalCrashHandlerFactory.LoggerModuleSwitch,resolveCustomLoggingEnabled, and theSDK_CUSTOM_LOGGINGgate.Logging.setOtelTelemetry/logToOtel, and the otel consumer R8dontwarnrules.CrashDirCleanup.kt/CrashDirCleanupTest.kt. Worth being precise about this one, since the file's history spans two PRs: feat(logger): [SDK-4939] wire Android FileLogStore for shared KMP legacy purge #2691 added it holding only the legacy-purge helpers, this PR added the retention policy to it, and then moved the whole thing into KMP. Net status in this diff is a deletion.Renamed, not deleted (audited as shared)
The ticket flagged these for audit, and all three turned out to be used by the
loggerpath, so they are renamed off the otel prefix rather than removed:OtelPlatformProvider→LoggerPlatformProvider, now implementingILoggerPlatformProviderdirectly. This retiresLoggerPlatformProviderAdapter, which existed only to bridge the two interfaces.OtelIdResolver→LoggerIdResolver(moved into thelogger.androidpackage, minus the flag resolver).OtelConfig/OtelConfigEvaluator/OtelSdkSupport→ObservabilityConfig/ObservabilityConfigEvaluator/ObservabilitySdkSupport.AnrConstantsandAnrCheckEvaluatorwere also audited — both are already shared byAndroidLogAnrDetector, so they are untouched.Deliberately unchanged: the crash directory path
The on-disk path stays
{cacheDir}/onesignal/otel/crashesdespite the name. Renaming it would orphan logger-owned.otlprecords that an upgrading install still has pending upload — a real data-loss regression. Records left by a pre-upgrade otel session are OTLP-disk-buffering blobs nothing can read anymore; they are reclaimed by the suffix-based purge inFileLogStore.deleteUnrecognizedEntries(driven byCrashRetention.selectUnrecognized), which satisfies the ticket's "uploaded or intentionally purged" criterion. The retained name is commented at the definition site.Restoring behavior OTel supplied implicitly
OTel's disk-buffering and OTLP exporter had bounded the crash cache and retried failed exports by default. Removing them without an explicit config change silently dropped both — nothing in the diff said so, which is precisely what made it easy to miss. Both are restored as shared KMP code so iOS gets the same guarantees rather than a second Android-only implementation:
Retention —
com.onesignal.logger.crash.CrashRetention/CrashRetentionPolicy, merged in OneSignal-KMP-SDK#20. Defaults: 72 h read-age ceiling, 50 records, 2 MiB budget claim, 512 KiB per-record write limit,.otlpowned suffix. Android'sFileLogStoreandOneSignalCrashUploaderWrappercall the shared selectors and apply the result withFileI/O.Both bounds are enforced on every path that touches the directory (
save,listReadable,deleteUnrecognizedEntries), and over-limit records are deleted rather than merely hidden fromlistReadable— otherwise a size-capped record would wedge the backlog forever. Future-dated records (clock skew) are reclaimed rather than treated as infinitely fresh.Export retry/backoff — bounded retry on the remote log export path, merged in OneSignal-KMP-SDK#21.
The
OneSignal-KMP-SDKsubmodule pin moves87e87fd2 → 64ce06b3to pick both up. Neither is visible in this repo's diff — review them in the KMP PRs.Cross-repo sequencing
No KMP release or published artifact is required. Both platforms consume the KMP module as a git submodule pinned to a commit and build it from source — Android via
substitute(module('com.onesignal:kmp')).using(project(':OneSignal:kmp'))insettings.gradle, iOS via the same submodule. The pin at64ce06b3already contains both merged PRs, so the shared code is live in this diff.Other behavioral changes worth reviewing
resolveCustomLoggingEnabled()returnedfalsewhen there was no cached config — i.e. on every first launch after install or upgrade. Deleting otel without addressing that would have left first-run sessions with no observability at all. Removing the switch entirely fixes this:LoggerLifecycleManagercomes up unconditionally, and features enable as soon as the first remote config arrives.Throwablethe ANR detector built, so ANR records stopped going throughstackTraceToString()and lost thetype: messageheader and\tatframe prefix. Ordinary crashes were unaffected, so the pipeline would have emitted two formats and anything parsingexception.stacktraceas a Java stacktrace — frame extraction, grouping, symbolication, the Grafana^\s*attransform — would have silently stopped matching ANR records only. Both ANR paths now sharebuildAnrCrashData/buildBackgroundBlockCrashDataover oneformatJvmStacktracehelper.LoggerLifecycleManagerretries on the next config refresh without tearing down healthy sinks.Testing
Unit testing
Full
:coresuite,spotlessCheckanddetektpass across every module. Diff-coverage gate passes at 362/385 touched executable lines (94.0%), against a required ≥ 80%.LoggerIdResolverTest,LoggerPlatformProviderTest,ObservabilitySdkSupportTest,ObservabilityConfigEvaluatorTest) so no coverage was lost with the otel deletions.LoggerLifecycleManagerTestandLoggerLifecycleManagerFaultTest— the logger lifecycle manager is now the SDK's only observability path but had no direct tests, since the deletedOtelLifecycleManagerTestwas the only lifecycle coverage. Asserts the config state machine by observing realUncaughtExceptionHandlerregistration, and covers the start-failure/retry path.LoggingRemoteTest, replacing the deletedLoggingOtelTest. The old suite could only assert "does not crash" because OpenTelemetry's types were not visible to mocks;ILogTelemetryRemoteis our own interface, so this verifies emission, filtering, exception attributes and sink-failure isolation.FileLogStoreTestfor retention: per-record write cap, count and byte caps enforced on the crash path, expiry, future-dated reclamation, that an over-cap record is evicted rather than hidden, and that an expired record whose unlink fails is still withheld from readers.keepNamewrite-path guarantee.selectOverflowOwnedclamps its sort key tomin(lastModified, now), so a just-written record can never sort strictly oldest — it lands in the top tie group, where its position comes fromlistFilesordering. The guarantee is therefore about tie ordering, not age, and a single-attempt test passes roughly three times in four even withkeepNamedropped. The test repeats the trial to make a false pass negligible, and was verified to fail against code with the wiring removed.OneSignalCrashUploaderWrapperTestproving a pre-upgrade otel record in the real crash dir is reclaimed while a pending logger-owned.otlprecord survives.CrashRetentionTest), so both platforms exercise one suite.Coverage tooling fix
The changed-lines coverage gate initially failed at 11%, which was a measurement bug rather than untested code. Robolectric loads classes through its own instrumenting classloader, which strips the source-location metadata JaCoCo uses to attribute execution, so any class exercised only by a
@RobolectricTestreported 0% no matter how well tested it was, while plain-JVM tests in the same module reported 96–100%.This is pre-existing and repo-wide —
AndroidLogAnrDetector.ktandAndroidLogCrashHandler.ktsit at 0% onmaintoday. It surfaced here only because renaming ~180 lines of Robolectric-only-tested code moved them into the diff-coverage denominator.Enabling
includeNoLocationClassesinjacoco.gradlefixes the attribution. No test changed — only what the report can see. Untouched Robolectric-tested classes are now measured honestly too, so repo-wide numbers reflect real coverage instead of an artifact.Happy to split this into its own PR if reviewers would rather keep the removal isolated.
Manual testing
Acceptance criteria verified locally:
io.opentelemetryin released AARsdependencies --configuration releaseRuntimeClasspathclean for:OneSignal,:core,:notifications,:in-app-messages,:locationio.opentelemetryin POMspublishToMavenLocal— no match in any published POM;corePOM no longer depends oncom.onesignal:otelopentelemetryentries in the built release APK:app:assembleRelease -Pandroid.enableR8.fullMode=truesucceeds for both GMS and Huawei flavors, zero missing-class diagnosticsMIGRATION_GUIDE.mdMerge prerequisites
loggeris the fallback — resolved by deleting the switch entirely.sdk_custom_loggingremotely is no longer a mitigation after this merges. Needs explicit sign-off.telemetry.sdk.name/.version/.languagedisappear from log records, and anything grouping oncom.onesignal.debug.internal.crash.OtelAnrDetector$ApplicationNotRespondingExceptionmust move toApplicationNotRespondingException(SDK-5053). The ANR stacktrace-format fix above means the^\s*atframe transform keeps working. Lives inOneSignal/infra:dashboards/sdk/.64ce06b3, which contains both. No KMP release is needed — the module is built from pinned source, not consumed as a published artifact.Follow-up (deliberately out of scope)
The
SDK_CUSTOM_LOGGINGenum constant lives inOneSignal-KMP-SDK, so removing it needs a KMP PR — cheap now that this PR already bumps the pin, but not worth folding in:FeatureManagerTests, which uses it purely as a fixture.APP_STARTUPflag in the catalog, so deleting it removes the sole test subject forFeatureManager's startup-latching behavior on both sides. That removal should land alongside a replacementAPP_STARTUPflag or a test-only fixture.Leaving the constant in place is harmless: the server may still send the key and
FeatureManagerwill track it, but no behavior depends on it.Affected code checklist
OneSignalLogHttpSender, which now retries with backoffIntegrator-facing dependency change (documented in
MIGRATION_GUIDE.md), but no source-compatible API change.Checklist
Overview
:otelinterfaces. The retention and retry work restores behavior the removal would otherwise have silently dropped.Testing
Final pass
Made with Cursor