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: Transactions for crashes #4504

Merged
merged 22 commits into from
Nov 8, 2024
Merged

Conversation

philipphofmann
Copy link
Member

@philipphofmann philipphofmann commented Nov 5, 2024

📜 Description

Finishes the transaction bound to the scope when the app crashes and stores it to disk. This experimental feature is disabled by default. You can enable it via the option enableTracingWhenCrashing.

I'm sorry about the large diff, but the PR only makes sense in one significant change. The largest part is the tests.

Docs PR: getsentry/sentry-docs#11732.

💡 Motivation and Context

Fixes GH-2306

💚 How did you test it?

Unit tests, simulator, and a release build via TestFlight.

CleanShot 2024-11-05 at 10 52 37@2x

https://sentry-sdks.sentry.io/issues/6044162827/events/4e25fdce217445b5ab3ecd6eb906a77e/?node=txn-efa80d62c9a4472fa8cfef87320638d2&project=5428557

📝 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

Copy link

github-actions bot commented Nov 5, 2024

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

Generated by 🚫 dangerJS against ae17e9a

Copy link

github-actions bot commented Nov 5, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1215.24 ms 1234.98 ms 19.74 ms
Size 21.90 KiB 729.72 KiB 707.82 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
282cc99 1238.27 ms 1253.46 ms 15.19 ms
9faf217 1268.86 ms 1274.82 ms 5.96 ms
0f8dc88 1243.82 ms 1263.08 ms 19.27 ms
7f691b5 1233.94 ms 1243.80 ms 9.86 ms
83acf3e 1240.06 ms 1261.38 ms 21.31 ms
8ef07fb 1231.45 ms 1243.98 ms 12.53 ms
a9103fe 1221.49 ms 1243.33 ms 21.84 ms
a423161 1234.41 ms 1246.41 ms 12.00 ms
4977fbc 1217.26 ms 1239.82 ms 22.56 ms
1bbcb9c 1214.25 ms 1230.04 ms 15.79 ms

App size

Revision Plain With Sentry Diff
282cc99 22.85 KiB 414.09 KiB 391.24 KiB
9faf217 20.76 KiB 419.70 KiB 398.94 KiB
0f8dc88 21.58 KiB 729.90 KiB 708.32 KiB
7f691b5 20.76 KiB 420.55 KiB 399.79 KiB
83acf3e 21.58 KiB 539.87 KiB 518.29 KiB
8ef07fb 21.58 KiB 707.42 KiB 685.84 KiB
a9103fe 20.76 KiB 426.95 KiB 406.19 KiB
a423161 22.85 KiB 406.69 KiB 383.85 KiB
4977fbc 20.76 KiB 419.85 KiB 399.10 KiB
1bbcb9c 20.76 KiB 426.10 KiB 405.34 KiB

Previous results on branch: feat/transactions-for-crashes

Startup times

Revision Plain With Sentry Diff
8222b3d 1212.98 ms 1240.35 ms 27.37 ms
fa979fd 1234.69 ms 1255.37 ms 20.67 ms
08981b2 1217.90 ms 1249.32 ms 31.42 ms
88aa4f6 1230.12 ms 1258.31 ms 28.18 ms

App size

Revision Plain With Sentry Diff
8222b3d 21.90 KiB 728.22 KiB 706.32 KiB
fa979fd 21.90 KiB 728.17 KiB 706.27 KiB
08981b2 21.90 KiB 728.19 KiB 706.29 KiB
88aa4f6 21.90 KiB 728.19 KiB 706.29 KiB

@philipphofmann
Copy link
Member Author

@brustolin, I still need to fix some problems with the context before you can review this.

Copy link

codecov bot commented Nov 5, 2024

Codecov Report

Attention: Patch coverage is 97.73585% with 12 lines in your changes missing coverage. Please review.

Project coverage is 91.509%. Comparing base (05ac767) to head (ae17e9a).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
Sources/Sentry/SentrySpotlightTransport.m 0.000% 3 Missing ⚠️
Sources/SentryCrash/Recording/SentryCrashC.c 78.571% 3 Missing ⚠️
Sources/Sentry/SentryTracer.m 96.875% 2 Missing ⚠️
...es/Sentry/SentryWatchdogTerminationScopeObserver.m 0.000% 2 Missing ⚠️
...ts/SentryTests/Transaction/SentryTracerTests.swift 97.368% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4504       +/-   ##
=============================================
+ Coverage   91.465%   91.509%   +0.043%     
=============================================
  Files          615       615               
  Lines        68992     69464      +472     
  Branches     24753     24892      +139     
=============================================
+ Hits         63104     63566      +462     
- Misses        5795      5804        +9     
- Partials        93        94        +1     
Files with missing lines Coverage Δ
SentryTestUtils/TestClient.swift 85.350% <100.000%> (+0.382%) ⬆️
SentryTestUtils/TestTransport.swift 87.500% <100.000%> (+2.500%) ⬆️
SentryTestUtils/TestTransportAdapter.swift 72.727% <100.000%> (+6.060%) ⬆️
Sources/Sentry/SentryClient.m 98.701% <100.000%> (+0.020%) ⬆️
Sources/Sentry/SentryCrashIntegration.m 99.159% <100.000%> (+0.112%) ⬆️
Sources/Sentry/SentryCrashReportConverter.m 96.049% <100.000%> (+0.049%) ⬆️
Sources/Sentry/SentryCrashScopeObserver.m 96.396% <100.000%> (+0.134%) ⬆️
Sources/Sentry/SentryHttpTransport.m 98.098% <100.000%> (+0.021%) ⬆️
Sources/Sentry/SentryHub.m 99.072% <100.000%> (+0.017%) ⬆️
Sources/Sentry/SentryOptions.m 98.698% <100.000%> (+0.007%) ⬆️
... and 19 more

... and 11 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 05ac767...ae17e9a. Read the comment docs.

@philipphofmann philipphofmann marked this pull request as ready for review November 5, 2024 15:27
@philipphofmann
Copy link
Member Author

@brustolin, this is ready for review now.

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.

Thanks for this. PR looks solid. I have a few non blocking comments.

The best approach would be to synchronize between Objective-C and C as transactions happens, but it would be challenging to make that implementation, Im happy with this implementation.

Also, this only finishes the transaction in the scope, but if the user starts a few transactions in parallel without adding it to the scope, those transactions don’t get finished. I think it would be possible to solve this with a message dispatcher, or the prior suggestion.

Sources/Sentry/Public/SentryOptions.h Outdated Show resolved Hide resolved
Sources/Sentry/SentryCrashIntegration.m Outdated Show resolved Hide resolved
Sources/Sentry/SentryScope.m Show resolved Hide resolved
Sources/Sentry/SentryTracer.m Outdated Show resolved Hide resolved
@philipphofmann
Copy link
Member Author

The best approach would be to synchronize between Objective-C and C as transactions happens

For being a good citizen when we're crashing yes, but it would also mean some of overhead synchronizing all the transaction data consistently to C.

Also, this only finishes the transaction in the scope, but if the user starts a few transactions in parallel without adding it to the scope, those transactions don’t get finished. I think it would be possible to solve this with a message dispatcher, or the prior suggestion.

If this solution works appropriately, and people ask for it, we cant try to capture all transactions when we're crashing. I think this is good enough for the first iteration.

@philipphofmann philipphofmann merged commit 0d38ada into main Nov 8, 2024
60 of 62 checks passed
@philipphofmann philipphofmann deleted the feat/transactions-for-crashes branch November 8, 2024 10:30
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.

Finish and capture transaction/span bound to the Scope in case of a fatal crash
2 participants