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

refactor: Change type of Swift String constants to NSString #4894

Merged
merged 6 commits into from
Feb 26, 2025

Conversation

philprime
Copy link
Contributor

📜 Description

Change the type of constants to NSString.

💡 Motivation and Context

We purposely defined constants such as SentryTraceOrigin, SentrySpanOperation and SentrySpanDataKey in Swift for our long-term effort in migrating to Swift.

Stack traces in #4887 indicate that the bridging of Swift String to NSString might be causing memory issues when used from Objective-C blocks (i.e. useSpan).

This PR is an attempt to reduce the amount of bridging performed.

💚 How did you test it?

Can not be reproduced/tested reliably.

@philprime philprime changed the title refactor: change type of Swift String constants to NSString refactor: Change type of Swift String constants to NSString Feb 25, 2025
Copy link

codecov bot commented Feb 25, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.371%. Comparing base (5ea7b5a) to head (fe714f1).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4894       +/-   ##
=============================================
+ Coverage   92.369%   92.371%   +0.001%     
=============================================
  Files          659       659               
  Lines        77548     77549        +1     
  Branches     28079     28081        +2     
=============================================
+ Hits         71631     71633        +2     
+ Misses        5821      5817        -4     
- Partials        96        99        +3     
Files with missing lines Coverage Δ
...SentryProfilerTests/SentryProfileTestFixture.swift 99.705% <100.000%> (ø)
...rformance/CoreData/SentryCoreDataTrackerTest.swift 88.130% <100.000%> (ø)
...ions/Performance/IO/SentryFileIOTrackerTests.swift 98.546% <100.000%> (ø)
...Network/SentryNetworkTrackerIntegrationTests.swift 70.036% <100.000%> (ø)
...iewController/SentryTimeToDisplayTrackerTest.swift 100.000% <100.000%> (ø)
...ntryTests/Transaction/SentrySpanContextTests.swift 100.000% <100.000%> (ø)
...ts/Transaction/SentryTransactionContextTests.swift 100.000% <100.000%> (ø)

... and 14 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 5ea7b5a...fe714f1. Read the comment docs.

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

The tests are failing. We should investigate how we can avoid this issue from happening again. It doesn't have to be in this PR, but we must come up with something cause it can happen again.

Copy link

github-actions bot commented Feb 25, 2025

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1209.96 ms 1236.96 ms 27.00 ms
Size 22.31 KiB 821.41 KiB 799.10 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
075a044 1237.26 ms 1255.36 ms 18.10 ms
7fb7afb 1235.00 ms 1256.81 ms 21.81 ms
2b19b82 1212.94 ms 1252.84 ms 39.90 ms
257c2a9 1231.45 ms 1252.12 ms 20.67 ms
6129be5 1215.65 ms 1247.18 ms 31.54 ms
3bf3c92 1236.94 ms 1253.00 ms 16.06 ms
5d6ce0e 1237.10 ms 1257.46 ms 20.36 ms
319f0f5 1229.68 ms 1247.67 ms 17.99 ms
7cd187e 1239.39 ms 1258.02 ms 18.63 ms
48e8c2e 1231.00 ms 1244.52 ms 13.52 ms

App size

Revision Plain With Sentry Diff
075a044 20.76 KiB 420.41 KiB 399.65 KiB
7fb7afb 20.76 KiB 419.69 KiB 398.94 KiB
2b19b82 21.58 KiB 542.18 KiB 520.59 KiB
257c2a9 20.76 KiB 401.36 KiB 380.60 KiB
6129be5 21.58 KiB 418.00 KiB 396.42 KiB
3bf3c92 21.58 KiB 706.06 KiB 684.48 KiB
5d6ce0e 22.85 KiB 405.38 KiB 382.53 KiB
319f0f5 22.30 KiB 749.70 KiB 727.40 KiB
7cd187e 20.76 KiB 401.65 KiB 380.89 KiB
48e8c2e 21.58 KiB 418.44 KiB 396.86 KiB

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

LGTM, if the tests are green. As already pointed out we should follow up with a strategy to prevent regressions.

@philprime
Copy link
Contributor Author

As already pointed out we should follow up with a strategy to prevent regressions.

#4897

@philprime
Copy link
Contributor Author

It keeps failing with flaky UI tests unrelated to the changes. Merging this now.

@philprime philprime merged commit b380503 into main Feb 26, 2025
73 of 74 checks passed
@philprime philprime deleted the philprime/issue-4887-change-constants-to-nsstring branch February 26, 2025 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants