chore: Logging lower severity#2531
Conversation
Severity lower to match what is sever to OneSignal, not necessary the app developer. The main motivation is to better classify errors, warning, ect we send in the otel format to OneSignal, which will lower volume as well.
abdulraqeeb33
left a comment
abdulraqeeb33
left a comment
There was a problem hiding this comment.
just couple of questions but looks fine to me
There was a problem hiding this comment.
Pull request overview
This PR reclassifies logging severity levels across the OneSignal Android SDK to reduce log volume sent to OneSignal in OTEL format. The changes focus on downgrading logs from error/fatal to more appropriate levels (warn/info/debug) for non-critical issues.
Changes:
- Downgrade configuration/setup errors (missing libraries, invalid settings) from fatal/error to warn/info
- Downgrade successful registration messages from error to debug
- Downgrade timeout and retry failures from error to info/warn
- Improve error logging by properly passing exception objects instead of using printStackTrace()
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| ADMMessageHandlerJob.kt | Registration errors downgraded from error to info |
| ADMMessageHandler.kt | Registration errors downgraded from error to info |
| NotificationRestoreProcessor.kt | Restoration errors downgraded from error to warn |
| PushRegistratorHMS.kt | Success log downgraded to debug, timeout to warn |
| PushRegistratorAbstractGoogle.kt | Configuration/library errors downgraded to warn, retry failures to info |
| PushRegistratorADM.kt | Success log downgraded to debug, timeout to info |
| ReceiveReceiptProcessor.kt | Backend failures downgraded from error to info |
| PushTokenManager.kt | Missing/outdated Jetpack library downgraded from fatal to info |
| NotificationLifecycleService.kt | Various downgrades: debug for intents, warn for ActivityNotFoundException, improved error logging with exception parameters |
| NotificationGenerationProcessor.kt | Callback timeouts and exceptions downgraded from error to info |
| NotificationRepository.kt | Cleanup errors downgraded from error to warn |
| OSWorkManagerHelper.kt | WorkManager initialization errors downgraded from error to warn |
| NotificationChannelManager.kt | Channel-related errors downgraded from error to warn |
| OneSignalHmsEventBridge.kt | JSON errors downgraded from error to warn |
| HmsLocationController.kt | Location service errors downgraded from error to warn |
| LocationManager.kt | Permission errors downgraded from error to info |
| InAppHydrator.kt | Missing HTML content log changed from debug to info |
| InAppMessageView.kt | Missing presenter log downgraded from error to info |
| InAppDisplayer.kt | WebView errors downgraded from error to info |
| InAppBackendService.kt | Backend request errors downgraded from error to info |
| LoginHelper.kt | Login failures downgraded from error to warn |
| SessionListener.kt | Invalid duration errors downgraded from error to info |
| OutcomeEventsController.kt | Outcome event failures downgraded: retryable to info, non-retryable to warn |
| OperationRepo.kt | Operation failures downgraded to warn/info, retry delay to debug |
| HttpClient.kt | Request timeouts downgraded from error to info |
| BackgroundManager.kt | Job scheduling errors downgraded from error to info |
| JSONUtils.kt | Nested value warnings downgraded from error to warn |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Logging.info("Notification opened confirmation failed with statusCode: ${ex.statusCode} response: ${ex.response}") | ||
| } else { | ||
| Logging.error("Unexpected error in notification opened confirmation", ex) | ||
| Logging.info("Unexpected error in notification opened confirmation", ex) |
There was a problem hiding this comment.
Downgrading notification opened confirmation failures from Logging.error to Logging.info may be too severe. While this is a non-critical backend operation that can fail, it represents a loss of analytics data that developers may want to be alerted about. Consider using Logging.warn instead to maintain better visibility while still reducing log volume.
| _backend.updateNotificationAsReceived(appId, notificationId, subscriptionId, deviceType) | ||
| } catch (ex: BackendException) { | ||
| Logging.error("Receive receipt failed with statusCode: ${ex.statusCode} response: ${ex.response}") | ||
| Logging.info("Receive receipt failed with statusCode: ${ex.statusCode} response: ${ex.response}") |
There was a problem hiding this comment.
Downgrading receive receipt backend failures from Logging.error to Logging.info may hide important analytics issues. While receipt confirmation is non-critical for app functionality, consistent failures could indicate backend or configuration problems. Consider using Logging.warn instead to maintain visibility of these issues while reducing log volume.
| Logging.info("Receive receipt failed with statusCode: ${ex.statusCode} response: ${ex.response}") | |
| Logging.warn("Receive receipt failed with statusCode: ${ex.statusCode} response: ${ex.response}") |
| Logging.info("notificationWillShowInForegroundHandler timed out, continuing with wantsToDisplay=$wantsToDisplay.", to) | ||
| } catch (t: Throwable) { | ||
| Logging.error( | ||
| Logging.info( |
There was a problem hiding this comment.
Downgrading callback exception handling from Logging.error to Logging.info is questionable. When a developer's callback throws an exception (remoteNotificationReceived or notificationWillShowInForegroundHandler), this represents a bug in the developer's code that they should be notified about. Consider keeping this at Logging.error or using Logging.warn to ensure developers are aware of issues in their notification handling code.
| Logging.info( | |
| Logging.warn( |
| val content = InAppMessageContent(jsonObject) | ||
| if (content.contentHtml == null) { | ||
| Logging.debug("displayMessage:OnSuccess: No HTML retrieved from loadMessageContent") | ||
| Logging.info("displayMessage:OnSuccess: No HTML retrieved from loadMessageContent") |
There was a problem hiding this comment.
Changing this log from Logging.debug to Logging.info may be unnecessary. This appears to be a normal case where HTML content is not available (not an error condition), and debug level seems more appropriate for such operational details. Consider reverting to debug to avoid cluttering info logs with normal flow conditions.
| Logging.info("displayMessage:OnSuccess: No HTML retrieved from loadMessageContent") | |
| Logging.debug("displayMessage:OnSuccess: No HTML retrieved from loadMessageContent") |
| when (_deviceService.jetpackLibraryStatus) { | ||
| IDeviceService.JetpackLibraryStatus.MISSING -> { | ||
| Logging.fatal("Could not find the Jetpack/AndroidX. Please make sure it has been correctly added to your project.") | ||
| Logging.info("Could not find the Jetpack/AndroidX. Please make sure it has been correctly added to your project.") |
There was a problem hiding this comment.
Downgrading from Logging.fatal to Logging.info for missing Jetpack/AndroidX library may be too severe a reduction. This is a critical configuration issue that prevents push notifications from working entirely. Consider using Logging.warn instead to maintain visibility of this important configuration problem while still reducing log volume.
| Logging.info("Could not find the Jetpack/AndroidX. Please make sure it has been correctly added to your project.") | |
| Logging.warn("Could not find the Jetpack/AndroidX. Please make sure it has been correctly added to your project.") |
| } | ||
| IDeviceService.JetpackLibraryStatus.OUTDATED -> { | ||
| Logging.fatal( | ||
| Logging.info( |
There was a problem hiding this comment.
Downgrading from Logging.fatal to Logging.info for outdated Jetpack/AndroidX library may be too severe a reduction. This is a critical configuration issue that prevents push notifications from working entirely. Consider using Logging.warn instead to maintain visibility of this important configuration problem while still reducing log volume.
| Logging.info( | |
| Logging.warn( |
Description
One Line Summary
Better classify errors, warning, ect we send to OneSignal, which will lower volume as well.
Details
Motivation
The main motivation is to better classify errors, warning, ect we send in the otel format to OneSignal, which will lower volume as well.
Considerations
There is only one severity per log entry, however the severity is different between the app developer and OneSignal.
Example, the app developer may have something configured wrong in their project preventing the SDK from registering for push, this would be an error in the context of the app developer, but OneSignal would consider this info instead. An error in the context of the app developer since this means a major feature is not working for their app, but only an info level since the SDK is not at fault.
Scope
Only effects logging
Manual testing
Tested on an Android 14 emulator
Affected code checklist
Checklist
Overview
Testing
Final pass
This change is