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

feat: add underlying error info to reported NSErrors #3230

Merged
merged 10 commits into from
Oct 23, 2023

Conversation

armcknight
Copy link
Member

📜 Description

Refactor SentryNSError to accept just the NSError instance in init, and don't expose properties that aren't used outside of its implementation.

Then, wrap all underlying errors in SentryNSError and serialize them.

💡 Motivation and Context

#3063

💚 How did you test it?

📝 Checklist

You have to check all boxes before merging:

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

@github-actions
Copy link

github-actions bot commented Aug 17, 2023

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against b5ee122

@github-actions
Copy link

github-actions bot commented Aug 17, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1219.17 ms 1242.50 ms 23.33 ms
Size 22.85 KiB 411.17 KiB 388.33 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
7192d9e 1221.86 ms 1248.28 ms 26.42 ms
7bb0873 1226.18 ms 1247.30 ms 21.12 ms
6604dbb 1248.35 ms 1256.14 ms 7.79 ms
6943de0 1235.98 ms 1246.88 ms 10.90 ms
fd6a31c 1204.73 ms 1222.34 ms 17.61 ms
257c2a9 1231.45 ms 1252.12 ms 20.67 ms
d3630c3 1248.52 ms 1262.70 ms 14.18 ms
e24290f 1239.94 ms 1248.28 ms 8.34 ms
4d68229 1233.50 ms 1262.92 ms 29.42 ms
7ce3cf6 1217.98 ms 1246.41 ms 28.43 ms

App size

Revision Plain With Sentry Diff
7192d9e 20.76 KiB 431.71 KiB 410.95 KiB
7bb0873 22.85 KiB 407.09 KiB 384.24 KiB
6604dbb 22.84 KiB 402.56 KiB 379.72 KiB
6943de0 20.76 KiB 393.33 KiB 372.57 KiB
fd6a31c 20.76 KiB 436.50 KiB 415.74 KiB
257c2a9 20.76 KiB 401.36 KiB 380.60 KiB
d3630c3 22.84 KiB 403.19 KiB 380.34 KiB
e24290f 22.84 KiB 403.51 KiB 380.66 KiB
4d68229 20.76 KiB 432.34 KiB 411.58 KiB
7ce3cf6 22.85 KiB 407.63 KiB 384.78 KiB

Previous results on branch: armcknight/feat/3063-add-underlying-error

Startup times

Revision Plain With Sentry Diff
c4b6d45 1240.67 ms 1259.48 ms 18.81 ms
4bb98a0 1230.71 ms 1238.65 ms 7.94 ms
a7f5d45 1223.92 ms 1249.50 ms 25.58 ms
8fa3bd8 1245.90 ms 1273.12 ms 27.22 ms

App size

Revision Plain With Sentry Diff
c4b6d45 22.85 KiB 407.53 KiB 384.68 KiB
4bb98a0 22.85 KiB 407.98 KiB 385.13 KiB
a7f5d45 22.85 KiB 411.18 KiB 388.33 KiB
8fa3bd8 22.84 KiB 403.83 KiB 380.99 KiB

@codecov
Copy link

codecov bot commented Aug 31, 2023

Codecov Report

Merging #3230 (b5ee122) into main (6001822) will increase coverage by 0.063%.
The diff coverage is 100.000%.

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3230       +/-   ##
=============================================
+ Coverage   89.187%   89.251%   +0.063%     
=============================================
  Files          500       500               
  Lines        54520     54555       +35     
  Branches     19568     19585       +17     
=============================================
+ Hits         48625     48691       +66     
+ Misses        5028      5002       -26     
+ Partials       867       862        -5     
Files Coverage Δ
Sources/Sentry/SentryClient.m 97.579% <100.000%> (+0.069%) ⬆️
...entryTests/Protocol/SentryMechanismMetaTests.swift 83.018% <100.000%> (ø)
Tests/SentryTests/SentryClientTests.swift 96.025% <100.000%> (+0.299%) ⬆️

... and 9 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6001822...b5ee122. Read the comment docs.

Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

According to the docs the correct way to report underlying errors is to add them to the event exception array.
The Exception Interface has no underlying_errors property.

@getsantry

This comment was marked as outdated.

@getsantry getsantry bot added the Stale label Oct 18, 2023
@armcknight
Copy link
Member Author

According to the docs the correct way to report underlying errors is to add them to the event exception array. The Exception Interface has no underlying_errors property.

Thanks for pointing me to this @brustolin! The pertinent part seems to be:

the exception attribute may be a flat list of objects...Multiple values represent chained exceptions and should be sorted oldest to newest

@armcknight armcknight removed the Stale label Oct 20, 2023
Sources/Sentry/Public/SentryEvent.h Outdated Show resolved Hide resolved
Tests/SentryTests/Protocol/SentryNSErrorTests.swift Outdated Show resolved Hide resolved
Comment on lines -1541 to -1545
guard let mechanism = exception.mechanism else {
XCTFail("Exception doesn't contain a mechanism"); return
}

let mechanism = try XCTUnwrap(exception.mechanism)
let meta = try XCTUnwrap(mechanism.meta)
let actualError = try XCTUnwrap(meta.error)
XCTAssertEqual("NSError", mechanism.type)
XCTAssertNotNil(mechanism.meta?.error)
Copy link
Member Author

Choose a reason for hiding this comment

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

This gives us a more granular idea of which thing is missing; It is responsible for all of the throws additions in this file.

Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

LGTM

@armcknight armcknight merged commit 5e78d2b into main Oct 23, 2023
74 checks passed
@armcknight armcknight deleted the armcknight/feat/3063-add-underlying-error branch October 23, 2023 22:54
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