Repository navigation
Conversation
Remote logging exports every line at or above the server-set level, and iOS still used levels chosen for the Xcode console. This pass applies the rule from SDK-3661: ERROR and FATAL mean the SDK did something wrong or hit a state it cannot recover from. Anything the SDK handles, retries, or that the app developer has to fix is WARN or below. Request failures in the User executors go through a shared helper: INFO when the response is retryable, WARN when the executor drops or holds the request. Create User keeps ERROR on the branch that pauses the Operation Repo until the next session, and logs INFO when it retries or holds for a token. Setup mistakes stay visible at the default console level as WARN: missing optional modules when their API is called, the missing location usage description, the missing push capability, the simulator, API calls before init, and invalid input to public APIs. Handled runtime conditions are INFO: location denied, in-app message fetch, impression and WebView errors, receive receipt and session outcome failures that retry, and the LiveActivities module check at init, which now has its own message so it does not group with the API-call warning. Housekeeping lines go to DEBUG. The consent guard in OneSignalClient now formats the request class into its message instead of printing a bare %@. Kept at ERROR: cache decode failures, the SDK's own JSON parsing, requests that cannot be built, Identity Verification refusals, and the "should never happen" guards. The invalid app ID FATAL stays: a native app with a bad app ID never fetches params, so remote logging never starts and nothing is exported.
Drop the note above the Create User ERROR branch. The comment below it already says the pause lasts until a new session. Cut the logRequestFailure doc to the caller rule its body cannot show.
abdulraqeeb33
left a comment
abdulraqeeb33
left a comment
There was a problem hiding this comment.
Create User still logs ERROR only on the branch that pauses the Operation Repo, and the helper is the right seam.
logRequestFailure logs WARN for every status that is not retryable, including 401 and 403. Create User logs INFO when it holds that same failure for a new token, so a routine expiry will WARN once per in-flight request.
| [self.delegate messageIsNotActive:self.message]; | ||
| } | ||
| [OneSignalLog onesignalLog:ONE_S_LL_ERROR message:message]; | ||
| [OneSignalLog onesignalLog:ONE_S_LL_INFO message:message]; |
There was a problem hiding this comment.
This is INFO, but neither the 404 path nor the other statuses clear isInAppMessageShowing. messageIsNotActive deletes the message and hides the window. The flag is cleared in evaluateMessageDisplayQueue, which this path never reaches, so later messages stay blocked. The default console level is WARN, so this line is now silent. WARN here.
| if (error) { | ||
| NSString *errorMessage = [NSString stringWithFormat:@"Javascript Method: %@ Evaluated with Error: %@", OS_JS_GET_PAGE_META_DATA_METHOD, error]; | ||
| [OneSignalLog onesignalLog:ONE_S_LL_ERROR message:errorMessage]; | ||
| [OneSignalLog onesignalLog:ONE_S_LL_INFO message:errorMessage]; |
There was a problem hiding this comment.
On rotation the message is hidden and its constraints are removed before this call. A JavaScript failure returns without completion, which is what unhides it. Dropped work. WARN, and still call completion with the last height.
| } | ||
| } failureBlock:^(NSError *error) { | ||
| [OneSignalLog onesignalLog:ONE_S_LL_ERROR message:[NSString stringWithFormat:@"OneSignal onNotificationReceived sendReceiveReceipt Failed for playerId: %@ error: %@", playerId, error]]; | ||
| [OneSignalLog onesignalLog:ONE_S_LL_INFO message:[NSString stringWithFormat:@"OneSignal onNotificationReceived sendReceiveReceipt Failed for playerId: %@ error: %@", playerId, error]]; |
There was a problem hiding this comment.
This failure is not retried. The block logs and signals the semaphore. Dropped work is WARN under the rule this PR states.
abdulraqeeb33
left a comment
abdulraqeeb33
left a comment
There was a problem hiding this comment.
Reviewed the accumulated state on 3d4fbc8bc, not just the hunks. Direction is right and the Android rule is the right one to port. Four things I think need to change before this lands, all stemming from the same place: logRequestFailure picks a level from the status code before the caller decides what to do with the failure.
1. A malformed 2xx body now logs INFO, against the PR's own carve-out
OneSignalClient.m:229 builds the parse failure as initWithCode:statusCode message:@"Error parsing JSON", and on a 200 with a bad body statusCode is 200. getResponseStatusType sends everything outside 400/401/402/403/404/409/410 to .retryable, so the helper logs INFO and nothing else above VERBOSE records it. The description says ERROR stays for "the SDK's own JSON parsing". Before this PR the executors logged ERROR unconditionally, so this is new. The executor also takes the retry branch for a body that will never parse.
Either give handleJSONNSURLResponse a distinguishable sentinel for jsonError that the helper maps to ERROR, or log the parse failure at ERROR in OneSignalClient.m before invoking the failure block, so the carve-out holds wherever it surfaces.
2. A recovered 401 logs WARN in five executors and INFO in Create User
OSCustomEventsExecutor.swift:355 calls the helper outside the dispatchQueue.async, so a 401 resolves to .unauthorized, logs WARN, and then :358 recovers it via handleUnauthorized with only a DEBUG "holding for a new token" line. Create User does this correctly at OSUserExecutor.swift:542, logging INFO for the identical condition. Same in OSIdentityOperationExecutor, OSPropertyOperationExecutor, OSSubscriptionOperationExecutor, and the fetch paths.
Default console level is WARN, so today the developer sees a failure and not the recovery. Moving the call inside the branches, the way Create User now does, makes the level follow the decision actually taken.
3. The OneSignalExtension demotions reduce no remote volume
setInternalLogSink is only called from Source/OSRemoteLoggingController.swift, which is not in the extension target, so the NSE process never installs a remote sink. The five demoted lines there (OneSignalNotificationServiceExtensionHandler.m:156 and the four in OneSignalAttachmentHandler.m) give up their only console signal at the WARN default and export nothing either way.
Worth keeping those at WARN. Separately, that receipt line is weak regardless: OneSignalReceiveReceiptsController.m forwards error.underlyingError, which is nil for an ordinary HTTP failure, and also calls failure(nil) when receipts are simply disabled, so it can print "Failed ... error: (null)" when nothing failed. Logging only when error != nil and carrying the status would fix both.
4. OSBackgroundTaskManager demotes one of three identical guards
beginBackgroundTask:48 and endBackgroundTask:57 keep ERROR with the same "cannot be executed due to no task handler" text; only setTaskInvalid:66 moved to DEBUG. The comment explaining why it is not really an error applies to all three but now sits under only one. Those two are also the pair that actually fire on background flush.
Demotions where the SDK does not recover
These read as INFO but nothing retries, so they are invisible at the default console level and the user-visible symptom has no trace:
OSInAppMessageViewController.m:342logs INFO for every content-load failure. 404 and 410 are handled viamessageIsNotActive; anything else leaves the IAM undisplayed. Suggest INFO for 404/410 and WARN otherwise, witherror.codein the message.OSInAppMessageView.m:159returns on a JS error without callingcompletion, afterresetWebViewToMaxBoundsAndResizeHeight:already stripped the webView constraints. The orientation path hides the message and only unhides inside that completion. The drop is pre-existing; this PR removes the only evidence of it.OneSignalWebView.m:94logs INFO ondidFailNavigationand never stops the spinner thatdidStartProvisionalNavigationstarted. A bad launch URL, ATS, or offline gives the user a spinning blank sheet, which is developer-actionable and belongs in the WARN bucket under this PR's own rule.
Smaller things
- Message text still shouts the old level:
OSNotificationsManager.m:474("ERROR! 'Push Notifications' capability missing"),:478("Error!"),OneSignalAttachmentHandler.m:52, and the twoaddAliases error:/removeAliases error:lines. These render as "WARNING: ERROR! ..." and still match a grep for "Error" in export. - Stale comments directly above changed lines: the two
// log errorinOSUserInternalImpl.swift:94and:109, and "But not necessarily an error" under the now-DEBUG line inOSBackgroundTaskManager.swift:67. - One session-end outcome failure now logs twice at two levels, DEBUG in
OneSignalOutcomeEventsController.m:126and INFO in both focus processors, and neither line carries the error or the status. - The IAM impression and click lines log
error.message, which is the constant "Error encountered making request" for any HTTP failure, so there is no status code in the line to demote around. - The helper is an
internalextension onOneSignalLoginside a request-model file, so the Objective-C modules andOSLiveActivitiesExecutor.swift:243(same failure family, still VERBOSE) cannot apply the rule. If it is meant to be SDK-wide, it probably belongs next toOSNetworkingUtilsin Core with an Objective-C entry point. addLogListeneris public and documented as "All log messages are always passed, LogLevel has no effect on this". Apps filtering onevent.level == .LL_ERRORto forward into Crashlytics will silently stop receiving these. Worth a CHANGELOG line even though the API signature is unchanged.- The headline motivating case, a 404 on fetch user, maps to
.missingand lands on WARN, one notch. If the alerting fires at WARN and above, that volume is still exported. Worth confirming the server-set level.
Tests
The rationale that no existing test asserts a changed level is accurate but I don't think it settles it. The helper is a pure function of an Int, OneSignalUserTests already does @testable import OneSignalUser, and there are five OSLogListener capture harnesses in the repo already, with UserJwtAskTests asserting emitted levels as the thing under test in that same target. MockOneSignalClient.setMockFailureResponseForRequest takes an error with a settable code.
A table test over the status-to-level mapping plus one case per Create User branch would be cheap, and writing the 200 case down next to the stated carve-out is exactly what would have surfaced finding 1. Worth reverting the change and confirming red before landing them.
Cross-platform
Since this follows OneSignal/OneSignal-Android-SDK#2531: Android's InputGuard.isMissing still logs Logging.error for both "contains a null byte" and "is required", and its doc comment says so. This PR demotes the direct iOS port, OSInputGuard.swift, to WARN. One of the two should move, and the Android doc comment would go stale with it.
Verified as correct, for the record: every path through the Create User closure logs exactly once, including the .unauthorized case where handleUnauthorized returns false, which correctly falls into the branch that pauses the repo and logs ERROR. The privacy-consent change in OneSignalClient.m is a real fix, since the old string passed a literal %@ with no argument; message is nonnull and onesignal_Log passes it as an argument to a literal format, so there is no format-string exposure.
One caveat: I could not measure the production server-set log_level, so the point about a 404 on fetch user still exporting at WARN is conditional on that value.
Description
One Line Summary
Log handled failures and setup mistakes below ERROR, so ERROR means an SDK fault.
Details
Motivation
Remote logging exports every line at or above the level the server sets. iOS logged ERROR for cases the SDK already handles, such as a 404 on fetch user or a denied location prompt, and that volume from 5.6.0+ installs trips alerts. Android made the same change in OneSignal-Android-SDK#2531.
Scope
OneSignalClientnames the request class instead of printing a bare%@. The LiveActivities check at init logs its own INFO line, apart from the warning when the API is called without the module.Testing
Unit testing
No new tests. No existing test asserts the level of a changed line.
Manual testing
I ran the User, OSCore, Core and InAppMessages test bundles from the Reduced plan on an iPhone 17 Pro simulator with iOS 26.5.
Affected code checklist
Checklist
Overview
Testing
Final pass
🤖 Generated with Claude Code