feat: [SDK-5047] drive iOS logging from remote params - #1722
Conversation
iOS parsed logging_config.log_level but never used it to control the logging lifecycle, so remote crash/OTel logging could not be turned on or adjusted server-side the way it can on Android. Adds OSRemoteLoggingConfiguration (Android's RemoteLoggingConfigModel: a persisted level plus an enabled flag derived from the server sending a valid level) and OSRemoteLoggingConfigEvaluator (Android's OtelConfigEvaluator: a pure old-vs-new diff yielding Enable/Disable/UpdateLogLevel/NoChange). The controller now applies those actions instead of only filtering log lines. Two behavioral consequences worth noting. NONE now means enabled-but-exporting -nothing, matching Android's hydrate path, so the crash handler stays armed rather than collapsing to fully disabled. And a level change rebuilds the logger, as Android does in startLogging, because the platform provider handed to KMP is built alongside the logger; reusing the instance would keep reporting the previous level into KMP and would never re-run the crash uploader after a level escalates away from NONE. The cache also stops round-tripping through a synthesized params-shaped dictionary and owns its own format, so it no longer depends on the shape of the params API response. Co-authored-by: Cursor <cursoragent@cursor.com>
Three pre-existing defects in the KMP adapter, all found while verifying the remote-logging lifecycle end to end. Kotlin/Native only permits calling exported suspend functions from the main thread, but all three crossings (LogLoggingHelper.log, ILogTelemetry .forceFlush, LogCrashUploader.start) were reached from the controller's serial queue and from URLSession callbacks, which trips a runtime failure as soon as the backend enables logging. They are now marshalled to main. The hop runs inline when already on main so a caller that blocks waiting on the completion cannot deadlock itself. telemetry.shutdown() drains buffered records under runBlocking with a five second cap, and was reachable from initialize: and from app-id changes through stateQueue.sync, so a slow network could stall the UI for that long. Only the drain is deferred to a background queue; unregistering the crash handler stays synchronous, because OSLogCrashHandler.initialize() bails when another handler is still registered and deferring it would silently disable crash capture after an app-id change. Because the drain is now asynchronous, start() also has to reject once shutdown has begun rather than once it has finished -- otherwise a logger told to shut down could still install a crash handler that nothing would ever unregister, permanently blocking later loggers. Finally, the HTTP exporter's diagnostics logged through OneSignalLog, whose sink is invoked unconditionally, so at verbose levels each export POST created a new exported record and guaranteed another POST. They now log console-only. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Multi-model review found a few issues worth addressing:
Also worth confirming the intended privacy behavior: |
Serializes forceFlush against teardown. The deferred drain in shutdown() also flushes the same telemetry, so entering KMP from both at once was unsafe; the flush is now skipped once teardown begins. Its completion still runs in every case, because callers end a UIBackgroundTask in it and swallowing it would leak that task. The gating predicate is renamed isActive and now covers emission, uploader start, and flushes alike, since all three cross the same boundary. Normalizes a cached config that claims to be enabled without a level this SDK can parse — possible when a newer build wrote the cache. Android falls back to ERROR when enabling without a level; matching that avoids starting a logger that is enabled yet can never export and reports no level into KMP. Also strips line-number artifacts that leaked into the new file's license header, and splits the test file's configuration cases into their own suite so the controller suite stops exceeding the type_body_length error threshold. That was the sole error-severity swiftlint violation in the repo and the reason the lint job failed; the remaining violations in these files are file_length warnings, which the linter tolerates. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Thanks — all four addressed in 86b7363, plus the lint failure. Details and one thing I could only partly fix:
Cached Active crash upload continuing after shutdown begins. Partly fixed, and I want to be straight about the limit. Starting is now blocked: the uploader's enqueue closure checks License header artifacts. Removed. Those were line-number gutter prefixes from a file read that leaked into the content when I wrote the file — good catch. Lint. Root cause was unrelated to the review: this branch was one commit behind On the Tests: 19 in |
|
One warning remains in The cache normalization and license-header fixes look good. The active crash-upload cancellation and |
Gating forceFlush on a flag was not enough. The flush is asynchronous, so shutdown could begin after the check passed and run its drain over the same telemetry while the flush was still crossing into KMP. Flushes now claim a slot on the lifecycle, and shutdown waits for outstanding slots before draining. `beginFlush` refuses new flushes once shutdown starts, so the set the drain waits on can only shrink. The wait is bounded at five seconds, matching the cap KMP already puts on its own drain, so a wedged flush cannot stop teardown from completing. Blocking is safe there because it runs on the teardown queue while the KMP completion that releases the slot resumes on main. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Good catch — the flag check was insufficient for exactly the reason you describe. Fixed in e900732 by tracking the operation rather than testing a flag.
Two deliberate choices in there: The wait is bounded at five seconds, matching the cap KMP already applies to its own drain in Blocking is safe where it happens: the wait runs on the teardown queue, while the KMP completion that releases the slot resumes on main. So the releasing thread is never the blocked one. Covered by two new lifecycle tests — Separately, on the red Build and Test: I believe that's an unrelated flake rather than anything on this branch. The failure was I tried re-running the failed job to confirm, but it errored in 19s with |
|
One timeout-path issue remains: after Please consider timing out or cancelling |
OSRemoteLoggingConfig mirrored two fields of OSRemoteLoggingConfiguration and existed only to be constructed at the one call site that invokes the evaluator. The near-identical name made the pair easy to confuse for no benefit. Android keeps OtelConfig separate because its config lives in a persistence -backed Model that cannot be compared directly. OSRemoteLoggingConfiguration is already an Equatable value type, so the evaluator now diffs it as-is. Co-authored-by: Cursor <cursoragent@cursor.com>
The KMP module sets objcExportSuspendFunctionLaunchThreadRestriction=none, so exported suspend functions can be called from any thread. The hop these calls went through was unnecessary; the exception that prompted it almost certainly came from an XCFramework built before that flag landed. Removing it also closes the check-then-cross window in log(), since the lifecycle guard and the KMP call are now sequential on one thread, and drops a main-queue dispatch per exported record — which added up at verbose levels. A new test calls an exported suspend function from a background queue and asserts it completes, so this fails loudly if the flag is ever dropped. forceFlush keeps its slot tracking: the flush completes asynchronously even when started inline, so shutdown's drain could still overlap it. Also drops canStartUploader, which had become a second name for isActive with a single reader. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # iOS_SDK/OneSignalSDK/Source/OSRemoteLoggingController.swift
3 participants
Summary
iOS already parsed
logging_config.log_levelfromios_params, but never used it to drive the logging lifecycle — so remote crash/OTel logging couldn't be enabled or adjusted server-side the way it can on Android. This ports Android's model and lifecycle, then fixes three pre-existing defects in the KMP adapter that surfaced once the backend actually started enabling logging.Two commits, independently reviewable:
feat:— config + lifecycle port. AddsOSRemoteLoggingConfiguration(Android'sRemoteLoggingConfigModel: persisted level + enabled flag derived from the server sending a valid level) andOSRemoteLoggingConfigEvaluator(Android'sOtelConfigEvaluator: pure old-vs-new diff yielding Enable / Disable / UpdateLogLevel / NoChange). The controller applies those actions rather than only filtering log lines. The cache also stops round-tripping through a synthesized params-shaped dictionary, so it no longer depends on the params API response shape — relevant while SDK-5046 is changing that shape.fix:— KMP boundary. Kotlin/Native only allows calling exportedsuspendfunctions from the main thread, but all three crossings were reached from the controller's serial queue and from URLSession callbacks; they're now marshalled to main.telemetry.shutdown()'s five-secondrunBlockingdrain was reachable frominitialize:and app-id changes viastateQueue.syncand could stall the UI, so the drain is deferred. And the HTTP exporter's diagnostics logged throughOneSignalLog, whose sink is invoked unconditionally, so at verbose levels each export POST generated a new record and guaranteed another POST.Behavior changes worth a close look
NONEnow means enabled-but-exporting-nothing, matching Android's hydrate path, so the crash handler stays armed instead of collapsing to fully disabled.startLogging. This isn't just parity: the platform provider handed to KMP is built alongside the logger, so reusing the instance would keep reporting the previous level into KMP and would never re-run the crash uploader after a level escalates away fromNONE.start()now rejects once shutdown has begun, not once it has finished. Required by the deferred drain — otherwise a logger told to shut down could still install a crash handler nothing would unregister, permanently blocking every later logger from installing its own.Verification
Verified end to end on the demo app against a real app id returning
log_level: VERBOSE:Enable(VERBOSE); cache round-trip preserved both fields./sdk/logreturning 202.NSExceptioncaptured byOSLogCrashHandler, persisted durably (7,861-byte.otlp), and uploaded byLogCrashUploaderon the following launch. Confirmed server-side.Tests: 18
OSRemoteLoggingControllerTests+ 67OneSignalOSCoreTests, all passing. Includes a verbatimios_paramspayload fixture, the full evaluator transition matrix, and direct lifecycle tests for the start-after-shutdown guard.This went through an adversarial multi-model review before opening; the critical finding (the
start()guard) and three warnings were fixed as a result.Test plan
ERROR→VERBOSE) rebuilds the logger and the new level reaches KMPNONEkeeps crash capture armed while exporting nothingFollow-ups filed separately
isOneSignalAtFaultdrops SDK crashes when the OneSignal frame is elidedNot included
The KMP submodule pointer is deliberately left off this branch; local testing ran against a newer KMP commit than
mainpins, and CI will usemain's.Made with Cursor