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 SIGABRT when modifying scope user #4274

Merged
merged 9 commits into from
Aug 19, 2024
Merged

Conversation

denrase
Copy link
Collaborator

@denrase denrase commented Aug 12, 2024

📜 Description

Fix SIGABRT when modifying scope user

💡 Motivation and Context

Closes #4231

💚 How did you test it?

Unit test

📝 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.

@denrase denrase marked this pull request as ready for review August 12, 2024 13:56
@denrase denrase enabled auto-merge (squash) August 12, 2024 14:14
Copy link

github-actions bot commented Aug 12, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1226.06 ms 1244.45 ms 18.39 ms
Size 21.58 KiB 706.66 KiB 685.08 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
e84bc3f 1201.49 ms 1232.82 ms 31.33 ms
e072ad1 1232.43 ms 1245.51 ms 13.08 ms
6c31077 1233.80 ms 1245.34 ms 11.54 ms
e1cd9e9 1190.64 ms 1221.90 ms 31.26 ms
25737cb 1235.02 ms 1250.06 ms 15.04 ms
c00eafe 1198.26 ms 1227.62 ms 29.36 ms
94e1968 1234.41 ms 1252.63 ms 18.22 ms
a7ca2d1 1237.42 ms 1248.98 ms 11.56 ms
cf724da 1226.61 ms 1235.70 ms 9.09 ms
881a955 1222.94 ms 1246.26 ms 23.32 ms

App size

Revision Plain With Sentry Diff
e84bc3f 20.76 KiB 434.72 KiB 413.96 KiB
e072ad1 21.58 KiB 625.83 KiB 604.24 KiB
6c31077 22.84 KiB 401.65 KiB 378.81 KiB
e1cd9e9 22.85 KiB 412.95 KiB 390.10 KiB
25737cb 20.76 KiB 436.29 KiB 415.53 KiB
c00eafe 20.76 KiB 432.87 KiB 412.11 KiB
94e1968 21.58 KiB 614.74 KiB 593.15 KiB
a7ca2d1 21.58 KiB 614.92 KiB 593.34 KiB
cf724da 20.76 KiB 430.52 KiB 409.76 KiB
881a955 22.85 KiB 407.63 KiB 384.78 KiB

Previous results on branch: fix/sigabrt-modify-scope-user

Startup times

Revision Plain With Sentry Diff
d582094 1226.80 ms 1235.85 ms 9.06 ms
fa19249 1218.38 ms 1245.06 ms 26.69 ms

App size

Revision Plain With Sentry Diff
d582094 21.58 KiB 705.05 KiB 683.47 KiB
fa19249 21.58 KiB 705.53 KiB 683.95 KiB

Copy link
Member

@armcknight armcknight left a comment

Choose a reason for hiding this comment

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

The fix looks good, but I think the test needs to be a little more strict.

Copy link

codecov bot commented Aug 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.634%. Comparing base (279351b) to head (a390a7e).
Report is 6 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4274       +/-   ##
=============================================
+ Coverage   91.580%   91.634%   +0.054%     
=============================================
  Files          616       617        +1     
  Lines        50096     50194       +98     
  Branches     18015     18107       +92     
=============================================
+ Hits         45878     45995      +117     
+ Misses        4125      4106       -19     
  Partials        93        93               
Files Coverage Δ
Sources/Sentry/SentryScope.m 96.694% <100.000%> (+0.013%) ⬆️
Tests/SentryTests/SentryScopeSwiftTests.swift 98.953% <100.000%> (+0.017%) ⬆️

... and 21 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 279351b...a390a7e. 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.

LGTM

@denrase denrase enabled auto-merge (squash) August 19, 2024 09:17
@denrase denrase merged commit 8ef07fb into main Aug 19, 2024
65 checks passed
@denrase denrase deleted the fix/sigabrt-modify-scope-user branch August 19, 2024 09:18
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.

SIGABRT when modifying scope.user from multiple threads
4 participants