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 the option swizzleClassNameExcludes #3813

Merged
merged 4 commits into from
Apr 3, 2024

Conversation

philipphofmann
Copy link
Member

@philipphofmann philipphofmann commented Apr 2, 2024

📜 Description

Add an option to exclude certain classes from swizzling.

Docs PR getsentry/sentry-docs#9596.

💡 Motivation and Context

Fixes GH-3798

💚 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

Add an option to exclude certain classes from swizzling.

Fixes GH-3798
Copy link

github-actions bot commented Apr 2, 2024

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

Generated by 🚫 dangerJS against 974ca11

Copy link

codecov bot commented Apr 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.941%. Comparing base (b15521e) to head (974ca11).
Report is 4 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3813       +/-   ##
=============================================
+ Coverage   89.588%   90.941%   +1.352%     
=============================================
  Files          560       560               
  Lines        60819     44167    -16652     
  Branches     21876     15757     -6119     
=============================================
- Hits         54487     40166    -14321     
+ Misses        5296      3931     -1365     
+ Partials      1036        70      -966     
Files Coverage Δ
Sources/Sentry/SentryOptions.m 99.171% <100.000%> (+1.453%) ⬆️
...rces/Sentry/SentryPerformanceTrackingIntegration.m 100.000% <100.000%> (ø)
Sources/Sentry/SentrySubClassFinder.m 93.023% <100.000%> (+3.439%) ⬆️
...ations/Performance/SentrySubClassFinderTests.swift 95.000% <100.000%> (+15.000%) ⬆️
...troller/SentryUIViewControllerSwizzlingTests.swift 98.924% <100.000%> (+0.487%) ⬆️
Tests/SentryTests/SentryOptionsTest.m 98.400% <100.000%> (+1.005%) ⬆️

... and 537 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 b15521e...974ca11. Read the comment docs.

Copy link

github-actions bot commented Apr 2, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1231.80 ms 1241.81 ms 10.01 ms
Size 21.58 KiB 572.22 KiB 550.63 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
e0f077c 1224.65 ms 1243.52 ms 18.87 ms
e998fd0 1254.41 ms 1272.78 ms 18.37 ms
ecd9ecd 1214.16 ms 1232.59 ms 18.43 ms
9e6665f 1237.58 ms 1253.84 ms 16.26 ms
20163bb 1248.92 ms 1258.48 ms 9.56 ms
645f63f 1231.33 ms 1247.51 ms 16.18 ms
efb0147 1245.26 ms 1266.94 ms 21.68 ms
a6ac3d2 1219.00 ms 1231.32 ms 12.32 ms
39b1c35 1209.73 ms 1232.40 ms 22.67 ms
11ccc16 1203.82 ms 1237.06 ms 33.24 ms

App size

Revision Plain With Sentry Diff
e0f077c 22.85 KiB 412.59 KiB 389.74 KiB
e998fd0 21.58 KiB 414.59 KiB 393.01 KiB
ecd9ecd 20.76 KiB 420.23 KiB 399.47 KiB
9e6665f 22.84 KiB 403.52 KiB 380.67 KiB
20163bb 20.76 KiB 426.82 KiB 406.06 KiB
645f63f 21.58 KiB 572.91 KiB 551.33 KiB
efb0147 22.84 KiB 403.52 KiB 380.67 KiB
a6ac3d2 20.76 KiB 436.65 KiB 415.89 KiB
39b1c35 22.85 KiB 408.88 KiB 386.03 KiB
11ccc16 20.76 KiB 431.71 KiB 410.95 KiB

Previous results on branch: feat/add-swizzle-class-name-excludes

Startup times

Revision Plain With Sentry Diff
85ef2ea 1224.23 ms 1242.32 ms 18.09 ms

App size

Revision Plain With Sentry Diff
85ef2ea 21.58 KiB 573.79 KiB 552.21 KiB

Copy link

github-actions bot commented Apr 2, 2024

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

  • Sources/Sentry/SentrySubClassFinder.m

@getsentry getsentry deleted a comment from github-actions bot Apr 2, 2024
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.

Looks good!

I have some suggestions to discuss.

Sources/Sentry/Public/SentryOptions.h Outdated Show resolved Hide resolved
Sources/Sentry/SentrySubClassFinder.m Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Apr 3, 2024

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

  • Sources/Sentry/SentrySubClassFinder.m

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!

Thanks for the updates

@philipphofmann philipphofmann merged commit f438610 into main Apr 3, 2024
71 of 72 checks passed
@philipphofmann philipphofmann deleted the feat/add-swizzle-class-name-excludes branch April 3, 2024 10:56
dKasabwala pushed a commit to dKasabwala/sentry-cocoa that referenced this pull request May 6, 2024
Add an option to exclude certain classes from swizzling.

Fixes getsentryGH-3798
threema-matteo pushed a commit to threema-ch/sentry-cocoa that referenced this pull request May 21, 2024
Add an option to exclude certain classes from swizzling.

Fixes getsentryGH-3798
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.

Crash in [SentrySubClassFinder actOnSubclassesOfViewControllerInImage] (SentrySubClassFinder.m:61)
2 participants