refactor: [SDK-5065] remove the otel observability path and OpenTelemetry dependency - #2724
Draft
abdulraqeeb33 wants to merge 5 commits into
Draft
refactor: [SDK-5065] remove the otel observability path and OpenTelemetry dependency#2724abdulraqeeb33 wants to merge 5 commits into
abdulraqeeb33 wants to merge 5 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)348/371 touched executable lines covered (93.8% — requires ≥ 80%) |
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>
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
1 participant
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.Closes SDK-5065.
Important
Draft — two prerequisites from the ticket are external and not yet met. See Merge prerequisites below. The code is complete and green; this should not merge until the rollout finishes and dashboards are migrated.
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), SDK-5006 (shading work), and 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: -5,558 / +662 lines across 84 files.
Scope
Deleted
:otelGradle module in its entirety, plus itssettings.gradleinclude/substitution and the:coredependency.build.gradle.OtelLifecycleManager,OtelAnrDetector,AndroidOtelLogger,OneSignalCrashHandlerFactory.LoggerModuleSwitch,resolveCustomLoggingEnabled, and theSDK_CUSTOM_LOGGINGgate.Logging.setOtelTelemetry/logToOtel, and the otel consumer R8dontwarnrules.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 existing suffix-based purge inLogCrashUploader, which satisfies the ticket's "uploaded or intentionally purged" criterion. The retained name is commented at the definition site.Behavioral change 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 (prerequisite 1 in the ticket). Removing the switch entirely fixes this:LoggerLifecycleManagernow comes up unconditionally, and features enable as soon as the first remote config arrives, exactly as the logger path already behaved.Testing
Unit testing
spotlessCheckanddetektacross every module.LoggerIdResolverTest,LoggerPlatformProviderTest) so no coverage was lost with the otel deletions.LoggerLifecycleManagerTest— the logger lifecycle manager is now the SDK's only observability path but previously had no direct tests, since the deletedOtelLifecycleManagerTestwas the only lifecycle coverage. Asserts the config state machine by observing realUncaughtExceptionHandlerregistration rather than just "does not throw".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 actually verifies emission, filtering, exception attributes and sink-failure isolation.OneSignalCrashUploaderWrapperTestproving a pre-upgrade otel record in the real crash dir is reclaimed while a pending logger-owned.otlprecord survives.Coverage tooling fix (second commit)
The changed-lines coverage gate initially failed at 11%, which turned out to be 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. Every 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%. The correlation was exact:CrashDirCleanup.ktAnrCheckEvaluator.ktObservabilityConfigEvaluator.ktFileLogStore.ktLoggerIdResolver.ktAndroidLogAnrDetector.ktCrashDirCleanup's existing doc comment already alludes to this — it notes that keeping the logic free of Robolectric is what gets it "counted by Jacoco on the plain JVM."This is pre-existing and repo-wide:
AndroidLogAnrDetector.ktandAndroidLogCrashHandler.ktsit at 0% onmaintoday and are untouched by this PR. It only surfaced here 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:LoggerPlatformProvider.ktLoggerIdResolver.ktLoggerLifecycleManager.ktOneSignalCrashUploaderWrapper.ktObservabilitySdkSupport.ktLogging.ktOneSignalImp.ktAggregate on changed lines: 11.0% → 95.6% (218/228), passing without a bypass label. Untouched Robolectric-tested classes are now measured honestly too (
AndroidLogAnrDetector0% → 49.5%,FileLogStore0% → 34.5%), so repo-wide figures 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
Items 1 and 5 from the ticket are resolved by this PR. The rest are not code and are not met yet:
loggeris the fallback — resolved by deleting the switch entirely.sdk_custom_loggingremotely is no longer a mitigation after this merges. Needs an explicit sign-off.telemetry.sdk.name/.version/.languagedisappear from log records. Anything grouping oncom.onesignal.debug.internal.crash.OtelAnrDetector$ApplicationNotRespondingExceptionmust move toApplicationNotRespondingException(SDK-5053). Lives inOneSignal/infra:dashboards/sdk/.Follow-up (deliberately out of scope)
The
SDK_CUSTOM_LOGGINGenum constant itself lives inOneSignal-KMP-SDK, not this repo, so removing it needs its own PR plus a submodule bump. It is not worth blocking on: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 the Android and KMP 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
OneSignalLogHttpSenderIntegrator-facing dependency change (documented in
MIGRATION_GUIDE.md), but no source-compatible API change.Checklist
Overview
:otelinterfaces.Testing
Final pass
Made with Cursor