Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Log entire NSError instead of just localizedDescription #1068

Merged
merged 1 commit into from
Apr 5, 2022

Conversation

nan-li
Copy link
Contributor

@nan-li nan-li commented Apr 4, 2022

Description

One Line Summary

Give developers more error details by logging the entire NSError instead of just the localizedDescription, which many not exist for the error.

Details

Motivation

These changes were brought up when a customer was getting failure for sending receive receipts but the error logged was (Error 0 in OneSignal Error.), not so informative. This is because the error's localizedDescription was being logged when it didn't have that information set. When missing, a default string is constructed from the domain and code.

Context

To properly provide localizedDescription, we would have to provide a key NSLocalizedDescriptionKey in the userInfo dictionary. Most of the NSErrors created in the SDK use a key of "error" instead, and in a few other cases, a key of "returned". Only 1 error at the time of this PR uses NSLocalizedDescriptionKey.

The SDK creates and handles errors in a variety of ways, and the scope of this PR is limited to logging.

I also looked at other error logging that used error.description and this does give the full error details as well.

Example Log

Here is an example of what is logged for error or error.description:

Error Domain=OneSignal Error Code=0 "(null)" UserInfo={error=Attempted to perform an HTTP request (OSRequestReceiveReceipts) before the user provided privacy consent.}

The "(null)" is the localized description.

Scope

Just affects logging only.

Manual testing

Tested what is logged on different errors using the error object itself, error. localizedDescription, and error.description (which is used in some other instances).

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
  • I have reviewed this PR myself, ensuring it meets each checklist item

This change is Reviewable

Some NSErrors being logged don't have a NSLocalizedDescriptionKey in the userInfo so that logging its localizedDescription gives no details.

We want to give developers adequate information. Since these are just logs and we want to provide more information, instead let's log the entire error object.
@nan-li nan-li requested review from emawby and jkasten2 April 5, 2022 16:01
Copy link
Contributor

@emawby emawby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. Thank you for addressing this across the entire SDK!. Can you add to the PR an example of how the log appears when logging the entire error?

@nan-li nan-li merged commit bf0f979 into main Apr 5, 2022
@nan-li nan-li deleted the fix/more_detailed_error_logs branch April 5, 2022 19:08
@emawby emawby mentioned this pull request Apr 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants