feat: Swift error names #2960

philipphofmann · 2023-04-25T14:45:45Z

📜 Description

Call into Swift to get the error description for Swift errors to get meaningful error names instead of only the error enum code. To avoid sending PII, the SDK strips parameter values and doesn't send the Swift error name for struct-based Swift errors.
This change has no impact on grouping.

I'm unsure about my data stripping approach in SwiftDescriptor.getSwiftErrorDescription to avoid sending PII. Maybe it's also OK to just attach the description. On the other hand, I feel safer changing that behavior in the next major.

Update: No need to worry about PII in a major as we already would send PII of Errors with

sentry-cocoa/Sources/Sentry/SentryClient.m

Lines 265 to 267 in 9d61bea

    
           // The description of the error can be especially useful for error from swift that 
        
           // use a simple enum. 
        
           mechanism.desc = error.description;

Docs PR getsentry/sentry-docs#6805

💡 Motivation and Context

Fixes GH-2958

💚 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

Call into Swift to get the error description for Swift errors to get meaningful error names instead of only the error enum code. To avoid sending PII the SDK strips parameter values and doesn't send the swift error name for struct based Swift errors. Fixes GH-2958

github-actions · 2023-04-25T14:47:01Z

	Fails
🚫	Please consider adding a changelog entry for the next release.

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

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- Swift error names ([#2960](https://github.com/getsentry/sentry-cocoa/pull/2960))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against e82f5a2

github-actions · 2023-04-25T15:02:41Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1216.82 ms	1234.10 ms	17.28 ms
Size	20.76 KiB	436.50 KiB	415.74 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`e71cf92`	1201.69 ms	1226.52 ms	24.83 ms
`1bd0055`	1207.57 ms	1223.10 ms	15.53 ms
`56ec5d0`	1194.39 ms	1236.94 ms	42.55 ms
`b6ba04e`	1230.48 ms	1253.20 ms	22.72 ms
`455619d`	1253.08 ms	1265.06 ms	11.98 ms
`efb2222`	1256.44 ms	1278.90 ms	22.46 ms
`2cf1b84`	6265.25 ms	6277.98 ms	12.73 ms
`6f4d20c`	1228.67 ms	1238.88 ms	10.21 ms
`90d17d3`	1261.18 ms	1278.18 ms	17.00 ms
`228a909`	1248.92 ms	1267.42 ms	18.50 ms

App size

Revision	Plain	With Sentry	Diff
`e71cf92`	20.76 KiB	419.85 KiB	399.10 KiB
`1bd0055`	20.76 KiB	420.22 KiB	399.46 KiB
`56ec5d0`	20.76 KiB	414.44 KiB	393.68 KiB
`b6ba04e`	20.76 KiB	414.44 KiB	393.68 KiB
`455619d`	20.76 KiB	432.87 KiB	412.11 KiB
`efb2222`	20.76 KiB	424.45 KiB	403.69 KiB
`2cf1b84`	20.76 KiB	431.91 KiB	411.15 KiB
`6f4d20c`	20.76 KiB	431.71 KiB	410.95 KiB
`90d17d3`	20.76 KiB	432.17 KiB	411.41 KiB
`228a909`	20.76 KiB	425.71 KiB	404.95 KiB

Previous results on branch: feat/swift-errors

Startup times

Revision	Plain	With Sentry	Diff
`51283b0`	1247.96 ms	1274.88 ms	26.92 ms
`d617f71`	1195.88 ms	1217.94 ms	22.06 ms
`eb467b7`	1218.43 ms	1240.43 ms	22.00 ms
`9b6a939`	1200.69 ms	1223.62 ms	22.93 ms
`60a6dd8`	1233.35 ms	1253.74 ms	20.39 ms
`cca8c20`	1229.80 ms	1246.08 ms	16.28 ms

App size

Revision	Plain	With Sentry	Diff
`51283b0`	20.76 KiB	437.69 KiB	416.93 KiB
`d617f71`	20.76 KiB	436.60 KiB	415.84 KiB
`eb467b7`	20.76 KiB	436.26 KiB	415.50 KiB
`9b6a939`	20.76 KiB	436.26 KiB	415.50 KiB
`60a6dd8`	20.76 KiB	436.50 KiB	415.74 KiB
`cca8c20`	20.76 KiB	436.50 KiB	415.74 KiB

arielelkin · 2023-04-25T15:17:59Z

Looks good, looking forward to this 👍

brustolin

It looks good, I have one question and one thing you should review.

Whats is preventing this from being ready for review?

Tests/SentryTests/SwiftDescriptorTests.swift

Sources/Swift/SwiftDescriptor.swift

philipphofmann · 2023-05-02T13:05:06Z

Whats is preventing this from being ready for review?

What I mentioned above in the description:

I'm unsure about my data stripping approach in SwiftDescriptor.getSwiftErrorDescription to avoid sending PII. Maybe it's also OK to just attach the description. On the other hand, I feel safer changing that behavior in the next major.

brustolin · 2023-05-02T14:39:29Z

Sources/Swift/SwiftDescriptor.swift

+    public static func getSwiftErrorDescription(_ error: Error) -> String? {
+        return String(describing: error)
+    }


h: Why are we not concern with PII anymore?

I guess you missed my update in the description above #2960 (comment)

Update: No need to worry about PII in a major as we already would send PII of Errors with

sentry-cocoa/Sources/Sentry/SentryClient.m

Lines 265 to 267 in 9d61bea

// The description of the error can be especially useful for error from swift that

// use a simple enum.

mechanism.desc = error.description;

I guess you missed my update in the description above

Yeap I did. I would like to suggest to only update description if something is wrong, change of direction in the PR should be describe in a new comment. Or both, since GH itself don't warn about description changing.

Thanks for clarifying!

codecov · 2023-05-03T09:37:41Z

Codecov Report

Merging #2960 (50014b0) into main (b1b7d72) will decrease coverage by 0.006%.
The diff coverage is 100.000%.

Additional details and impacted files

@@              Coverage Diff              @@
##              main     #2960       +/-   ##
=============================================
- Coverage   80.560%   80.555%   -0.006%     
=============================================
  Files          265       265               
  Lines        24728     24706       -22     
  Branches     10968     10953       -15     
=============================================
- Hits         19921     19902       -19     
+ Misses        4190      4189        -1     
+ Partials       617       615        -2

Impacted Files	Coverage Δ
Sources/Sentry/SentryClient.m	`97.908% <100.000%> (+0.032%)`	⬆️
Sources/Swift/SwiftDescriptor.swift	`100.000% <100.000%> (ø)`

... and 3 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 b1b7d72...50014b0. Read the comment docs.

philipphofmann added 2 commits April 25, 2023 16:42

add note on grouping

bc60da8

philipphofmann mentioned this pull request Apr 25, 2023

Displaying error name instead of Error code #2958

Closed

fix changelog

5f8ee36

brustolin reviewed Apr 26, 2023

View reviewed changes

Tests/SentryTests/SwiftDescriptorTests.swift Outdated Show resolved Hide resolved

Sources/Swift/SwiftDescriptor.swift Outdated Show resolved Hide resolved

philipphofmann added 8 commits May 2, 2023 15:05

Merge branch 'main' into feat/swift-errors

cb8b597

fix changelog

7bdffea

remove swift description logic

726e533

undo changes in ErrorsViewController

7bbe87a

fix ClientTests

7ea5066

Merge branch 'main' into feat/swift-errors

3147bf3

update readme

4355dff

again changelog

4531ee1

philipphofmann marked this pull request as ready for review May 2, 2023 14:01

philipphofmann requested a review from armcknight as a code owner May 2, 2023 14:01

brustolin reviewed May 2, 2023

View reviewed changes

Merge branch 'main' into feat/swift-errors

4c77aa0

brustolin approved these changes May 3, 2023

View reviewed changes

Merge branch 'main' into feat/swift-errors

50014b0

update changelog

e82f5a2

philipphofmann merged commit fd6a31c into main May 3, 2023

philipphofmann deleted the feat/swift-errors branch May 3, 2023 10:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Swift error names #2960

feat: Swift error names #2960

philipphofmann commented Apr 25, 2023 •

edited

Loading

github-actions bot commented Apr 25, 2023 •

edited

Loading

github-actions bot commented Apr 25, 2023 •

edited

Loading

Baseline results on branch: main

Startup times

App size

Previous results on branch: feat/swift-errors

Startup times

App size

arielelkin commented Apr 25, 2023

brustolin left a comment

philipphofmann commented May 2, 2023

brustolin May 2, 2023

philipphofmann May 3, 2023 •

edited

Loading

brustolin May 3, 2023

codecov bot commented May 3, 2023 •

edited

Loading

	// The description of the error can be especially useful for error from swift that
	// use a simple enum.
	mechanism.desc = error.description;

feat: Swift error names #2960

feat: Swift error names #2960

Conversation

philipphofmann commented Apr 25, 2023 • edited Loading

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

github-actions bot commented Apr 25, 2023 • edited Loading

Instructions and example for changelog

github-actions bot commented Apr 25, 2023 • edited Loading

Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

Previous results on branch: feat/swift-errors

Startup times

App size

arielelkin commented Apr 25, 2023

brustolin left a comment

Choose a reason for hiding this comment

philipphofmann commented May 2, 2023

brustolin May 2, 2023

Choose a reason for hiding this comment

philipphofmann May 3, 2023 • edited Loading

Choose a reason for hiding this comment

brustolin May 3, 2023

Choose a reason for hiding this comment

codecov bot commented May 3, 2023 • edited Loading

Codecov Report

philipphofmann commented Apr 25, 2023 •

edited

Loading

github-actions bot commented Apr 25, 2023 •

edited

Loading

github-actions bot commented Apr 25, 2023 •

edited

Loading

philipphofmann May 3, 2023 •

edited

Loading

codecov bot commented May 3, 2023 •

edited

Loading