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: dont add boot time to enriched scopes #3912

Merged
merged 3 commits into from
Apr 27, 2024

Conversation

armcknight
Copy link
Member

Fixes an issue raised after reopening #3775, to avoid sending boot time off device, which is explicitly forbidden by Apple.

Still searching for an appropriate place to put a test for this. No test failed after making the change, and we have lots of test files for crash reporting, so it's not obvious to me ATM.

Copy link

codecov bot commented Apr 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.639%. Comparing base (704d854) to head (1e9eaca).

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3912       +/-   ##
=============================================
+ Coverage   90.630%   90.639%   +0.008%     
=============================================
  Files          582       582               
  Lines        45469     45468        -1     
  Branches     16198     16201        +3     
=============================================
+ Hits         41209     41212        +3     
+ Misses        4081      4074        -7     
- Partials       179       182        +3     
Files Coverage Δ
Sources/Sentry/SentryCrashWrapper.m 94.520% <ø> (-0.075%) ⬇️

... and 8 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 704d854...1e9eaca. Read the comment docs.

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

to unblock

@bruno-garcia
Copy link
Member

We could add a test later asserting this property isn't in a captured error. In the test we can expand the reasoning.

Copy link

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1235.65 ms 1249.69 ms 14.05 ms
Size 21.58 KiB 615.91 KiB 594.33 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
1656cf6 1229.59 ms 1245.52 ms 15.93 ms
6bc5ae5 1227.78 ms 1231.76 ms 3.98 ms
2124551 1201.23 ms 1225.34 ms 24.11 ms
0d32275 1230.96 ms 1237.90 ms 6.94 ms
4d3df92 1235.18 ms 1252.29 ms 17.10 ms
326b7eb 1252.86 ms 1259.56 ms 6.70 ms
340fb46 1224.60 ms 1239.27 ms 14.67 ms
e5dcbd5 1223.47 ms 1243.90 ms 20.43 ms
d60f70a 1219.63 ms 1228.54 ms 8.91 ms
02a972c 1207.08 ms 1221.78 ms 14.70 ms

App size

Revision Plain With Sentry Diff
1656cf6 21.58 KiB 546.19 KiB 524.61 KiB
6bc5ae5 20.76 KiB 401.39 KiB 380.63 KiB
2124551 22.85 KiB 411.69 KiB 388.84 KiB
0d32275 22.84 KiB 403.14 KiB 380.29 KiB
4d3df92 22.85 KiB 413.44 KiB 390.59 KiB
326b7eb 20.76 KiB 432.31 KiB 411.55 KiB
340fb46 21.58 KiB 418.78 KiB 397.20 KiB
e5dcbd5 22.85 KiB 414.09 KiB 391.24 KiB
d60f70a 20.76 KiB 430.97 KiB 410.21 KiB
02a972c 22.85 KiB 413.42 KiB 390.57 KiB

@armcknight armcknight merged commit c8dbe73 into main Apr 27, 2024
69 of 70 checks passed
@armcknight armcknight deleted the armcknight/fix/boot-time-transmission branch April 27, 2024 00:07
philipphofmann added a commit that referenced this pull request Apr 29, 2024
philipphofmann added a commit that referenced this pull request Apr 29, 2024
dKasabwala pushed a commit to dKasabwala/sentry-cocoa that referenced this pull request May 6, 2024
dKasabwala pushed a commit to dKasabwala/sentry-cocoa that referenced this pull request May 6, 2024
threema-matteo pushed a commit to threema-ch/sentry-cocoa that referenced this pull request May 21, 2024
threema-matteo pushed a commit to threema-ch/sentry-cocoa that referenced this pull request May 21, 2024
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.

2 participants