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

fix: Write NSException reason for crash report #3705

Merged
merged 3 commits into from
Mar 6, 2024

Conversation

philipphofmann
Copy link
Member

📜 Description

Correctly write the NSException reason when writing a crash report. When writing a crash report for NSException the crash report writer wrote the crash reason to ReferencedObject, which didn't make any sense. This is fixed now by using the reason field.

💡 Motivation and Context

Fixes GH-3265

💚 How did you test it?

Unit tests.

📝 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

Correctly write the NSException reason when writing a crash report. When
writing a crash report for NSException the crash report writer wrote the
crash reason to ReferencedObject, which didn't make any sense. This is
fixed now by using the reason field.

Fixes GH-3265
Copy link

codecov bot commented Mar 4, 2024

Codecov Report

Attention: Patch coverage is 90.69767% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 89.224%. Comparing base (e3adcd5) to head (a7976f0).
Report is 5 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3705       +/-   ##
=============================================
- Coverage   89.299%   89.224%   -0.075%     
=============================================
  Files          534       533        -1     
  Lines        58976     58706      -270     
  Branches     21175     20800      -375     
=============================================
- Hits         52665     52380      -285     
- Misses        5390      5400       +10     
- Partials       921       926        +5     
Files Coverage Δ
Sources/SentryCrash/Recording/SentryCrashReport.c 31.693% <100.000%> (+1.247%) ⬆️
...egrations/SentryCrash/SentryCrashReportTests.swift 90.517% <90.000%> (-0.273%) ⬇️

... and 91 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 e3adcd5...a7976f0. Read the comment docs.

Copy link

github-actions bot commented Mar 4, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1230.35 ms 1247.74 ms 17.40 ms
Size 21.58 KiB 540.04 KiB 518.45 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
3297d6e 1228.57 ms 1255.04 ms 26.47 ms
6129be5 1215.65 ms 1247.18 ms 31.54 ms
dacf894 1232.32 ms 1236.34 ms 4.02 ms
7bbb7c3 1232.40 ms 1249.78 ms 17.38 ms
ddc9b9a 1210.00 ms 1234.18 ms 24.18 ms
06548c0 1226.71 ms 1252.37 ms 25.66 ms
ee8b48f 1196.80 ms 1213.48 ms 16.68 ms
881a955 1209.47 ms 1225.94 ms 16.47 ms
8f397a7 1219.12 ms 1236.67 ms 17.55 ms
f576153 1210.02 ms 1228.94 ms 18.92 ms

App size

Revision Plain With Sentry Diff
3297d6e 21.58 KiB 418.45 KiB 396.86 KiB
6129be5 21.58 KiB 418.00 KiB 396.42 KiB
dacf894 20.76 KiB 426.34 KiB 405.58 KiB
7bbb7c3 21.58 KiB 418.78 KiB 397.20 KiB
ddc9b9a 20.76 KiB 420.40 KiB 399.64 KiB
06548c0 20.76 KiB 427.36 KiB 406.59 KiB
ee8b48f 21.58 KiB 418.70 KiB 397.11 KiB
881a955 22.85 KiB 407.63 KiB 384.78 KiB
8f397a7 20.76 KiB 420.55 KiB 399.79 KiB
f576153 20.76 KiB 425.77 KiB 405.01 KiB

Previous results on branch: fix/write-nsexception-reason

Startup times

Revision Plain With Sentry Diff
a90fed0 1235.18 ms 1245.20 ms 10.02 ms
7c44990 1233.92 ms 1246.12 ms 12.21 ms

App size

Revision Plain With Sentry Diff
a90fed0 21.58 KiB 540.04 KiB 518.46 KiB
7c44990 21.58 KiB 539.81 KiB 518.23 KiB

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

philipphofmann and others added 2 commits March 5, 2024 14:15
…sts.swift

Co-authored-by: Dhiogo Brustolin <dhiogo.brustolin@sentry.io>
@philipphofmann philipphofmann merged commit 66c3b39 into main Mar 6, 2024
65 of 70 checks passed
@philipphofmann philipphofmann deleted the fix/write-nsexception-reason branch March 6, 2024 12:07
threema-matteo pushed a commit to threema-ch/sentry-cocoa that referenced this pull request May 21, 2024
Correctly write the NSException reason when writing a crash report. When
writing a crash report for NSException the crash report writer wrote the
crash reason to ReferencedObject, which didn't make any sense. This is
fixed now by using the reason field.

Fixes getsentryGH-3265

Co-authored-by: Dhiogo Brustolin <dhiogo.brustolin@sentry.io>
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.

NSException does not include reason
2 participants